Acq: lineitems display as "on order" even after all copies have been cancelled

Bug #978095 reported by Kathy Lussier on 2012-04-10
56
This bug affects 11 people
Affects Status Importance Assigned to Milestone
Evergreen
Medium
Unassigned
3.0
Medium
Unassigned
3.1
Undecided
Unassigned

Bug Description

In the PO interface, you can either choose to cancel a selected lineitem or you can choose to cancel individual copies belonging to that lineitem. If all individual copies belonging to the lineitem are set to canceled, the lineitem status on the main PO screen still display as "on order". I have attached a screenshot that shows the copies screen where all copies are set to cancelled followed by the PO screen where the status still displays as on order.

This behavior differs from what happens if individual copies are set to received. Once all copies are set to received, the lineitem status on the main PO screen also updates to received.

This issue is particularly problematic when using EDI. If a vendor posts an EDI response message saying that an order is cancelled, it updates the status at the copy level. However, it would take some digging for staff to see that these copies were cancelled since the status on the PO screen remains as "on order."

Revision history for this message
Kathy Lussier (klussier) wrote :
Revision history for this message
Bill Erickson (berick) wrote :

This issue is resolved with bug #1065270 -- which should be merged soon, after we hammer out the remaining edge cases. Marking as duplicate.

-----
commit a22546583284a9b9f67fc53b79220ca6bf6ddb56
Author: Bill Erickson <email address hidden>
Date: Tue Dec 4 10:00:26 2012 -0500

    EDI: ensure lineitem 'state' matches cancel state

    When cancelling a lineitem becuase all linked copies are cancelled,
    ensure that the lineitem state is set to "cancelled".

Revision history for this message
Kathy Lussier (klussier) wrote :

I just tested cancellations in master. When manually setting copies to cancelled, the lineitem still displays as "on order." However, I'm fairly sure EDI is now marking the entire lineitem as cancelled. Perhaps 1065270 fixed this issue with EDI cancellations, but not manual ones?

In any case, I'm removing the duplicate status.

Ben Shum (bshum) on 2013-02-04
Changed in evergreen:
milestone: none → 2.4.0-alpha
status: New → Triaged
importance: Undecided → Medium
Ben Shum (bshum) on 2013-03-03
Changed in evergreen:
milestone: 2.4.0-alpha1 → 2.4.0-beta
Ben Shum (bshum) on 2013-03-17
Changed in evergreen:
milestone: 2.4.0-beta → 2.4.0-rc
Ben Shum (bshum) on 2013-04-27
Changed in evergreen:
milestone: 2.4.0-rc → none
Revision history for this message
Jennifer Pringle (jpringle-u) wrote :

Confirmed that this bug is still present on 2.4.0.

Revision history for this message
Kathy Lussier (klussier) wrote :

From a discussion in IRC on 7/10/2013

# 09:55:51 berick kmlussier: i assume the LI would just adopt the cancel_reason of the last-canceled item?
# 09:56:23 berick even though the copies themselves may have a variety of cancel reasons..
# 09:57:48 berick hm, or do we need a new cancel reason, "OMG, All My Copies Are Gone!?!"
# 09:58:24 kmlussier berick: Just checked with my acq person. Could it list all the cancel reasons?
# 10:00:13 berick kmlussier: so, my main question is, we need to cancel the lineitem too, and a lineitem can only have 1 cancel reason
# 10:00:52 berick there's no reason the UI can't show all of the copy cancel reasons, but that's a separate question, i think
# 10:55:09 kmlussier berick: In lieu of a new LI cancel reason of "multiple" (or, as you suggest, OMG, All My Copies Are Gone!?!), I think using the reason from the last-canceled copy makes sense. In most cases, I imagine the cancel reasons would be the same.

Changed in evergreen:
status: Triaged → Confirmed
Revision history for this message
Leslie St. John (lstjohn-deactivatedaccount) wrote :

still present in 2.7.1. Manual cancellation of all copies on a LI does not change LI state. One result - can not search on LI state with any accuracy.

Liam Whalen (whalen-ld) on 2015-11-05
Changed in evergreen:
assignee: nobody → Liam Whalen (whalen-ld)
no longer affects: evergreen/2.3
no longer affects: evergreen/2.4
no longer affects: evergreen/2.8
no longer affects: evergreen/2.9
Revision history for this message
Galen Charlton (gmc) wrote :

Just to confirm, am I correct in assuming that cancelling all of the line item copies should result in doing the full cancellation on the line item, including cancelling any hold requests?

Revision history for this message
Galen Charlton (gmc) wrote :

Following up on my comment #7, I note that it appears that if an EDI cancellation cancels the last line item copy, then therefore marks the line item as cancelled... any hold requests on the linked bib get orphaned.

Revision history for this message
Jason Etheridge (phasefx) wrote :

If I wanted to move forward with this, would it be sane to modify sub cancel_lineitem_detail in Acq/Order.pm, right before the final return, so that it examines the cancel_reason for all sibling LIDs, and if they're all cancelled (and cancelled with a reason where keep_debits = false?), then invoke sub cancel_lineitem (ignoring whether it succeeds or not) with the same cancel_reason being fed to cancel_lineitem_detail? And then maybe double-check whether the UI needs a tickle or not to refresh the lineitem state? Then any question on canceling holds could be punted to another bug if needed for fixing sub cancel_lineitem?

Revision history for this message
Mike Rylander (mrylander) wrote :

Jason, the approach sound sane to me. As for whether to cancel if some LIDs have keep_debits=true, I think we need some clarity from someone that has a better memory than me on the exact semantics of that field. To wit:

Bill, should the LI cancel if there are canceled keep_debits=true LIDS?

