Description
claire.hernandez@biblibre.com
2012-07-13 08:10:37 UTC
Created attachment 10821 [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 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/> Created attachment 11377 [details] [review] Bug 8435: If IndependantBranches is on, Serials can't be manage from others libraries Rebased patch Created attachment 11411 [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: Chris Cormack <chris@bigballofwax.co.nz> 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.. Created attachment 11892 [details] [review] Bug 8435: Followup: FIX if IndependantBranches is OFF > If IndependantBranches is off, you may not edit? Somewhat confusing. Please > clarify.. Hi Marcel, Yes ! Good catch, thanks. Switch back to needs SO. Created attachment 13396 [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 rebased and squashed patch What is the behaviour with indybranches turned OFF? (In reply to comment #9) > What is the behaviour with indybranches turned OFF? Normally there is no change. 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. (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. 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. (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 ? 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? (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...) Created attachment 15673 [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 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" No error, just warnings. 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. Created attachment 16404 [details] [review] Bug 8435 [Follow-up] Check superserials permission on subscription-add So subscription creation is now possible for other branches when you have this permission enabled. Patch tested with a sandbox, by Frederic Durand <frederic.durand@unilim.fr> Created attachment 16462 [details] [review] Bug 8435 [Follow-up] Check superserials permission on subscription-add So subscription creation is now possible for other branches when you have this permission enabled. Signed-off-by: Frederic Durand <frederic.durand@unilim.fr> Taking a look at this now :) This patches fail xt/permissions.t because the new permissions are missing in 2 files - can you please provide a follow up? 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) Created attachment 16740 [details]
Screenshot - count on tab is wrong now
With indybranches = ON and user not having superserials permission the shown subscriptions are alright, but the shown number on the tab is wrong.
Created attachment 16750 [details] [review] Bug 8435: Followup Fix permission description Created attachment 16751 [details] [review] Bug 8435: Fix hide subscriptions if not allow If a user does not have the superserials permission and the syspref IndependantBranches is ON, he cannot show subscriptions from others branches. 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. Created attachment 16798 [details] [review] [PASSED QA] Bug 8435 [Follow-up] Check superserials permission on subscription-add So subscription creation is now possible for other branches when you have this permission enabled. Signed-off-by: Frederic Durand <frederic.durand@unilim.fr> Signed-off-by: Katrin Fischer <Katrin.Fischer.83@web.de> Created attachment 16799 [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 Created attachment 16801 [details] [review] [PASSED QA] Bug 8435 [Follow-up] Check superserials permission on subscription-add So subscription creation is now possible for other branches when you have this permission enabled. Signed-off-by: Frederic Durand <frederic.durand@unilim.fr> Signed-off-by: Katrin Fischer <Katrin.Fischer.83@web.de> Created attachment 16802 [details] [review] [PASSED QA] Bug 8435: Followup Fix permission description Signed-off-by: Katrin Fischer <Katrin.Fischer.83@web.de> Created attachment 16803 [details] [review] [PASSED QA] Bug 8435: Fix hide subscriptions if not allow If a user does not have the superserials permission and the syspref IndependantBranches is ON, he cannot show subscriptions from others branches. Signed-off-by: Katrin Fischer <Katrin.Fischer.83@web.de> These patches no longer apply and the merge conflict is too complex for me to resolve on my own. Bumping for 3.14 release. Created attachment 16943 [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> Created attachment 16944 [details] [review] Bug 8435: Followup add unit tests for can_edit_subscription 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. Created attachment 19260 [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> Created attachment 19261 [details] [review] Bug 8435: Followup add unit tests for can_edit_subscription Rebased patches. Fix conflicts with bug 10080. Only the last patch (unit tests) needs QA. The first one already passed QA. Created attachment 19266 [details] [review] QAed patch Created attachment 19268 [details] [review] Bug 8435: use a transaction for unit tests Created attachment 20536 [details] [review] Bug 8435: The unit tests should create the biblio Previous version does not create the biblio. Created attachment 21286 [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> Created attachment 21287 [details] [review] Bug 8435: Followup add unit tests for can_edit_subscription Signed-off-by: Chris Cormack <chrisc@catalyst.net.nz> Created attachment 21288 [details] [review] Bug 8435: use a transaction for unit tests Created attachment 21289 [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> 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. 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> Created attachment 22625 [details] [review] Bug 8435: Followup add unit tests for can_edit_subscription Signed-off-by: Chris Cormack <chrisc@catalyst.net.nz> Created attachment 22626 [details] [review] Bug 8435: use a transaction for unit tests 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> Created attachment 22628 [details] [review] Bug 8435: Fix unit tests C4::Serials::can_edit_subscription now deals with C4::Context->userenv. (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. Pushed to master. Thanks, Jonathan! |