Bug 8438 - Allow staff to only claim serial issues of their own branch
Summary: Allow staff to only claim serial issues of their own branch
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: Marcel de Rooy
URL:
Whiteboard:
Keywords:
Depends on: 8435
Blocks:
  Show dependency treegraph
 
Reported: 2012-07-13 09:39 UTC by Jonathan Druart
Modified: 2016-06-21 21:40 UTC (History)
12 users (show)

See Also:
Change sponsored?: ---
Patch complexity: Small patch
Bot Control: Ignore this bug
When did the bot last check this:
Who signed the patch off:
Text to go in the release notes:


Attachments
Bug 8438: Users can only claim for serials related to their branch. (2.55 KB, patch)
2012-08-23 11:53 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 8438: Followup Set the superserials permission for all users with the claim_serials permission (1.35 KB, patch)
2012-10-16 08:35 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 8438: Followup Set the superserials permission for all users with the claim_serials permission (1.31 KB, patch)
2014-01-09 13:48 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 8438: Users can only claim for serials related to their branch. (2.26 KB, patch)
2014-03-17 09:10 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 8438: Followup Set the superserials permission for all users with the claim_serials permission (1.31 KB, patch)
2014-03-17 09:10 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 8438: Users can only claim for serials related to their branch. (2.09 KB, patch)
2014-06-20 13:05 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 8438: Followup Set the superserials permission for all users with the claim_serials permission (1.31 KB, patch)
2014-06-20 13:05 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 8438: Users can only claim for serials related to their branch. (1.70 KB, patch)
2014-06-24 14:10 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 8438: Followup Set the superserials permission for all users with the claim_serials permission (1.31 KB, patch)
2014-06-24 14:10 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 8438: Users can only claim for serials related to their branch. (6.60 KB, patch)
2014-06-25 10:55 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 8438: Followup Set the superserials permission for all users with the claim_serials permission (1.31 KB, patch)
2014-06-25 10:55 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 8438: Users can only claim for serials related to their branch. (6.63 KB, patch)
2015-01-14 09:32 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 8438: Followup Set the superserials permission for all users with the claim_serials permission (1.34 KB, patch)
2015-01-14 09:32 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 8438: Users can only claim for serials related to their branch. (6.67 KB, patch)
2015-01-15 14:00 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 8438: Followup Set the superserials permission for all users with the claim_serials permission (1.34 KB, patch)
2015-01-15 14:00 UTC, Jonathan Druart
Details | Diff | Splinter Review
[SIGNED OFF] Bug 8438: Users can only claim for serials related to their branch. (6.89 KB, patch)
2015-01-15 16:05 UTC, Paola Rossi
Details | Diff | Splinter Review
[SIGNED OFF] Bug 8438: Followup Set the superserials permission for all users with the claim_serials permission (1.44 KB, patch)
2015-01-15 16:06 UTC, Paola Rossi
Details | Diff | Splinter Review
[PASSED QA] Bug 8438: Users can only claim for serials related to their branch. (8.62 KB, patch)
2015-10-18 09:45 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 Jonathan Druart 2012-07-13 09:39:27 UTC
Only superlibrarian users and users with superserials permission could override this limitation.
Comment 1 Jonathan Druart 2012-08-23 11:53:18 UTC Comment hidden (obsolete)
Comment 2 Chris Cormack 2012-08-23 21:41:43 UTC
Not sure about this, it is a change of existing behavour. I think you should link it to independent branches syspref.

