Bug 6898 - circulation librarians can't run overdues without reports permissions
Summary: circulation librarians can't run overdues without reports permissions
Status: CLOSED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: Reports (show other bugs)
Version: master
Hardware: All All
: P3 major (vote)
Assignee: Fridolin SOMERS
QA Contact: Jonathan Druart
URL:
Keywords:
: 5874 (view as bug list)
Depends on:
Blocks:
 
Reported: 2011-09-21 13:00 UTC by Nicole C. Engard
Modified: 2014-12-07 20:03 UTC (History)
12 users (show)

See Also:
Change sponsored?: ---
Patch complexity: Medium patch
Who signed the patch off:
Text to go in the release notes:
Version(s) released in:


Attachments
Bug 6898 - circulation librarians can't run overdues without reports permissions (2.78 KB, patch)
2012-02-10 15:50 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 6898 - circulation librarians can't run overdues without reports permissions (2.62 KB, patch)
2012-03-16 14:55 UTC, Kyle M Hall
Details | Diff | Splinter Review
Proposed patch (circulation permission) (11.01 KB, patch)
2013-01-17 17:17 UTC, Fridolin SOMERS
Details | Diff | Splinter Review
Bug 6898 - circulation librarians can't run overdues without reports permissions (11.04 KB, patch)
2013-02-08 13:15 UTC, Kyle M Hall
Details | Diff | Splinter Review
Proposed patch (followup) (1.81 KB, patch)
2013-03-07 16:13 UTC, Fridolin SOMERS
Details | Diff | Splinter Review
Proposed patch (followup) (1.64 KB, patch)
2013-03-07 16:29 UTC, Fridolin SOMERS
Details | Diff | Splinter Review
[SIGNED-OFF] Bug 6898 - circulation librarians can't run overdues without reports permissions followup (1.69 KB, patch)
2013-03-07 17:18 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 6898 - circulation librarians can't run overdues without reports permissions (11.12 KB, patch)
2013-03-07 17:20 UTC, Kyle M Hall
Details | Diff | Splinter Review
[SIGNED-OFF] Bug 6898 - circulation librarians can't run overdues without reports permissions followup (1.69 KB, patch)
2013-03-07 17:20 UTC, Kyle M Hall
Details | Diff | Splinter Review
[SIGNED-OFF] Bug 6898 - circulation librarians can't run overdues without reports permissions (11.27 KB, patch)
2013-03-08 08:37 UTC, Fridolin SOMERS
Details | Diff | Splinter Review
Bug 6898 - circulation librarians can't run overdues without reports permissions (11.10 KB, patch)
2013-03-08 13:52 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 6898 - circulation librarians can't run overdues without reports permissions followup (1.68 KB, patch)
2013-03-08 13:52 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 6898 - circulation librarians can't run overdues without reports permissions (11.33 KB, patch)
2013-03-11 15:34 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 6898 - circulation librarians can't run overdues without reports permissions followup (1.75 KB, patch)
2013-03-11 15:34 UTC, Jonathan Druart
Details | Diff | Splinter Review
update screen .26 to .30 (30.06 KB, image/png)
2013-03-18 00:00 UTC, Nicole C. Engard
Details
update summary (66.72 KB, image/png)
2013-03-18 00:01 UTC, Nicole C. Engard
Details
Bug 6898 - updatedatabase second query not executed (1.00 KB, patch)
2013-06-05 12:21 UTC, Fridolin SOMERS
Details | Diff | Splinter Review
Bug 6898 - updatedatabase second query not executed (1.05 KB, patch)
2013-07-06 08:36 UTC, Chris Cormack
Details | Diff | Splinter Review
[PASSED QA] Bug 6898 - updatedatabase second query not executed (1.35 KB, patch)
2013-07-11 06:21 UTC, Katrin Fischer
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Nicole C. Engard 2011-09-21 13:00:22 UTC
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?
Comment 1 Owen Leonard 2011-10-27 16:52:19 UTC
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)
Comment 2 Nicole C. Engard 2011-11-05 19:19:53 UTC
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.
Comment 3 Kyle M Hall 2012-02-10 15:50:21 UTC Comment hidden (obsolete)
Comment 4 Kyle M Hall 2012-02-10 15:54:01 UTC
*** Bug 5874 has been marked as a duplicate of this bug. ***
Comment 5 Nicole C. Engard 2012-03-16 14:44:24 UTC
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
Comment 6 Kyle M Hall 2012-03-16 14:55:49 UTC Comment hidden (obsolete)
Comment 7 Nicole C. Engard 2012-03-16 15:04:30 UTC
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.
Comment 8 Kyle M Hall 2012-03-19 14:09:36 UTC
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.
Comment 9 Nicole C. Engard 2012-03-19 14:31:45 UTC
Kyle,

You are right, the problem with the overdues with fines report is broken in master. But there were other issues as well.

