| Summary: | Call to C4::Context->userenv happens before it's gets populated breaks code logic in circulation | ||
|---|---|---|---|
| Product: | Koha | Reporter: | Slava Shishkin <slavashishkin> |
| Component: | Circulation | Assignee: | Nick Clemens (kidclamp) <nick> |
| Status: | Pushed to oldstable --- | QA Contact: | Martin Renvoize (ashimema) <martin.renvoize> |
| Severity: | critical | ||
| Priority: | P5 - low | CC: | aleisha, eric, fridolin.somers, gmcharlt, jonathan.druart, kyle.m.hall, kyle, lucas, madamyk, michael.hafen, mspinney, nick, nugged, tomascohen |
| Version: | Main | ||
| Hardware: | All | ||
| OS: | All | ||
| Change sponsored?: | --- | Patch complexity: | Trivial patch |
| Documentation contact: | Documentation submission: | ||
| Text to go in the release notes: | Version(s) released in: |
24.05.00,23.11.03,23.05.09
|
|
| Circulation function: | |||
| Bug Depends on: | 17798 | ||
| Bug Blocks: | 36139 | ||
| Attachments: |
Bug 35518: Check authentication and set userenv before fetching userenv variables
Bug 35518: Check authentication and set userenv before fetching userenv variables Bug 35518: Check authentication and set userenv before fetching userenv variables Bug 35518: Tidy the moved blocks Bug 35518: follow up - clear the barcode list in AutoSwitchPatron block Bug 35518: Follow-up for AutoSwitchPatron - clear $patron variable too |
||
Created attachment 161539 [details] [review] Bug 35518: Check authentication and set userenv before fetching userenv variables Currently we get the userenv before we have set it correctly for the session To test: 1 - Sign in as a user with fast cataloging permission 2 - Bring up a patron, type gibberish into barcode field to get a fast cataloging link 3 - Check the link, it should have your current signed in barcode 4 - Sign in to a different browser with a different user and at a different branch 5 - Bring up a aptron in circulation and type gibberish into barcode field to get a fast cataloging link 6 - It may have your branch, but it may also have the other user's branch from the other window 7 - Keep entering gibberish to get a link until one user has the correct branch 8 - Then switch to the other browser, and keep entering gibberish, watch the branchcode change 9 - Apply patch, restart all 10 - Test switching between browsers. generating fast cataloging links 11 - Users should now consistently have the correct branch Created attachment 161707 [details] [review] Bug 35518: Check authentication and set userenv before fetching userenv variables Currently we get the userenv before we have set it correctly for the session To test: 1 - Sign in as a user with fast cataloging permission 2 - Bring up a patron, type gibberish into barcode field to get a fast cataloging link 3 - Check the link, it should have your current signed in barcode 4 - Sign in to a different browser with a different user and at a different branch 5 - Bring up a aptron in circulation and type gibberish into barcode field to get a fast cataloging link 6 - It may have your branch, but it may also have the other user's branch from the other window 7 - Keep entering gibberish to get a link until one user has the correct branch 8 - Then switch to the other browser, and keep entering gibberish, watch the branchcode change 9 - Apply patch, restart all 10 - Test switching between browsers. generating fast cataloging links 11 - Users should now consistently have the correct branch Signed-off-by: David Nind <david@davidnind.com> Created attachment 161929 [details] [review] Bug 35518: Check authentication and set userenv before fetching userenv variables Currently we get the userenv before we have set it correctly for the session To test: 1 - Sign in as a user with fast cataloging permission 2 - Bring up a patron, type gibberish into barcode field to get a fast cataloging link 3 - Check the link, it should have your current signed in barcode 4 - Sign in to a different browser with a different user and at a different branch 5 - Bring up a aptron in circulation and type gibberish into barcode field to get a fast cataloging link 6 - It may have your branch, but it may also have the other user's branch from the other window 7 - Keep entering gibberish to get a link until one user has the correct branch 8 - Then switch to the other browser, and keep entering gibberish, watch the branchcode change 9 - Apply patch, restart all 10 - Test switching between browsers. generating fast cataloging links 11 - Users should now consistently have the correct branch Signed-off-by: David Nind <david@davidnind.com> Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Created attachment 161930 [details] [review] Bug 35518: Tidy the moved blocks This patch just tidies the moved blocks to get us past the QA script check. Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> No regressions found, QA script happy. Passing QA this comment is not related to the code/solution, but to the problem as before the ticket: we recently found that before this solution Koha brings the wrong returning branches when the item is returned on circ/circulation.pl page (moreover, it looks like (unsure) that it reads branch data of another user from the Plack session because the current wasn't populated properly) so this bug should have "increased" priority, IMO. It's already PQA, we're in a security freeze right now. It'll be pushed as soon as that freeze is unlocked. Pushed to master for 24.05.00, thanks for the hard work everyone. Pushed to 23.11.x for 23.11.03 I found a bug with this patch. Because the AutoSwitch check has been moved to after the @$barcodes variable is filled, if I scan a patron barcode into the item barcode field (as my librarians do all the time), the @$barcodes variable gets filled, but not emptied, and the 'Barcode not found' error surfaces. I'll be submitting a (trivial) patch to correct that in a moment. Created attachment 162216 [details] [review] Bug 35518: follow up - clear the barcode list in AutoSwitchPatron block 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. Created attachment 162218 [details] [review] Bug 35518: Follow-up for AutoSwitchPatron - clear $patron variable too Another follow up: 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. Hi Michael Thanks for that. You should create a new bug report with those patches since the previous ones have already been pushed. And beware of commit messages for follow-ups : https://wiki.koha-community.org/wiki/Commit_messages#Follow-ups Would it be possible for this to be backported to 23.05? +1 to Eric's question. This is causing significant confusion in several different contexts in our consortium. Backported to 23.05.x for upcoming 23.05.09 (In reply to Fridolin Somers from comment #13) > Hi Michael > Thanks for that. > > You should create a new bug report with those patches since the previous > ones have already been pushed. > And beware of commit messages for follow-ups : > https://wiki.koha-community.org/wiki/Commit_messages#Follow-ups I was to late in marking those two patches obsolete, they were included in the push to 23.05.x. I made bug 36139 to track my patches. I foresee some confusion here. in 23.05.x those two extra patches should be reverted, or bug 36139 needs to Not be backported. (In reply to Michael Hafen from comment #17) > (In reply to Fridolin Somers from comment #13) > > Hi Michael > > Thanks for that. > > > > You should create a new bug report with those patches since the previous > > ones have already been pushed. > > And beware of commit messages for follow-ups : > > https://wiki.koha-community.org/wiki/Commit_messages#Follow-ups > > I was to late in marking those two patches obsolete, they were included in > the push to 23.05.x. > > I made bug 36139 to track my patches. > > I foresee some confusion here. in 23.05.x those two extra patches should be > reverted, or bug 36139 needs to Not be backported. Hey Lucas, do you understand what is required here for 23.05.x? > Hey Lucas, do you understand what is required here for 23.05.x? Do we need to revert Bug 36139 from 23.05.x? Don't need to revert Bug 36139 since it doesn't seem to have been back-ported at this point. Just need to make sure it doesn't get pulled into 23.05.x. (In reply to Michael Hafen from comment #20) > Don't need to revert Bug 36139 since it doesn't seem to have been > back-ported at this point. Just need to make sure it doesn't get pulled > into 23.05.x. We are seeing this bug in 23.05 so just want to check that the next release will have this fixed. You mentioned earlier "in 23.05.x those two extra patches should be reverted" - are you referring to the patches on this report, Bug 35518 need to be reverted from 23.05? There are two extra patches on this bug that are now obsolete. This bug was backported to 23.05.x before those two patches were marked obsolete. Those two patches are now on Bug 36139. My advise was that either Bug 36139 should be skipped (not backported to 23.05.x) OR the two extra patches should be reverted. I'll leave which option is the more appropriate to your discretion, in my opinion you should leave those two patches as is on this bug (don't revert them) and skip Bug 36139. Ah yes sorry that makes sense! |
We getting results from `C4::Context->userenv` on line 72: my $userenv = C4::Context->userenv; but `C4::Context->userenv` gets populated only far below in `get_template_and_user` call, line 142, this means that everything that we getting further from `$userenv` is empty. And more, because autovivification occurs for the `$userenv` at line 73: my $branch = $userenv->{'branch'} // ''; that does not cause any visible errors, but it causes malfunctions at places where $branch, $desk_id is used, and also where $userenv is accessed, at line 153: if (!C4::Auth::haspermission( $userenv->{id} , { circulate => 'force_checkout' } ) ). To check that we have undef and it gets autovivificated, let's add some dumps into code when we open any borrower, e.g. /cgi-bin/koha/circ/circulation.pl?borrowernumber=19, Let's dump $userenv before and after `$branch = $userenv->{'branch'} // '';` line: 72 my $userenv = C4::Context->userenv; 73 use Data::Dumper; 74 warn __LINE__, ' ', Dumper($userenv); 75 my $branch = $userenv->{'branch'} // ''; 76 warn __LINE__, ' ', Dumper($userenv); this gives in server logs: 74 $VAR1 = undef; 76 $VAR1 = {}; and also to check when `C4::Context->userenv` gets its real values, let's dump before and after the `get_template_and_user()` call at line 142 (line 145 if to continue above dumps code): 145 warn __LINE__, ' ', Dumper([C4::Context->userenv]); 146 my ( $template, $loggedinuser, $cookie ) = get_template_and_user ( ... 153 ); 154 warn __LINE__, ' ', Dumper([C4::Context->userenv]); this gives in server logs: 145 $VAR1 = []; 154 $VAR1 = [ { ... 'id' => 'admin', ... } ]; what depicts the whole bug.