Bug 9448 - Add separate permission for writing off fees
Summary: Add separate permission for writing off fees
Status: CLOSED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: Patrons (show other bugs)
Version: Main
Hardware: All All
: P5 - low enhancement (vote)
Assignee: Kyle M Hall
QA Contact:
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2013-01-22 18:52 UTC by Kyle M Hall
Modified: 2014-12-07 20:07 UTC (History)
10 users (show)

See Also:
Change sponsored?: ---
Patch complexity: Medium patch
Documentation contact:
Documentation submission:
Text to go in the release notes:
Version(s) released in:


Attachments
Bug 9448 - Add separate permission for writing off fees (15.44 KB, patch)
2013-01-22 18:57 UTC, Kyle M Hall
Details | Diff | Splinter Review
Kyle M Hall <kyle@bywatersolutions.com> (15.49 KB, patch)
2013-03-21 11:25 UTC, Brendan Gallagher
Details | Diff | Splinter Review
Bug 9448 - Add separate permission for writing off fees (15.51 KB, patch)
2013-10-02 18:36 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 9448 - Add separate permission for writing off fees (15.48 KB, patch)
2013-12-13 14:50 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 9448 - Add separate permission for writing off fees (15.54 KB, patch)
2013-12-13 16:15 UTC, Biblibre Sandboxes
Details | Diff | Splinter Review
Bug 9448 - Add separate permission for writing off fees (15.58 KB, patch)
2014-04-17 11:52 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 9448 [QA Followup] (9.23 KB, patch)
2014-04-17 11:52 UTC, Kyle M Hall
Details | Diff | Splinter Review
[PASSED QA] Bug 9448 - Add separate permission for writing off fees (15.88 KB, patch)
2014-04-17 12:31 UTC, Katrin Fischer
Details | Diff | Splinter Review
[PASSED QA] Bug 9448 [QA Followup] (9.89 KB, patch)
2014-04-17 12:32 UTC, Katrin Fischer
Details | Diff | Splinter Review
Bug 9448 [QA Followup] - Hide 'Forgive overdue charges' for returns if user does not have writeoff permission (1.17 KB, patch)
2014-04-17 12:39 UTC, Kyle M Hall
Details | Diff | Splinter Review
[PASSED QA] Bug 9448 [QA Followup] - Hide 'Forgive overdue charges' for returns if user does not have writeoff permission (1.27 KB, patch)
2014-04-17 12:53 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 Kyle M Hall 2013-01-22 18:52:39 UTC
Right now, Koha has a single all or nothing permission for viewing/altering fees. It would be good to have a separate sub-permission for write-offs and a new sub-permission for all remaining fee related activities ( which could also be split up in the future ).
Comment 1 Kyle M Hall 2013-01-22 18:57:31 UTC Comment hidden (obsolete)
Comment 2 Alex Arnaud 2013-03-19 16:24:48 UTC
Doesn't apply on master
Comment 3 Brendan Gallagher 2013-03-21 11:25:24 UTC Comment hidden (obsolete)
Comment 4 Brendan Gallagher 2013-03-21 11:26:47 UTC
Comment on attachment 16606 [details] [review]
Kyle M Hall <kyle@bywatersolutions.com>

