|
Description
Nick Clemens (kidclamp)
2023-11-01 16:04:12 UTC
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. |