This new feature would indeed be a regression of a lot of my libraries. Where they have a serials librarian who looks after serials for multiple branches. But can't do circulation, cataloguing etc. Now they simply can't do that. Because that user would have to be super librarian.
Comment 3 Chris Cormack 2012-08-23 21:43:53 UTC
Actually I take that back, they would need superserials. So instead my only issue now is that this is going to change behaviour, and without warning (in updatedatabase or anything)
So people upgrading will not have any idea what happened, we need some way to warn people (this is a general koha issue)
Comment 4 Katrin Fischer 2012-08-23 21:47:18 UTC
Hm, maybe we could assign the users with serials permissions now the superserial permission on update? This way we would have no change of existing behaviour and now surprises. But not sure how hard that is - I know the permission flags are a bit cryptic.
Comment 5 Nicole C. Engard 2012-09-28 13:49:07 UTC
I too am worried about this changing the current behavior. When the change was made to the purchase suggestions by limiting to one branch it confused a lot of our users and isn't really the desired behavior for them either.  So I say either a preference or some way to make it less confusing.
Comment 6 Kyle M Hall 2012-10-05 14:20:45 UTC
Marking as Failed QA until the concerns in comments have been addressed.
Comment 7 Jonathan Druart 2012-10-16 08:35:38 UTC Comment hidden (obsolete)
Comment 8 Jonathan Druart 2013-03-19 10:31:50 UTC
Patches apply correctly here
Comment 9 Jonathan Druart 2014-01-09 13:48:11 UTC Comment hidden (obsolete)
Comment 10 Jonathan Druart 2014-01-09 13:48:25 UTC
Rebased patch.
Comment 11 Jonathan Druart 2014-03-17 09:10:20 UTC Comment hidden (obsolete)
Comment 12 Jonathan Druart 2014-03-17 09:10:37 UTC Comment hidden (obsolete)
Comment 13 Koha Team Lyon 3 2014-06-19 14:11:09 UTC
When logged as :

superlibrarian => OK : you can select subs from all branches

user without superserials permission => OK : only checkbox corresponding to loggin branch subscriptions are displayed. You can't select others

user with superserials permission => NOT OK : no difference from previous case. Only loggin branch subscriptions display checkboxes.


Olivier Crouzet
Comment 14 Jonathan Druart 2014-06-20 13:05:08 UTC Comment hidden (obsolete)
Comment 15 Jonathan Druart 2014-06-20 13:05:20 UTC Comment hidden (obsolete)
Comment 16 Koha Team Lyon 3 2014-06-24 13:36:40 UTC
Sorry Jonathan, when clicking on Claims link, I got this :
SOFTWARE ERROR
Global symbol "$subscription" requires explicit package name at /home/kohabac/src/serials/claims.pl line 71.

