Bug 31303 - Fatal error when viewing OPAC user account with waiting holds
Summary: Fatal error when viewing OPAC user account with waiting holds
Status: Pushed to master
Alias: None
Product: Koha
Classification: Unclassified
Component: OPAC (show other bugs)
Version: master
Hardware: All All
: P5 - low critical (vote)
Assignee: David Cook
QA Contact: Testopia
URL:
Keywords:
Depends on: 22456
Blocks:
  Show dependency treegraph
 
Reported: 2022-08-08 04:47 UTC by David Cook
Modified: 2022-08-09 00:51 UTC (History)
1 user (show)

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


Attachments
Bug 31303: Prevent fatal errors when missing "waiting hold cancellation" rule (1.84 KB, patch)
2022-08-08 05:09 UTC, David Cook
Details | Diff | Splinter Review
Bug 31303: Prevent fatal errors when missing "waiting hold cancellation" rule (1.89 KB, patch)
2022-08-08 15:11 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 31303: (follow-up) Use get_effective_rule_value instead (1.03 KB, patch)
2022-08-08 15:11 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description David Cook 2022-08-08 04:47:07 UTC
Bug 22456 added a "waiting_hold_cancellation" rule but it doesn't look it's being installed correctly, which is causing fatal errors when you view an OPAC user account with waiting holds...
Comment 1 David Cook 2022-08-08 04:56:26 UTC
I don't know how the MySQL database is being set up in koha-testing-docker, but if "installer/data/mysql/db_revs/220600023.pl" is involved, it's clearly not working. 

While I can see the "hold_cancellation_requests" table in the database, the "waiting_hold_cancellation" circulation_rules entry is not getting created.
Comment 2 David Cook 2022-08-08 05:02:13 UTC
I reckon koha-testing-docker is created from kohastructure.sql and then uses the koha-testing-docker scripts to populate from there.

So really... this just means that any new Koha instance won't work because it's missing this circulation rule. 

Koha shouldn't need this circulation rule to be set, to the problem is with Koha::Hold::cancellation_requestable_from_opac which uses method chaining when it shouldn't. 

I'd say the fix here is probably to just remove the method chaining from Koha::Hold::cancellation_requestable_from_opac. Should be easy peasy lemon squeezy.
Comment 3 David Cook 2022-08-08 05:09:32 UTC
Created attachment 138771 [details] [review]
Bug 31303: Prevent fatal errors when missing "waiting hold cancellation" rule

This patch removes method chaining which didn't account for the fact that
the "waiting hold cancellation" circulation rule might not exist.

Test plan:
0. Apply patch and koha-plack --restart kohadev
1. Go to http://localhost:8081/cgi-bin/koha/reserve/request.pl?biblionumber=29
2. Search for "koha" user
3. Choose the specific item and click "Place hold"
4. Check in "39999000001310" and click "Confirm hold (Y)"
5. Go to http://localhost:8080/cgi-bin/koha/opac-user.pl#opac-user-holds
6. Rejoice in Koha not exploding
7. Go to http://localhost:8081/cgi-bin/koha/admin/smart-rules.pl
8. Go to "Default waiting hold cancellation policy" and change "Cancellation allowed" to "Yes" and click "Add"
9. Go to http://localhost:8080/cgi-bin/koha/opac-user.pl#opac-user-holds
10. Note the "Cancel" button appears
11. Rejoice again that the "Cancel" button appeared
Comment 4 Katrin Fischer 2022-08-08 06:04:28 UTC
Please use the depends on, that way the author of the patch that might have caused the issue will also be informed and we can avoid backporting with open bugs (not in this case, but in general :))
Comment 5 Katrin Fischer 2022-08-08 06:07:12 UTC
(In reply to Katrin Fischer from comment #4)
> Please use the depends on, that way the author of the patch that might have
> caused the issue will also be informed and we can avoid backporting with
> open bugs (not in this case, but in general :))

I see the comments now and the patch! (still early here...), still useful to link :)
Comment 6 David Cook 2022-08-08 06:19:02 UTC
(In reply to Katrin Fischer from comment #4)
> Please use the depends on, that way the author of the patch that might have
> caused the issue will also be informed and we can avoid backporting with
> open bugs (not in this case, but in general :))

Nice one! I was so focused on bug 15516 that I forgot to link it to bug 22456 too! Thanks :)
Comment 7 Tomás Cohen Arazi 2022-08-08 14:24:37 UTC
(In reply to David Cook from comment #1)
> I don't know how the MySQL database is being set up in koha-testing-docker,
> but if "installer/data/mysql/db_revs/220600023.pl" is involved, it's clearly
> not working. 

You can do:

$ git checkout v22.05.00 -b test
$ reset_all
$ git checkout master
$ updatedatabase

and check yourself if it works or not.
Comment 8 Tomás Cohen Arazi 2022-08-08 15:11:26 UTC
Created attachment 138835 [details] [review]
Bug 31303: Prevent fatal errors when missing "waiting hold cancellation" rule

This patch removes method chaining which didn't account for the fact that
the "waiting hold cancellation" circulation rule might not exist.

Test plan:
0. Apply patch and koha-plack --restart kohadev
1. Go to http://localhost:8081/cgi-bin/koha/reserve/request.pl?biblionumber=29
2. Search for "koha" user
3. Choose the specific item and click "Place hold"
4. Check in "39999000001310" and click "Confirm hold (Y)"
5. Go to http://localhost:8080/cgi-bin/koha/opac-user.pl#opac-user-holds
6. Rejoice in Koha not exploding
7. Go to http://localhost:8081/cgi-bin/koha/admin/smart-rules.pl
8. Go to "Default waiting hold cancellation policy" and change "Cancellation allowed" to "Yes" and click "Add"
9. Go to http://localhost:8080/cgi-bin/koha/opac-user.pl#opac-user-holds
10. Note the "Cancel" button appears
11. Rejoice again that the "Cancel" button appeared

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 9 Tomás Cohen Arazi 2022-08-08 15:11:31 UTC
Created attachment 138836 [details] [review]
Bug 31303: (follow-up) Use get_effective_rule_value instead

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 10 Tomás Cohen Arazi 2022-08-08 15:13:55 UTC
Thanks for taking care, David. I think my follow-up highlights what we should be doing in the codebase, in general.
Comment 11 Tomás Cohen Arazi 2022-08-08 16:21:24 UTC
Pushed to master for 22.11.

Nice work everyone, thanks!
Comment 12 David Cook 2022-08-08 23:56:17 UTC
(In reply to Tomás Cohen Arazi from comment #10)
> Thanks for taking care, David. I think my follow-up highlights what we
> should be doing in the codebase, in general.

Ah yes. Makes sense to me!