CanBookBeIssued returns: issuingimpossible and needsconfirmation variables In circulation.pl they are called errors and question Let's be consistent
Created attachment 158185 [details] [review] Bug 35216: Rename question to needsconfirmation
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
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
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;
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.
Are you able to add your sign-off?
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;
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
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
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.
Hi Brendan, always happy to have a new tester around! Let us know if we can help with anything :)
Created attachment 159039 [details] [review] Bug 35216: Rename question to needsconfirmation
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
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
Created attachment 159056 [details] [review] Bug 35216: Rename question to needsconfirmation
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
(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
still needs work
Created attachment 159072 [details] [review] Bug 35216: Rename question to needsconfirmation
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
Created attachment 159085 [details] [review] Bug 35216: Rename question to needsconfirmation Signed-off-by: Brendan Lawlor <blawlor@clamsnet.org>
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>
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>
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>
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
When reading the bug report, I expected the change to be the other way around, but that's ok :)
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.
Pushed for 24.05! Well done everyone, thank you!
Pushed to 23.11.x for 23.11.02
Doesn't apply clean to 23.05.x, no backport.