Bug 19024 - Order cancelled status is reset on basket close
Summary: Order cancelled status is reset on basket close
Status: Pushed to Stable
Alias: None
Product: Koha
Classification: Unclassified
Component: Acquisitions (show other bugs)
Version: master
Hardware: All All
: P5 - low normal (vote)
Assignee: Colin Campbell
QA Contact: Testopia
URL:
Whiteboard:
Keywords:
Depends on: 15036
Blocks:
  Show dependency treegraph
 
Reported: 2017-08-02 13:20 UTC by Colin Campbell
Modified: 2017-10-06 06:08 UTC (History)
5 users (show)

See Also:
Change sponsored?: ---
Patch complexity: ---
Bot Control: ---
When did the bot last check this:
Who signed the patch off:
Text to go in the release notes:


Attachments
Patch to prevent cancelled status from being reset in error (1.33 KB, patch)
2017-08-02 16:29 UTC, Colin Campbell
Details | Diff | Splinter Review
Bug 19024 Do not unset order cancelled status on basket close (1.12 KB, patch)
2017-08-03 02:06 UTC, Dilan Johnpullé
Details | Diff | Splinter Review
Bug 19024: Add tests (1.76 KB, patch)
2017-08-09 19:10 UTC, Jonathan Druart
Details | Diff | Splinter Review
Correct original patch to test status == complete (1.33 KB, patch)
2017-08-15 10:31 UTC, Colin Campbell
Details | Diff | Splinter Review
Improve phrasing of test messages (1.52 KB, patch)
2017-08-15 11:03 UTC, Colin Campbell
Details | Diff | Splinter Review
[SIGNED-OFF] Bug 19024: Add tests (1.82 KB, patch)
2017-08-15 18:58 UTC, M. Tompsett
Details | Diff | Splinter Review
[SIGNED-OFF] Bug 19024 Fix some infelicities of phrasing in test messages (1.58 KB, patch)
2017-08-15 18:58 UTC, M. Tompsett
Details | Diff | Splinter Review
[SIGNED-OFF] Bug 19024 Do not unset order cancelled status on basket close (1.39 KB, patch)
2017-08-15 18:58 UTC, M. Tompsett
Details | Diff | Splinter Review
[SIGNED OFF] Bug 19024: Add tests (1.88 KB, patch)
2017-08-15 19:00 UTC, Katrin Fischer
Details | Diff | Splinter Review
[SIGNED OFF] Bug 19024 Fix some infelicities of phrasing in test messages (1.64 KB, patch)
2017-08-15 19:00 UTC, Katrin Fischer
Details | Diff | Splinter Review
[SIGNED OFF] Bug 19024 Do not unset order cancelled status on basket close (1.45 KB, patch)
2017-08-15 19:00 UTC, Katrin Fischer
Details | Diff | Splinter Review
Bug 19024: Add tests (1.92 KB, patch)
2017-09-01 12:51 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 19024 Fix some infelicities of phrasing in test messages (1.68 KB, patch)
2017-09-01 12:51 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 19024 Do not unset order cancelled status on basket close (1.49 KB, patch)
2017-09-01 12:51 UTC, Kyle M Hall
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Colin Campbell 2017-08-02 13:20:14 UTC
Similar to bug 15036 orderstatus is being set to ordered when a basket is closed including for lines which have been cancelled. In the acquisition screens if you view the basket the lines are clearly cancelled as this uses cancellationdate as the determiner of status. But from the staff title view the acquisitions tab now shows ordered incorrectly.

Simple fix - need to check cancellation_date and cancelled status are not getting out of sync elsewhere
Comment 1 Colin Campbell 2017-08-02 16:29:47 UTC
Created attachment 65434 [details] [review]
Patch to prevent cancelled status from being reset in error

Straightforward test. On a basket cancel an orderline. Search for the title and on the Acquisitions Details tab it shows status as Cancelled.
Close the basket
Search the title again Acquisitions Details now show 'ordered' (in basket display it remains cancelled)
Apply patch and repeat above sequence with another order, this time it should remain as 'Cancelled' after the basket closes.

A special problem is if an order has been placed, and is cancelled because its now out of print you have to reopen the basket to cancel it and on reclosing you undo part of the change
Comment 2 Dilan Johnpullé 2017-08-03 02:06:45 UTC
Created attachment 65439 [details] [review]
Bug 19024 Do not unset order cancelled status on basket close

Followed the test plan and works as described.

Signed-off-by: Dilan Johnpullé <dilan@calyx.net.au>
Comment 3 Jonathan Druart 2017-08-09 19:10:36 UTC
Created attachment 65776 [details] [review]
Bug 19024: Add tests
Comment 4 Katrin Fischer 2017-08-09 20:25:15 UTC
I think there is another bug here. I just tested with received orders, they get reset to ordered too. Actually, should we not just move new > ordered and leave all other status alone?
Comment 5 Katrin Fischer 2017-08-09 20:40:12 UTC
Ok, thinking more about this, I think this is a failed QA:

