Bug 32417 - Cannot insert order: Mandatory parameter biblionumber is missing
Summary: Cannot insert order: Mandatory parameter biblionumber is missing
Status: RESOLVED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: Acquisitions (show other bugs)
Version: master
Hardware: All All
: P5 - low minor (vote)
Assignee: Marcel de Rooy
QA Contact: Testopia
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2022-12-07 13:26 UTC by Marcel de Rooy
Modified: 2023-06-08 22:32 UTC (History)
2 users (show)

See Also:
Change sponsored?: ---
Patch complexity: Small patch
Documentation contact:
Documentation submission:
Text to go in the release notes:
Version(s) released in:
23.05.00,22.11.01


Attachments
Bug 32417: Add unit test (2.20 KB, patch)
2022-12-07 13:55 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 32417: Allow NULL in biblionumber in $order->store (1.53 KB, patch)
2022-12-07 13:55 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 32417: Add unit test (2.23 KB, patch)
2022-12-09 07:22 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 32417: Allow NULL in biblionumber in $order->store (1.88 KB, patch)
2022-12-09 07:22 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 32417: Add unit test (2.27 KB, patch)
2022-12-09 08:30 UTC, David Nind
Details | Diff | Splinter Review
Bug 32417: Allow NULL in biblionumber in $order->store (1.93 KB, patch)
2022-12-09 08:30 UTC, David Nind
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Marcel de Rooy 2022-12-07 13:26:19 UTC
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.
Comment 1 Marcel de Rooy 2022-12-07 13:55:54 UTC
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>
Comment 2 Marcel de Rooy 2022-12-07 13:55:58 UTC
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>
Comment 3 Martin Renvoize 2022-12-09 07:22:11 UTC
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>
Comment 4 Martin Renvoize 2022-12-09 07:22:15 UTC
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>
Comment 5 Martin Renvoize 2022-12-09 07:26:29 UTC
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
Comment 6 David Nind 2022-12-09 08:30:51 UTC
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>
Comment 7 David Nind 2022-12-09 08:30:55 UTC
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>
Comment 8 David Nind 2022-12-09 08:39:06 UTC
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.
Comment 9 Tomás Cohen Arazi 2022-12-13 12:27:44 UTC
Pushed to master for 23.05.

Nice work everyone, thanks!
Comment 10 Martin Renvoize 2022-12-15 13:35:11 UTC
Nice work everyone!

Pushed to 22.11.x for the next release