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
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
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>
Created attachment 65776 [details] [review] Bug 19024: Add tests
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?
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
Created attachment 66004 [details] [review] Correct original patch to test status == complete Fixes error where it was checking completed
(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)
Created attachment 66005 [details] [review] Improve phrasing of test messages Changed the awkward phrases of the original tests to be more grammatical
(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?
Fix error in sql of original patch, added patch to enhance the test messages
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.
(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. :)
Created attachment 66039 [details] [review] [SIGNED-OFF] Bug 19024: Add tests Signed-off-by: Mark Tompsett <mtompset@hotmail.com>
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>
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>
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>
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>
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>
I have filed bug 19120 for the issue of resetting the canceled status on opening a basket.
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>
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>
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>
Pushed to master for 17.11, thanks to everybody involved!
Pushed to 17.05.x, will be in 17.05.05.
These patches have been pushed to 16.11.x and will be in 16.11.13.
Pushed to 16.05.x, for 16.05.18 release