Bug 11625

Summary: Default to logged in library for circ rules and notices & slips
Product: Koha Reporter: Kyle M Hall <kyle>
Component: Architecture, internals, and plumbingAssignee: Kyle M Hall <kyle>
Status: CLOSED FIXED QA Contact:
Severity: enhancement    
Priority: P5 - low CC: brendan, gmcharlt, jonathan.druart, katrin.fischer, m.de.rooy, martin.renvoize, nick, nicole, paul.poulain
Version: master   
Hardware: All   
OS: All   
See Also: http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=11747
Change sponsored?: --- Patch complexity: Medium patch
Documentation contact: Documentation submission:
Text to go in the release notes:
Version(s) released in:
Bug Depends on:    
Bug Blocks: 11747, 15568    
Attachments: Bug 11625 - Default to logged in library for circ rules and notices & slips
[SIGNED-OFF] Bug 11625 - Default to logged in library for circ rules and notices & slips
Bug 11625 [QA Followup] - Default to logged in library for circ rules and notices & slips
[PASSED QA] Bug 11625 - Default to logged in library for circ rules and notices & slips
[PASSED QA] Bug 11625 [QA Followup] - Default to logged in library for circ rules and notices & slips
Bug 11625 [QA Followup 2] - Don't default to branch specific rules if only one branch exists
Bug 11625 [QA Followup 2] - Don't default to branch specific rules if only one branch exists
Bug 11625 - Default to logged in library for circ rules and notices & slips
Bug 11625 [QA Followup] - Default to logged in library for circ rules and notices & slips
Bug 11625 [QA Followup 2] - Don't default to branch specific rules if only one branch exists
Bug 11625 [QA Followup 3] - Use system preferences to control behavior
Bug 11625 - Default to logged in library for circ rules and notices & slips
Bug 11625 [QA Followup] - Default to logged in library for circ rules and notices & slips
Bug 11625 [QA Followup 2] - Don't default to branch specific rules if only one branch exists
Bug 11625 [QA Followup 3] - Use system preferences to control behavior
Bug 11625 - Default to logged in library for circ rules and notices & slips
Bug 11625 [QA Followup] - Default to logged in library for circ rules and notices & slips
Bug 11625 [QA Followup 2] - Don't default to branch specific rules if only one branch exists
Bug 11625 [QA Followup 3] - Use system preferences to control behavior
Bug 11625 - Add system preferences DefaultToLoggedInLibraryCircRules and DefaultToLoggedInLibraryNoticesSlips
Bug 11625 - Implement DefaultToLoggedInLibraryNoticesSlips
Bug 11625 - Implement DefaultToLoggedInLibraryCircRules
Bug 11625: [COUNTER-PATCH] Implement DefaultToLoggedInLibraryNoticesSlips
Bug 11625: [COUNTER-PATCH] Implement DefaultToLoggedInLibraryNoticesSlips
[SIGNED-OFF] Bug 11625 - Add system preferences DefaultToLoggedInLibraryCircRules and DefaultToLoggedInLibraryNoticesSlips
[SIGNED-OFF] Bug 11625 - Implement DefaultToLoggedInLibraryCircRules
[SIGNED-OFF] Bug 11625: Implement DefaultToLoggedInLibraryNoticesSlips
Bug 11625 - Add system preferences DefaultToLoggedInLibraryCircRules and DefaultToLoggedInLibraryNoticesSlips
Bug 11625 - Implement DefaultToLoggedInLibraryCircRules
Bug 11625: Implement DefaultToLoggedInLibraryNoticesSlips

Description Kyle M Hall 2014-01-28 15:26:13 UTC
It has come to our attention that some librarians will accidentally change default circulation rules and notices be accident, assuming they are working on rules and notices for their library by default.

If a librarian is logged in with a set branchcode, we should display that branches rules by default, and require them to choose the "All libraries" sections by hand.
Comment 1 Kyle M Hall 2014-01-28 15:49:31 UTC Comment hidden (obsolete)
Comment 2 Owen Leonard 2014-01-29 12:45:18 UTC Comment hidden (obsolete)
Comment 3 Katrin Fischer 2014-01-31 06:52:16 UTC
Hm, I noticed that now when you log in as superlibrarian, there is a difference in behaviour. If you try to save a rule for "All libraries" without picking a library first, it won't save.

