|
Description
Nick Clemens (kidclamp)
2020-02-28 01:42:20 UTC
Created attachment 99768 [details] [review] 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) Comment on attachment 99768 [details] [review] Bug 24754: Set userenv for ilsdi requests Review of attachment 99768 [details] [review]: ----------------------------------------------------------------- ::: opac/ilsdi.pl @@ +129,5 @@ > > +# Set the userenv > +C4::Context->_new_userenv(1); > +C4::Context->set_userenv( > + undef, undef, undef, 'CRON', 'CRON', undef, Those 'CRON' values stand out as a little weird here to me. Comment on attachment 99768 [details] [review] Bug 24754: Set userenv for ilsdi requests Review of attachment 99768 [details] [review]: ----------------------------------------------------------------- ::: opac/ilsdi.pl @@ +129,5 @@ > > +# Set the userenv > +C4::Context->_new_userenv(1); > +C4::Context->set_userenv( > + undef, undef, undef, 'CRON', 'CRON', undef, OK.. so those are being used as 'firstname' and 'surname'.. weird.. CRON is probably OK @@ +132,5 @@ > +C4::Context->set_userenv( > + undef, undef, undef, 'CRON', 'CRON', undef, > + undef, undef, undef, undef, undef > +); > +C4::Context->interface('opac'); #FIXME 'ilsdi' should be a valid value? Valid values are currently 'api', 'opac', 'intranet', 'sip', 'cron' and 'commandline'.. I reckon 'api' would fit reasonably well here? Created attachment 99772 [details] [review] 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) Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Created attachment 99773 [details] [review] Bug 24754: (QA follow-up) opac -> api for 'interface' Created attachment 99778 [details] [review] 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 Created attachment 100000 [details] [review] Bug 24754: Simplify tests Created attachment 100001 [details] [review] Bug 24754: Test intranet at the same time Created attachment 100006 [details] [review] 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 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. Created attachment 100035 [details] [review] 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 Why do you pass the branchcode's checkout and do not let renewal_branchcode/OpacRenewalBranch does the job? (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? Please change status if there is something with the patch and it needs an answer. This helps us to skip such patches. It is the responsibility of the author to respond. Created attachment 100269 [details] [review] 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 (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 Ok but then it will explode in some cases Nick. For instance the logged in user does not exist. (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" Created attachment 100305 [details] [review] 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> Looking here 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
(In reply to Marcel de Rooy from comment #20) > Koha::Script does this: > Might be that the author copied this part of the code? Yes, copied from there > 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? Any recommendation on the digits? Use a timestamp or the same for each? > 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 Koha Script appears to be right, just a typo when changing to cron I think Coming back here now (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. (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. Created attachment 101094 [details] [review] Bug 24754: (QA follow-up) Add timestamp, remove 11th parameter Signed-off-by: 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? (In reply to Marcel de Rooy from comment #26) > 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. ILSDI does not depend on user authentication, the permissions of the user to edit items or place holds etc do not match. ILSDI is its own way to interact with KOha - I think in these instances it is more correct to indicate a system user as opposed to sign in user. The trigger for this patch was actually the Aspen discovery system. The ILSDI calls are routed from a central server which is also passing other calls to Koha. We began to see holds being manipulated by random patrons - because without a session set explicitly for ILSDI any other session from the server was being picked. It was causing logging issues and confusion > Please add why we need the dependency. Only for the test plan? Yes, I think there were small differences in logs with those patches, so they are related And would it be interesting to differentiate between no context (from that server) and with context from a logged-in user? I agree ILSDI should set its own userenv as it is really not dependent on the current cookie. This is a nasty side effect of our coding practices. Created attachment 101584 [details] [review] 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> Created attachment 101585 [details] [review] Bug 24754: (QA follow-up) Add timestamp, remove 11th parameter Signed-off-by: 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.. Nice work everyone! Pushed to master for 20.05 not backported to 19.11. due to dependencies |