Olivier Crouzet
Comment 17 Jonathan Druart 2014-06-24 14:10:07 UTC Comment hidden (obsolete)
Comment 18 Jonathan Druart 2014-06-24 14:10:13 UTC Comment hidden (obsolete)
Comment 19 Jonathan Druart 2014-06-24 14:11:01 UTC
(In reply to Koha Team Lyon 3 from comment #16)
> Sorry Jonathan, when clicking on Claims link, I got this :

Erk, sorry. Should be fixed now.
Comment 20 Koha Team Lyon 3 2014-06-24 14:35:03 UTC
It's OK now for superlibrarians and users with superserials permissions, but it doesn't work any more for simple users : they are not restricted any more to the login branch...
Comment 21 Jonathan Druart 2014-06-25 10:50:56 UTC
(In reply to Koha Team Lyon 3 from comment #20)
> It's OK now for superlibrarians and users with superserials permissions, but
> it doesn't work any more for simple users : they are not restricted any more
> to the login branch...

Yes sorry, I changed the behavior without notifying the change.
But actually I am not sure. I decided to use the same method as on editing a subscription, but it's wrong!
I will submit another patch.
Comment 22 Jonathan Druart 2014-06-25 10:55:07 UTC Comment hidden (obsolete)
Comment 23 Jonathan Druart 2014-06-25 10:55:14 UTC Comment hidden (obsolete)
Comment 24 Jonathan Druart 2014-06-25 10:56:07 UTC
I didn't test all cases but the unit tests told me that should work! :)
Comment 25 Paola Rossi 2015-01-13 16:50:35 UTC
I've tried to apply the patch against master 3.19.00.005 head 12868

Applying: Bug 8438: Users can only claim for serials related to their branch.
Using index info to reconstruct a base tree...
Falling back to patching base and 3-way merge...
Auto-merging t/db_dependent/Serials_2.t
CONFLICT (content): Merge conflict in t/db_dependent/Serials_2.t
Auto-merging serials/claims.pl
CONFLICT (content): Merge conflict in serials/claims.pl
Auto-merging koha-tmpl/intranet-tmpl/prog/en/modules/serials/claims.tt
Auto-merging C4/Serials.pm
Failed to merge in the changes.
Patch failed at 0001 Bug 8438: Users can only claim for serials related to their branch.

So I pass the patch to "Patch doesn't apply" status.
Comment 26 Jonathan Druart 2015-01-14 09:32:35 UTC Comment hidden (obsolete)
Comment 27 Jonathan Druart 2015-01-14 09:32:42 UTC Comment hidden (obsolete)
Comment 28 Paola Rossi 2015-01-15 13:12:39 UTC
I've applied the patches against master 3.19.00.006 head 12889

On Home › Serials , clicking on Claims' link:

Software error:

Global symbol "@missingissues" requires explicit package name at [...]/serials/claims.pl line 83.
Execution of [...]/serials/claims.pl aborted due to compilation errors.

I pass the patch to "Failed QA" status.
Comment 29 Jonathan Druart 2015-01-15 14:00:28 UTC Comment hidden (obsolete)
Comment 30 Jonathan Druart 2015-01-15 14:00:34 UTC Comment hidden (obsolete)
Comment 31 Jonathan Druart 2015-01-15 14:01:24 UTC
(In reply to Paola Rossi from comment #28)
> I've applied the patches against master 3.19.00.006 head 12889
> 
> On Home › Serials , clicking on Claims' link:
> 
> Software error:

Sorry about that.
Comment 32 Paola Rossi 2015-01-15 16:05:25 UTC Comment hidden (obsolete)
Comment 33 Paola Rossi 2015-01-15 16:06:09 UTC Comment hidden (obsolete)
Comment 34 Paola Rossi 2015-01-15 16:15:43 UTC
I add to my comment 32:

I reset the PT patrons-users to:
[permission claim_serials: YES]
permission superserials: YES
under the syspref "IndependentBranches" set either to "Prevent" or to "Don't prevent".

For:
> Only users with superserials permission can override this limitation.
the PT patron-user could list AND claim:
A) subscriptions of his own branch,
B) subscriptions of other branch,
C) subscriptions without branch.
Comment 35 Katrin Fischer 2015-03-04 12:58:32 UTC
Ok, here it is: 

- We failed to get superserials and this patch set into the same version. 
- Superserials permission has been in use for quite a while now (see bug 8435) 
- This patch currently updates your permission set to add 'superserials' to it, if you had the permission to claim serials before.
- We have a time gap here that will cause some change in behaviour, no matter how we do it.

So this is the situation as I see it:

1) Not using IndependentBranches
= Nothing needs to be done, because nothing should change. 
No need to update the permissions in that case. 
We could add an additional check for IndependentBranches to the database update.

2) Using IndependentBranches

a) Having superserials permission already
= Nothing changes

b) Not having superserials permission before update
= Can now suddenly manage serials from other branches after the update.
If we don't update the permission:
= Can suddenly no longer claim issues from other branches

So what can we do here?
- Introducing a 'superserialclaim permission' seems silly.
- Taking away a smaller permission seems making more sense than giving access to even more data than before.
- I'd vote for 'don't add the superserials permission on update'.

Another thing: If I am not allowed to claim the issues from other branches, should these issues be visible to me? A user without superlibrarian/superserials will still see all issues that need claiming in the serial claim view, but they will miss the checkbox for claiming them. This doesn't seem to make sense, as I can't find the subscription using the search... but I can access it form the link. This is not very clean.

I think the access problem could be on a separate bug ("Can still access other branches subsriptions with IndyBranches and no superserials"), but I'd really like to see a filtered view for the claims.

Jonathan, could you take a look? Woudl also be good to get more opinions about b).
Comment 36 Jonathan Druart 2015-03-05 14:24:01 UTC
Comment on attachment 35312 [details] [review]
[SIGNED OFF] Bug 8438: Followup Set the superserials permission for all users with the claim_serials permission

So it is preferable not to update the user permissions and maybe let a note in the release notes to explain that if they have issues with serial permissions to check the user permissions.
Comment 37 Marcel de Rooy 2015-04-17 08:05:06 UTC
> If we don't update the permission:
> Can suddenly no longer claim issues from other branches