Could it be this line here?
-my $branch = $input->param('branch') || ( C4::Branch::onlymine() ? ( C4::Branch
+my $branch = $input->param('branch') || C4::Branch::mybranch();
Comment 4 Katrin Fischer 2014-01-31 06:53:16 UTC
I am not a fan of using the superlibrarian, but I think in this case it is a little bug that could be fixed by keeping a fallback?
Comment 5 Kyle M Hall 2014-01-31 13:02:03 UTC
Katrin, do you mean *any* superlibrarian? Or just the database login superlibrarian?

(In reply to Katrin Fischer from comment #3)
> Hm, I noticed that now when you log in as superlibrarian, there is a
> difference in behaviour. If you try to save a rule for "All libraries"
> without picking a library first, it won't save.
> 
> Could it be this line here?
> -my $branch = $input->param('branch') || ( C4::Branch::onlymine() ? (
> C4::Branch
> +my $branch = $input->param('branch') || C4::Branch::mybranch();
Comment 6 Katrin Fischer 2014-01-31 13:07:37 UTC
Ah sorry Kyle, I meant the database user.
Comment 7 Kyle M Hall 2014-01-31 13:47:20 UTC Comment hidden (obsolete)
Comment 8 Kyle M Hall 2014-01-31 13:50:03 UTC
Thanks! This followup should resolve the problem. As an aside, I wonder if you should rid ourselves of the 'database' user altogether, and just provide a command line script for creating users ( or at least superlibrarians ). It just seems to cause more issues than it solves.

(In reply to Katrin Fischer from comment #6)
> Ah sorry Kyle, I meant the database user.
Comment 9 Katrin Fischer 2014-01-31 13:57:49 UTC
I agree with you, but you would also have to provide a way to create the branch and category code... and that complicates things a bit more. Also you would have to prevent people from using it after running the web installer with the database user.
Comment 10 Katrin Fischer 2014-02-02 13:36:33 UTC Comment hidden (obsolete)
Comment 11 Katrin Fischer 2014-02-02 13:36:55 UTC Comment hidden (obsolete)
Comment 12 Galen Charlton 2014-02-19 17:37:26 UTC
I very much agree that this is a usability improvement -- *if* more than one library is sharing the database.

However, I'm not convinced that this is an improvement for single-branch libraries or library systems that have multiple branches but no variation in circulation policy.  For such libraries, this patch means that going to circulation rules or notices & slips immediately requires them to make at least two more clicks.

Setting to in discussion for feedback from such libraries and suggestions for ways to make this work well both for consortia and single-branch/single-system users.

I'm also setting this bug as a blocker for bug 11747 since they cover the same underlying concept.
Comment 13 Kyle M Hall 2014-03-05 12:50:06 UTC Comment hidden (obsolete)
Comment 14 Kyle M Hall 2014-03-05 12:51:20 UTC Comment hidden (obsolete)
Comment 15 Jonathan Druart 2014-03-18 11:46:55 UTC
After some reflexion, I am not sure it is the right way to fix it.
For instance, if there is no specific rules and all libraries use the same (default) rules, the librarian will be confused. Before this patch, the rules were displayed and after the table will be empty.
Maybe should we check if rules are defined for the branch selected automatically?

I need additional QA points of view.
Comment 16 Katrin Fischer 2014-03-18 11:51:39 UTC
I think this is a tricky one, it's hard to tell, what the expected behaviour is. We don't use the default rules much, even if there is only one librray we use the specific page for the rules - maybe a bad habit? Maybe there are just multiple ways?
Comment 17 Kyle M Hall 2014-04-21 18:08:23 UTC
(In reply to Katrin Fischer from comment #16)
> I think this is a tricky one, it's hard to tell, what the expected behaviour
> is. We don't use the default rules much, even if there is only one librray
> we use the specific page for the rules - maybe a bad habit? Maybe there are
> just multiple ways?

I can tell you that we always use only the default rules in the case of a single library.
Comment 18 Katrin Fischer 2014-04-27 16:13:17 UTC
I think we haven't been able to find consensus on how this should work so far. I'd suggest to try and get some more opinions from libraries on this.
Comment 19 Nick Clemens 2014-05-21 14:05:46 UTC
Would it be reasonable to control this with user permissions?

-If user has access to all circ rules they default to All Libraries

-If user has access only to their circ rules it defaults to logged in library (or home library)

The issue we have in seeking this enhancement is that we want to give all libraries the ability to control their own circ rules, but prevent them from altering defaults rules of the consortia 

This would apply to Bug 11747 in terms of overdue notice/status triggers as well and could be one permission for both, determining whether they are library admins or system admins
Comment 20 Kyle M Hall 2015-01-22 13:02:23 UTC Comment hidden (obsolete)
Comment 21 Kyle M Hall 2015-01-22 13:02:33 UTC Comment hidden (obsolete)
Comment 22 Kyle M Hall 2015-01-22 13:02:36 UTC Comment hidden (obsolete)
Comment 23 Kyle M Hall 2015-01-22 13:41:11 UTC Comment hidden (obsolete)
Comment 24 Kyle M Hall 2015-01-22 13:47:11 UTC
I don't think we'll ever get consensus on this, so I think the best way to move forward is to make this behavior controlled via system preferences.
Comment 25 Jonathan Druart 2015-01-23 15:22:54 UTC
Kyle, do you think it's relevant to have 2 prefs for 11747 and this one?
Maybe 1 generic pref could be used for the both places.

Katrin, Nick: an opinion?
Comment 26 Nick Clemens 2015-01-31 15:19:33 UTC
I am in favor of one syspref but would accept two if someone has a strong preference to keep them separate
Comment 27 Jonathan Druart 2015-02-11 11:34:55 UTC
Back to in discussion for the number of pref choice...
Comment 28 Kyle M Hall 2015-02-13 16:31:53 UTC
My opinion is that these two sections of Koha have little to do with each other. They aren't even in the same sections! One is in tools, the other in administration.

As such, I feel that is more appropriate to keep the behaviors as two separate preferences.
Comment 29 Kyle M Hall 2015-03-20 16:06:30 UTC
It's been a month with no more feedback, resetting the status.
Comment 30 Kyle M Hall 2015-03-20 16:08:54 UTC Comment hidden (obsolete)
Comment 31 Kyle M Hall 2015-03-20 16:09:02 UTC Comment hidden (obsolete)
Comment 32 Kyle M Hall 2015-03-20 16:09:05 UTC Comment hidden (obsolete)
Comment 33 Kyle M Hall 2015-03-20 16:09:07 UTC Comment hidden (obsolete)
Comment 34 Jonathan Druart 2015-04-17 09:19:31 UTC
Applying: Bug 11625 [QA Followup 3] - Use system preferences to control behavior
fatal: sha1 information is lacking or useless (admin/smart-rules.pl).
Comment 35 Kyle M Hall 2015-04-17 16:35:17 UTC Comment hidden (obsolete)
Comment 36 Kyle M Hall 2015-04-17 16:35:25 UTC Comment hidden (obsolete)
Comment 37 Kyle M Hall 2015-04-17 16:35:28 UTC Comment hidden (obsolete)
Comment 38 Kyle M Hall 2015-04-17 16:35:31 UTC Comment hidden (obsolete)
Comment 39 Kyle M Hall 2015-04-21 15:04:53 UTC Comment hidden (obsolete)
Comment 40 Kyle M Hall 2015-04-21 15:05:03 UTC Comment hidden (obsolete)
Comment 41 Kyle M Hall 2015-04-21 15:05:06 UTC Comment hidden (obsolete)
Comment 42 Jonathan Druart 2015-04-22 15:47:04 UTC
Comment on attachment 38277 [details] [review]
Bug 11625 - Implement DefaultToLoggedInLibraryNoticesSlips

Review of attachment 38277 [details] [review]:
-----------------------------------------------------------------

::: tools/letter.pl
@@ +85,5 @@
> +  : $input->param('branchcode')                                      ? $input->param('branchcode')
> +  : $op                                                              ? $input->param('branchcode')
> +  : GetBranchesCount() == 1                                          ? undef
> +  :                                                                    C4::Branch::mybranch();
> +

Sorry but I am not confident enough with this part of code to pass qa on it today.
This script is a house of cards and the branchcode variable is used at several place.
You modify it at the beginning of the script, but it could introduce side effects.
Comment 43 Jonathan Druart 2015-04-23 13:42:12 UTC Comment hidden (obsolete)
Comment 44 Jonathan Druart 2015-04-23 13:46:51 UTC
(In reply to Jonathan Druart from comment #43)
> Created attachment 38422 [details] [review] [review]
> Bug 11625: [COUNTER-PATCH] Implement DefaultToLoggedInLibraryNoticesSlips

Kyle, either I missed something and this patch is wrong or it does the same as yours. It looks simpler.
Could you please take a look?
Comment 45 Jonathan Druart 2015-04-23 14:23:31 UTC Comment hidden (obsolete)
Comment 46 Jonathan Druart 2015-04-23 14:25:40 UTC
(In reply to Jonathan Druart from comment #45)
> Created attachment 38424 [details] [review] [review]
> Bug 11625: [COUNTER-PATCH] Implement DefaultToLoggedInLibraryNoticesSlips

I tried to clean the branchcode value is this patch.
It is the same idea as in your patch, if branchcode eq '*', the user has selected the 'all libraries' in the dropdown list.
Comment 47 Jonathan Druart 2015-04-27 13:27:04 UTC
I would like to get point of view from another QAer on this one and bug 11747.
I don't know to be sure Kyle's patch won't introduce regression on the tools/letter.pl page.
Moreover I am still concerned by the addition of 3 prefs to do the same thing.
Comment 48 Jonathan Druart 2015-08-05 12:12:25 UTC
Ccing some QAers since we are waiting for feedbacks for 3 months now.
Comment 49 Marcel de Rooy 2015-08-20 08:30:42 UTC
I put this patch in FQA now. I think first Kyle should comment on the counter patch before anyone else does anything else :)
Comment 50 Kyle M Hall 2015-08-21 19:01:05 UTC Comment hidden (obsolete)
Comment 51 Kyle M Hall 2015-08-21 19:01:17 UTC Comment hidden (obsolete)
Comment 52 Kyle M Hall 2015-08-21 19:01:20 UTC Comment hidden (obsolete)
Comment 53 Kyle M Hall 2015-08-21 19:01:28 UTC
(In reply to Marcel de Rooy from comment #49)
> I put this patch in FQA now. I think first Kyle should comment on the
> counter patch before anyone else does anything else :)

Looks good to me!
Comment 54 Brendan Gallagher 2015-10-15 15:20:53 UTC
(In reply to Jonathan Druart from comment #47)
> I would like to get point of view from another QAer on this one and bug
> 11747.
> I don't know to be sure Kyle's patch won't introduce regression on the
> tools/letter.pl page.
> Moreover I am still concerned by the addition of 3 prefs to do the same
> thing.

I did not see a regression in the tools/letter.pl area - do you have an exact process - I test but not find problem with the letter I have already.
Comment 55 Jonathan Druart 2015-10-16 11:15:47 UTC
(In reply to Brendan Gallagher from comment #54)
> (In reply to Jonathan Druart from comment #47)
> > I would like to get point of view from another QAer on this one and bug
> > 11747.
> > I don't know to be sure Kyle's patch won't introduce regression on the
> > tools/letter.pl page.
> > Moreover I am still concerned by the addition of 3 prefs to do the same
> > thing.
> 
> I did not see a regression in the tools/letter.pl area - do you have an
> exact process - I test but not find problem with the letter I have already.

This is no longer valid, Kyle validated my counter patch.
Comment 56 Jonathan Druart 2015-10-16 11:21:27 UTC
For both 11625 and 11747 (I have squashed the patch, I don't which patch is guilty):

1)
 FAIL   installer/data/mysql/updatedatabase.pl
   FAIL   forbidden patterns
                forbidden pattern: Use INSERT IGNORE INTO on inserting a new syspref (see bug 9071) (line 11101)
                forbidden pattern: Use INSERT IGNORE INTO on inserting a new syspref (see bug 9071) (line 11116)

and

11109     print "Upgrade to $DBversion done (Bug 13379 - Modify authorised_values.category to varchar(32))\n";
11120     print "Upgrade to $DBversion done (Bug 13379 - Modify authorised_values.category to varchar(32))\n";

Wrong c/p.

Please prefer to amend the original patch than submit another follow-up (to prevent future conflicts).

Otherwise the code looks good. I am redoing some UI tests.
Comment 57 Jonathan Druart 2015-10-16 11:31:29 UTC
It does not work:
Try and delete a notice for a given library (not the default one).
Comment 58 Jonathan Druart 2015-10-16 11:44:14 UTC
(In reply to Jonathan Druart from comment #57)
> It does not work:
> Try and delete a notice for a given library (not the default one).

It looks like a bug from master, if IndependentBranches is ON.
Comment 59 Jonathan Druart 2015-10-16 11:49:59 UTC
Created attachment 43496 [details] [review]
Bug 11625 - Add system preferences DefaultToLoggedInLibraryCircRules and DefaultToLoggedInLibraryNoticesSlips

Signed-off-by: Owen Leonard <oleonard@myacpl.org>
Signed-off-by: Katrin Fischer <Katrin.Fischer.83@web.de>

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>
Amended patch: INSERT IGNORE INTO + msg
Comment 60 Jonathan Druart 2015-10-16 11:50:05 UTC
Created attachment 43497 [details] [review]
Bug 11625 - Implement DefaultToLoggedInLibraryCircRules

This patch set makes the default circ rules and notices/slips to edit
the logged in branch, if one is set.

Test Plan:
1) Apply this patch
2) Log into staff side, and set your library
3) Browse to Administration/Circulation and fines rules
4) Note the rule set selected is for your library
5) Select "All libraries"
6) Note the rule set selected is for "All libraries"
7) Browse to Toos/Notices & slips
8) Note the notice set selected is for your library
9) Select "All libraries"
10) Not the rule set selected is for "All libraries"
11) Click "New Notice"
12) Note the Library field is set to "All libraries"

Signed-off-by: Owen Leonard <oleonard@myacpl.org>
Signed-off-by: Katrin Fischer <Katrin.Fischer.83@web.de>
Passes QA script and tests.

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>

Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>
Comment 61 Jonathan Druart 2015-10-16 11:50:10 UTC
Created attachment 43498 [details] [review]
Bug 11625: Implement DefaultToLoggedInLibraryNoticesSlips

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>

Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>
Comment 62 Kyle M Hall 2015-12-30 11:47:19 UTC
Patches pushed to master!