Bug 10904

Patch tested with a sandbox, by Christopher Brannon <cbrannon@cdalibrary.org>

The behavior introduced by this patch goes against the IndependentBranches pref value. I'm not sure it is relevant.

Or maybe should we add the 2 values on the mainpage (total changes/lib changes) and introduce a filter by branch on the members update page.

Don't hesitate to argue if you think I am wrong.

(In reply to Jonathan Druart from comment #4)
> The behavior introduced by this patch goes against the IndependentBranches
> pref value. I'm not sure it is relevant.
> 
> Or maybe should we add the 2 values on the mainpage (total changes/lib
> changes) and introduce a filter by branch on the members update page.
> 
> Don't hesitate to argue if you think I am wrong.

Jonathan,
I see what you are getting at.  While this patch adds information and functionality that the current feature lacks, it does seem to limit outside of the scope of IndependentBranches pref.  While our consortium would not choose to make such a broad sweep of limits as the IndependenBrances does, it seems as though this feature should, at some point, be controlled by the IndependentBranches pref.

As it is now, this patch appears to conform to the same limits that have been imposed on the suggestions counter, and in my opinion, doesn't function much different from that other feature.  However, it would be preferable if an enhancement were made down the road where both of these limits (and any others) had individual prefs, and the IndependentBranches set to Enable overrode those individual settings.

I think it is very important to maintain individual preferences, where consortiums like ours might want some limits, but not all.  But there should also be the ability to have a master switch, if you want it all.  Kind of like how staff permissions are set in a tree fashion right now.

I hope that makes sense.  I still think this patch is acceptable to how Koha currently behaves, and am keeping this signed off, based on other functions, but I suggest another enhancement be considered and submitted to allow the IndependentBranches pref to be a master over this feature and other similar features with the same imposed limits.

This needs others QAer points of view.

In my 7-library system this change would be acceptable. We don't manage so many updates that we get overwhelmed, but it would be an acceptable change to our workflow.

One idea to help this be more broadly-acceptable would be to add a permission like "modify patrons from any branch" and alter the display based on that permission.

I think this behavior should be controlled by a new system preference "IndependentBranchesPatronModifications". I also agree, IndependentBranches behaviors need to be broken up into smaller sets in the future. This would be a good place to start. Any new modules such as this one that need an IndependentBranches behavior should have a separate preference.

I think in order to pass qa, this patch needs to revert the changes to the branch selector, but use IndependentBranchesPatronModifications as the system preference instead.

Patch applied cleanly, go forth and signoff

Kyle,
Did more testing.  A few issues:

First, this may be an issue with the sandbox.  Not sure.  I'll need to do a fresh install with the patch.  I tested on master first, then I applied the patch, and I received this e-mail:

The sandbox you've requested is not ready.
Some problems occurred applying patches from bug 10904:
<h1>Something went wrong !</h1>Applying: Bug 10904 - would like to limit patron update request management by branch
Applying: Bug 10904 [QA Followup] - would like to limit patron update request management by branch Using index info to reconstruct a base tree...
Falling back to patching base and 3-way merge...
Auto-merging installer/data/mysql/sysprefs.sql Auto-merging installer/data/mysql/updatedatabase.pl
CONFLICT (content): Merge conflict in installer/data/mysql/updatedatabase.pl
Failed to merge in the changes.
Patch failed at 0001 Bug 10904 [QA Followup] - would like to limit patron update request management by branch When you have resolved this problem run git bz apply --continue.
If you would prefer to skip this patch, instead run git bz apply --skip.
To restore the original branch and stop patching run git bz apply --abort.
Bug 10904 - would like to limit patron update request management by branch

22494 - Bug 10904 - would like to limit patron update request management by branch
23104 - Bug 10904 [QA Followup] - would like to limit patron update request management by branch

Apply? [(y)es, (n)o, (i)nteractive] Patch left in /tmp/Bug-10904-QA-Followup---would-like-to-limit-patron-3o7VHx.patch .


Second issue, it looks like from your input on this ticket there should be two preference settings: IndependentBranches and IndependentBranchesPatronModifications.  I only see the first.

Third issue, I turned on IndependentBranches, and it works in letting the staff see only their patron requests, but we've lost some of the other features you had at first.  Parts 6 and 7 of your test plan fail.  This modification does not exist.  They should be there with or without the IndependentBranches set, and they are not there at all.

Christopher

Kyle,
It appears that the patch gives the same error message on the sandbox setup that I posted previously, even if I do a new database on setup.  Didn't have anything to do with a previous master install and using "No change" on the database.

Christopher

(In reply to Kyle M Hall from comment #8)
> I think this behavior should be controlled by a new system preference
> "IndependentBranchesPatronModifications". I also agree, IndependentBranches
> behaviors need to be broken up into smaller sets in the future. This would
> be a good place to start. Any new modules such as this one that need an
> IndependentBranches behavior should have a separate preference.

Katrin, I would like to get your opinion about that.
Do you agree with Kyle?

I think one of the effects IndependentBranches is supposed to have, is that it hides the patron data from other branches. For example, it is not possible to search for other branches' patrons.

But I agree that IndependentBranches is not for every library and a more granular approach would be better for the future. 

I'd propose:

IndependentBranches = ON - ignore system preference, never show other branches' patrons modification requests.

IndependentBranches = OFF - use the new preference.

This would fix a 'bug' with IndependentBranches supposed behaviour, but still allow flexibility.

Kyle was reading my mind... ;)

(In reply to Kyle M Hall from comment #8)
> I think this behavior should be controlled by a new system preference
> "IndependentBranchesPatronModifications". I also agree, IndependentBranches
> behaviors need to be broken up into smaller sets in the future. This would
> be a good place to start. Any new modules such as this one that need an
> IndependentBranches behavior should have a separate preference.

I disagree that a new system preference is needed.  I think it is sufficient to make this behave like similar interface with regards to library selection:

* If IndependentBranches is on, you see only your libraries' stuff unless you're a superlibrarian
* If IndependentBranches is off, it *defaults* to showing your library, but there's a selector available on the form to switch to another library.

In the later case, it can be coupled with notifications on the main page and patron home page that look like

"Patrons requesting modifications: X at my library, Y in the entire system"

That's not to say that there won't be other occasion for making finer-grained visibility distinctions, but this really needs a broader discussion.  For one thing, it's currently being done differently in serials with the "superserials" permission, and I am very concerned about the prospect of letting a pile of slightly different approaches occur.

I must respectfully disagree with your assertions. The primary complain I receive about IndependentBranches is the lack of finer control with the system preference. It's all or nothing. I see no advantage to the default with a pulldown you propose. In fact, if a librarian needs the ability to see and approve changes for multiple libraries, this only slows them down, as he or she will need to select each library in turn. This if fine for a system with 3 branches, but what about one with 20?

Setting back to passed qa so the current RM can make this call.

> That's not to say that there won't be other occasion for making
> finer-grained visibility distinctions, but this really needs a broader
> discussion.  For one thing, it's currently being done differently in serials
> with the "superserials" permission, and I am very concerned about the
> prospect of letting a pile of slightly different approaches occur.

If this is a concern, then it should be put in the Koha coding guidelines so it's no longer a decision that hinges on personal preference of one of more persons. I can agree with this in general. Koha has extremely varied implementations of the same generic pattern in many, many areas. This only further reinforces my point though.

Kyle

(In reply to Galen Charlton from comment #23)
> (In reply to Kyle M Hall from comment #8)
> > I think this behavior should be controlled by a new system preference
> > "IndependentBranchesPatronModifications". I also agree, IndependentBranches
> > behaviors need to be broken up into smaller sets in the future. This would
> > be a good place to start. Any new modules such as this one that need an
> > IndependentBranches behavior should have a separate preference.
> 
> I disagree that a new system preference is needed.  I think it is sufficient
> to make this behave like similar interface with regards to library selection:
> 
> * If IndependentBranches is on, you see only your libraries' stuff unless
> you're a superlibrarian
> * If IndependentBranches is off, it *defaults* to showing your library, but
> there's a selector available on the form to switch to another library.
> 
> In the later case, it can be coupled with notifications on the main page and
> patron home page that look like
> 
> "Patrons requesting modifications: X at my library, Y in the entire system"
> 
> That's not to say that there won't be other occasion for making
> finer-grained visibility distinctions, but this really needs a broader
> discussion.  For one thing, it's currently being done differently in serials
> with the "superserials" permission, and I am very concerned about the
> prospect of letting a pile of slightly different approaches occur.

(In reply to Galen Charlton from comment #23)
> That's not to say that there won't be other occasion for making
> finer-grained visibility distinctions, but this really needs a broader
> discussion.

(In reply to Kyle M Hall from comment #24)
> Setting back to passed qa so the current RM can make this call.

Kyle, Maybe we should have this discussion during the next dev meeting?

(In reply to Galen Charlton from comment #23)
> (In reply to Kyle M Hall from comment #8)
> > I think this behavior should be controlled by a new system preference
> > "IndependentBranchesPatronModifications". I also agree, IndependentBranches
> > behaviors need to be broken up into smaller sets in the future. This would
> > be a good place to start. Any new modules such as this one that need an
> > IndependentBranches behavior should have a separate preference.
> 
> I disagree that a new system preference is needed.  I think it is sufficient
> to make this behave like similar interface with regards to library selection:
> 
> * If IndependentBranches is on, you see only your libraries' stuff unless
> you're a superlibrarian
> * If IndependentBranches is off, it *defaults* to showing your library, but
> there's a selector available on the form to switch to another library.

Galen and Kyle,
Being the library that initially asked for this, I should add my two cents.  Our consortium has this preference off (Don't prevent), because we do want to allow staff from other libraries the ability to update patron records from any library, update holds, etc.  I think initially, if when IndependentBranches  is off (Don't prevent), this feature should default to the logged in library, and should have an option to change to other libraries, as Galen stated above.  However, it would be preferable to prevent libraries from touching some things of other libraries, and allow them to touch others.

We opted to turn off the patron's ability to update their record when Koha changed from e-mail notifications to this new setup, because ALL requests were being displayed, and we didn't want everyone feeling like they had to manage all request, or had to sift through it all.  Plus, there were some other crucial elements missing from the new feature.  Having the library default to the logged in library is fine by us.

As long as the number indicated on mainpage.pl reflects that.

Currently, the suggestions do NOT work that way, and has created problems.  I can see the same problems occurring if this isn't addressed now.  If this number doesn't reflect the default, then people are seeing a number and having to click on the link to check if there are any requests from their patrons, which is a waste of time.  The number should reflect the number of requests your library has.  Maybe there should be two numbers.  One for the logged in library, and a second for all requests.

I DO think that there should eventually be an enhancement to the IndependentBranches for granularity control so that libraries don't even have an option to touch certain things of other libraries, rather than the all or nothing approach this preference dictates.  But I think that needs to be another enhancement.  Unless you want to start tackling it one feature at a time, like with this feature.

It seems that if we do go in the direction of granular permissions under this preference, once it has been established that this is the direction we are moving, then such granular preferences could be added under IndependentBranches for features as they are added, and pre-existing features can be addressed in independent enhancement requests.  But until this direction has been established, I would leave it alone.

Anyway, I hope my ramblings help.  I would like to see this enhancement move on (with the exception that the number indicated on mainpage.pl reflects the logged in library), so we can let patrons update their accounts again.  :)

Christopher

I'm sort of in agreement with both sides here...

I've been thinking for a while that the IndependantBranches syspref should really be a granular preference, probably with it's own tab under the prefs pages as it could well grow rather quickly!  A heirachical structure much like the current permissions system would make great sense.

However, I also agree with Galens defaults and the idea that there should be some coding guidelines to enforce such a standard structure, so long as within that dropdown he mentions there's the top 'option', not default, but first selectable option is to allow 'no filtering', i.e. 'All Libraries' to allow 'pro users' to still 'get things done' quickly.

Just my two cents.

The first QA follow up won't apply:

git am -iu3 /tmp/PASSED-QA-Bug-10904-QA-Followup---would-like-to-li-Ta0Gwp.patch
Commit Body is:
--------------------------
Bug 10904 [QA Followup] - would like to limit patron update request management by branch

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Signed-off-by: Katrin Fischer <Katrin.Fischer.83@web.de>
--------------------------
Apply? [y]es/[n]o/[e]dit/[v]iew patch/[a]ccept all y
Applying: Bug 10904 [QA Followup] - would like to limit patron update request management by branch
fatal: sha1 information is lacking or useless (mainpage.pl).
Repository lacks necessary blobs to fall back on 3-way merge.
Cannot fall back to three-way merge.
Patch failed at 0001 Bug 10904 [QA Followup] - would like to limit patron update request management by branch
The copy of the patch that failed is found in:
   /home/katrin/kohaclone/.git/rebase-apply/patch
When you have resolved this problem, run "git am --continue".
If you prefer to skip this patch, run "git am --skip" instead.
To restore the original branch and stop patching, run "git am --abort".

Could you please rebase and fix the small conflict ASAP?

Created attachment 42599 [details] [review]
Bug 10904 - would like to limit patron update request management by branch

By not limiting the patron modification lists by branch, it can quickly
become overwhelming for a librarian. This patch makes the following
modifications:

* Only superlibrians will see the full list of borrowers with pending
  modifications.
* Adds the patron cardnumber after the patrons name on the modification
  list.
* Adds a link to the patron's record from the modification list.

Test Plan:
1) Apply this patch
2) Set up some pending modifications for two branches
3) Log in to the staff intranet as one of those branches
4) You should note the number of pending modifications counts
   only your logged in branch's patrons
5) Click the link, you should only see your logged in branches
   patrons in the pending modifications list.
6) You should note the cardnumber is displayed next to the patron name
7) You should note the "View record" link below each patron's name

Signed-off-by: Christopher Brannon <cbrannon@cdalibrary.org>
Signed-off-by: Katrin Fischer <Katrin.Fischer.83@web.de>
Comments on last patch.

Created attachment 42600 [details] [review]
Bug 10904 [QA Followup] - would like to limit patron update request management by branch

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Signed-off-by: Katrin Fischer <Katrin.Fischer.83@web.de>

Created attachment 42601 [details] [review]
Bug 10904 [QA Followup] - Use branch limit if either IndependentBranches or IndependentBranchesPatronModifications is enabled

Signed-off-by: Katrin Fischer <Katrin.Fischer.83@web.de>
Passes all tests and QA script.

Behaviour now is:
IndependentBranches = Prevent
or
IndependentBranches = Don't prevent,
IndependentBranchesPatronModifications = Prevent:
You will always only see the modification requests of your branch's patrons.

IndependentBrnaches = Don't prevent,
IndependentBranchesPatronModifications = Don't prevent
All modification requests are shown.

Link to patron record works and cardnumber is shown.

IndependentBranches = OFF - use the new preference.

Signed-off-by: Katrin Fischer <Katrin.Fischer.83@web.de>

Patches pushed to master.

Thanks Kyle!