For the UI tickle, perhaps we could add a key to the hash returned by cancel_lineitem_detail that lists the IDs of the remaining uncanceled LIDs. When the list is 0 long the UI could refresh (maybe re-fetch the LI and replace the object)?

Thanks!

Revision history for this message
Jason Etheridge (phasefx) wrote :

So one suggestion I received (and apologies if this is spelled out already in the ticket) was cancel the lineitem if all the LIDs are cancelled, but use the last cancel reason where keep_debits=true if there is one, otherwise, use the cancel_reason just used for the LID.

Revision history for this message
Jason Etheridge (phasefx) wrote :

I have a branch for this here:

collab/phasefx/lp978095-autocanceling-lineitems @ working

http://git.evergreen-ils.org/?p=working/Evergreen.git;a=shortlog;h=refs/heads/collab/phasefx/lp978095-autocanceling-lineitems

tags: added: pullrequest
Revision history for this message
Jason Etheridge (phasefx) wrote :

For testing, this is what I did with a fresh concerto dataset:

Web Client -> Acquisitions -> Purchase Orders -> Search -> 2 -> Copies(2) on lineitem #3
on copy ACQ0001, cancel, Delayed: Backordered, Cancel Copy, OK
on copy ACQ0002, cancel, Canceled: By Vendor, Cancel Copy, OK
the UI should refresh and show lineitem #3 as Backordered

For refreshing the data for iterative testing, I used the following SQL in the database:

BEGIN;

TRUNCATE acq.fund CASCADE;
TRUNCATE acq.fund_allocation CASCADE;
TRUNCATE acq.fund_debit CASCADE;
TRUNCATE acq.funding_source CASCADE;
TRUNCATE acq.funding_source_credit CASCADE;
TRUNCATE acq.lineitem CASCADE;
TRUNCATE acq.lineitem_detail CASCADE;
TRUNCATE acq.provider CASCADE;
TRUNCATE acq.provider_holding_subfield_map CASCADE;
TRUNCATE acq.purchase_order CASCADE;

\i ~/git/Evergreen/Open-ILS/tests/datasets/sql/acq.sql

COMMIT;

Revision history for this message
Jason Etheridge (phasefx) wrote :

random aside, lineitems #4 and #5 are already canceled in that dataset, but their copies were canceled with different cancel reasons :)

no longer affects: evergreen/2.11
no longer affects: evergreen/2.10
Revision history for this message
Andrea Neiman (aneiman) wrote :

Patch is working for me in light testing, though I'd appreciate someone more knowledgeable about Acq taking a look for signoff purposes.

no longer affects: evergreen/master
Changed in evergreen:
milestone: 3.1-beta → 3.1-rc
Revision history for this message
Bill Erickson (berick) wrote :

Hey Jason, I'm curious about the choice to handle the lineitem cancellation via a secondary transaction. Any reason not to tack it on to the existing transaction?

Changed in evergreen:
milestone: 3.1-rc → 3.1.1
Revision history for this message
Jason Etheridge (phasefx) wrote :

Oops, sorry Bill, I missed your comment. I think my mindset was: don't want some hypothetical failure at canceling the lineitem to negate the attempt to cancel the lineitem detail.

Revision history for this message
Jason Etheridge (phasefx) wrote :

In other words, I was thinking of auto-canceling the lineitem as a convenience thing, not an all or nothing behavior to be bundled with the more specific cancelation. But I could be argued into a different view; don't feel strongly about it.

Revision history for this message
Bill Erickson (berick) wrote :

Thanks, Jason. I would argue it's not a convenience thing, but how the application should function (or die loudly trying). I think it should be in the same transaction.

Changed in evergreen:
milestone: 3.1.1 → 3.1.2
Changed in evergreen:
milestone: 3.1.2 → 3.1.3
Changed in evergreen:
milestone: 3.1.3 → 3.1.4
Revision history for this message
Jason Etheridge (phasefx) wrote :

Just for reference, I noticed two other branches in the working repo that identify this bug:
  working/user/csharp/lp978095_acq_lineitems_with_canceled_copies
  working/user/ldw/lp978095_cancel_lineitem_no_copies

Revision history for this message
Jason Etheridge (phasefx) wrote :

I changed this to use just one transaction, rebased it against master, and force pushed a new branch here:
collab/phasefx/lp978095-autocanceling-lineitems

Changed in evergreen:
milestone: 3.1.4 → 3.1.5
Bill Erickson (berick) on 2018-07-27
Changed in evergreen:
assignee: nobody → Bill Erickson (berick)
Revision history for this message
Bill Erickson (berick) wrote :

Thanks, Jason! I've pushed a follow up branch which includes sign-off for your commit, plus another commit. The additional commit ensures that the cancel reason for the currently-canceled copy is used if it's a keep-debits reason, instead of choosing one essentially at random from the keep-debits true copies. Updated the docs to clarify. Removed the eval, since it won't save us from a failed transaction (and could mask a useful error message).

http://git.evergreen-ils.org/?p=working/Evergreen.git;a=shortlog;h=refs/heads/user/berick/lp978095-auto-cancel-lineitem-signoff

Revision history for this message
Mike Rylander (mrylander) wrote :

Picked to master through rel_3_0. Thanks, Jason and Bill!

Changed in evergreen:
assignee: Bill Erickson (berick) → Mike Rylander (mrylander)
status: Confirmed → Fix Committed
assignee: Mike Rylander (mrylander) → nobody
Dan Wells (dbw2) on 2018-08-30
Changed in evergreen:
milestone: 3.1.5 → 3.2-beta
no longer affects: evergreen/2.12
Changed in evergreen:
status: Fix Committed → Fix Released
To post a comment you must log in.
This report contains Public information  Edit
Everyone can see this information.

Other bug subscribers