> Woudl also be good to get more opinions about b).

In view of the discussion above (including older comments at the top), this patch does not seem to be ready for inclusion yet.
I am moving it to FQA for now. Will ask for additional comments on the list.
Comment 38 Katrin Fischer 2015-04-17 08:30:50 UTC
(In reply to Jonathan Druart from comment #36)
> Comment on attachment 35312 [details] [review] [review]
> [SIGNED OFF] Bug 8438: Followup Set the superserials permission for all
> users with the claim_serials permission
> 
> So it is preferable not to update the user permissions and maybe let a note
> in the release notes to explain that if they have issues with serial
> permissions to check the user permissions.

Sounds like the best possible solution to me.

Could you take a look if filtering out the issues from other branches would be possible instead of just hiding the claim options?
Comment 39 Katrin Fischer 2015-04-17 09:04:55 UTC
After discussing the problem with "seeing issues from other branches" with Jonathan a bit more I think we can keep it like that for now. - You will see them, but you can't claim them. If libraries think this should be more restrictive, please open a new bug report.
Comment 40 Jonathan Druart 2015-04-17 11:10:37 UTC
So back in the QA queue?
Comment 41 Marcel de Rooy 2015-04-17 11:42:33 UTC
(In reply to Jonathan Druart from comment #40)
> So back in the QA queue?

The permission problem has not been solved? So, I would say: No. We can wait for some responses on the list first btw.
Comment 42 Jonathan Druart 2015-04-17 11:53:09 UTC
To me, there is no permission problem :)
Comment 43 Katrin Fischer 2015-04-17 12:04:30 UTC
I'd vote for: don't mingle with permissions, put a warn that now the behaviour is stricter and you can't claim issues from other branches without the proper permission for doing so.
Comment 44 Katrin Fischer 2015-07-09 10:02:02 UTC
Sadly, there was no reaction on the mailing list to the e-mail Marcel sent (thx for that!) I suggest moving this back into the queue - I personally see no cleaner way to do that, the only additions I can think of is a small warning in Release notes.

To me this kind of fixes a bug - I can't search for other branches subscriptions without superserials/superlibrarian, but I can claim their late issues... that appears illogical to me.
Comment 45 Katrin Fischer 2015-10-18 09:45:04 UTC
Created attachment 43560 [details] [review]
[PASSED QA] Bug 8438: Users can only claim for serials related to their branch.

Only superlibrarian users and users with superserials permission
can override this limitation.

This patch adds a new subroutine C4::Serials::can_claim_subscription.

Signed-off-by: Paola Rossi <paola.rossi@cineca.it>

The upgrading of the DB was as required:
for each [PT/S] patron with the permission "claim_serials" ON,
the permission "superserials" became[/was kept to] ON.

Then, after having checked the DB upgrading, to test the currently
adding limitation:
> Users can only claim for serials related to their branch
, I reset 2 PT/S patrons-users to:
permission claim_serials: YES
permission superserials: NO
and I set the syspref "IndependendeBranches" to "Prevent".

For:
> Only superlibrarian users can override this limitation.
the S patron-user could list AND claim:
A) subscriptions of his own branch,
B) subscriptions of other branch,
C) subscriptions without branch.

For:
> Only users with superserials permission can override this limitation.
the PT patron-user could list:
A) subscriptions of his own branch,
B) subscriptions of other branch,
C) subscriptions without branch,
and could claim only:
A) subscriptions of his own branch,
C) subscriptions without branch.

NB: a subscription is selected to be claimed.

Then I set the syspref "IndependendeBranches" to "Don't prevent".
The behaviour was [exactly the same as in master] without the added
limitation.

On [S/PT] patrons, if permission claim_serials was NO, no Claims
link was available on Serials' page, either under "Prevent" or under
"Don't prevent".

Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Added the results of Paola's testing to the commit message
as test plan.

Note to RM: Maybe we could add a note to the release notes, that
Koha now enforces superserials with independent branches better,
so people might have to adapt permissions in order to claim
for other branches.
Comment 46 Tomás Cohen Arazi 2015-11-04 14:31:47 UTC
Patch pushed to master.

Nice one Jonathan!