LP1929593 UPDATE_COPY_BARCODE permission
authorJason Etheridge <jason@EquinoxOLI.org>
Tue, 24 Jan 2023 13:29:33 +0000 (08:29 -0500)
committerChris Sharp <csharp@georgialibraries.org>
Wed, 19 Apr 2023 14:01:49 +0000 (10:01 -0400)
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>
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 9b25b33..e20f451 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 1cbbf70..84f7aa8 100644 (file)
@@ -1976,7 +1976,9 @@ INSERT INTO permission.perm_list ( id, code, description ) VALUES
     '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')),
- ( 1101, 'ADMIN_STUDENT_CARDS', oils_i18n_gettext(636,
+ ( 642, 'UPDATE_COPY_BARCODE', oils_i18n_gettext(642,
+    'Update the barcode for an item.', 'ppl', 'description')),
+( 1101, 'ADMIN_STUDENT_CARDS', oils_i18n_gettext(636,
     'Modify student card settings', 'ppl', 'description'))
 ;
 
@@ -2192,6 +2194,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');
@@ -2273,6 +2276,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',
@@ -2717,6 +2721,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 3e82d65..70c389f 100644 (file)
@@ -1088,16 +1088,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 e9916cc..e316700 100644 (file)
   library setting are updated for greater clarity (LP#1982031)
 * The Cash Reports interface (under Local Administration) is ported to
   Angular.
+* Add patron home library code as a column to the View Holds grid in the staff catalog record details page (LP#1991726)
+* Include template ID in the template table in the Reporter (LP#1998386)
+* Remove the `pub` flag from the `biblio.record_note` table (LP#1978978)
+* Add the publication date to the Staff Catalog's Shelf Browse (LP#1999432)
+* Resolve search performance degradation with PostgreSQL version 12 and up (LP#1999274)
+* Improved styling of paid line items in acquisitions screens (LP#1999270)
+* Improved styling of the keyboard shortcut info modal (LP#1999955)
+* (Developer) Add Emacs mode to `fm_IDL.xml` (LP#1914625)
+* `autogen.sh` can now accept a `-c` switch to specify the location of `opensrf_core.xml`. This is useful for certain multi-tenant setups of Evergreen. (LP#2003707)
+* Better organization of acquisitions line item alert fields (LP#2002977)
+* Prevent templates from applying or changing magical status in angular holdings editor (LP#1999401)
+* 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.
+