Nicole
Comment 10 JaTara Barnes 2012-12-05 21:59:57 UTC
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.
Comment 11 Fridolin SOMERS 2013-01-17 17:17:31 UTC Comment hidden (obsolete)
Comment 12 Jonathan Druart 2013-01-30 16:06:20 UTC
Kyle: What do you think about this last patch? Do you agree with it?

Fridolyn: Why do you remove the circulate_remaining_permissions permission?
Comment 13 Paul Poulain 2013-02-08 10:22:23 UTC
(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)
Comment 14 Fridolin SOMERS 2013-02-08 10:31:01 UTC
(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.
Comment 15 Kyle M Hall 2013-02-08 13:14:12 UTC
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.
Comment 16 Kyle M Hall 2013-02-08 13:15:22 UTC Comment hidden (obsolete)
Comment 17 Fridolin SOMERS 2013-02-08 13:31:53 UTC
I have to create a follow-up to have a better updatedatabase : when a user has report permissions, this new permission must be set.
Comment 18 Kyle M Hall 2013-02-08 14:57:25 UTC
(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
Comment 19 Fridolin SOMERS 2013-03-07 16:13:46 UTC Comment hidden (obsolete)
Comment 20 Fridolin SOMERS 2013-03-07 16:24:43 UTC
I set status to signed-off.
Followup can be validated by QA ?
Comment 21 Fridolin SOMERS 2013-03-07 16:29:22 UTC Comment hidden (obsolete)
Comment 22 Kyle M Hall 2013-03-07 17:18:16 UTC Comment hidden (obsolete)
Comment 23 Kyle M Hall 2013-03-07 17:19:52 UTC
(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
Comment 24 Kyle M Hall 2013-03-07 17:20:15 UTC Comment hidden (obsolete)
Comment 25 Kyle M Hall 2013-03-07 17:20:37 UTC Comment hidden (obsolete)
Comment 26 Kyle M Hall 2013-03-07 17:21:45 UTC
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
Comment 27 Fridolin SOMERS 2013-03-08 08:37:20 UTC Comment hidden (obsolete)
Comment 28 Fridolin SOMERS 2013-03-08 08:38:22 UTC
(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.
Comment 29 Kyle M Hall 2013-03-08 13:52:14 UTC Comment hidden (obsolete)
Comment 30 Kyle M Hall 2013-03-08 13:52:47 UTC Comment hidden (obsolete)
Comment 31 Jonathan Druart 2013-03-11 15:33:31 UTC
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.
Comment 32 Jonathan Druart 2013-03-11 15:34:34 UTC Comment hidden (obsolete)
Comment 33 Jonathan Druart 2013-03-11 15:34:37 UTC Comment hidden (obsolete)
Comment 34 Jared Camins-Esakov 2013-03-13 12:15:44 UTC
This patch has been pushed to master.
Comment 35 Nicole C. Engard 2013-03-17 02:07:16 UTC
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
Comment 36 Jared Camins-Esakov 2013-03-17 02:10:08 UTC
Shows up in updatedatabase for me. "Add circ permission overdues_report"
Comment 37 Nicole C. Engard 2013-03-17 02:57:34 UTC
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.
Comment 38 Jared Camins-Esakov 2013-03-17 13:26:25 UTC
(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.
Comment 39 Nicole C. Engard 2013-03-18 00:00:42 UTC
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)
Comment 40 Nicole C. Engard 2013-03-18 00:01:16 UTC
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.
Comment 41 Jared Camins-Esakov 2013-03-18 00:03:57 UTC
And you updated master again?
Comment 42 Nicole C. Engard 2013-03-18 00:20:47 UTC
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
Comment 43 Melia Meggs 2013-04-26 22:19:22 UTC
Is this patch going to make it into 3.10.x stable?
Comment 44 Chris Cormack 2013-04-27 05:20:31 UTC
Pushed to 3.10.x will be in 3.10.6
Comment 45 Fridolin SOMERS 2013-06-05 12:18:31 UTC
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.
Comment 46 Fridolin SOMERS 2013-06-05 12:21:00 UTC Comment hidden (obsolete)
Comment 47 Marcel de Rooy 2013-06-05 12:27:12 UTC
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..
Comment 48 Fridolin SOMERS 2013-06-05 13:07:19 UTC
(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 ? ;)
Comment 49 Chris Cormack 2013-07-06 08:36:57 UTC Comment hidden (obsolete)
Comment 50 Katrin Fischer 2013-07-11 06:21:54 UTC
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>
Comment 51 Galen Charlton 2013-07-11 14:53:15 UTC
Pushed follow-up to master.  Thanks, Fridolyn!
Comment 52 Tomás Cohen Arazi 2013-07-25 17:19:51 UTC
Follow up has been pushed to 3.12.x, will be in 3.12.3.

Thanks Fridolyn!
Comment 53 Bernardo Gonzalez Kriegel 2013-08-04 22:57:44 UTC
Followup pushed to 3.10.x, will be in 3.10.10