https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=24754 --- Comment #1 from Nick Clemens <nick@bywatersolutions.com> --- Created attachment 99768 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=99768&action=edit Bug 24754: Set userenv for ilsdi requests To test: 1 - Enable ILSDI in systempreferences 2 - Checkout an item to a patron 3 - Enable Renewal logs 4 - Ensure you will be able to renew the item several times 5 - Open an incognito/private browser 6 - Renew item via ilsdi: http://localhost:8080/cgi-bin/koha/ilsdi.pl?service=RenewLoan&patron_id=5&item_id=1128 7 - Check action logs - note renewal is indicated as opac 8 - In private browser open a new tab 9 - Sign in to staff client 10 - Return to other tab 11 - Refresh page to repeat renewal 12 - Check action_logs 13 - The second renewal indicates it was renewed via intranet by logged in staff member 14 - Apply patch 15 - restart_all 16 - Ensure staff client still signed in 17 - Repeate renewal again 18 - Check logs, renewal now indicates opac again (as it should) -- You are receiving this mail because: You are the assignee for the bug. You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=24754 Nick Clemens <nick@bywatersolutions.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Assignee|koha-bugs@lists.koha-commun |nick@bywatersolutions.com |ity.org | Severity|normal |major -- You are receiving this mail because: You are the assignee for the bug. You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=24754 --- Comment #5 from Martin Renvoize <martin.renvoize@ptfs-europe.com> --- Created attachment 99773 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=99773&action=edit Bug 24754: (QA follow-up) opac -> api for 'interface' -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=24754 --- Comment #6 from Nick Clemens <nick@bywatersolutions.com> --- Created attachment 99778 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=99778&action=edit Bug 23165: Set branch for renewal Originally I used 'opac' as the interface so that we would follow opac rules for setting the branches for renewals. If using api we must explicitly state the branch - falling back to the checkout branch is the simplest fix as we don't have a user here. This also corrects typo if setting surname/firstname to CRON instead of ILSDI -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=24754 --- Comment #7 from Jonathan Druart <jonathan.druart@bugs.koha-community.org> --- Created attachment 100000 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=100000&action=edit Bug 24754: Simplify tests -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=24754 Jonathan Druart <jonathan.druart@bugs.koha-community.org> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #100000|0 |1 is obsolete| | -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=24754 --- Comment #9 from Jonathan Druart <jonathan.druart@bugs.koha-community.org> --- Created attachment 100006 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=100006&action=edit Bug 24754: Set branch for renewal Originally I used 'opac' as the interface so that we would follow opac rules for setting the branches for renewals. If using api we must explicitly state the branch - falling back to the checkout branch is the simplest fix as we don't have a user here. This also corrects typo if setting surname/firstname to CRON instead of ILSDI -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=24754 --- Comment #10 from Jonathan Druart <jonathan.druart@bugs.koha-community.org> --- Note: If I tested well, on top of bug 24759: Without the patch, step 7 indicates "intranet" (test plan expects opac) Also, test plan is wrong at 18, as the patch now put "api", not "opac". However I think we should record opac, and depends on bug 24759. It could help to have an updated test plan, as well as a quick message in the git body explaining what we really want to achieve with this patch. -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=24754 Nick Clemens <nick@bywatersolutions.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #99772|0 |1 is obsolete| | --- Comment #11 from Nick Clemens <nick@bywatersolutions.com> --- Created attachment 100035 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=100035&action=edit Bug 24754: Set userenv for ilsdi requests Currently ILSDI scripts do not have a userenv set. This affects both action_logs and statistics. For action_logs we either get no user or, if there is an active session, we use that one. This can have unintended consequences and we should explicitly set the env so that system user (0) is recorded For statistics we need to speify a branch to record for actions like renewals. With bug 24759 we add an items method for this. We should take advantage of this method and use the 'opac' interface as the ilsdi scripts live there. To test: 1 - Enable ILSDI in systempreferences 2 - Checkout an item to a patron 3 - Enable Renewal logs 4 - Ensure you will be able to renew the item several times 5 - Open an incognito/private browser 6 - Renew item via ilsdi: http://localhost:8080/cgi-bin/koha/ilsdi.pl?service=RenewLoan&patron_id=5&item_id=1128 7 - Check action logs, interface may be random, user should be 0, if not make sure the user in step 9 is a different one 8 - In private browser open a new tab 9 - Sign in to staff client 10 - Return to other tab 11 - Refresh page to repeat renewal 12 - Check action_logs 13 - The second renewal indicates it was renewed via intranet by logged in staff member 14 - Apply patch 15 - restart_all 16 - Ensure staff client still signed in 17 - Repeate renewal again 18 - Check logs, renewal now indicates opac again (as it should), user is 0 even if logged in on other tab -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=24754 --- Comment #12 from Jonathan Druart <jonathan.druart@bugs.koha-community.org> --- Why do you pass the branchcode's checkout and do not let renewal_branchcode/OpacRenewalBranch does the job? -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=24754 Nick Clemens <nick@bywatersolutions.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Failed QA |Needs Signoff -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=24754 --- Comment #15 from Nick Clemens <nick@bywatersolutions.com> --- (In reply to Jonathan Druart from comment #12)

