From cf88a1bfba5d9476388cea8d763b96455a0d3bb6 Mon Sep 17 00:00:00 2001 From: Jason Stephenson Date: Sat, 27 Oct 2018 15:48:54 -0400 Subject: [PATCH] LP 1779467: Enhance Mark Items Functionality A new option to "Mark Item as Discard/Weed" is added to many actions menus in the staff client. This command is connected to the back end function open-ils.circ.mark_item_discard. The back end functionality for the open-ils.circ.mark_item_* family of functions is altered to provide more consistent behavior and to avoid some strange situations that have come up in the past, such as items with the Missing status having active transits or open circulations. The code for "Mark Item as Damaged" and "Mark Item as Missing" are altered to take advantage of the back end changes. NB: These changes do not affect the "Mark Item as Missing Pieces" function, as that is handled by different back end code. Perl live tests are added for the backend functionality changes to test that certain conditions works. Like most of our tests these could be expanded to cover more potential situations. See the release notes for more detail on changes in functionality. Signed-off-by: Jason Stephenson Signed-off-by: Kathy Lussier Signed-off-by: Terran McCanna Signed-off-by: Chris Sharp --- Open-ILS/src/extras/ils_events.xml | 12 + .../perlmods/lib/OpenILS/Application/Circ.pm | 95 +++++-- .../live_t/zz-lp1779467-mark-item-discard.t | 238 ++++++++++++++++++ .../staff/cat/catalog/t_holdings.tt2 | 2 + .../templates/staff/cat/catalog/t_holds.tt2 | 2 + .../src/templates/staff/cat/item/index.tt2 | 1 + .../src/templates/staff/cat/item/t_list.tt2 | 2 + .../staff/circ/checkin/t_checkin_table.tt2 | 4 + .../staff/circ/holds/t_pull_list.tt2 | 2 + .../staff/circ/holds/t_shelf_list.tt2 | 2 + .../staff/circ/patron/t_holds_list.tt2 | 2 + .../templates/staff/circ/renew/t_renew.tt2 | 4 + .../staff/circ/share/circ_strings.tt2 | 9 + .../js/ui/default/staff/cat/catalog/app.js | 16 +- .../web/js/ui/default/staff/cat/item/app.js | 13 +- .../js/ui/default/staff/circ/checkin/app.js | 14 ++ .../web/js/ui/default/staff/circ/renew/app.js | 13 + .../js/ui/default/staff/circ/services/circ.js | 182 +++++++++++--- .../ui/default/staff/circ/services/holds.js | 36 ++- .../js/ui/default/staff/circ/services/item.js | 8 +- .../enhanced-mark-item-functionality.adoc | 112 +++++++++ 21 files changed, 712 insertions(+), 57 deletions(-) create mode 100644 Open-ILS/src/perlmods/live_t/zz-lp1779467-mark-item-discard.t create mode 100644 docs/RELEASE_NOTES_NEXT/Circulation/enhanced-mark-item-functionality.adoc diff --git a/Open-ILS/src/extras/ils_events.xml b/Open-ILS/src/extras/ils_events.xml index 9f92944cc5..362fff563d 100644 --- a/Open-ILS/src/extras/ils_events.xml +++ b/Open-ILS/src/extras/ils_events.xml @@ -1060,6 +1060,18 @@ The prediction pattern still has dependent objects + + + + + The item to be marked is checked out to a patron. + + + The item to be marked is in transit. + + + The item to be marked is the last possible target for a hold. + diff --git a/Open-ILS/src/perlmods/lib/OpenILS/Application/Circ.pm b/Open-ILS/src/perlmods/lib/OpenILS/Application/Circ.pm index bad3be3819..f3cec222d0 100644 --- a/Open-ILS/src/perlmods/lib/OpenILS/Application/Circ.pm +++ b/Open-ILS/src/perlmods/lib/OpenILS/Application/Circ.pm @@ -1307,31 +1307,24 @@ sub mark_item { my( $self, $conn, $auth, $copy_id, $args ) = @_; $args ||= {}; - # Items must be checked in before any attempt is made to mark damaged - my $evt = try_checkin($auth, $copy_id) if - ($self->api_name=~ /damaged/ && $args->{handle_checkin}); - return $evt if $evt; - - my $e = new_editor(authtoken=>$auth, xact =>1); + my $e = new_editor(authtoken=>$auth); return $e->die_event unless $e->checkauth; my $copy = $e->retrieve_asset_copy([ $copy_id, - {flesh => 1, flesh_fields => {'acp' => ['call_number']}}]) + {flesh => 1, flesh_fields => {'acp' => ['call_number','status']}}]) or return $e->die_event; - my $owning_lib = + my $owning_lib = ($copy->call_number->id == OILS_PRECAT_CALL_NUMBER) ? $copy->circ_lib : $copy->call_number->owning_lib; + my $evt; # For later. my $perm = 'MARK_ITEM_MISSING'; my $stat = OILS_COPY_STATUS_MISSING; if( $self->api_name =~ /damaged/ ) { $perm = 'MARK_ITEM_DAMAGED'; $stat = OILS_COPY_STATUS_DAMAGED; - my $evt = handle_mark_damaged($e, $copy, $owning_lib, $args); - return $evt if $evt; - } elsif ( $self->api_name =~ /bindery/ ) { $perm = 'MARK_ITEM_BINDERY'; $stat = OILS_COPY_STATUS_BINDERY; @@ -1355,20 +1348,73 @@ sub mark_item { # caller may proceed if either perm is allowed return $e->die_event unless $e->allowed([$perm, 'UPDATE_COPY'], $owning_lib); - $copy->status($stat); - $copy->edit_date('now'); - $copy->editor($e->requestor->id); - - $e->update_asset_copy($copy) or return $e->die_event; + # Copy status checks. + if ($copy->status->id() == OILS_COPY_STATUS_CHECKED_OUT) { + # Items must be checked in before any attempt is made to change its status. + if ($args->{handle_checkin}) { + $evt = try_checkin($auth, $copy_id); + } else { + $evt = OpenILS::Event->new('ITEM_TO_MARK_CHECKED_OUT'); + } + } elsif ($copy->status->id() == OILS_COPY_STATUS_IN_TRANSIT) { + # Items in transit need to have the transit aborted before being marked. + if ($args->{handle_transit}) { + $evt = try_abort_transit($auth, $copy_id); + } else { + $evt = OpenILS::Event->new('ITEM_TO_MARK_IN_TRANSIT'); + } + } elsif ($U->is_true($copy->status->restrict_copy_delete()) && $self->api_name =~ /discard/) { + # Items with restrict_copy_delete status require the + # COPY_DELETE_WARNING.override permission to be marked for + # discard. + if ($args->{handle_copy_delete_warning}) { + $evt = $e->event unless $e->allowed(['COPY_DELETE_WARNING.override'], $owning_lib); + } else { + $evt = OpenILS::Event->new('COPY_DELETE_WARNING'); + } + } + return $evt if $evt; + # Retrieving holds for later use. my $holds = $e->search_action_hold_request( - { + { current_copy => $copy->id, fulfillment_time => undef, cancel_time => undef, - } + }, + {flesh=>1, flesh_fields=>{ahr=>['eligible_copies']}} ); + # Throw event if attempting to mark discard the only copy to fill a hold. + if ($self->api_name =~ /discard/) { + if (!$args->{handle_last_hold_copy}) { + for my $hold (@$holds) { + my $eligible = $hold->eligible_copies(); + if (scalar(@{$eligible}) < 2) { + $evt = OpenILS::Event->new('ITEM_TO_MARK_LAST_HOLD_COPY'); + last; + } + } + } + } + return $evt if $evt; + + # Things below here require a transaction and there is nothing left to interfere with it. + $e->xact_begin; + + # Handle extra mark damaged charges, etc. + if ($self->api_name =~ /damaged/) { + $evt = handle_mark_damaged($e, $copy, $owning_lib, $args); + return $evt if $evt; + } + + # Mark the copy. + $copy->status($stat); + $copy->edit_date('now'); + $copy->editor($e->requestor->id); + + $e->update_asset_copy($copy) or return $e->die_event; + $e->commit; if( $self->api_name =~ /damaged/ ) { @@ -1408,6 +1454,19 @@ sub try_checkin { } } +sub try_abort_transit { + my ($auth, $copy_id) = @_; + + my $abort = $U->simplereq( + 'open-ils.circ', + 'open-ils.circ.transit.abort', + $auth, {copyid => $copy_id} + ); + # Above returns 1 or an event. + return $abort if (ref $abort); + return undef; +} + sub handle_mark_damaged { my($e, $copy, $owning_lib, $args) = @_; diff --git a/Open-ILS/src/perlmods/live_t/zz-lp1779467-mark-item-discard.t b/Open-ILS/src/perlmods/live_t/zz-lp1779467-mark-item-discard.t new file mode 100644 index 0000000000..08f99b1c3d --- /dev/null +++ b/Open-ILS/src/perlmods/live_t/zz-lp1779467-mark-item-discard.t @@ -0,0 +1,238 @@ +#!perl +use strict; use warnings; +use Test::More tests => 17; +use OpenILS::Utils::TestUtils; +use OpenILS::Const qw(:const); + +my $script = OpenILS::Utils::TestUtils->new(); +my $U = 'OpenILS::Application::AppUtils'; + +diag("Test LP 1779467 Enhance Mark Item Discard/Weed."); + +use constant { + BR1_ID => 4, + BR3_ID => 6, + WORKSTATION => 'BR1-lp1779467-test-mark-item-discard' +}; + +# 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' +); + +# Mark it discard/weed. +my $result = $U->simplereq( + 'open-ils.circ', + 'open-ils.circ.mark_item_discard', + $authtoken, + $copy->id() +); +is( + $result, + 1, + 'Mark available copy Discard/Weed' +); + +# Check its status. +$copy = $U->simplereq( + 'open-ils.pcrud', + 'open-ils.pcrud.retrieve.acp', + $authtoken, + $copy->id() +); +is( + $copy->status(), + OILS_COPY_STATUS_DISCARD, + 'Copy has Discard/Weed status' +); + +# Find available copy at BR3. +$acps = $U->simplereq( + 'open-ils.pcrud', + 'open-ils.pcrud.search.acp.atomic', + $authtoken, + {circ_lib => BR3_ID, status => OILS_COPY_STATUS_AVAILABLE}, + {limit => 1} +); +$copy = $acps->[0]; +isa_ok( + ref $copy, + 'Fieldmapper::asset::copy', + 'Got available copy from BR3' +); + +# Attempt to mark it discard/weed. +# Should fail with a perm error. +$result = $U->simplereq( + 'open-ils.circ', + 'open-ils.circ.mark_item_discard', + $authtoken, + $copy->id() +); +is( + $result->{textcode}, + 'PERM_FAILURE', + 'Mark BR3 copy Discard/Weed' +); + +# Find checked out copy at BR1. +$acps = $U->simplereq( + 'open-ils.pcrud', + 'open-ils.pcrud.search.acp.atomic', + $authtoken, + {circ_lib => BR1_ID, status => OILS_COPY_STATUS_CHECKED_OUT}, + {limit => 1} +); +$copy = $acps->[0]; +isa_ok( + ref $copy, + 'Fieldmapper::asset::copy', + 'Got checked out copy from BR1' +); + +# Mark it discard/weed with handle_checkin: 1. +$result = $U->simplereq( + 'open-ils.circ', + 'open-ils.circ.mark_item_discard', + $authtoken, + $copy->id(), + {handle_checkin => 1} +); +ok( + $result == 1, + 'Mark checked out item discard' +); + +# Check its status. +$copy = $U->simplereq( + 'open-ils.pcrud', + 'open-ils.pcrud.retrieve.acp', + $authtoken, + $copy->id() +); +is( + $copy->status(), + OILS_COPY_STATUS_DISCARD, + 'Checked out copy has Discard/Weed status' +); + +# Check that it is no longer checked out. +my $circ = $U->simplereq( + 'open-ils.pcrud', + 'open-ils.pcrud.search.circ', + $authtoken, + {target_copy => $copy->id(), checkin_time => undef} +); +ok( + ! defined $circ, + 'No circulation for marked copy' +); + +# Find another checked out copy at BR1. +$acps = $U->simplereq( + 'open-ils.pcrud', + 'open-ils.pcrud.search.acp.atomic', + $authtoken, + {circ_lib => BR1_ID, status => OILS_COPY_STATUS_CHECKED_OUT}, + {limit => 1} +); +$copy = $acps->[0]; +isa_ok( + ref $copy, + 'Fieldmapper::asset::copy', + 'Got another checked out copy from BR1' +); + +# Mark it discard/weed with handle_checkin: 0. +$result = $U->simplereq( + 'open-ils.circ', + 'open-ils.circ.mark_item_discard', + $authtoken, + $copy->id(), + {handle_checkin => 0} +); +# Check that we got the appropriate event: ITEM_TO_MARK_CHECKED_OUT +is( + $result->{textcode}, + 'ITEM_TO_MARK_CHECKED_OUT', + 'Mark second checked out item discard' +); + +# Check its status. +$copy = $U->simplereq( + 'open-ils.pcrud', + 'open-ils.pcrud.retrieve.acp', + $authtoken, + $copy->id() +); +is( + $copy->status(), + OILS_COPY_STATUS_CHECKED_OUT, + 'Second checked out copy has Checked Out status' +); + +# Check that it is still checked out. +$circ = $U->simplereq( + 'open-ils.pcrud', + 'open-ils.pcrud.search.circ', + $authtoken, + {target_copy => $copy->id(), checkin_time => undef} +); +isa_ok( + $circ, + 'Fieldmapper::action::circulation', + 'Second copy still has a circulation' +); + +# We could add more tests for other conditions, i.e. a copy in transit +# and for marking other statuses. + +# Logout +$script->logout(); # Not a test, just to be pedantic. diff --git a/Open-ILS/src/templates/staff/cat/catalog/t_holdings.tt2 b/Open-ILS/src/templates/staff/cat/catalog/t_holdings.tt2 index b694d6bc6c..d30ad1ab67 100644 --- a/Open-ILS/src/templates/staff/cat/catalog/t_holdings.tt2 +++ b/Open-ILS/src/templates/staff/cat/catalog/t_holdings.tt2 @@ -62,6 +62,8 @@ + + [% l('Mark') %]

  • [% l('Item as Damaged') %]
    • +
  • [% l('Item as Discard/Weed') %]
  • [% l('Item as Missing') %]

    • [% l('Add') %]

    diff --git a/Open-ILS/src/templates/staff/cat/item/t_list.tt2 b/Open-ILS/src/templates/staff/cat/item/t_list.tt2 index c0ac0c25d5..024271e767 100644 --- a/Open-ILS/src/templates/staff/cat/item/t_list.tt2 +++ b/Open-ILS/src/templates/staff/cat/item/t_list.tt2 @@ -47,6 +47,8 @@ + diff --git a/Open-ILS/src/templates/staff/circ/checkin/t_checkin_table.tt2 b/Open-ILS/src/templates/staff/circ/checkin/t_checkin_table.tt2 index eff1448663..85fd44ec97 100644 --- a/Open-ILS/src/templates/staff/circ/checkin/t_checkin_table.tt2 +++ b/Open-ILS/src/templates/staff/circ/checkin/t_checkin_table.tt2 @@ -23,6 +23,10 @@ handler="showMarkDamaged" label="[% l('Mark Items Damaged') %]">     + + diff --git a/Open-ILS/src/templates/staff/circ/holds/t_pull_list.tt2 b/Open-ILS/src/templates/staff/circ/holds/t_pull_list.tt2 index 4bd3762cab..2348f992cd 100644 --- a/Open-ILS/src/templates/staff/circ/holds/t_pull_list.tt2 +++ b/Open-ILS/src/templates/staff/circ/holds/t_pull_list.tt2 @@ -45,6 +45,8 @@ label="[% l('Transfer To Marked Title') %]"> + diff --git a/Open-ILS/src/templates/staff/circ/holds/t_shelf_list.tt2 b/Open-ILS/src/templates/staff/circ/holds/t_shelf_list.tt2 index 083edec8be..c385a6f8a2 100644 --- a/Open-ILS/src/templates/staff/circ/holds/t_shelf_list.tt2 +++ b/Open-ILS/src/templates/staff/circ/holds/t_shelf_list.tt2 @@ -48,6 +48,8 @@ label="[% l('Transfer To Marked Title') %]"> + diff --git a/Open-ILS/src/templates/staff/circ/patron/t_holds_list.tt2 b/Open-ILS/src/templates/staff/circ/patron/t_holds_list.tt2 index 14a6569653..48700fcc22 100644 --- a/Open-ILS/src/templates/staff/circ/patron/t_holds_list.tt2 +++ b/Open-ILS/src/templates/staff/circ/patron/t_holds_list.tt2 @@ -35,6 +35,8 @@ label="[% l('Transfer To Marked Title') %]"> + diff --git a/Open-ILS/src/templates/staff/circ/renew/t_renew.tt2 b/Open-ILS/src/templates/staff/circ/renew/t_renew.tt2 index 886e93fdbb..209570973d 100644 --- a/Open-ILS/src/templates/staff/circ/renew/t_renew.tt2 +++ b/Open-ILS/src/templates/staff/circ/renew/t_renew.tt2 @@ -61,6 +61,10 @@ handler="showMarkDamaged" label="[% l('Mark Items Damaged') %]"> + +