Mistake with the subject name when rebasing.  My mistake.
Comment 5 I'm just a bot 2013-09-18 11:15:22 UTC
Applying: Bug 9448 - Add separate permission for writing off fees
Using index info to reconstruct a base tree...
M	installer/data/mysql/de-DE/mandatory/userpermissions.sql
M	installer/data/mysql/en/mandatory/userpermissions.sql
M	installer/data/mysql/es-ES/mandatory/userpermissions.sql
M	installer/data/mysql/fr-FR/1-Obligatoire/userpermissions.sql
M	installer/data/mysql/it-IT/necessari/userpermissions.sql
M	installer/data/mysql/nb-NO/1-Obligatorisk/userpermissions.sql
M	installer/data/mysql/pl-PL/mandatory/userpermissions.sql
M	installer/data/mysql/ru-RU/mandatory/permissions_and_user_flags.sql
M	installer/data/mysql/uk-UA/mandatory/permissions_and_user_flags.sql
M	installer/data/mysql/updatedatabase.pl
M	koha-tmpl/intranet-tmpl/prog/en/modules/members/pay.tt
M	members/boraccount.pl
M	members/mancredit.pl
M	members/maninvoice.pl
M	members/pay.pl
M	members/paycollect.pl
M	members/printinvoice.pl
Falling back to patching base and 3-way merge...
Auto-merging members/printinvoice.pl
CONFLICT (content): Merge conflict in members/printinvoice.pl
Auto-merging members/paycollect.pl
Auto-merging members/pay.pl
Auto-merging members/maninvoice.pl
Auto-merging members/mancredit.pl
CONFLICT (content): Merge conflict in members/mancredit.pl
Auto-merging members/boraccount.pl
Auto-merging koha-tmpl/intranet-tmpl/prog/en/modules/members/pay.tt
CONFLICT (content): Merge conflict in koha-tmpl/intranet-tmpl/prog/en/modules/members/pay.tt
Auto-merging installer/data/mysql/updatedatabase.pl
CONFLICT (content): Merge conflict in installer/data/mysql/updatedatabase.pl
Auto-merging installer/data/mysql/uk-UA/mandatory/permissions_and_user_flags.sql
Auto-merging installer/data/mysql/ru-RU/mandatory/permissions_and_user_flags.sql
Auto-merging installer/data/mysql/pl-PL/mandatory/userpermissions.sql
Auto-merging installer/data/mysql/nb-NO/1-Obligatorisk/userpermissions.sql
Auto-merging installer/data/mysql/it-IT/necessari/userpermissions.sql
Auto-merging installer/data/mysql/fr-FR/1-Obligatoire/userpermissions.sql
Auto-merging installer/data/mysql/es-ES/mandatory/userpermissions.sql
Auto-merging installer/data/mysql/en/mandatory/userpermissions.sql
Auto-merging installer/data/mysql/de-DE/mandatory/userpermissions.sql
Patch failed at 0001 Bug 9448 - Add separate permission for writing off fees
The copy of the patch that failed is found in:
   /home/christopher/git/koha/.git/rebase-apply/patch
When you have resolved this problem, run "git am --continue".
If you prefer to skip this patch, run "git am --skip" instead.
To restore the original branch and stop patching, run "git am --abort".
Comment 6 Kyle M Hall 2013-10-02 18:36:31 UTC Comment hidden (obsolete)
Comment 7 I'm just a bot 2013-10-03 07:01:24 UTC
Patch applied cleanly, go forth and signoff
Comment 8 I'm just a bot 2013-10-28 06:21:52 UTC
Patch applied cleanly, go forth and signoff
Comment 9 Kyle M Hall 2013-12-13 14:50:57 UTC Comment hidden (obsolete)
Comment 10 Biblibre Sandboxes 2013-12-13 16:14:54 UTC
Patch tested with a sandbox, by Joel Sasse <jsasse@plumcreeklibrary.net>
Comment 11 Biblibre Sandboxes 2013-12-13 16:15:37 UTC Comment hidden (obsolete)
Comment 12 Jonathan Druart 2013-12-24 11:29:12 UTC
Kyle,
Since bug 6427 deletes all the files that this patch changes, is it still valid?
Comment 13 Kyle M Hall 2014-01-10 12:18:56 UTC
Yes, it's still valid unless 6427 manages to get pushed first. In either case I will have to add a followup to 6427 for this same behavior. This patch will at the very least implement the permission itself. Adding the limit to the new accounting system will be a fairly simple process.