Why do you pass the branchcode's checkout and do not let renewal_branchcode/OpacRenewalBranch does the job?

Old code, was before dependency. Patch updated, back to NSO -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=24754 --- Comment #17 from Jonathan Druart <jonathan.druart@bugs.koha-community.org> --- (In reply to Jonathan Druart from comment #16)

Ok but then it will explode in some cases Nick. For instance the logged in user does not exist.

Scratch that, it won't reach that code for "opac" -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=24754 David Nind <david@davidnind.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #100269|0 |1 is obsolete| | --- Comment #18 from David Nind <david@davidnind.com> --- Created attachment 100305 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=100305&action=edit Bug 24754: Set userenv for ilsdi requests Currently ILSDI scripts do not have a userenv set. This affects both action_logs and statistics. For action_logs we either get no user or, if there is an active session, we use that one. This can have unintended consequences and we should explicitly set the env so that system user (0) is recorded For statistics we need to specify a branch to record for actions like renewals. With bug 24759 we add an items method for this. We should take advantage of this method and use the 'opac' interface as the ilsdi scripts live there. To test: 1 - Enable ILS-DI in systempreferences 2 - Checkout an item to a patron 3 - Enable Renewal logs (RenewalLog system preference) 4 - Ensure you will be able to renew the item several times 5 - Open an incognito/private browser 6 - Renew item via ilsdi: http://localhost:8080/cgi-bin/koha/ilsdi.pl?service=RenewLoan&patron_id=5&item_id=1128 7 - Check action logs, interface may be random, user should be 0, if not make sure the user in step 9 is a different one 8 - In private browser open a new tab 9 - Sign in to staff client 10 - Return to other tab 11 - Refresh page to repeat renewal 12 - Check action_logs 13 - The second renewal indicates it was renewed via intranet by logged in staff member 14 - Apply patch 15 - restart_all 16 - Ensure staff client still signed in 17 - Repeate renewal again 18 - Check logs, renewal now indicates opac again (as it should), user is 0 even if logged in on other tab Signed-off-by: David Nind <david@davidnind.com> -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=24754 Marcel de Rooy <m.de.rooy@rijksmuseum.nl> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|BLOCKED |Failed QA --- Comment #20 from Marcel de Rooy <m.de.rooy@rijksmuseum.nl> --- The patch does this: +# Set the userenv +C4::Context->_new_userenv(1); +C4::Context->set_userenv( + undef, undef, undef, 'ILSDI', 'ILSDI', undef, + undef, undef, undef, undef, undef +); +C4::Context->interface('opac'); For comparison, misc/commit_file.pl does this: # FIXME dummy user so that logging won't fail # in future, probably should tie to a real user account C4::Context->set_userenv(0, 'batch', 0, 'batch', 'batch', 'batch', 'batch'); And Koha::Script does this: C4::Context->_new_userenv(1); C4::Context->set_userenv( undef, undef, undef, 'CRON', 'CRON', undef, undef, undef, undef, undef ); Might be that the author copied this part of the code? (I think it should be adapted; see below.) The sub _new_userenv stores a sessionID in activeuser (which is quite confusing). I would not recommend to always use session 1. (Session id is a varchar(32) btw.) Why not use "ILSDI_session", possibly followed by a few digits or so? Another note: 11 parameters are passed in ilsdi.pl and Koha::Script. I only count 10. We're copy&pasting an error here. shift @_; # this is the object my ($usernum, $userid, $usercnum, $userfirstname, $usersurname, $userbranch, $branchname, $userflags, $emailaddress, $shibboleth)= [etc] # these are 10 -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=24754 --- Comment #22 from Marcel de Rooy <m.de.rooy@rijksmuseum.nl> --- Coming back here now -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=24754 --- Comment #24 from Marcel de Rooy <m.de.rooy@rijksmuseum.nl> --- (In reply to Jonathan Druart from comment #23)

