Summary: | Bug 35518 follow-up: fix AutoSwitchPatron | ||
---|---|---|---|
Product: | Koha | Reporter: | Michael Hafen <michael.hafen> |
Component: | Circulation | Assignee: | Bugs List <koha-bugs> |
Status: | CLOSED FIXED | QA Contact: | Testopia <testopia> |
Severity: | trivial | ||
Priority: | P5 - low | CC: | aleisha, david, fridolin.somers, gmcharlt, jonathan.druart, kyle.m.hall, lucas |
Version: | Main | Keywords: | rel_23_05_candidate, rel_23_11_candidate |
Hardware: | All | ||
OS: | All | ||
Change sponsored?: | --- | Patch complexity: | --- |
Documentation contact: | Documentation submission: | ||
Text to go in the release notes: |
This fixes an issue when the AutoSwitchPatron system preference is enabled (the issue was caused by bug 35518 - added to Koha 24.05.00, 23.11.03, and 23.05.09).
If you went to check out an item to a patron, and then entered another patron's card number in the item bar code, it was correctly:
- switching to that patron
- showing a message to say that the patron was switched.
However, it was also incorrectly showing a "Barcode not found" message - this is now fixed, and is no longer displayed.
|
Version(s) released in: |
24.05.00,23.11.05
|
Circulation function: | |||
Bug Depends on: | 35518 | ||
Bug Blocks: | |||
Attachments: |
Bug 35518 Follow-up to fix AutoSwitchPatron - clear variables
Bug 36139: Bug 35518 Follow-up to fix AutoSwitchPatron - clear variables Bug 36139: Bug 35518 (follow-up) to fix AutoSwitchPatron - clear variables |
Description
Michael Hafen
2024-02-20 21:18:25 UTC
Created attachment 162310 [details] [review] Bug 35518 Follow-up to fix AutoSwitchPatron - clear variables Is this ready for signoff? Yes, sorry. I'll change the status. The patch(es) didn't apply for me - I'm confused why there are four patches shown when applying, versus only one listed under the attachments in the bug. See [1]. In addition, before the patches are applied the current behaviour in master doesn't match with what is described in the bug. For me, the patron successfully switches, but the barcode error is displayed (tested using koha-testing-docker with the sample data): 1. Enable the AutoSwitchPatron system preference. 2. Go to check out an item to a patron: enter name, such as Mary, in the check out search box. 3. On the checkout page, enter the card number for another patron instead of an item barcode, for example: Lisa - 23529000197047. 4. The patron switches to Lisa. 5. Two messages are displayed: Patron was automatically switched by reading the patron card during checking out. Ensure you are working with the right patron. Barcode not found The barcode was not found: 23529000197047 Add record using fast cataloging [1] Result of trying to apply the patch(es): git bz apply 35518 Bug 35518 - Call to C4::Context->userenv happens before it's gets populated breaks code logic in circulation 161929 - Bug 35518: Check authentication and set userenv before fetching userenv variables 161930 - Bug 35518: Tidy the moved blocks 162216 - Bug 35518: follow up - clear the barcode list in AutoSwitchPatron block 162218 - Bug 35518: Follow-up for AutoSwitchPatron - clear $patron variable too Apply? [(y)es, (n)o, (i)nteractive] y Applying: Bug 35518: Check authentication and set userenv before fetching userenv variables Using index info to reconstruct a base tree... M circ/circulation.pl Falling back to patching base and 3-way merge... Auto-merging circ/circulation.pl CONFLICT (content): Merge conflict in circ/circulation.pl error: Failed to merge in the changes. Patch failed at 0001 Bug 35518: Check authentication and set userenv before fetching userenv variables I've also changed the assignee. There are four patches because the `git bz apply` command references the parent bug, which is already in master as commits 90b6f68616 and b577b65670. Please try `git bz apply 36139` instead. The patch file here squashes the last two patches on the parent bug (those two should probably be removed from there or something, since I mistakenly added them after the patch had been pushed). This will at least take care of the 'Barcode not found' error, and I think clearing the $patron variable should be harmless at that point in the file. (Looking over the code again, I don't know why it wasn't switching patrons for me, the $patron variable clearly gets reset in the $findborrower block below.) I'll leave the status as is, just in case there is anything else that causes the patch to not apply. (In reply to Michael Hafen from comment #6) > There are four patches because the `git bz apply` command references the > parent bug, which is already in master as commits 90b6f68616 and b577b65670. > Please try `git bz apply 36139` instead. > The patch file here squashes the last two patches on the parent bug (those > two should probably be removed from there or something, since I mistakenly > added them after the patch had been pushed). > This will at least take care of the 'Barcode not found' error, and I think > clearing the $patron variable should be harmless at that point in the file. > (Looking over the code again, I don't know why it wasn't switching patrons > for me, the $patron variable clearly gets reset in the $findborrower block > below.) > I'll leave the status as is, just in case there is anything else that > causes the patch to not apply. Hi Michael. My apologies! I was reading things incorrectly, and as you spotted I applied the wrong patch! The patch does apply, now that I have used the correct bug number! Things now work as expected: with AutoSwitchPatron enabled, the patron is swicthed and the only message that is now displayed is: Patron was automatically switched by reading the patron card during checking out. Ensure you are working with the right patron. Sign off on the way. David Created attachment 163287 [details] [review] Bug 36139: Bug 35518 Follow-up to fix AutoSwitchPatron - clear variables Bug 35518 moved some code blocks to after the call to get_user_and_template() so that userenv would be populated before it was needed. This caused a couple variables to be set before the AutoSwitchPatron block could prevent them from being set. Which broke AutoSwitchPatron functionality. This clears two variable so that AuthSwitchPatron works again. The AutoSwitchPatron clears the $borrowernumber variable to switch patrons. With the AuthSwitchPatron block moved, the $patron variable still gets set, and the patron doesn't get switched. The clears the $patron variable too. Also clear the barcode list. The AutoSwitchPatron block got moved, and now the @$barcodes variable gets filled and not cleared. Leading to a 'Barcode not found' error when the patron is auto switched. Test plan: 1. Ensure AutoSwitchPatron is turned on. 2. Select the card number of two patron accounts. 3. Find the first patron in circulation. 4. Enter the second patron's card number in the item barcode field to switch patrons. 5. Observe the error about item barcode not existing, and the patron did not switch. 6. Apply patch and restart services. 7. Enter the second patron's card number in the item barcode field again. 8. Observe that the patron was switched with no error about an invalid barcode. Signed-off-by: David Nind <david@davidnind.com> Testing notes (using koha-testing-docker (KTD): 1. Enable the AutoSwitchPatron system preference. 2. Go to check out an item to a patron: enter name, such as Mary, in the check out search box. 3. On the check out page, enter the card number for another patron instead of an item barcode, for example: Lisa - 23529000197047. 4. The patron correctly switches to Lisa. 5. Two messages are displayed: Patron was automatically switched by reading the patron card during checking out. Ensure you are working with the right patron. Barcode not found The barcode was not found: 23529000197047 Add record using fast cataloging 6. Only the first message should be displayed. 7. Apply the patch and repeat. 8. The barcode not found message is no longer displayed. Created attachment 163708 [details] [review] Bug 36139: Bug 35518 (follow-up) to fix AutoSwitchPatron - clear variables Bug 35518 moved some code blocks to after the call to get_user_and_template() so that userenv would be populated before it was needed. This caused a couple variables to be set before the AutoSwitchPatron block could prevent them from being set. Which broke AutoSwitchPatron functionality. This clears two variable so that AuthSwitchPatron works again. The AutoSwitchPatron clears the $borrowernumber variable to switch patrons. With the AuthSwitchPatron block moved, the $patron variable still gets set, and the patron doesn't get switched. The clears the $patron variable too. Also clear the barcode list. The AutoSwitchPatron block got moved, and now the @$barcodes variable gets filled and not cleared. Leading to a 'Barcode not found' error when the patron is auto switched. Test plan: 1. Ensure AutoSwitchPatron is turned on. 2. Select the card number of two patron accounts. 3. Find the first patron in circulation. 4. Enter the second patron's card number in the item barcode field to switch patrons. 5. Observe the error about item barcode not existing, and the patron did not switch. 6. Apply patch and restart services. 7. Enter the second patron's card number in the item barcode field again. 8. Observe that the patron was switched with no error about an invalid barcode. kidclamp amended patch - tidy and fix commmit message Signed-off-by: David Nind <david@davidnind.com> Hm, that must have been a close miss - patch doesn't apply anymore. Can you please rebase? Pushed for 24.05! Well done everyone, thank you! This fixes a bug in 23.11 and 23.05. Can we please ensure it is backported? Pushed to 23.11.x for 23.11.05 This patch must NOT be back ported to 23.05.x. these changes are already in that branch due to a mistake on my part. There is a note on the parent bug about it. (In reply to Michael Hafen from comment #15) > This patch must NOT be back ported to 23.05.x. these changes are already in > that branch due to a mistake on my part. There is a note on the parent bug > about it. Michael, you got it! No backport for 23.05.x. |