Skip to content

Script to import and export layout group permissions in bulk [D: 1825]#641

Open
Oliver-ctrlo wants to merge 1 commit into
ctrlo:devfrom
Oliver-ctrlo:patch-8
Open

Script to import and export layout group permissions in bulk [D: 1825]#641
Oliver-ctrlo wants to merge 1 commit into
ctrlo:devfrom
Oliver-ctrlo:patch-8

Conversation

@Oliver-ctrlo

Copy link
Copy Markdown
Contributor

This script allows bulk importing and exporting group layout permissions.

This script allows bulk importing and exporting group layout permissions.

@pwlodarski-ctrlo pwlodarski-ctrlo left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

PR reviewed - couple comments added for consideration.

die "ERROR: Invalid Group ID '$group_id' on row $RowCount.\n";
}
unless (defined $permission && $permission =~ /^(?:read|write_new|write_existing|write_existing_no_approval|write_new_no_approval)$/) {
die "ERROR: Invalid Permission '$permission' on row $RowCount. Value must be read, write_new, write_existing, or write_existing_no_approval.\n";

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would write_new_no_approval be included too?

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That permission is a left over from GADS 1.0 and is no longer used

}

sub Export_permissions {
my $export_file = './Group_permission_export.csv';

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would it be beneficial for the filename to be unique? E.g. it could have the group IDs or the current datetime appended.

{
prefetch => [ 'layout' ],
}
)->all;

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would it be worth moving the $guard->commit; to below this search, just in case any exceptions are thrown?

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No - we want the guard to fall out of scope and therefore roll back any and all changes, rather than doing a partial commit if only some changes are made before the error.

@droberts-ctrlo droberts-ctrlo left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Various observations - the majority of which are from the use of die rather than error or fault - you can also use info rather than say, but that's a matter of personal preference and not an issue.


sub Import_permissions {
say "Enter the file you want to import from:";
my $file = <STDIN>;

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could this not be a script argument?

my $guard = schema->txn_scope_guard;

while (my $row = $csv->getline($fh)) {
my ($layout_id, $group_id, $permission) = @$row;

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This assumes CSV is set up correctly - you can import the rows as a hash containing {column1 => value, column2 => value ... } where column1, column2, etc are the header of the file which would mitigate if the file data is in the incorrect order (currently, it will assume it's correct and at worst, import the data incorrectly, at best, die)

}

sub Import_permissions {
say "Enter the file you want to import from:";

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe set up the option to allow for logging here as well? Output information to a log file to show what's been done? Otherwise there's no audit trail and if something goes wrong we won't easily know what's borken.

});

if ($existing) {
say "Skipping permission '$permission' for group '$group_id' on layout '$layout_id' because this permission already exists";

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we need to say anything here? Could just put next if $existing

my $file = <STDIN>;
chomp($file);

-f $file or die "ERROR: File '$file' does not exist";

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should use error rather than die here (remember to import logreport)

die "ERROR: Invalid Permission '$permission' on row $RowCount. Value must be read, write_new, write_existing, or write_existing_no_approval.\n";
}

rset('Layout')->find($layout_id) or die "ERROR: Layout ID '$layout_id' does not exist\n";

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should be error, not die

}

sub Export_permissions {
my $export_file = './Group_permission_export.csv';

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Parameter? use --file or something and only set this if it's not set as a parameter (i.e. $export_file //= './Group_permission_export.csv'


sub Export_permissions {
my $export_file = './Group_permission_export.csv';
! -f $export_file or die "ERROR: $export_file already exists, unable to overwrite file!";

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should be error rather than die

chomp($input);

if (!$input || $input !~ /^\s*\d+(?:\s*,\s*\d+)*\s*$/) {
die "Error: You need to enter a number or a comma-separated list of numbers.\n";

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should be error rather than die

my @groups = split(/\s*,\s*/, $input);

my $csv = Text::CSV->new({ binary => 1, auto_diag => 1 });
open my $fh, ">:encoding(utf8)", $export_file or die "$export_file: $!";

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should be fault rather than die - this will also remove the need for $!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants