This is related to bug 5874 which made it so that you couldn't run overdues without the execute reports permissions. This change is not seen as a fix but more as a bug by most libraries who don't want their circ staff running reports via the reports module but thought that reports listed on the circulation page were not linked to that. I move that we instead have a new permission for running circulation reports so that those reports can be controlled separately from the reports module - otherwise why are they not listed with the other reports on the reports module?
What separate permissions should there be? I think the reason for wrapping the overdues report in a reports permission was to prevent people from running a resource-intensive script. If we add a circulation reports permission, do we need two "levels" of circ report permission? - run standard circ reports (like hold queue) - run heavy circ reports (overdues)
I'm open to anything that allows libraries to have their circ staff run the circulation reports without needing access to run all reports. That's the way it has been since 3.0 and seems strange to remove those rights at this point. As for the resource intensive script - that's why we have the preference to show filters first.
Created attachment 7568 [details] [review] Bug 6898 - circulation librarians can't run overdues without reports permissions This commit adds a new user permission execute_overdues_report for the reports module, and requires this permission to run the overdue items report, rather than requiring full reports permissions.
*** Bug 5874 has been marked as a duplicate of this bug. ***
Bug 6898 - circulation librarians can't run overdues without reports permissions Apply? [yn] y Applying: Bug 6898 - circulation librarians can't run overdues without reports permissions error: patch failed: installer/data/mysql/updatedatabase.pl:4663 error: installer/data/mysql/updatedatabase.pl: patch does not apply Patch failed at 0001 Bug 6898 - circulation librarians can't run overdues without reports permissions When you have resolved this problem run "git am --resolved". If you would prefer to skip this patch, instead run "git am --skip". To restore the original branch and stop patching run "git am --abort". Patch left in /tmp/Bug-6898---circulation-librarians-cant-run-overdue-8sdIWz.patch
Created attachment 8233 [details] [review] Bug 6898 - circulation librarians can't run overdues without reports permissions This commit adds a new user permission execute_overdues_report for the reports module, and requires this permission to run the overdue items report, rather than requiring full reports permissions.
With execute_overdues_report and all circulation permissions the link to the overdues report does not show and if you click Overdues with Fines you get this: Software error: Undefined subroutine &C4::Members::checkoverdues called at /home/nengard/kohaclone/C4/Members.pm line 475. For help, please send mail to the webmaster (webmaster@debian), giving this error message and the time and date of the error. With permission to execute overdues but not access circ you can't see the report. I think for this we should just add a note that circ permissions is also required to execute overdues.
Are you sure your error is related to this patch? It likely that it may be that the codebase you applied that patch to was already borked beforehand. (In reply to comment #7) > With execute_overdues_report and all circulation permissions the link to the > overdues report does not show and if you click Overdues with Fines you get > this: > > > Software error: > > Undefined subroutine &C4::Members::checkoverdues called at > /home/nengard/kohaclone/C4/Members.pm line 475. > > For help, please send mail to the webmaster (webmaster@debian), giving this > error message and the time and date of the error. > > > With permission to execute overdues but not access circ you can't see the > report. I think for this we should just add a note that circ permissions is > also required to execute overdues.
Kyle, You are right, the problem with the overdues with fines report is broken in master. But there were other issues as well. Nicole
Are there any updates to this? It is now December and a fix hasn't made it into the codebase yet. It is worth trying to get this completed because this feature is very much needed. There is no reason that every staff member should have access to the entire set of reports.
Created attachment 14680 [details] [review] Proposed patch (circulation permission) I renew discussion with a rewritten patch. I tested proposed patch and noticed a permission into reports module gives access to nearly entire reports module. I think this module is designed for only the two existing permissions. I propose a permission of circulation module dedicated to overdues page. Patch adds database scripts for all languages. I only translated french, translations are welcome. Also adds management of the "Overdues" link in circulation home page.
Kyle: What do you think about this last patch? Do you agree with it? Fridolyn: Why do you remove the circulate_remaining_permissions permission?
(In reply to comment #12) > Kyle: What do you think about this last patch? Do you agree with it? > > Fridolyn: Why do you remove the circulate_remaining_permissions permission? Looking at both patches, my comments: * the initial one (2.62kb) fails QA : a permission is added in english only. I also think it should be related to circulation, because the report is in circ directory, so let be consistent * Frido, your patch (the 11.01KB one) is written "From: Kyle Hall". Am I misunderstanding something, or that's a mistake ? Will mark the 1st one as obsolete, and back to need signoff for the 11.01KB one ! (note : I haven't tested any of those patches yet)
(In reply to comment #12) > Kyle: What do you think about this last patch? Do you agree with it? > > Fridolyn: Why do you remove the circulate_remaining_permissions permission? I think "circulate_remaining_permissions" is not needed when a permission exists for a page. Like it says in its name "circulate_remaining_permissions" is for pages that do not have a dedicated permission. Of course, giving only overdues permission will rarely be used. But we can imagine a librarian having only reports permissions, he can have only overdues permission to access the page (via a bookmark maybe) and not the rest of circulation module.
That makes sense to me! (In reply to comment #14) > (In reply to comment #12) > > Kyle: What do you think about this last patch? Do you agree with it? > > > > Fridolyn: Why do you remove the circulate_remaining_permissions permission? > > I think "circulate_remaining_permissions" is not needed when a permission > exists for a page. Like it says in its name > "circulate_remaining_permissions" is for pages that do not have a dedicated > permission. > > Of course, giving only overdues permission will rarely be used. > But we can imagine a librarian having only reports permissions, he can have > only overdues permission to access the page (via a bookmark maybe) and not > the rest of circulation module.
Created attachment 15162 [details] [review] Bug 6898 - circulation librarians can't run overdues without reports permissions This patch adds a new user permission overdues_report for the circulation module, and requires this permission to run the overdue items report, rather than requiring full reports permissions. Test plan : With a user with permissions circulate_remaining_permissions : - Add overdues_report permission to user - Go to circulation home => you see "Overdues" link - Clik on it => you are allowed to access and use this page - Remove overdues_report permission to user - Go to circulation home => you do not see "Overdues" link - Edit URL : <server>/cgi-bin/koha/circ/overdue.pl => you are not allowed to access this page - Go to main page => you don't have access to reports module Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
I have to create a follow-up to have a better updatedatabase : when a user has report permissions, this new permission must be set.
(In reply to comment #17) > I have to create a follow-up to have a better updatedatabase : when a user > has report permissions, this new permission must be set. Good point. Setting to 'Failed QA' for now. Kyle
Created attachment 15939 [details] [review] Proposed patch (followup) Follow up for better update database. Adds new permission for users with all report permissions and circulation remaining permission. This will not concern user having all circulation permissions. There is no need because when all module is permitted, sub-permissions does not exist in user_permissions, module permission is stored into borrowers.flags.
I set status to signed-off. Followup can be validated by QA ?
Created attachment 15940 [details] [review] Proposed patch (followup) Small error in first patch.
Created attachment 15946 [details] [review] [SIGNED-OFF] Bug 6898 - circulation librarians can't run overdues without reports permissions followup Better update database. Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
(In reply to comment #20) > I set status to signed-off. > Followup can be validated by QA ? Fridolyn, I don't see your SO on my original patch. Kyle
Created attachment 15947 [details] [review] Bug 6898 - circulation librarians can't run overdues without reports permissions This patch adds a new user permission overdues_report for the circulation module, and requires this permission to run the overdue items report, rather than requiring full reports permissions. Test plan : With a user with permissions circulate_remaining_permissions : - Add overdues_report permission to user - Go to circulation home => you see "Overdues" link - Clik on it => you are allowed to access and use this page - Remove overdues_report permission to user - Go to circulation home => you do not see "Overdues" link - Edit URL : <server>/cgi-bin/koha/circ/overdue.pl => you are not allowed to access this page - Go to main page => you don't have access to reports module Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Created attachment 15948 [details] [review] [SIGNED-OFF] Bug 6898 - circulation librarians can't run overdues without reports permissions followup Better update database. Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
I've uploaded a rebased version of the patch. Also, thanks for the followup Fridolyn! (In reply to comment #23) > (In reply to comment #20) > > I set status to signed-off. > > Followup can be validated by QA ? > > Fridolyn, I don't see your SO on my original patch. > > Kyle
Created attachment 15966 [details] [review] [SIGNED-OFF] Bug 6898 - circulation librarians can't run overdues without reports permissions
(In reply to comment #23) > (In reply to comment #20) > > I set status to signed-off. > > Followup can be validated by QA ? > > Fridolyn, I don't see your SO on my original patch. > > Kyle Here it is.
Created attachment 15974 [details] [review] Bug 6898 - circulation librarians can't run overdues without reports permissions Reuploading to correct patch order.
Created attachment 15975 [details] [review] Bug 6898 - circulation librarians can't run overdues without reports permissions followup Reuploading to correct patch order.
QA comment: This patch adds a new permission (overdues_report) for the circulation module. This permission allows a librarian to access to the overdues page. The updatedb script updates user permissions for users having permissions for the report module + circulate_remaining_permissions (same behavior as before). There is no link to this page (so nothing to hide). Marked as Passed QA.
Created attachment 16034 [details] [review] Bug 6898 - circulation librarians can't run overdues without reports permissions This patch adds a new user permission overdues_report for the circulation module, and requires this permission to run the overdue items report, rather than requiring full reports permissions. Test plan : With a user with permissions circulate_remaining_permissions : - Add overdues_report permission to user - Go to circulation home => you see "Overdues" link - Clik on it => you are allowed to access and use this page - Remove overdues_report permission to user - Go to circulation home => you do not see "Overdues" link - Edit URL : <server>/cgi-bin/koha/circ/overdue.pl => you are not allowed to access this page - Go to main page => you don't have access to reports module Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com> Signed-off-by: Fridolyn SOMERS <fridolyn.somers@biblibre.com> Signed-off-by: Jonathan Druart <jonathan.druart@biblibre.com>
Created attachment 16035 [details] [review] Bug 6898 - circulation librarians can't run overdues without reports permissions followup Better update database. Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com> Signed-off-by: Jonathan Druart <jonathan.druart@biblibre.com>
This patch has been pushed to master.
I did a pull from master just now and I don't see a new permission. I was prompted to upgrade my database, but the upgrade notes said nothing about adding a new permission. Are we sure that this new permission is added properly on upgrade? Nicole
Shows up in updatedatabase for me. "Add circ permission overdues_report"
How would I force another update database? Like I said it did prompt me to upgrade and I did, but there was no note about the new permission and the new permission is not showing at all on my patron records.
(In reply to comment #37) > How would I force another update database? Like I said it did prompt me to > upgrade and I did, but there was no note about the new permission and the > new permission is not showing at all on my patron records. Set the version back to 3.1100026. I was on a slightly older version of master when I tested it. I found and fixed the problem so all should be working now.
Created attachment 16207 [details] update screen .26 to .30 I'm sorry but it's not updating the database with the new permissions. This is all I get when upgrading from .26 to .30: Update report : Upgrade to 3.11.00.030 done (Add system preference FinesIncludeGracePeriod)
Created attachment 16208 [details] update summary there is no new patron permission added to the db and when i look at a patron record there is no new permission to choose.
And you updated master again?
I did update master again. I tested on another machine and it worked okay so I went back further on this one and it finally updated - something maybe with the update from .26-.30 because when you do .25 to .100 it works a-ok. Anyway, it's not a probably with this patch or one that most people upgrading will encounter cause they will be going from stable to stable. Nicole
Is this patch going to make it into 3.10.x stable?
Pushed to 3.10.x will be in 3.10.6
Small problem : Second query in updatedatabase is not executed. This second query added by followup patch is not mandatory, so maybe correcting updatedatabase.pl is enough.
Created attachment 18670 [details] [review] Bug 6898 - updatedatabase second query not executed Patch to correct updatedatabase. I created it on the bug branch but it applies on a master.
Fridolyn: Are you sure that this will really fix your problem? Just updating an older dbrev will not help someone that never comes back to that piece of code..
(In reply to comment #47) > Fridolyn: > Are you sure that this will really fix your problem? > Just updating an older dbrev will not help someone that never comes back to > that piece of code.. I know but this query adds the new permission to some borrowers. Now that this permission in not new, I think it whould be dangerous to add it to borrowers. This was only a way to ease the set of the new permission. For libraries that have already used updatedatabase, they will easily see for who the permission is missing and set it manually. Rather less permissions than to much no ? ;)
Created attachment 19434 [details] [review] Bug 6898 - updatedatabase second query not executed Signed-off-by: Chris Cormack <chrisc@catalyst.net.nz>
Created attachment 19541 [details] [review] [PASSED QA] Bug 6898 - updatedatabase second query not executed Signed-off-by: Chris Cormack <chrisc@catalyst.net.nz> Signed-off-by: Katrin Fischer <Katrin.Fischer.83@web.de> This will only fix the permissions for updates done after the patch has been applied. But I agree with Fridolyn, that we can not safely update permissions for existing installations. Signed-off-by: Katrin Fischer <Katrin.Fischer.83@web.de>
Pushed follow-up to master. Thanks, Fridolyn!
Follow up has been pushed to 3.12.x, will be in 3.12.3. Thanks Fridolyn!
Followup pushed to 3.10.x, will be in 3.10.10