Revisiting this part of the code. We still have a problem there. When we cancel an order with deleting the biblio, the constraint on aqorders.biblionumber clears biblionumber in the table while the Koha object still contains the biblionumber. After we called DelBiblio in sub cancel, we should discard changes or clear the biblionumber. This makes the bug in sub store more visible: It should allow a NULL in biblionumber when we are storing a cancellation! So croak only on that condition.
Created attachment 144467 [details] [review] Bug 32417: Add unit test Test plan: Run t/db_dependent/Koha/Acquisition/Order.t This test should fail without the follow-up and pass with it. * not ok 53 - No croak on missing biblionumber when cancelling an order Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Created attachment 144468 [details] [review] Bug 32417: Allow NULL in biblionumber in $order->store If we are cancelling an order and call DelBiblio, we should clear the biblionumber but not be crashing on it in ->store. The absence of the clear prevented the crash. One bug sometimes solves another one. Test plan: Create basket, order line. Delete order with biblio record. Verify result and logfile. Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Created attachment 144514 [details] [review] Bug 32417: Add unit test Test plan: Run t/db_dependent/Koha/Acquisition/Order.t This test should fail without the follow-up and pass with it. * not ok 53 - No croak on missing biblionumber when cancelling an order Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl> Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Created attachment 144515 [details] [review] Bug 32417: Allow NULL in biblionumber in $order->store If we are cancelling an order and call DelBiblio, we should clear the biblionumber but not be crashing on it in ->store. The absence of the clear prevented the crash. One bug sometimes solves another one. Test plan: Create basket, order line. Delete order with biblio record. Verify result and logfile. Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl> Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
I moved too fast, David is about to test and add a signoff stamp... I worked through the code and ran the QA scripts.. so I'll pass QA
Created attachment 144517 [details] [review] Bug 32417: Add unit test Test plan: Run t/db_dependent/Koha/Acquisition/Order.t This test should fail without the follow-up and pass with it. * not ok 53 - No croak on missing biblionumber when cancelling an order Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl> Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Signed-off-by: David Nind <david@davidnind.com>
Created attachment 144518 [details] [review] Bug 32417: Allow NULL in biblionumber in $order->store If we are cancelling an order and call DelBiblio, we should clear the biblionumber but not be crashing on it in ->store. The absence of the clear prevented the crash. One bug sometimes solves another one. Test plan: Create basket, order line. Delete order with biblio record. Verify result and logfile. Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl> Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Signed-off-by: David Nind <david@davidnind.com>
Added my sign off. Testing notes: 1. Before applying the patch: 1.1 Added an order to a basket (from a new (empty) record) 1.2 Deleted the item and then the record created in step 1.1 1.3 Options for cancelling the order are: . Cancel order . Can't cancel order and delete catalog record (NOTE is greyed out) 1.4 Selecting 'Cancel order' results in error trace and can't canel the order: Cannot insert order: Mandatory parameter biblionumber is missing at /kohadevbox/koha/acqui/cancelorder.pl line 67. 2. Applied 'Add unit test' patch, tests fail as per the test plan. 3. Applied the second patch - tests now pass, if you now cancel the order you don't get the error trace and the order is now cancelled.
Pushed to master for 23.05. Nice work everyone, thanks!
Nice work everyone! Pushed to 22.11.x for the next release