(In reply to Marcel de Rooy from comment #22)

...

Coming back here now

Unassigning me as QA contact then. I was waiting for dependent bug to be ready actually.

Sorry for that. If you want to, go ahead. But I was wondering if we need to define these two as dependent actually. -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=24754 --- Comment #26 from Marcel de Rooy <m.de.rooy@rijksmuseum.nl> --- Having doubts if we should add userenv if there is already a login. If there is no login, the current approach seems fine to me. But if we have a login (by cookie from opac or intranet login), we loose the information which user triggered the action. Currently, such actions are saved into action_logs with the current user. | 2646 | 2020-03-20 13:05:58 | 0 | HOLDS | CANCEL | 181 | => This patch causes 0 | 2648 | 2020-03-20 13:17:12 | 51 | HOLDS | CANCEL | 182 | => Without patch, login from intranet | 2652 | 2020-03-20 13:23:29 | 51 | HOLDS | CANCEL | 183 | => Without patch, login from opac Keeping in FQA. Needs feedback Please add why we need the dependency. Only for the test plan? -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=24754 --- Comment #28 from Marcel de Rooy <m.de.rooy@rijksmuseum.nl> --- And would it be interesting to differentiate between no context (from that server) and with context from a logged-in user? -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=24754 Nick Clemens <nick@bywatersolutions.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Failed QA |Signed Off -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=24754 Marcel de Rooy <m.de.rooy@rijksmuseum.nl> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #100305|0 |1 is obsolete| | --- Comment #30 from Marcel de Rooy <m.de.rooy@rijksmuseum.nl> --- Created attachment 101584 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=101584&action=edit Bug 24754: Set userenv for ilsdi requests Currently ILSDI scripts do not have a userenv set. This affects both action_logs and statistics. For action_logs we either get no user or, if there is an active session, we use that one. This can have unintended consequences and we should explicitly set the env so that system user (0) is recorded For statistics we need to specify a branch to record for actions like renewals. With bug 24759 we add an items method for this. We should take advantage of this method and use the 'opac' interface as the ilsdi scripts live there. To test: 1 - Enable ILSDI in systempreferences 2 - Checkout an item to a patron 3 - Enable Renewal logs 4 - Ensure you will be able to renew the item several times 5 - Open an incognito/private browser 6 - Renew item via ilsdi: http://localhost:8080/cgi-bin/koha/ilsdi.pl?service=RenewLoan&patron_id=5&item_id=1128 7 - Check action logs, interface may be random, user should be 0, if not make sure the user in step 9 is a different one 8 - In private browser open a new tab 9 - Sign in to staff client 10 - Return to other tab 11 - Refresh page to repeat renewal 12 - Check action_logs 13 - The second renewal indicates it was renewed via intranet by logged in staff member 14 - Apply patch 15 - restart_all 16 - Ensure staff client still signed in 17 - Repeate renewal again 18 - Check logs, renewal now indicates opac again (as it should), user is 0 even if logged in on other tab Signed-off-by: David Nind <david@davidnind.com> Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl> -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=24754 Marcel de Rooy <m.de.rooy@rijksmuseum.nl> changed: What |Removed |Added ---------------------------------------------------------------------------- QA Contact|testopia@bugs.koha-communit |m.de.rooy@rijksmuseum.nl |y.org | --- Comment #32 from Marcel de Rooy <m.de.rooy@rijksmuseum.nl> --- @RM: There is a dependency ! But note that it is imo not strictly needed to wait for it.. -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=24754 --- Comment #33 from Martin Renvoize <martin.renvoize@ptfs-europe.com> --- Nice work everyone! Pushed to master for 20.05 -- You are receiving this mail because: You are watching all bug changes.