From cc631e94da2aa12ac90dd3ed5a09a778eac0d30d Mon Sep 17 00:00:00 2001 From: Jason Etheridge Date: Tue, 24 Jan 2023 08:29:33 -0500 Subject: [PATCH] LP1929593 UPDATE_COPY_BARCODE permission This adds the permission UPDATE_COPY_BARCODE and a new API call, open-ils.cat.update_copy_barcode which explicitly tests for both UPDATE_COPY_BARCODE and UPDATE_COPY, with either being sufficient for allowing a barcode change. Existing Replace Barcode UI's in both Angular and AngularJS have been modified to use this API call instead of the pcrud service. One side-effect of this has been better surfacing of errors, as errors in pcrud were uncaught and bypassing the normal error handling. This addresses LP1951469. The upgrade script gives any permission groups that already have the UPDATE_COPY permission the new UPDATE_COPY_BARCODE permission at the same depth, though it's technically not needed. Signed-off-by: Jason Etheridge Signed-off-by: Ruth Frasur Signed-off-by: Jane Sandberg --- .../holdings/replace-barcode-dialog.component.ts | 41 +++- .../src/perlmods/lib/OpenILS/Application/Cat.pm | 38 +++ .../live_t/lp1929593-update-copy-barcode-perm.t | 252 ++++++++++++++++++++ Open-ILS/src/sql/Pg/950.data.seed-values.sql | 7 +- .../sql/Pg/upgrade/XXXX.data.edit_barcode_perm.sql | 35 +++ .../web/js/ui/default/staff/cat/catalog/app.js | 26 ++- .../default/staff/cat/item/replace_barcode/app.js | 17 +- .../web/js/ui/default/staff/circ/services/item.js | 25 ++- docs/RELEASE_NOTES_NEXT/miscellaneous.adoc | 18 ++ 9 files changed, 432 insertions(+), 27 deletions(-) create mode 100644 Open-ILS/src/perlmods/live_t/lp1929593-update-copy-barcode-perm.t create mode 100644 Open-ILS/src/sql/Pg/upgrade/XXXX.data.edit_barcode_perm.sql diff --git a/Open-ILS/src/eg2/src/app/staff/share/holdings/replace-barcode-dialog.component.ts b/Open-ILS/src/eg2/src/app/staff/share/holdings/replace-barcode-dialog.component.ts index accc2c6..445e603 100644 --- a/Open-ILS/src/eg2/src/app/staff/share/holdings/replace-barcode-dialog.component.ts +++ b/Open-ILS/src/eg2/src/app/staff/share/holdings/replace-barcode-dialog.component.ts @@ -1,7 +1,10 @@ import {Component, Input, ViewChild, Renderer2} from '@angular/core'; import {Observable} from 'rxjs'; import {switchMap, map, tap} from 'rxjs/operators'; +import {AuthService} from '@eg/core/auth.service'; import {IdlObject} from '@eg/core/idl.service'; +import {EventService} from '@eg/core/event.service'; +import {NetService} from '@eg/core/net.service'; import {PcrudService} from '@eg/core/pcrud.service'; import {ToastService} from '@eg/share/toast/toast.service'; import {NgbModal, NgbModalOptions} from '@ng-bootstrap/ng-bootstrap'; @@ -40,6 +43,9 @@ export class ReplaceBarcodeDialogComponent constructor( private modal: NgbModal, // required for passing to parent private toast: ToastService, + private auth: AuthService, + private net: NetService, + private evt: EventService, private pcrud: PcrudService, private renderer: Renderer2) { super(modal); // required for subclassing @@ -59,6 +65,11 @@ export class ReplaceBarcodeDialogComponent getNextCopy(): Observable { + if (this.auth.opChangeIsActive()) { + // FIXME: kludge for now, opChange has been reverting mid-dialog with batch use when handling permission elevation + this.auth.undoOpChange(); + } + if (this.ids.length === 0) { this.close(this.numSucceeded > 0); } @@ -71,7 +82,7 @@ export class ReplaceBarcodeDialogComponent .pipe(map(c => this.copy = c)); } - async replaceOneBarcode(): Promise { + replaceOneBarcode() { this.barcodeExists = false; // First see if the barcode is in use @@ -82,17 +93,29 @@ export class ReplaceBarcodeDialogComponent return; } - this.copy.barcode(this.newBarcode); - this.pcrud.update(this.copy).toPromise().then( - async (ok) => { - this.numSucceeded++; - this.toast.success(await this.successMsg.current()); - return this.getNextCopy().toPromise(); + this.net.request( + 'open-ils.cat', + 'open-ils.cat.update_copy_barcode', + this.auth.token(), this.copy.id(), this.newBarcode + ).subscribe( + (res) => { + if (this.evt.parse(res)) { + console.error('parsed error response', res); + } else { + console.log('success', res); + this.numSucceeded++; + this.successMsg.current().then(m => this.toast.success(m)); + this.getNextCopy().toPromise(); + } }, - async (err) => { + (err) => { + console.error('error', err); this.numFailed++; console.error('Replace barcode failed: ', err); - this.toast.warning(await this.errorMsg.current()); + this.errorMsg.current().then(m => this.toast.warning(m)); + }, + () => { + console.log('finis'); } ); }); diff --git a/Open-ILS/src/perlmods/lib/OpenILS/Application/Cat.pm b/Open-ILS/src/perlmods/lib/OpenILS/Application/Cat.pm index deb7ad4..b6cd996 100644 --- a/Open-ILS/src/perlmods/lib/OpenILS/Application/Cat.pm +++ b/Open-ILS/src/perlmods/lib/OpenILS/Application/Cat.pm @@ -2086,6 +2086,44 @@ sub volcopy_data { return undef; } +__PACKAGE__->register_method( + method => "update_copy_barcode", + api_name => "open-ils.cat.update_copy_barcode", + argc => 3, + signature => { + desc => q|Updates the barcode for a item, checking for either the UPDATE_COPY permission or the UPDATE_COPY_BARCODE permission.|, + params => [ + {desc => 'Authtoken', type => 'string'}, + {desc => 'Copy ID', type => 'number'}, + {desc => 'New Barcode', type => 'string'} + ] + }, + return => {desc => 'Returns the copy ID if successful, an ILS event otherwise.', type => 'string'} +); + +sub update_copy_barcode { + my ($self, $client, $auth, $copy_id, $barcode) = @_; + my $e = new_editor(authtoken => $auth, xact => 1); + + $e->checkauth or return $e->event; + + my $copy = $e->retrieve_asset_copy($copy_id) + or return $e->event; + + # make sure there is no undeleted copy (including the same one?) with the same barcode + my $existing = $e->search_asset_copy({barcode => $barcode, deleted => 'f'}, {idlist=>1}); + return OpenILS::Event->new('ITEM_BARCODE_EXISTS') if @$existing; + + # if both of these perm checks fail, we'll report it for UPDATE_COPY_BARCODE as it is more specific + return $e->event unless $e->allowed('UPDATE_COPY', $copy->circ_lib) || $e->allowed('UPDATE_COPY_BARCODE', $copy->circ_lib); + + $copy->barcode( $barcode ); + + $e->update_asset_copy( $copy ) or return $e->event; + $e->commit or return $e->event; + + return $copy->id; +} 1; diff --git a/Open-ILS/src/perlmods/live_t/lp1929593-update-copy-barcode-perm.t b/Open-ILS/src/perlmods/live_t/lp1929593-update-copy-barcode-perm.t new file mode 100644 index 0000000..55e71e0 --- /dev/null +++ b/Open-ILS/src/perlmods/live_t/lp1929593-update-copy-barcode-perm.t @@ -0,0 +1,252 @@ +#!perl +use strict; use warnings; +use Test::More tests => 17; +use OpenILS::Utils::TestUtils; +use OpenILS::Const qw(:const); + +my $U = 'OpenILS::Application::AppUtils'; +my $script = OpenILS::Utils::TestUtils->new(); + +diag("Test LP1929593 Wishlist: need separate permission for editing barcode"); + +use constant { + BR1_ID => 4, + BR3_ID => 6, + WORKSTATION => 'BR1-lp1929593-ebarc' +}; + +# We are deliberately NOT using the admin user to check for a perm failure. +my $credentials = { + username => 'br1mtownsend', + password => 'maryt1234', + type => 'staff' +}; + +# Log in as staff. +my $authtoken = $script->authenticate($credentials); +ok( + $authtoken, + 'Logged in' +) or BAIL_OUT('Must log in'); + +# Find or register workstation. +my $ws = $script->find_or_register_workstation(WORKSTATION, BR1_ID); +ok( + ! ref $ws, + 'Found or registered workstation' +) or BAIL_OUT('Need Workstation'); + +# Logout. +$script->logout(); +ok( + ! $script->authtoken, + 'Logged out' +); + +# Login with workstation. +$credentials->{workstation} = WORKSTATION; +$credentials->{password} = 'maryt1234'; +$authtoken = $script->authenticate($credentials); +ok( + $script->authtoken, + 'Logged in with workstation' +) or BAIL_OUT('Must log in'); + +# Find available copy at BR1 +my $acps = $U->simplereq( + 'open-ils.pcrud', + 'open-ils.pcrud.search.acp.atomic', + $authtoken, + {circ_lib => BR1_ID, status => OILS_COPY_STATUS_AVAILABLE}, + {limit => 1} +); +my $copy = $acps->[0]; +isa_ok( + ref $copy, + 'Fieldmapper::asset::copy', + 'Got available copy from BR1' +); + +sub test_barcode_rename_expecting_success { + my $copy = shift; + + diag('Testing re-barcoding of ' . $copy->barcode() . ', expecting successful re-barcoding.'); + my $original_barcode = $copy->barcode(); + + # Re-barcode it + my $result = $U->simplereq( + 'open-ils.cat', + 'open-ils.cat.update_copy_barcode', + $authtoken, + $copy->id(), + 'new' . $original_barcode + ); + is( + $result, + $copy->id(), + 'open-ils.cat.update_copy_barcode indicates success' + ); + + # Double-check to be sure + $copy = $U->simplereq( + 'open-ils.pcrud', + 'open-ils.pcrud.retrieve.acp', + $authtoken, + $copy->id() + ); + is( + $copy->barcode(), + 'new' . $original_barcode, + 'Copy was indeed re-barcoded' + ); + diag('Current barcode: ' . $copy->barcode()); + + # Change it back + $result = $U->simplereq( + 'open-ils.cat', + 'open-ils.cat.update_copy_barcode', + $authtoken, + $copy->id(), + $original_barcode + ); + is( + $result, + $copy->id(), + 'open-ils.cat.update_copy_barcode indicates success' + ); + + # Double-check to be sure + $copy = $U->simplereq( + 'open-ils.pcrud', + 'open-ils.pcrud.retrieve.acp', + $authtoken, + $copy->id() + ); + is( + $copy->barcode(), + $original_barcode, + 'Copy was indeed re-barcoded' + ); + diag('Current barcode: ' . $copy->barcode()); +} + +diag('Testing when user has both UPDATE_COPY and UPDATE_COPY_BARCODE'); +test_barcode_rename_expecting_success($copy); + +sub changePermCode { + my $from = shift; + my $to = shift; + diag('Changing ' . $from . ' permission to ' . $to); + + # stateful cstore session + my $cstore_ses = $script->session('open-ils.cstore'); + $cstore_ses->connect; + + # Now let's fetch the $from perm + my $xact = $cstore_ses->request('open-ils.cstore.transaction.begin')->gather(1); + my $retrieve_req = $cstore_ses->request( + 'open-ils.cstore.direct.permission.perm_list.search', + { 'code' => $from } + ); + my $perm = $retrieve_req->gather(1); + is( + $perm->code(), + $from, + "Fetched $from permission" + ); + + # now let's change the code + $perm->code($to); + my $update_req = $cstore_ses->request( + 'open-ils.cstore.direct.permission.perm_list.update', + $perm + ); + my $update_resp = $update_req->gather(1); + is( + $update_resp, + $perm->id(), + 'cstore update successful' + ); + + $cstore_ses->request('open-ils.cstore.transaction.commit')->gather(1); + $cstore_ses->disconnect(); +} + +changePermCode('UPDATE_COPY', 'WAS_UPDATE_COPY'); +diag('Testing when user only has UPDATE_COPY_BARCODE'); +test_barcode_rename_expecting_success($copy); + +sub test_barcode_rename_expecting_failure { + my $copy = shift; + + diag('Testing re-barcoding of ' . $copy->barcode() . ', expecting unsuccessful re-barcoding.'); + my $original_barcode = $copy->barcode(); + + # Re-barcode it + my $result = $U->simplereq( + 'open-ils.cat', + 'open-ils.cat.update_copy_barcode', + $authtoken, + $copy->id(), + 'new' . $original_barcode + ); + isnt( + $result, + $copy->id(), + 'open-ils.cat.update_copy_barcode indicates failure' + ); + + # Double-check to be sure + $copy = $U->simplereq( + 'open-ils.pcrud', + 'open-ils.pcrud.retrieve.acp', + $authtoken, + $copy->id() + ); + is( + $copy->barcode(), + $original_barcode, + 'Copy was indeed not re-barcoded' + ); + diag('Current barcode: ' . $copy->barcode()); + + # Attempt to change it back, just in case things are succeeding when they're not supposed to + $result = $U->simplereq( + 'open-ils.cat', + 'open-ils.cat.update_copy_barcode', + $authtoken, + $copy->id(), + $original_barcode + ); + isnt( + $result, + $copy->id(), + 'open-ils.cat.update_copy_barcode indicates failure' + ); + + # Double-check to be sure + $copy = $U->simplereq( + 'open-ils.pcrud', + 'open-ils.pcrud.retrieve.acp', + $authtoken, + $copy->id() + ); + is( + $copy->barcode(), + $original_barcode, + 'Copy was indeed not re-barcoded' + ); + diag('Current barcode: ' . $copy->barcode()); +} + +changePermCode('UPDATE_COPY_BARCODE', 'WAS_UPDATE_COPY_BARCODE'); + +diag('Testing when user has neither UPDATE_COPY_BARCODE nor UPDATE_COPY'); +test_barcode_rename_expecting_failure($copy); + +# back to the way they were +changePermCode('WAS_UPDATE_COPY', 'UPDATE_COPY'); +changePermCode('WAS_UPDATE_COPY_BARCODE', 'UPDATE_COPY_BARCODE'); + +# Logout +$script->logout(); # Not a test, just to be pedantic. diff --git a/Open-ILS/src/sql/Pg/950.data.seed-values.sql b/Open-ILS/src/sql/Pg/950.data.seed-values.sql index 346c273..ee9e110 100644 --- a/Open-ILS/src/sql/Pg/950.data.seed-values.sql +++ b/Open-ILS/src/sql/Pg/950.data.seed-values.sql @@ -1974,7 +1974,9 @@ INSERT INTO permission.perm_list ( id, code, description ) VALUES ( 640, 'ACCESS_ANGULAR_CIRC', oils_i18n_gettext(640, 'Allow a user to access the experimental Angular circulation interfaces', 'ppl', 'description')), ( 641, 'ADMIN_FUND_ROLLOVER', oils_i18n_gettext(641, - 'Allow the user to perform fund propagation and rollover', 'ppl', 'description')) + 'Allow the user to perform fund propagation and rollover', 'ppl', 'description')), + ( 642, 'UPDATE_COPY_BARCODE', oils_i18n_gettext(642, + 'Update the barcode for an item.', 'ppl', 'description')) ; SELECT SETVAL('permission.perm_list_id_seq'::TEXT, 1000); @@ -2189,6 +2191,7 @@ INSERT INTO permission.grp_perm_map (grp, perm, depth, grantable) 'CREATE_PAYMENT', 'RENEW_HOLD_OVERRIDE', 'UPDATE_COPY', + 'UPDATE_COPY_BARCODE', 'UPDATE_VOLUME', 'ADMIN_TOOLBAR', 'VOLUME_HOLDS'); @@ -2270,6 +2273,7 @@ INSERT INTO permission.grp_perm_map (grp, perm, depth, grantable) 'MARK_ITEM_ON_ORDER', 'MARK_ITEM_RESHELVING', 'UPDATE_COPY', + 'UPDATE_COPY_BARCODE', 'UPDATE_COPY_NOTE', 'UPDATE_IMPORT_ITEM', 'UPDATE_MFHD_RECORD', @@ -2714,6 +2718,7 @@ INSERT INTO permission.grp_perm_map (grp, perm, depth, grantable) 'UPDATE_BATCH_COPY', 'UPDATE_BIB_IMPORT_QUEUE', 'UPDATE_COPY', + 'UPDATE_COPY_BARCODE', 'UPDATE_FUND', 'UPDATE_FUND_ALLOCATION', 'UPDATE_FUNDING_SOURCE', diff --git a/Open-ILS/src/sql/Pg/upgrade/XXXX.data.edit_barcode_perm.sql b/Open-ILS/src/sql/Pg/upgrade/XXXX.data.edit_barcode_perm.sql new file mode 100644 index 0000000..3d747f2 --- /dev/null +++ b/Open-ILS/src/sql/Pg/upgrade/XXXX.data.edit_barcode_perm.sql @@ -0,0 +1,35 @@ +BEGIN; + +-- check whether patch can be applied +SELECT evergreen.upgrade_deps_block_check('XXXX', :eg_version); + +-- 950.data.seed-values.sql + +INSERT INTO permission.perm_list ( id, code, description ) VALUES + ( 642, 'UPDATE_COPY_BARCODE', oils_i18n_gettext(642, + 'Update the barcode for an item.', 'ppl', 'description')) +; + +-- give this perm to perm groups that already have UPDATE_COPY +WITH perms_to_add AS + (SELECT id FROM + permission.perm_list + WHERE code IN ('UPDATE_COPY_BARCODE')) +INSERT INTO permission.grp_perm_map (grp, perm, depth, grantable) + SELECT grp, perms_to_add.id as perm, depth, grantable + FROM perms_to_add, + permission.grp_perm_map + + --- Don't add the permissions if they have already been assigned + WHERE grp NOT IN + (SELECT DISTINCT grp FROM permission.grp_perm_map + INNER JOIN perms_to_add ON perm=perms_to_add.id) + + --- we're going to match the depth of their existing perm + AND perm = ( + SELECT id + FROM permission.perm_list + WHERE code = 'UPDATE_COPY' + ); + +COMMIT; diff --git a/Open-ILS/web/js/ui/default/staff/cat/catalog/app.js b/Open-ILS/web/js/ui/default/staff/cat/catalog/app.js index 8428df1..9952525 100644 --- a/Open-ILS/web/js/ui/default/staff/cat/catalog/app.js +++ b/Open-ILS/web/js/ui/default/staff/cat/catalog/app.js @@ -1076,16 +1076,28 @@ function($scope , $routeParams , $location , $window , $q , egCore , egHolds , e } $scope.copyId = copy.id(); - copy.barcode($scope.barcode2); - egCore.pcrud.update(copy).then(function(stat) { - $scope.updateOK = stat; - $scope.focusBarcode = true; - holdingsSvc.fetchAgain().then(function (){ - holdingsGridDataProviderRef.refresh(); - }); + egCore.net.request( + 'open-ils.cat', + 'open-ils.cat.update_copy_barcode', + egCore.auth.token(), $scope.copyId, $scope.barcode2 + ).then(function(resp) { + var evt = egCore.evt.parse(resp); + if (evt) { + console.log('toast 0 here', evt); + } else { + $scope.updateOK = stat; + $scope.focusBarcode = true; + holdingsSvc.fetchAgain().then(function (){ + holdingsGridDataProviderRef.refresh(); + }); + } }); + },function(E) { + console.log('toast 1 here',E); + },function(E) { + console.log('toast 2 here',E); }); $uibModalInstance.close(); } diff --git a/Open-ILS/web/js/ui/default/staff/cat/item/replace_barcode/app.js b/Open-ILS/web/js/ui/default/staff/cat/item/replace_barcode/app.js index d9d9d3d..cd2ae0f 100644 --- a/Open-ILS/web/js/ui/default/staff/cat/item/replace_barcode/app.js +++ b/Open-ILS/web/js/ui/default/staff/cat/item/replace_barcode/app.js @@ -37,12 +37,21 @@ function($scope , egCore) { } $scope.copyId = copy.id(); - copy.barcode($scope.barcode2); - egCore.pcrud.update(copy).then(function(stat) { - $scope.updateOK = stat; - $scope.focusBarcode = true; + egCore.net.request( + 'open-ils.cat', + 'open-ils.cat.update_copy_barcode', + egCore.auth.token(), $scope.copyId, $scope.barcode2 + ).then(function(resp) { + var evt = egCore.evt.parse(resp); + if (evt) { + console.log('toast 0 here 2', evt); + } else { + $scope.updateOK = true; + $scope.focusBarcode = true; + } }); + }); }); } diff --git a/Open-ILS/web/js/ui/default/staff/circ/services/item.js b/Open-ILS/web/js/ui/default/staff/circ/services/item.js index bd509d7..7309a4e 100644 --- a/Open-ILS/web/js/ui/default/staff/circ/services/item.js +++ b/Open-ILS/web/js/ui/default/staff/circ/services/item.js @@ -831,16 +831,29 @@ function(egCore , egOrg , egCirc , $uibModal , $q , $timeout , $window , ngToast } $scope.copyId = copy.id(); - copy.barcode($scope.barcode2); - egCore.pcrud.update(copy).then(function(stat) { - $scope.updateOK = stat; - $scope.focusBarcode = true; - if (stat) service.add_barcode_to_list(copy.barcode()); - $uibModalInstance.close(); + egCore.net.request( + 'open-ils.cat', + 'open-ils.cat.update_copy_barcode', + egCore.auth.token(), $scope.copyId, $scope.barcode2 + ).then(function(resp) { + var evt = egCore.evt.parse(resp); + if (evt) { + console.log('toast 0 here 2', evt); + } else { + $scope.updateOK = true; + $scope.focusBarcode = true; + $scope.focusBarcode2 = false; + service.add_barcode_to_list($scope.barcode2); + $uibModalInstance.close(); + } }); }); + },function(E) { + console.log('toast 1 here 2',E); + },function(E) { + console.log('toast 2 here 2',E); }); } diff --git a/docs/RELEASE_NOTES_NEXT/miscellaneous.adoc b/docs/RELEASE_NOTES_NEXT/miscellaneous.adoc index bb32c83..54a181e 100644 --- a/docs/RELEASE_NOTES_NEXT/miscellaneous.adoc +++ b/docs/RELEASE_NOTES_NEXT/miscellaneous.adoc @@ -11,3 +11,21 @@ * Prevent templates from applying or changing magical status in angular holdings editor (LP#1999401) * Prevent directly editing the shelving location deleted field in the Shelving Locations Editor (LP#2002435) * The "Strict Barcode" checkbox is now closer to the barcode input on the Check Out, Check In, and Renew Items pages (LP#1990968) +* LP1929593 UPDATE_COPY_BARCODE permission + + This adds the permission UPDATE_COPY_BARCODE and a new API call, + + open-ils.cat.update_copy_barcode + + which explicitly tests for both UPDATE_COPY_BARCODE and UPDATE_COPY, + with either being sufficient for allowing a barcode change. Existing + Replace Barcode UI's in both Angular and AngularJS have been modified + to use this API call instead of the pcrud service. One side-effect of + this has been better surfacing of errors, as errors in pcrud were + uncaught and bypassing the normal error handling. This addresses + LP1951469. + + The upgrade script gives any permission groups that already have the + UPDATE_COPY permission the new UPDATE_COPY_BARCODE permission at the + same depth, though it's technically not needed. + -- 1.7.2.5