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();

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?

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();

Ah sorry Kyle, I meant the database user.

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.

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.

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.

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.

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?

(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.

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.

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

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.

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?

I am in favor of one syspref but would accept two if someone has a strong preference to keep them separate

Back to in discussion for the number of pref choice...

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.

It's been a month with no more feedback, resetting the status.

Applying: Bug 11625 [QA Followup 3] - Use system preferences to control behavior
fatal: sha1 information is lacking or useless (admin/smart-rules.pl).

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.

(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?

(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.

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.

Ccing some QAers since we are waiting for feedbacks for 3 months now.

I put this patch in FQA now. I think first Kyle should comment on the counter patch before anyone else does anything else :)

(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!

(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.

(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.

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.

It does not work:
Try and delete a notice for a given library (not the default one).

(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.

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

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>

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>

Patches pushed to master!