Bug 35216 - Use return variable names from CanBookBeIssued in circulation.pl for consistency
Summary: Use return variable names from CanBookBeIssued in circulation.pl for consistency
Status: RESOLVED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: Circulation (show other bugs)
Version: Main
Hardware: All All
: P5 - low normal (vote)
Assignee: Nick Clemens (kidclamp)
QA Contact: Testopia
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2023-11-01 16:04 UTC by Nick Clemens (kidclamp)
Modified: 2024-01-04 20:30 UTC (History)
7 users (show)

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


Attachments
Bug 35216: Rename question to needsconfirmation (2.58 KB, patch)
2023-11-01 16:06 UTC, Nick Clemens (kidclamp)
Details | Diff | Splinter Review
Bug 35216: Rename error to issuingimpossible (2.35 KB, patch)
2023-11-01 16:06 UTC, Nick Clemens (kidclamp)
Details | Diff | Splinter Review
Bug 35216: Rename error to issuingimpossible (2.34 KB, patch)
2023-11-01 16:14 UTC, Nick Clemens (kidclamp)
Details | Diff | Splinter Review
Bug 35216: Rename question to needsconfirmation (2.58 KB, patch)
2023-11-16 12:04 UTC, Nick Clemens (kidclamp)
Details | Diff | Splinter Review
Bug 35216: Rename error to issuingimpossible (2.50 KB, patch)
2023-11-16 12:04 UTC, Nick Clemens (kidclamp)
Details | Diff | Splinter Review
Bug 35216: Rename question to needsconfirmation (3.45 KB, patch)
2023-11-16 15:55 UTC, Nick Clemens (kidclamp)
Details | Diff | Splinter Review
Bug 35216: Rename error to issuingimpossible (2.71 KB, patch)
2023-11-16 15:55 UTC, Nick Clemens (kidclamp)
Details | Diff | Splinter Review
Bug 35216: Rename question to needsconfirmation (3.46 KB, patch)
2023-11-17 03:28 UTC, Nick Clemens (kidclamp)
Details | Diff | Splinter Review
Bug 35216: Rename error to issuingimpossible (2.71 KB, patch)
2023-11-17 03:28 UTC, Nick Clemens (kidclamp)
Details | Diff | Splinter Review
Bug 35216: Rename question to needsconfirmation (3.51 KB, patch)
2023-11-17 14:35 UTC, Brendan Lawlor
Details | Diff | Splinter Review
Bug 35216: Rename error to issuingimpossible (2.76 KB, patch)
2023-11-17 14:35 UTC, Brendan Lawlor
Details | Diff | Splinter Review
Bug 35216: Rename question to needsconfirmation (3.57 KB, patch)
2023-11-21 21:12 UTC, Victor Grousset/tuxayo
Details | Diff | Splinter Review
Bug 35216: Rename error to issuingimpossible (2.82 KB, patch)
2023-11-21 21:12 UTC, Victor Grousset/tuxayo
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Nick Clemens (kidclamp) 2023-11-01 16:04:12 UTC
CanBookBeIssued returns:
issuingimpossible and needsconfirmation variables

In circulation.pl they are called errors and question

Let's be consistent
Comment 1 Nick Clemens (kidclamp) 2023-11-01 16:06:21 UTC
Created attachment 158185 [details] [review]
Bug 35216: Rename question to needsconfirmation
Comment 2 Nick Clemens (kidclamp) 2023-11-01 16:06:23 UTC
Created attachment 158186 [details] [review]
Bug 35216: Rename error to issuingimpossible

Test plan:
1 - Apply two patches
2 - Confirm code changes make sense
3 - Confirm you can check out items
4 - Confirm checkout blocks still block
Comment 3 Nick Clemens (kidclamp) 2023-11-01 16:14:05 UTC
Created attachment 158187 [details] [review]
Bug 35216: Rename error to issuingimpossible

Test plan:
1 - Apply two patches
2 - Confirm code changes make sense
3 - Confirm you can check out items
4 - Confirm checkout blocks still block
Comment 4 Brendan Lawlor 2023-11-09 16:38:45 UTC
I tried testing this out and verified the variable name changes look consistent.

