LP1929593 UPDATE_COPY_BARCODE permission
authorJason Etheridge <jason@EquinoxOLI.org>
Tue, 24 Jan 2023 13:29:33 +0000 (08:29 -0500)
committerJane Sandberg <js7389@princeton.edu>
Wed, 19 Apr 2023 02:24:26 +0000 (19:24 -0700)
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 <jason@EquinoxOLI.org>
Signed-off-by: Ruth Frasur <rfrasur@library.in.gov>
Signed-off-by: Jane Sandberg <js7389@princeton.edu>

Open-ILS/src/eg2/src/app/staff/share/holdings/replace-barcode-dialog.component.ts
Open-ILS/src/perlmods/lib/OpenILS/Application/Cat.pm
Open-ILS/src/perlmods/live_t/lp1929593-update-copy-barcode-perm.t [new file with mode: 0644]
Open-ILS/src/sql/Pg/950.data.seed-values.sql
Open-ILS/src/sql/Pg/upgrade/XXXX.data.edit_barcode_perm.sql [new file with mode: 0644]
Open-ILS/web/js/ui/default/staff/cat/catalog/app.js
Open-ILS/web/js/ui/default/staff/cat/item/replace_barcode/app.js
Open-ILS/web/js/ui/default/staff/circ/services/item.js
docs/RELEASE_NOTES_NEXT/miscellaneous.adoc

index accc2c6..445e603 100644 (file)
@@ -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<any> {
 
+        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<any> {
+    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');
                 }
             );
         });
index deb7ad4..b6cd996 100644 (file)
@@ -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 (file)
index 0000000..55e71e0
--- /dev/null
@@ -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.
index 346c273..ee9e110 100644 (file)
@@ -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 (file)
index 0000000..3d747f2
--- /dev/null
@@ -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;
index 8428df1..9952525 100644 (file)
@@ -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();
                         }
index d9d9d3d..cd2ae0f 100644 (file)
@@ -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;
+                    }
                 });
+
             });
         });
     }
index bd509d7..7309a4e 100644 (file)
@@ -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);
                             });
                         }
 
index bb32c83..54a181e 100644 (file)
 * 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.
+