I propose: When a basket is closed, change new to ordered, leave all other status alone.

The workflow usually is something like:

new > ordered
then either:
> complete
or 
partial > complete

Note: The SQL in the patch has a typo and looks for completeD - reintroducing an already fixed bug by reseting complete to ordered when the basket is closed.


Reasoning: 

New - If the basket is closed, we want to set it to ordered.

Ordered - If a basket is reopened, we can reset this to new. If the basket is closed again, those should be set from new to ordered.

Partial - Partial means a part of the order has already been received. To keep that information, we should probably leave the status alone and not change it when the basket is opened/closed.

Complete - If an item is received already and linked to an invoice, we don't want to reset it to ordered or new on opening or closing the basket. The only way to change the status at this point should be to undo the receive
Comment 6 Colin Campbell 2017-08-15 10:31:24 UTC
Created attachment 66004 [details] [review]
Correct original patch to test status == complete

Fixes error where it was checking completed
Comment 7 Colin Campbell 2017-08-15 11:01:02 UTC
(In reply to Katrin Fischer from comment #5)
> Ok, thinking more about this, I think this is a failed QA:
> 
> I propose: When a basket is closed, change new to ordered, leave all other
> status alone.
> 
> The workflow usually is something like:
> 
> new > ordered
> then either:
> > complete
> or 
> partial > complete
> 
> Note: The SQL in the patch has a typo and looks for completeD -
> reintroducing an already fixed bug by reseting complete to ordered when the
> basket is closed.
> 
> 
> Reasoning: 
> 
> New - If the basket is closed, we want to set it to ordered.
> 
> Ordered - If a basket is reopened, we can reset this to new. If the basket
> is closed again, those should be set from new to ordered.
> 
> Partial - Partial means a part of the order has already been received. To
> keep that information, we should probably leave the status alone and not
> change it when the basket is opened/closed.
> 
> Complete - If an item is received already and linked to an invoice, we don't
> want to reset it to ordered or new on opening or closing the basket. The
> only way to change the status at this point should be to undo the receive

Not sure of the reasoning behind partial. When an ordered is part-received it is split into two the received copies go on a new order marked complete the remainder stay on an order marked partial. partial is a special case of ordered, so I think it should probably continue to be treated like ordered (a reason for reopening the basket may well be to cancel the outstanding partial order)
Comment 8 Colin Campbell 2017-08-15 11:03:29 UTC
Created attachment 66005 [details] [review]
Improve phrasing of test messages

Changed the awkward phrases of the original tests to be more grammatical
Comment 9 Katrin Fischer 2017-08-15 11:04:03 UTC
(In reply to Colin Campbell from comment #7)
> (In reply to Katrin Fischer from comment #5)
> > Ok, thinking more about this, I think this is a failed QA:
> > 
> > I propose: When a basket is closed, change new to ordered, leave all other
> > status alone.
> > 
> > The workflow usually is something like:
> > 
> > new > ordered
> > then either:
> > > complete
> > or 
> > partial > complete
> > 
> > Note: The SQL in the patch has a typo and looks for completeD -
> > reintroducing an already fixed bug by reseting complete to ordered when the
> > basket is closed.
> > 
> > 
> > Reasoning: 
> > 
> > New - If the basket is closed, we want to set it to ordered.
> > 
> > Ordered - If a basket is reopened, we can reset this to new. If the basket
> > is closed again, those should be set from new to ordered.
> > 
> > Partial - Partial means a part of the order has already been received. To
> > keep that information, we should probably leave the status alone and not
> > change it when the basket is opened/closed.
> > 
> > Complete - If an item is received already and linked to an invoice, we don't
> > want to reset it to ordered or new on opening or closing the basket. The
> > only way to change the status at this point should be to undo the receive
> 
> Not sure of the reasoning behind partial. When an ordered is part-received
> it is split into two the received copies go on a new order marked complete
> the remainder stay on an order marked partial. partial is a special case of
> ordered, so I think it should probably continue to be treated like ordered
> (a reason for reopening the basket may well be to cancel the outstanding
> partial order)

It's difficult, I think partial doesn't quite fit into the process. Maybe there should be: partial-new, partial-ordered, partial-cancelled - or partial should be removed as a status. It's still indicated by the existance of the linked parent order I tihnk? At the moment partial is only a special case of ordered, as you said. I am ok with treating it one way or the other (moving it to new on opening or not). Can we set this back to Needs Signoff?
Comment 10 Colin Campbell 2017-08-15 11:05:06 UTC
Fix error in sql of original patch, added patch to enhance the test messages
Comment 11 Katrin Fischer 2017-08-15 13:48:00 UTC
Sorry, but there is still a bug here. If you reopen a basket that already has an order line that is cancelled, it will be reset to new. We can deal with this on a separate bug if you want or can fix both together, as the test plan is really similar.
Comment 12 M. Tompsett 2017-08-15 18:56:41 UTC
(In reply to Katrin Fischer from comment #11)
> Sorry, but there is still a bug here. If you reopen a basket that already
> has an order line that is cancelled, it will be reset to new. We can deal
> with this on a separate bug if you want or can fix both together, as the
> test plan is really similar.

I suspect where the fix is done is slightly different, let's move forward on what is currently working here. :)
Comment 13 M. Tompsett 2017-08-15 18:58:22 UTC
Created attachment 66039 [details] [review]
[SIGNED-OFF] Bug 19024: Add tests

Signed-off-by: Mark Tompsett <mtompset@hotmail.com>
Comment 14 M. Tompsett 2017-08-15 18:58:25 UTC
Created attachment 66040 [details] [review]
[SIGNED-OFF] Bug 19024 Fix some infelicities of phrasing in test messages

The test messages were awkwardly phrased, re phrase them to
sound more natuaral. Patch is cosmetic (grammar) only

Signed-off-by: Mark Tompsett <mtompset@hotmail.com>
Comment 15 M. Tompsett 2017-08-15 18:58:28 UTC
Created attachment 66041 [details] [review]
[SIGNED-OFF] Bug 19024 Do not unset order cancelled status on basket close

On closing a basket, status is updated to ordered for orders not
completed. However the operation  was resetting the status for
cancelled as well as new orders.
While display is correct from the basket view (it checks the
cancellation date). The status in the acquisitions tab from the
catalogue view reverts erroneously to ordered.

This patch adds cancelled to the statuses not updated on basket
close.

Signed-off-by: Mark Tompsett <mtompset@hotmail.com>
Comment 16 Katrin Fischer 2017-08-15 19:00:33 UTC
Created attachment 66042 [details] [review]
[SIGNED OFF] Bug 19024: Add tests

Signed-off-by: Mark Tompsett <mtompset@hotmail.com>

Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Comment 17 Katrin Fischer 2017-08-15 19:00:37 UTC
Created attachment 66043 [details] [review]
[SIGNED OFF] Bug 19024 Fix some infelicities of phrasing in test messages

The test messages were awkwardly phrased, re phrase them to
sound more natuaral. Patch is cosmetic (grammar) only

Signed-off-by: Mark Tompsett <mtompset@hotmail.com>

Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Comment 18 Katrin Fischer 2017-08-15 19:00:40 UTC
Created attachment 66044 [details] [review]
[SIGNED OFF] Bug 19024 Do not unset order cancelled status on basket close

On closing a basket, status is updated to ordered for orders not
completed. However the operation  was resetting the status for
cancelled as well as new orders.
While display is correct from the basket view (it checks the
cancellation date). The status in the acquisitions tab from the
catalogue view reverts erroneously to ordered.

This patch adds cancelled to the statuses not updated on basket
close.

Signed-off-by: Mark Tompsett <mtompset@hotmail.com>

Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Comment 19 Katrin Fischer 2017-08-15 19:08:23 UTC
I have filed bug 19120 for the issue of resetting the canceled status on opening a basket.
Comment 20 Kyle M Hall 2017-09-01 12:51:20 UTC
Created attachment 66725 [details] [review]
Bug 19024: Add tests

Signed-off-by: Mark Tompsett <mtompset@hotmail.com>

Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Comment 21 Kyle M Hall 2017-09-01 12:51:27 UTC
Created attachment 66726 [details] [review]
Bug 19024 Fix some infelicities of phrasing in test messages

The test messages were awkwardly phrased, re phrase them to
sound more natuaral. Patch is cosmetic (grammar) only

Signed-off-by: Mark Tompsett <mtompset@hotmail.com>

Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Comment 22 Kyle M Hall 2017-09-01 12:51:30 UTC
Created attachment 66727 [details] [review]
Bug 19024 Do not unset order cancelled status on basket close

On closing a basket, status is updated to ordered for orders not
completed. However the operation  was resetting the status for
cancelled as well as new orders.
While display is correct from the basket view (it checks the
cancellation date). The status in the acquisitions tab from the
catalogue view reverts erroneously to ordered.

This patch adds cancelled to the statuses not updated on basket
close.

Signed-off-by: Mark Tompsett <mtompset@hotmail.com>

Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Comment 23 Jonathan Druart 2017-09-01 16:07:28 UTC
Pushed to master for 17.11, thanks to everybody involved!
Comment 24 Fridolin SOMERS 2017-10-03 09:18:28 UTC
Pushed to 17.05.x, will be in 17.05.05.
Comment 25 Katrin Fischer 2017-10-06 06:08:20 UTC
These patches have been pushed to 16.11.x and will be in 16.11.13.