I tested checking out and checking in items, which mostly worked but if I tried to checkout a barcode that was already checked out I got this error with line 395 highlighted:

Can't use string ("RENEW_ISSUE") as a HASH ref while "strict refs" in use at /kohadevbox/koha/circ/circulation.pl line 395
in (eval) at /kohadevbox/koha/circ/circulation.pl line 395
  392: 
  393:             # pass needsconfirmation to template if issuing is possible and user hasn't yet confirmed.
  394:             foreach my $needsconfirmation ( keys %$needsconfirmation ) {
  395:                 $template_params->{$needsconfirmation} = $$needsconfirmation{$needsconfirmation};
  396:                 $template_params->{getTitleMessageIteminfo} = $biblio->title;
  397:                 $template_params->{getBarcodeMessageIteminfo} = $item->barcode;
  398:                 $template_params->{NEEDSCONFIRMATION} = 1;

Also got this error once but I can't remember how:
Can't use string ("issued_cardnumber") as a HASH ref while "strict refs" in use at /kohadevbox/koha/circ/circulation.pl line 395
in (eval) at /kohadevbox/koha/circ/circulation.pl line 395
  392: 
  393:             # pass needsconfirmation to template if issuing is possible and user hasn't yet confirmed.
  394:             foreach my $needsconfirmation ( keys %$needsconfirmation ) {
  395:                 $template_params->{$needsconfirmation} = $$needsconfirmation{$needsconfirmation};
  396:                 $template_params->{getTitleMessageIteminfo} = $biblio->title;
  397:                 $template_params->{getBarcodeMessageIteminfo} = $item->barcode;
  398:                 $template_params->{NEEDSCONFIRMATION} = 1;
Comment 5 Brendan Lawlor 2023-11-09 19:00:46 UTC
I think the error I got was due to something wrong in my dev environment. I set everything up again fresh, applied the bug and re tested.

This time everything worked as expected and I could not recreate the error.
Comment 6 David Nind 2023-11-13 18:34:04 UTC
Are you able to add your sign-off?
Comment 7 Brendan Lawlor 2023-11-13 20:21:37 UTC
Hi David, 

I am having trouble going through the process with git bz and now am afraid I could be doing something wrong. I reset the Koha clone, deleted the local branch I was testing on and tried again. 

git reset --hard origin/master
git checkout -b bug_35216

ktd up

ktd --shell

git bz apply 35216

This time I got a merge conflict in circ/circulation.pl. I'm using VS Code and I went through and accepted the incoming changes and checked to make sure the code looked right.

Then when I tested again, if I checked out an item that was already checked out, I got the error that I got the first time:

Can't use string ("RENEW_ISSUE") as a HASH ref while "strict refs" in use at /kohadevbox/koha/circ/circulation.pl line 419
in (eval) at /kohadevbox/koha/circ/circulation.pl line 419
  416: 
  417:             # pass needsconfirmation to template if issuing is possible and user hasn't yet confirmed.
  418:             foreach my $needsconfirmation ( keys %$needsconfirmation ) {
  419:                 $template_params->{$needsconfirmation} = $$needsconfirmation{$needsconfirmation};
  420:                 $template_params->{getTitleMessageIteminfo} = $biblio->title;
  421:                 $template_params->{getBarcodeMessageIteminfo} = $item->barcode;
  422:                 $template_params->{NEEDSCONFIRMATION} = 1;
Comment 8 David Nind 2023-11-13 21:18:42 UTC
Hi Brendan.

This is what I normally do in KTD (assuming your Bugzilla credentials are in the .env file):

1. Koha clone - up-to-date: on master branch, git pull
2. KTD up-to-date: git pull, ktd pull
3. Start up KTD: ktd up
4. Access Koha using the shell: ktd --shell
5. Checkout a branch for testing the bug: git checkout -B bzXXXXXX 
6. Apply the patches: git bz apply XXXXXXX
7. Test using the test plan, if there is one, or you know what it is about
8. Sign-off once happy: 
   8.1 git bz so X (X = the number of patches, in this case 2) (if you git log, your sign-off line should show at the end of the patch)
   8.2 git bz attach -e XXXXXX HEAD (for one patch)
       or
       git bz attach -e XXXXXX HEAD~X.. (for multiple patches, where X = number of patches, in this case 2) 
9. Check on Bugzilla that the status was changed to signed off.

XXXXXX = the bug number

I tried to apply the patch, but it no longer applies - I'm not a developer, so don't normally try and resolve the conflict, and change the status in Bugzilla to Patch doesn't apply.

If I get an error when going through the test plan, or it doesn't work, I would normally indicate this in a comment on the bug and change the status to Failed QA.

Patch doesn't apply message
~~~~~~~~~~~~~~~~~~~~~~~~~~~~

kohadev-koha@kohadevbox:koha(bz35307)$ git bz apply 35216

Bug 35216 - Use return variable names from CanBookBeIssued in circulation.pl for consistency

158185 - Bug 35216: Rename question to needsconfirmation
158187 - Bug 35216: Rename error to issuingimpossible

Apply? [(y)es, (n)o, (i)nteractive] y
Applying: Bug 35216: Rename question to needsconfirmation
Applying: Bug 35216: Rename error to issuingimpossible
error: sha1 information is lacking or useless (circ/circulation.pl).
error: could not build fake ancestor
Patch failed at 0001 Bug 35216: Rename error to issuingimpossible
hint: Use 'git am --show-current-patch=diff' to see the failed patch
When you have resolved this problem run "git bz apply --continue".
If you would prefer to skip this patch, instead run "git bz apply --skip".
To restore the original branch and stop patching run "git bz apply --abort".
Patch left in /tmp/Bug-35216-Rename-error-to-issuingimpossible-mjakx8dk.patch
Comment 9 David Nind 2023-11-13 21:26:30 UTC
Also, once I've tested something and signed off, etc., I normally do:

1. git checkout master
2. git branch -D bzXXXXXX
3. git reset --hard origin/master
4. reset_all (resets Koha - gives you a clean setup, with needing to shut down and restart KTD)

Here is a link to my KTD cheat sheet if that helps: https://gitlab.com/-/snippets/1893788
Comment 10 Brendan Lawlor 2023-11-14 14:13:20 UTC
Thanks, David. That's a really helpful resource. 

I thought this bug would be a good beginner bug to test since it's just changing a couple variable names, but I guess there's more to it in this case.

I'll keep watching this to see where it goes.
Comment 11 Katrin Fischer 2023-11-14 22:10:59 UTC
Hi Brendan, always happy to have a new tester around! Let us know if we can help with anything :)
Comment 12 Nick Clemens (kidclamp) 2023-11-16 12:04:11 UTC
Created attachment 159039 [details] [review]
Bug 35216: Rename question to needsconfirmation
Comment 13 Nick Clemens (kidclamp) 2023-11-16 12:04:13 UTC
Created attachment 159040 [details] [review]
Bug 35216: Rename error to issuingimpossible

Test plan:
1 - Apply two patches
2 - Confirm code changes make sense
3 - Confirm you can check out items
4 - Confirm checkout blocks still block
Comment 14 Brendan Lawlor 2023-11-16 15:10:55 UTC
I tried testing this again, but when I tested circ, searching for a patron raised errors that $error and $question were undefined.

Could not compile /kohadevbox/koha/circ/circulation.pl: Global symbol "$error" requires explicit package name (did you forget to declare "my $error"?) at /kohadevbox/koha/circ/circulation.pl line 391.
Global symbol "$error" requires explicit package name (did you forget to declare "my $error"?) at /kohadevbox/koha/circ/circulation.pl line 392.
Global symbol "$question" requires explicit package name (did you forget to declare "my $question"?) at /kohadevbox/koha/circ/circulation.pl line 427.
Global symbol "$question" requires explicit package name (did you forget to declare "my $question"?) at /kohadevbox/koha/circ/circulation.pl line 448.
BEGIN not safe after errors--compilation aborted at /kohadevbox/koha/circ/circulation.pl line 714.
 at /usr/share/perl5/CGI/Compile.pm line 144

I looked at the code for those lines and replaced a couple more instances of $error with $issuingimpossible and $question with $needsconfirmation. That resolved the error, and I could look up a patron and checkout items.

But if I checked out an item that was already checked out I got the same error when testing the first patch:
Can't use string ("RENEW_ISSUE") as a HASH ref while "strict refs" in use at /kohadevbox/koha/circ/circulation.pl line 420
Comment 15 Nick Clemens (kidclamp) 2023-11-16 15:55:02 UTC
Created attachment 159056 [details] [review]
Bug 35216: Rename question to needsconfirmation
Comment 16 Nick Clemens (kidclamp) 2023-11-16 15:55:04 UTC
Created attachment 159057 [details] [review]
Bug 35216: Rename error to issuingimpossible

Test plan:
1 - Apply two patches
2 - Confirm code changes make sense
3 - Confirm you can check out items
4 - Confirm checkout blocks still block
Comment 17 Nick Clemens (kidclamp) 2023-11-16 15:55:38 UTC
(In reply to Brendan Lawlor from comment #14)
> I tried testing this again, but when I tested circ, searching for a patron
> raised errors that $error and $question were undefined.
> 
Ah, bookings introduced new uses, patches updated
Comment 18 Nick Clemens (kidclamp) 2023-11-16 15:56:56 UTC
still needs work
Comment 19 Nick Clemens (kidclamp) 2023-11-17 03:28:10 UTC
Created attachment 159072 [details] [review]
Bug 35216: Rename question to needsconfirmation
Comment 20 Nick Clemens (kidclamp) 2023-11-17 03:28:12 UTC
Created attachment 159073 [details] [review]
Bug 35216: Rename error to issuingimpossible

Test plan:
1 - Apply two patches
2 - Confirm code changes make sense
3 - Confirm you can check out items
4 - Confirm checkout blocks still block
Comment 21 Brendan Lawlor 2023-11-17 14:35:03 UTC
Created attachment 159085 [details] [review]
Bug 35216: Rename question to needsconfirmation

Signed-off-by: Brendan Lawlor <blawlor@clamsnet.org>
Comment 22 Brendan Lawlor 2023-11-17 14:35:05 UTC
Created attachment 159086 [details] [review]
Bug 35216: Rename error to issuingimpossible

Test plan:
1 - Apply two patches
2 - Confirm code changes make sense
3 - Confirm you can check out items
4 - Confirm checkout blocks still block

Signed-off-by: Brendan Lawlor <blawlor@clamsnet.org>
Comment 23 Victor Grousset/tuxayo 2023-11-21 21:12:43 UTC
Created attachment 159180 [details] [review]
Bug 35216: Rename question to needsconfirmation

Signed-off-by: Brendan Lawlor <blawlor@clamsnet.org>
Signed-off-by: Victor Grousset/tuxayo <victor@tuxayo.net>
Comment 24 Victor Grousset/tuxayo 2023-11-21 21:12:46 UTC
Created attachment 159181 [details] [review]
Bug 35216: Rename error to issuingimpossible

Test plan:
1 - Apply two patches
2 - Confirm code changes make sense
3 - Confirm you can check out items
4 - Confirm checkout blocks still block

Signed-off-by: Brendan Lawlor <blawlor@clamsnet.org>
Signed-off-by: Victor Grousset/tuxayo <victor@tuxayo.net>
Comment 25 Victor Grousset/tuxayo 2023-11-21 21:14:12 UTC
Works, makes sense, QA script happy, code looks good, passing QA :)

tested checkout
- with lots of money owed
- conflicting with someone else's hold
- already checked out
- with a past date
Comment 26 Katrin Fischer 2023-12-19 10:39:02 UTC
When reading the bug report, I expected the change to be the other way around, but that's ok :)
Comment 27 Katrin Fischer 2023-12-19 10:42:45 UTC
It looks like t/db_dependent/Circulation.t also uses $error and $question. I'll let you decide if that is worth a follow-up.
Comment 28 Katrin Fischer 2023-12-19 12:44:04 UTC
Pushed for 24.05!

Well done everyone, thank you!
Comment 29 Fridolin Somers 2024-01-03 14:55:15 UTC
Pushed to 23.11.x for 23.11.02
Comment 30 Lucas Gass 2024-01-04 20:30:55 UTC
Doesn't apply clean to 23.05.x, no backport.