Bug 12098 - User can edit some subcriptions he should be prevented to
Summary: User can edit some subcriptions he should be prevented to
Status: CLOSED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: Serials (show other bugs)
Version: master
Hardware: All All
: P5 - low minor (vote)
Assignee: Bugs List
QA Contact: Testopia
URL:
Keywords:
Depends on: 12080
Blocks:
  Show dependency treegraph
 
Reported: 2014-04-16 19:12 UTC by Frédéric Demians
Modified: 2015-06-04 23:30 UTC (History)
7 users (show)

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


Attachments
Refactor can_*_subscription in C4::Serials (13.99 KB, patch)
2014-04-17 13:41 UTC, Frédéric Demians
Details | Diff | Splinter Review
Fix C4::Serials::can_edit_subscription (3.39 KB, patch)
2014-04-17 13:42 UTC, Frédéric Demians
Details | Diff | Splinter Review
Bug 12080: Fix C4::Serials::can_edit_subscription (14.09 KB, patch)
2014-04-18 11:37 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 12080: Refactor can_*_subscription in C4::Serials (3.49 KB, patch)
2014-04-18 11:38 UTC, Kyle M Hall
Details | Diff | Splinter Review
[PASSED QA] Bug 12098: Fix C4::Serials::can_edit_subscription (14.18 KB, patch)
2014-04-18 13:46 UTC, Katrin Fischer
Details | Diff | Splinter Review
[PASSED QA] Bug 12098: Refactor can_*_subscription in C4::Serials (4.56 KB, patch)
2014-04-18 13:46 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 Frédéric Demians 2014-04-16 19:12:42 UTC
If the user doesn't have the superserials permission, or isn't superlibrarian, you can still edit a subscription accessing the subscription detail page through the serial collection page (for records with multiple subscriptions) or accessing the detail page directly.
Comment 1 Mark Tompsett 2014-04-16 22:37:30 UTC
Perhaps we should move JD's patches from bug 12080 here? I don't like the thought that his patches are hidden by being obsoleted on another bug.
Comment 2 Frédéric Demians 2014-04-17 10:33:39 UTC
(In reply to M. Tompsett from comment #1)
> Perhaps we should move JD's patches from bug 12080 here? I don't like the
> thought that his patches are hidden by being obsoleted on another bug.

Jonathan have them on his repo, and he will re-post them here. That's my understanding. If you prefer, or would like to test them now, I can attach his patches here as they are. Just tell.
Comment 3 Katrin Fischer 2014-04-17 10:43:30 UTC
Jonathan said he would be gone for a week - so reattaching the patches here would be nice.
Comment 4 Frédéric Demians 2014-04-17 13:41:49 UTC Comment hidden (obsolete)
Comment 5 Frédéric Demians 2014-04-17 13:42:30 UTC Comment hidden (obsolete)
Comment 6 Kyle M Hall 2014-04-18 11:37:22 UTC Comment hidden (obsolete)
Comment 7 Kyle M Hall 2014-04-18 11:38:36 UTC Comment hidden (obsolete)
Comment 8 Katrin Fischer 2014-04-18 12:20:00 UTC
After applying all the patches for 12048 - 12080 - 12098 t/db_dependent/Bookseller.t fails for me. I can imagine the test might be wrong now or needs to be rewritten a bit. I will continue testing for now.
Comment 9 Katrin Fischer 2014-04-18 13:46:13 UTC
Created attachment 27290 [details] [review]
[PASSED QA] Bug 12098: Fix C4::Serials::can_edit_subscription

This patch adds some unit tests for the can_edit_subscription and add a
new can_show_subscription subroutines.

http://bugs.koha-community.org/show_bug.cgi?id=12098

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Signed-off-by: Katrin Fischer <Katrin.Fischer.83@web.de>
Notes on second patch.
Comment 10 Katrin Fischer 2014-04-18 13:46:22 UTC
Created attachment 27291 [details] [review]
[PASSED QA] Bug 12098: Refactor can_*_subscription in C4::Serials

http://bugs.koha-community.org/show_bug.cgi?id=12098

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Signed-off-by: Katrin Fischer <Katrin.Fischer.83@web.de>
Tested on top of patches for 12048 and 12080.

Subscription search
- superlibrarian, IndyBranches on/off - always sees all subscriptions
- superserials, IndyBranches on/off - always sees all subscriptions
- no superserials, IndyBranches on - only sees own subscriptions
Note: Subscriptions without branches will only show, when all subscriptions
      are visible. In a future enh it might be good to enforce setting a
      branch, when IndyBranches is used.
- no superserials, IndyBranches off - always sees all subscriptions

Subscription editing
- superlibrarian, IndyBranches on/off - can edit all subscriptions
- superserials, IndyBranches on/off - can edit all subscriptions
- no superserials, IndyBranches on - can only edit own subscriptons and
  subscriptions without branch
  NOTE: it would make sense to also allow Edit > Edit as new (duplicate)
  here, so one can copy the subscription from another branch to modify
  it for the own branch.

Passes tests in t, xt and QA script, also newly provided unit tests.
Comment 11 Galen Charlton 2014-04-18 20:53:57 UTC
Pushed to master, along with an important follow-up that puts the C4::Serials::can_show_subscription routine into use (as otherwise, it would be a routine that had zero actual affect).

BUT NOTE:

    Note that there is user-visible change: if a subscription has a
    blank library, all users with serials permissions will be able
    to view and/or edit it.  It remains to be determined whether
    we *want* such subscriptions to exist, or if they should only
    be tied to specific libraries.

If this change in behavior isn't desirable, please open a new bug.

Thanks, Jonathan!
Comment 12 Fridolin Somers 2014-08-07 08:32:16 UTC
Pushed to 3.14.x, will be in 3.14.10