(In reply to Jonathan Druart from comment #12)
> Kyle,
> Since bug 6427 deletes all the files that this patch changes, is it still
> valid?
Comment 14 Jonathan Druart 2014-01-16 13:03:16 UTC
QA comment:

It works well but could be improved.
The permissions in the .pl are not checked in the template files. So it could happen a link is displayed but the user is rejected if he clicks.
Example: set circulate, catalogue and updatecharges>remaining_permissions. Go on a patron page, the 'Fines' tab is displayed on the left. Click on it, you are redirected to the loggin page with an error "Error: You do not have permission to access this page."

Marked as Failed QA.
Comment 15 Kyle M Hall 2014-02-18 18:51:10 UTC
Jonathan, can you point out which template files you believe this is an issue for? I'm not seeing it.

Kyle

(In reply to Jonathan Druart from comment #14)
> QA comment:
> 
> It works well but could be improved.
> The permissions in the .pl are not checked in the template files. So it
> could happen a link is displayed but the user is rejected if he clicks.
> Example: set circulate, catalogue and updatecharges>remaining_permissions.
> Go on a patron page, the 'Fines' tab is displayed on the left. Click on it,
> you are redirected to the loggin page with an error "Error: You do not have
> permission to access this page."
> 
> Marked as Failed QA.
Comment 16 Jonathan Druart 2014-02-19 10:50:40 UTC
(In reply to Kyle M Hall from comment #15)
> Jonathan, can you point out which template files you believe this is an
> issue for? I'm not seeing it.

Did you try to reproduce my example?
Comment 17 Jonathan Druart 2014-04-11 11:54:32 UTC
Kyle: ping!
Comment 18 Kyle M Hall 2014-04-17 11:52:17 UTC Comment hidden (obsolete)
Comment 19 Kyle M Hall 2014-04-17 11:52:25 UTC Comment hidden (obsolete)
Comment 20 Katrin Fischer 2014-04-17 12:31:50 UTC
Created attachment 27237 [details] [review]
[PASSED QA] Bug 9448 - Add separate permission for writing off fees

Adds new sub-permissions to updatecharges, 'writeoff' for writing off fees
and 'remaining_permissions' for all other fee related activites.

Test plan:
1) Apply patch
2) Run updatedatabase.pl
3) Remove the 'writeoff' permission from your privileges
4) Attempt to write off a fee, you should not be able to
5) Re-add the 'writeoff' permission
6) Attempt to write off a fee, you should now be able to do so

Signed-off-by: Joel Sasse <jsasse@plumcreeklibrary.net>
Signed-off-by: Katrin Fischer <Katrin.Fischer.83@web.de>
Comments on second patch.
Comment 21 Katrin Fischer 2014-04-17 12:32:04 UTC
Created attachment 27238 [details] [review]
[PASSED QA] Bug 9448 [QA Followup]

Rephrases permission description a bit to make it clearer and easier to
translate.

Adds German descriptions as requested by katrin.

Signed-off-by: Katrin Fischer <Katrin.Fischer.83@web.de>
Passes QA script and all tests.
Tested with various permission combinations:
- superlibrarian
- updatecharges (full permission)
- only 'remaining permissions for updating charges'
It works as expected.

Note: I think we need a follow up to include the
'Forgive overdue charges' checkbox on the check in page
in the new permission.
Comment 22 Kyle M Hall 2014-04-17 12:39:25 UTC Comment hidden (obsolete)
Comment 23 Katrin Fischer 2014-04-17 12:53:43 UTC
Created attachment 27240 [details] [review]
[PASSED QA] Bug 9448 [QA Followup] - Hide 'Forgive overdue charges' for returns if user does not have writeoff permission

Signed-off-by: Katrin Fischer <Katrin.Fischer.83@web.de>
Works as described, template change only.
Comment 24 Galen Charlton 2014-04-17 16:40:19 UTC
Pushed to master, along with a couple follow-ups to tweak wording and keep circ staff honest.

Thanks, Kyle!
Comment 25 Katrin Fischer 2014-04-18 10:13:24 UTC
Hm, looking at permissions again today, I wonder if    

(10, 'remaining_permissions',...

should be

(10, 'updatecharge_remaining_permissions',...

To match with 

( 1, 'circulate_remaining_permissions', 'Remaining circulation permissions'),
( 3, 'parameters_remaining_permissions', 'Remaining system parameters permissions'),