Bug 8435 - new permissions on subscriptions "superserials"
Summary: new permissions on subscriptions "superserials"
Status: CLOSED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: Serials (show other bugs)
Version: master
Hardware: All All
: P5 - low enhancement (vote)
Assignee: Jonathan Druart
QA Contact:
URL:
Keywords:
Depends on:
Blocks: 8436 8438 14932
  Show dependency treegraph
 
Reported: 2012-07-13 08:10 UTC by claire.hernandez@biblibre.com
Modified: 2017-04-12 17:25 UTC (History)
12 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 8435: If IndependantBranches is on, Serials can't be manage from others libraries (28.12 KB, patch)
2012-07-13 14:52 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 8435: If IndependantBranches is on, Serials can't be manage from others libraries (27.13 KB, patch)
2012-08-06 10:03 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 8435: If IndependantBranches is on, Serials can't be manage from others libraries (27.18 KB, patch)
2012-08-07 10:36 UTC, Chris Cormack
Details | Diff | Splinter Review
Bug 8435: Followup: FIX if IndependantBranches is OFF (1.43 KB, patch)
2012-08-30 12:37 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 8435: If IndependantBranches is on, Serials can't be manage from others libraries (25.78 KB, patch)
2012-11-12 13:20 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 8435: If IndependantBranches is on, Serials can't be manage from others libraries (24.11 KB, patch)
2013-02-26 09:19 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 8435 [Follow-up] Check superserials permission on subscription-add (2.08 KB, patch)
2013-03-19 15:07 UTC, Julian Maurice
Details | Diff | Splinter Review
Bug 8435 [Follow-up] Check superserials permission on subscription-add (2.14 KB, patch)
2013-03-20 09:33 UTC, Paul Poulain
Details | Diff | Splinter Review
Screenshot - count on tab is wrong now (16.15 KB, image/png)
2013-03-22 09:48 UTC, Katrin Fischer
Details
Bug 8435: Followup Fix permission description (9.48 KB, patch)
2013-03-22 10:36 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 8435: Fix hide subscriptions if not allow (3.71 KB, patch)
2013-03-22 10:36 UTC, Jonathan Druart
Details | Diff | Splinter Review
[PASSED QA] Bug 8435 [Follow-up] Check superserials permission on subscription-add (2.21 KB, patch)
2013-03-22 15:08 UTC, Katrin Fischer
Details | Diff | Splinter Review
Bug 8435: If IndependantBranches is on, Serials can't be manage from others libraries (24.58 KB, patch)
2013-03-22 15:09 UTC, Katrin Fischer
Details | Diff | Splinter Review
[PASSED QA] Bug 8435 [Follow-up] Check superserials permission on subscription-add (2.21 KB, patch)
2013-03-22 15:10 UTC, Katrin Fischer
Details | Diff | Splinter Review
[PASSED QA] Bug 8435: Followup Fix permission description (9.42 KB, patch)
2013-03-22 15:10 UTC, Katrin Fischer
Details | Diff | Splinter Review
[PASSED QA] Bug 8435: Fix hide subscriptions if not allow (3.74 KB, patch)
2013-03-22 15:11 UTC, Katrin Fischer
Details | Diff | Splinter Review
Bug 8435: If IndependantBranches is on, Serials can't be manage from others libraries (28.47 KB, patch)
2013-03-27 15:55 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 8435: Followup add unit tests for can_edit_subscription (3.71 KB, patch)
2013-03-27 15:55 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 8435: If IndependantBranches is on, Serials can't be manage from others libraries (28.51 KB, patch)
2013-06-27 14:00 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 8435: Followup add unit tests for can_edit_subscription (3.28 KB, patch)
2013-06-27 14:00 UTC, Jonathan Druart
Details | Diff | Splinter Review
QAed patch (3.33 KB, patch)
2013-06-27 21:41 UTC, Chris Cormack
Details | Diff | Splinter Review
Bug 8435: use a transaction for unit tests (1.17 KB, patch)
2013-06-28 07:28 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 8435: The unit tests should create the biblio (1.80 KB, patch)
2013-08-21 12:24 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 8435: If IndependantBranches is on, Serials can't be manage from others libraries (28.49 KB, patch)
2013-09-20 19:54 UTC, Chris Cormack
Details | Diff | Splinter Review
Bug 8435: Followup add unit tests for can_edit_subscription (3.33 KB, patch)
2013-09-20 19:54 UTC, Chris Cormack
Details | Diff | Splinter Review
Bug 8435: use a transaction for unit tests (1.17 KB, patch)
2013-09-20 19:55 UTC, Chris Cormack
Details | Diff | Splinter Review
Bug 8435: The unit tests should create the biblio (1.86 KB, patch)
2013-09-20 19:55 UTC, Chris Cormack
Details | Diff | Splinter Review
Bug 8435: If IndependantBranches is on, Serials can't be manage from others libraries (28.52 KB, patch)
2013-10-31 08:51 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 8435: Followup add unit tests for can_edit_subscription (3.32 KB, patch)
2013-10-31 08:51 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 8435: use a transaction for unit tests (1.17 KB, patch)
2013-10-31 08:51 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 8435: The unit tests should create the biblio (1.86 KB, patch)
2013-10-31 08:51 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 8435: Fix unit tests (3.16 KB, patch)
2013-10-31 08:51 UTC, Jonathan Druart
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description claire.hernandez@biblibre.com 2012-07-13 08:10:37 UTC
Adds superserials permission "Manage subscriptions from any branch". Permission on subscription: superserials permission will let a librarian manage any subscription, otherwise subscriptions can be managed only by a librarian of the branch of the subscription (for libraries with independantbranches=ON). (Don't take into account 'cannotedit' value from C4::Serials subs)

Hide other branch's subscriptions: "Set $sub->{'cannotedit'} for subs the user should not see. If a subscription is not linked to the current working branch, hide it.

(BibLibre MT6723 and MT6710)
Comment 1 Jonathan Druart 2012-07-13 14:52:56 UTC Comment hidden (obsolete)
Comment 2 Alexandre Lengereau 2012-08-06 08:53:06 UTC
Sandbox test:

CONFLICT (content): Merge conflict in installer/data/mysql/updatedatabase.pl<br/>
CONFLICT (content): Merge conflict in koha-tmpl/intranet-tmpl/prog/en/modules/serials/serials-home.tt<br/>
Comment 3 Jonathan Druart 2012-08-06 10:03:29 UTC Comment hidden (obsolete)
Comment 4 Chris Cormack 2012-08-07 10:36:08 UTC Comment hidden (obsolete)
Comment 5 Marcel de Rooy 2012-08-30 11:15:34 UTC
Just a question about:
sub can_edit_subscription {
+    my ( $subscription, $userid ) = @_;
+    my $flags = C4::Context->userenv->{flags};
+    $userid ||= C4::Context->userenv->{'id'};
+    if( C4::Context->preference('IndependantBranches')
+        and (
+               $flags % 2 == 1
+               or C4::Auth::haspermission( $userid, {serials => 'superserials'}),
+            or not defined $subscription->{branchcode}
+            or $subscription->{branchcode} eq ''
+            or $subscription->{branchcode} eq C4::Context->userenv->{'branch'}
+        )
+    ) {
+        return 1;
+    }
+     return 0;
+}

If IndependantBranches is off, you may not edit? Somewhat confusing. Please clarify..
Comment 6 Jonathan Druart 2012-08-30 12:37:13 UTC Comment hidden (obsolete)
Comment 7 Jonathan Druart 2012-11-12 13:20:32 UTC Comment hidden (obsolete)
Comment 8 Jonathan Druart 2012-11-12 13:21:34 UTC
rebased and squashed patch
Comment 9 Katrin Fischer 2012-11-12 13:35:27 UTC
What is the behaviour with indybranches turned OFF?
Comment 10 Jonathan Druart 2012-11-12 14:18:51 UTC
(In reply to comment #9)
> What is the behaviour with indybranches turned OFF?

Normally there is no change.
Comment 11 Katrin Fischer 2012-11-12 14:56:01 UTC
Ok, I think that is a problem.

Because if I have a permission, I expect it to do something. If there is a dependence on another setting, we need to find a better way to implement this - I think.
Comment 12 Jonathan Druart 2012-11-12 15:45:41 UTC
(In reply to comment #11)
> Ok, I think that is a problem.
> 
> Because if I have a permission, I expect it to do something. If there is a
> dependence on another setting, we need to find a better way to implement
> this - I think.

Hi Katrin,
What do you propose ?
I don't see a lot of ways.
I had 2 choices : 1 permission or yet another syspref.
I thought the fact to hide/show subscriptions is close to a permission than a syspref.
Comment 13 Katrin Fischer 2012-11-12 16:00:29 UTC
I am not sure. 

Maybe it should do something if independant branches is OFF too?
We would have to be careful not to introduce a change of behaviour. 

Could the permission only be visible if the syspref is set to ON?

Another option, I like less, is to add a note to the permission. But I think as the majority of libraries is not using independentbranches it might still be confusing.
Comment 14 Jonathan Druart 2012-11-12 16:41:09 UTC
(In reply to comment #13)
> I am not sure. 
> 
> Maybe it should do something if independant branches is OFF too?
> We would have to be careful not to introduce a change of behaviour. 
> 
> Could the permission only be visible if the syspref is set to ON?

I don't like this option. We don't want to add this kind of tests in the permissions management page. The permissions are enough complicated :)

> Another option, I like less, is to add a note to the permission. But I think
> as the majority of libraries is not using independentbranches it might still
> be confusing.

Yes and it is the same kind of tests.

Don't you think a short explanation in the manual would be enough ?
Comment 15 Katrin Fischer 2012-11-13 14:17:20 UTC
I really think the manual would not be enough for people to not get confused. Would it make sense to make it not dependend on independent branches at all? I guess then we would have to have it turned on by default for all staff members with permissions to work with subscriptions?
Comment 16 Paul Poulain 2013-01-07 14:30:13 UTC
(In reply to comment #13)
> Another option, I like less, is to add a note to the permission. But I think
> as the majority of libraries is not using independentbranches it might still
> be confusing.
Katrin,

Jonathan & I just spoke of this problem. We investigated many options, and feel the only one that is consistent is to:
 * have the permission defaulted to 0
 * update the permission description to say "For libraries with systemPreference IndependantBranche=ON, Manage subscriptions from any branch"

Thus:
 * libraries with IndependantBranches= OFF => no change, nothing to do
 * libraries with IndependantBranches=ON => default value for existing users is correct, the description is clear for new librarians


Overall, we agree it's a little bit wobbly, because it mixes sysprefs and permission. But all this stack of features activated or not when IndependantBranches is set should probably be rewritten (and it's far from the scope of this patch...)
Comment 17 Jonathan Druart 2013-02-26 09:19:27 UTC Comment hidden (obsolete)
Comment 18 Frederic Durand 2013-03-19 10:23:00 UTC
Tried to test it on sandbox, did not work :
Applying: Bug 8435: If IndependantBranches is on, Serials can't be manage from others libraries Using index info to reconstruct a base tree... Falling back to patching base and 3-way merge... Auto-merging installer/data/mysql/de-DE/mandatory/userpermissions.sql Auto-merging installer/data/mysql/en/mandatory/userpermissions.sql Auto-merging installer/data/mysql/es-ES/mandatory/userpermissions.sql Auto-merging installer/data/mysql/fr-FR/1-Obligatoire/userpermissions.sql Auto-merging installer/data/mysql/it-IT/necessari/userpermissions.sql Auto-merging installer/data/mysql/nb-NO/1-Obligatorisk/userpermissions.sql Auto-merging installer/data/mysql/pl-PL/mandatory/userpermissions.sql Auto-merging installer/data/mysql/updatedatabase.pl Auto-merging serials/subscription-add.pl Bug 8435 - new permissions on subscriptions "superserials"
Comment 19 Jonathan Druart 2013-03-19 10:29:14 UTC
No error, just warnings.
Comment 20 Frederic Durand 2013-03-19 13:42:55 UTC
Testing is ok, superserial let you see and manage every serials.
But I found a lack for serial creation. As a superserial you can only create a subscription for your own site, no options to choose another library on subscription-add.pl.
Comment 21 Julian Maurice 2013-03-19 15:07:17 UTC Comment hidden (obsolete)
Comment 22 Paul Poulain 2013-03-20 09:33:04 UTC
Patch tested with a sandbox, by Frederic Durand <frederic.durand@unilim.fr>
Comment 23 Paul Poulain 2013-03-20 09:33:22 UTC Comment hidden (obsolete)
Comment 24 Katrin Fischer 2013-03-22 08:46:48 UTC
Taking a look at this now :)
Comment 25 Katrin Fischer 2013-03-22 09:30:38 UTC
This patches fail xt/permissions.t because the new permissions are missing in 2 files - can you please provide a follow up?
Comment 26 Katrin Fischer 2013-03-22 09:43:46 UTC
Can we agree on amending the permissions description to indicate the relationship with indybranches?

Suggestion:
Manage subscriptions from any branch (only applies when IndependantBranches is used)
Comment 27 Katrin Fischer 2013-03-22 09:48:04 UTC Comment hidden (obsolete)
Comment 28 Jonathan Druart 2013-03-22 10:36:04 UTC Comment hidden (obsolete)
Comment 29 Jonathan Druart 2013-03-22 10:36:06 UTC Comment hidden (obsolete)
Comment 30 Jonathan Druart 2013-03-22 10:37:25 UTC
Katrin, I don't know if you require another sign-off on these 2 last patches.
Feel free to switch to Needs Signed-off if needed.
Comment 31 Katrin Fischer 2013-03-22 15:08:38 UTC Comment hidden (obsolete)
Comment 32 Katrin Fischer 2013-03-22 15:09:21 UTC Comment hidden (obsolete)
Comment 33 Katrin Fischer 2013-03-22 15:10:29 UTC Comment hidden (obsolete)
Comment 34 Katrin Fischer 2013-03-22 15:10:50 UTC Comment hidden (obsolete)
Comment 35 Katrin Fischer 2013-03-22 15:11:09 UTC Comment hidden (obsolete)
Comment 36 Jared Camins-Esakov 2013-03-23 04:16:02 UTC
These patches no longer apply and the merge conflict is too complex for me to resolve on my own. Bumping for 3.14 release.
Comment 37 Jonathan Druart 2013-03-27 15:55:07 UTC Comment hidden (obsolete)
Comment 38 Jonathan Druart 2013-03-27 15:55:15 UTC Comment hidden (obsolete)
Comment 39 Jonathan Druart 2013-03-27 15:56:29 UTC
attachment 16943 [details] [review] is a rebased and squashed patch.
Last patch is a new patch (introduced a little unit test for the can_edit_subscription routine).
The last patch needs QA.
Comment 40 Jonathan Druart 2013-06-27 14:00:13 UTC Comment hidden (obsolete)
Comment 41 Jonathan Druart 2013-06-27 14:00:21 UTC Comment hidden (obsolete)
Comment 42 Jonathan Druart 2013-06-27 14:01:20 UTC
Rebased patches. Fix conflicts with bug 10080.
Comment 43 Jonathan Druart 2013-06-27 14:02:08 UTC
Only the last patch (unit tests) needs QA. The first one already passed QA.
Comment 44 Chris Cormack 2013-06-27 21:41:56 UTC Comment hidden (obsolete)
Comment 45 Jonathan Druart 2013-06-28 07:28:03 UTC Comment hidden (obsolete)
Comment 46 Jonathan Druart 2013-08-21 12:24:21 UTC Comment hidden (obsolete)
Comment 47 Chris Cormack 2013-09-20 19:54:18 UTC Comment hidden (obsolete)
Comment 48 Chris Cormack 2013-09-20 19:54:54 UTC Comment hidden (obsolete)
Comment 49 Chris Cormack 2013-09-20 19:55:05 UTC Comment hidden (obsolete)
Comment 50 Chris Cormack 2013-09-20 19:55:29 UTC Comment hidden (obsolete)
Comment 51 Galen Charlton 2013-10-31 03:02:40 UTC
Marking as failed QA, sorry.

This patch series (after fixing merge) conflicts, blows up every nearly every DB-dependent serials unit test that calls GetSubscription() with an error similar to this:

Can't use an undefined value as a HASH reference at /home/gmc/koha/C4/Serials.pm line 2808, <DATA> line 558.

This, ultimately, is because the patch series makes it *required* that there is a valid userenv, which was not the case before. can_edit_subscription() should be revised so that it doesn't crash if there's no userenv.
Comment 52 Jonathan Druart 2013-10-31 08:51:19 UTC
Created attachment 22624 [details] [review]
Bug 8435: If IndependantBranches is on, Serials can't be manage from others libraries

In the serial module, we want to hide serials from others libraries.
New permission 'superserials'. If a patron have this permission, he can
override this restriction.

Test plan:
- Switch on the IndependantBranches syspref
- Add the permission 'superserials' for a patron and test you can
  navigate and see all serials
- Remove this permission and test you cannot manage/view subscriptions
  from others libraries

Signed-off-by: Frederic Durand <frederic.durand@unilim.fr>
Signed-off-by: Katrin Fischer <Katrin.Fischer.83@web.de>
Comment 53 Jonathan Druart 2013-10-31 08:51:25 UTC
Created attachment 22625 [details] [review]
Bug 8435: Followup add unit tests for can_edit_subscription

Signed-off-by: Chris Cormack <chrisc@catalyst.net.nz>
Comment 54 Jonathan Druart 2013-10-31 08:51:31 UTC
Created attachment 22626 [details] [review]
Bug 8435: use a transaction for unit tests
Comment 55 Jonathan Druart 2013-10-31 08:51:36 UTC
Created attachment 22627 [details] [review]
Bug 8435: The unit tests should create the biblio

Previous version does not create the biblio.

Signed-off-by: Chris Cormack <chris@bigballofwax.co.nz>
Comment 56 Jonathan Druart 2013-10-31 08:51:41 UTC
Created attachment 22628 [details] [review]
Bug 8435: Fix unit tests

C4::Serials::can_edit_subscription now deals with C4::Context->userenv.
Comment 57 Jonathan Druart 2013-10-31 08:53:50 UTC
(In reply to Galen Charlton from comment #51)
> This, ultimately, is because the patch series makes it *required* that there
> is a valid userenv, which was not the case before. can_edit_subscription()
> should be revised so that it doesn't crash if there's no userenv.

Galen, I fixed that and added a new unit tests to check the userenv value.
The behavior is: without userenv, subscriptions cannot be edited.
Comment 58 Galen Charlton 2013-10-31 15:38:38 UTC
Pushed to master.  Thanks, Jonathan!