Created attachment 9274 [details] without parameters permission See attached. The "Modification Log" tab does not appear while viewing a patron's record (Detail, Check Out) unless the staff member has the "Set Koha system parameters" priviledge enabled.
Created attachment 9275 [details] with parameters permission
Maybe this would be a good one to clean up for the academy too. As far I can see there are 4 files linking to the modification logs: koha-tmpl/intranet-tmpl/prog/en/includes/biblio-view-menu.inc 33 [% IF ( CAN_user_tools_view_system_logs ) %][% IF ( logview ) %]<li class="active">[% ELSE %]<li>[% END %]<a href="/cgi-bin/koha/tools/viewlog.pl?do_it=1&modules=CATALOGUING&action=MODIFY&object=[% biblionumber %]">Modification log</a> </li>[% END %] koha-tmpl/intranet-tmpl/prog/en/includes/circ-menu.inc 77 [% IF ( logview ) %]<li class="active">[% ELSE %]<li>[% END %]<a href="/cgi-bin/koha/tools/viewlog.pl?do_it=1&modules=MEMBERS&modules=circulation&object=[% borrowernumber %]&src=circ">Modification log</a></li> koha-tmpl/intranet-tmpl/prog/en/includes/circ-menu.tt 77 [% IF ( CAN_user_parameters ) %][% IF ( logview ) %]<li class="active">[% ELSE %]<li>[% END %]<a href="/cgi-bin/koha/tools/viewlog.pl?do_it=1&modules=MEMBERS&modules=circulation&object=[% borrower.borrowernumber %]&src=circ">Modification log</a></li>[% END %] koha-tmpl/intranet-tmpl/prog/en/includes/members-menu.inc 13 [% IF ( logview ) %]<li class="active">[% ELSE %]<li>[% END %]<a href="/cgi-bin/koha/tools/viewlog.pl?do_it=1&modules=MEMBERS&action=MODIFY&object=[% borrowernumber %]">Modification log</a></li> Only the first seems to be using the correct permission check.
Created attachment 35262 [details] [review] Bug 7996: Patron Modification Log Requires Parameters Permission To test 1- Open 'set permissions' on patron page and uncheck '(parameters) Set koha system parameters' 2- Open patron page and notice that there is no 'modification log' tab 3- Apply patch 4- Refresh page and notice 'modification log' tab is there
Hi Rochelle, the way it works after your patch, it will need both the permission 'parameters' and 'view_system_logs'. I think what we need to do here is take a look at all the conditions [IF ...] that apply to show the link and check which are really needed / make sense. My feeling is that it would make sense to use 'view_system_logs' everywhere, as this is the more specific permission. What do others think?
Created attachment 77974 [details] [review] Bug 7996: Correct wrong permissions for modification log To test: 1 - Create a staff patron with catalogue,borrowers,and view_system_logs permissions 2 - Log in using that patron 3 - Go to tools 4 - Click 'Log viewer' 5 - You get a blank page 6 - View borrower circ screen, note you do not see 'Modification log' 7 - Edit a borrower, note you do not see 'Modification log' 8 - Apply patch 9 - 'Log Viewer' should now load correctly 10 - You should see 'Modification log' tab during circ 11 - You should see 'Modification log' tab when editing patron
Created attachment 77991 [details] [review] Bug 7996: Correct wrong permissions for modification log To test: 1 - Create a staff patron with catalogue,borrowers,and view_system_logs permissions 2 - Log in using that patron 3 - Go to tools 4 - Click 'Log viewer' 5 - You get a blank page 6 - View borrower circ screen, note you do not see 'Modification log' 7 - Edit a borrower, note you do not see 'Modification log' 8 - Apply patch 9 - 'Log Viewer' should now load correctly 10 - You should see 'Modification log' tab during circ 11 - You should see 'Modification log' tab when editing patron Signed-off-by: Owen Leonard <oleonard@myacpl.org>
It seems that we could remove the condition in viewlog.tt, do you confirm?
Created attachment 77993 [details] [review] Bug 7996: Correct wrong permissions for modification log To test: 1 - Create a staff patron with catalogue,borrowers,and view_system_logs permissions 2 - Log in using that patron 3 - Go to tools 4 - Click 'Log viewer' 5 - You get a blank page 6 - View borrower circ screen, note you do not see 'Modification log' 7 - Edit a borrower, note you do not see 'Modification log' 8 - Apply patch 9 - 'Log Viewer' should now load correctly 10 - You should see 'Modification log' tab during circ 11 - You should see 'Modification log' tab when editing patron Signed-off-by: Owen Leonard <oleonard@myacpl.org>
(In reply to Jonathan Druart from comment #8) > Created attachment 77993 [details] [review] [review] Rebased on top of bug 13618.
(In reply to Jonathan Druart from comment #7) > It seems that we could remove the condition in viewlog.tt, do you confirm? Yes, the permission check is done in the script, we shouldn't need a second
Created attachment 78052 [details] [review] Bug 7996: Correct wrong permissions for modification log To test: 1 - Create a staff patron with catalogue,borrowers,and view_system_logs permissions 2 - Log in using that patron 3 - Go to tools 4 - Click 'Log viewer' 5 - You get a blank page 6 - View borrower circ screen, note you do not see 'Modification log' 7 - Edit a borrower, note you do not see 'Modification log' 8 - Apply patch 9 - 'Log Viewer' should now load correctly 10 - You should see 'Modification log' tab during circ 11 - You should see 'Modification log' tab when editing patron Signed-off-by: Owen Leonard <oleonard@myacpl.org> Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Created attachment 78053 [details] [review] Bug 7996: (QA follow-up) Remove unnecessary permission check view logs template Permission is checked in the perl file, no need to check again in the template.
Awesome work all! Pushed to master for 18.11
Pushed to 18.05.x for 18.05.03
Pushed to 17.11.x for 17.11.10