Bug 9055 - Mandumah.com's multiple branch search, multiple branch security mod
Summary: Mandumah.com's multiple branch search, multiple branch security mod
Status: CLOSED WONTFIX
Alias: None
Product: Koha
Classification: Unclassified
Component: Authentication (show other bugs)
Version: Main
Hardware: All All
: P5 - low enhancement
Assignee: Waylon Robertson
QA Contact:
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2012-11-11 04:24 UTC by Waylon Robertson
Modified: 2024-07-25 11:17 UTC (History)
7 users (show)

See Also:
Change sponsored?: ---
Patch complexity: ---
Documentation contact:
Documentation submission:
Text to go in the release notes:
Version(s) released in:
Circulation function:


Attachments
Multibranch search and security (43.55 KB, patch)
2012-11-11 04:29 UTC, Waylon Robertson
Details | Diff | Splinter Review
Multibranch search and security (53.23 KB, patch)
2012-11-16 05:10 UTC, Waylon Robertson
Details | Diff | Splinter Review
Multibranch search and security (53.23 KB, patch)
2012-11-16 05:18 UTC, Waylon Robertson
Details | Diff | Splinter Review
Multibranch search/security patch (51.08 KB, patch)
2013-10-15 05:08 UTC, Waylon Robertson
Details | Diff | Splinter Review
Fix for patch, fixes updatedatabase.pl mistake made by WaylonR (1.33 KB, patch)
2013-10-15 05:09 UTC, Waylon Robertson
Details | Diff | Splinter Review
Bug 9055 full patch to Nov 1st 2013 master (51.43 KB, patch)
2013-11-06 05:01 UTC, Waylon Robertson
Details | Diff | Splinter Review
Bug 9055: Rebased for master as of 19th Nov 2013 (51.43 KB, patch)
2013-11-19 04:22 UTC, Waylon Robertson
Details | Diff | Splinter Review
Bug 9055: Rebased for master as of 19th Nov 2013 (45.05 KB, patch)
2013-11-19 04:59 UTC, Waylon Robertson
Details | Diff | Splinter Review
Bug 9055: Rebased for master as of 19th Nov 2013 (46.74 KB, patch)
2013-11-19 05:27 UTC, Waylon Robertson
Details | Diff | Splinter Review
Bug 9055: Mandumah.com's multiple branch search, multiple branch security mod (50.70 KB, patch)
2013-11-19 15:54 UTC, Mark Tompsett
Details | Diff | Splinter Review
Bug 9055: Mandumah.com's multiple branch search, multiple branch security mod (54.57 KB, patch)
2013-12-03 03:32 UTC, Mark Tompsett
Details | Diff | Splinter Review
Bug 9055: Mandumah.com's multiple branch search, multiple branch security mod (54.57 KB, patch)
2013-12-03 03:34 UTC, Mark Tompsett
Details | Diff | Splinter Review
Bug 9055: Mandumah.com's multiple branch search, multiple branch security mod (53.10 KB, patch)
2013-12-03 03:35 UTC, Mark Tompsett
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Waylon Robertson 2012-11-11 04:24:13 UTC
This mod creates the ability to search multiple branches using a jquery dropdown, instead of just one or all branches.

This mod also replaces SearchMyLibraryFirst preference, giving three options.
All = no change from standard koha setup.
Preferhome = Prefers patron's home library (others may be searched), koha's current functionality of SearchMyLibraryFirst.
Securehome = Only patrons' home library and other assigned libraries (if any) can be searched.
With multiple instances of branchcode inside Borrower extended attributes, a patron can be given multiple branches to search within. The Securehome concept also includes the branch given in the apache enviroment of the host/virtualhost contacted, from OPAC_SEARCH_LIMIT = "branch: blah"

This patch not only filters the GetBranches results, filtering the patrons ability to see other branches other than the ones given, it also filters the branches given in the limit branch: search options, so that even if a user manually inputted a branch, they are still only getting what they are ment to get.

This enhancement would be useful for big libraries, in which its members would be restricted to searching and borrowing from a subset of its books. 

Overall, the multiple branch search dropdown should prove useful, even with unsecure installations. What is not included yet, is a method to decide what branches to show to a guest, not-logged-in, user.

To use this patch, you need to create a branchcode Patron Extended Attribute, and have the patron your working with, have one or more branchcode extended attributes.
So, total branches that patron will get:
OPAC_SEARCH_LIMIT branch + Patron record home branch + whatever branchcodes you put into Borrower_attributes.
You also need to set the preferences, 
In opac, set SearchableBranches
In Patrons, set multibranch.
In Searching, set MultiBranchSelect
Comment 1 Waylon Robertson 2012-11-11 04:29:54 UTC Comment hidden (obsolete)
Comment 2 Jared Camins-Esakov 2012-11-11 14:14:24 UTC
Looks like a good start, but I have a few comments:
1) The jQuery plugin and its associated assets need to go in koha-tmpl/opac-tmpl/lib/jquery/plugins.
2) Please provide the following in the commit message: the bug number at the beginning of the first line, a description (you can just copy what you put into Bugzilla), and a test plan.
3) All the database changes seem to be missing. You need to update sysprefs.sql as well as add an updatedatabase stanza to add the new sysprefs.
4) Once you have updated the patch, please change the status to "Needs signoff."
Comment 3 Waylon Robertson 2012-11-16 05:10:21 UTC Comment hidden (obsolete)
Comment 4 Waylon Robertson 2012-11-16 05:11:10 UTC
Okay, work done!
Comment 5 Waylon Robertson 2012-11-16 05:18:20 UTC Comment hidden (obsolete)
Comment 6 Elliott Davis 2012-11-16 19:49:04 UTC
Waylon,

Just as a jump in comment I applied the patch and attempted to test and found that the way this patch is trying to solve the problem seems overly complex.  Perhaps if we could consolidate the 3 sysprefs into 1 and add a more detailed testing plan?
Comment 7 Waylon Robertson 2012-11-19 05:40:10 UTC
(In reply to comment #6)
> Waylon,
> 
> Just as a jump in comment I applied the patch and attempted to test and
> found that the way this patch is trying to solve the problem seems overly
> complex.  Perhaps if we could consolidate the 3 sysprefs into 1 and add a
> more detailed testing plan?

More detailed testing plan, i agree with. One thing I have not learned is how to write a good testing plan.
However, in regards to 3 vs 1 syspref's... I have to disagree. There are libraries that want the extra security, but don't need the multibranch search dropdown or the multiple branches on a patron, to be used in the search/branches filter, hence the three sysprefs. And, going to be adding a fourth, as a email on koha@ list has asked for the mod to also effect staff members searches. This too should be optional, at the choice of the library administrator.
Comment 8 Mirko Tietgen 2012-12-15 10:27:49 UTC
I second the request for a detailed test plan. I would have had a look, but don't have the time to go through the code of larger patches and find out what to do myself.

Looks like your patch is still missing the description Jared mentioned. It should be in the patch as commit message, not only in a bugzilla posting.
Comment 9 cedric.vita 2013-03-19 16:12:41 UTC
Something went wrong !
Applying: Multibranch search, patrons and security mod This mod creates the ability to search multiple branches using a jquery dropdown, instead of just one or all branches.
Using index info to reconstruct a base tree...
Falling back to patching base and 3-way merge...
Auto-merging C4/Auth.pm
Auto-merging C4/Branch.pm
Auto-merging C4/Koha.pm
Auto-merging C4/Members/Attributes.pm
Auto-merging installer/data/mysql/sysprefs.sql
Auto-merging installer/data/mysql/updatedatabase.pl
CONFLICT (content): Merge conflict in installer/data/mysql/updatedatabase.pl
Auto-merging koha-tmpl/intranet-tmpl/prog/en/modules/admin/preferences/opac.pref
Auto-merging koha-tmpl/intranet-tmpl/prog/en/modules/admin/preferences/searching.pref
Auto-merging koha-tmpl/opac-tmpl/prog/en/modules/opac-advsearch.tt
Auto-merging opac/opac-search.pl
CONFLICT (content): Merge conflict in opac/opac-search.pl
Failed to merge in the changes.
Patch failed at 0001 Multibranch search, patrons and security mod This mod creates the ability to search multiple branches using a jquery dropdown, instead of just one or all branches.
When you have resolved this problem run "git am --resolved".
If you would prefer to skip this patch, instead run "git am --skip".
To restore the original branch and stop patching run "git am --abort".
Bug 9055 - Mandumah.com's multiple branch search, multiple branch security mod

Multibranch search and security
Apply? [yn]
Patch left in /tmp/Multibranch-search-and-security-i581g9.patch
Comment 10 Waylon Robertson 2013-10-15 05:08:48 UTC Comment hidden (obsolete)
Comment 11 Waylon Robertson 2013-10-15 05:09:55 UTC Comment hidden (obsolete)
Comment 12 Waylon Robertson 2013-10-15 05:11:31 UTC
Brought the code up to the current master - 3.14 first alpha.
Comment 13 Mark Tompsett 2013-10-15 15:22:00 UTC
Patch still has updatedatabase.pl issues.

However, this comment is about the transition from one system preference to this set of system preferences. I do think this extends the functionality of the SearchMyLibraryFirst, but there is no attempt to translate the old system setting to a newer but equivalent one.

I have not tested the functionalities yet, but perhaps the mapping might be something like:
    SearchMyLibraryFirst:yes => SearchableBranches:preferhome
    SearchMyLibraryFirst:no  => SearchableBranches:all

I would expect to see such a transition in the -- updatedatabase.pl section of code. Also, you can add 3 system preferences in one section of code for updatedatabase.pl, you don't need 3 "XXX" sections.

As such, I'll leave the status as is, "Patch doesn't apply", and await a newer version which does apply and that has this mapping in the updatedatabase.pl code.
Comment 14 Waylon Robertson 2013-11-06 05:01:31 UTC
Created attachment 22742 [details] [review]
Bug 9055 full patch to Nov 1st 2013 master

Patch to resolve M. Thompsett's comments.
Comment 15 Waylon Robertson 2013-11-19 04:22:36 UTC Comment hidden (obsolete)
Comment 16 Waylon Robertson 2013-11-19 04:59:03 UTC Comment hidden (obsolete)
Comment 17 Waylon Robertson 2013-11-19 05:27:22 UTC Comment hidden (obsolete)
Comment 18 Mark Tompsett 2013-11-19 14:08:25 UTC
The patch dropped drastically in size (51.43, to 46.74) while cleaning it up. Some elements have gone missing. I'm changing to "In Discussion", since I was hoping for a patch up date to look at, and I don't believe this is in a state to sign off on.
Comment 19 Mark Tompsett 2013-11-19 15:45:24 UTC
Comment on attachment 22742 [details] [review]
Bug 9055 full patch to Nov 1st 2013 master

Unobsoleting the initial patch and fixing myself.
Comment 20 Mark Tompsett 2013-11-19 15:54:04 UTC Comment hidden (obsolete)
Comment 21 Mark Tompsett 2013-12-03 03:32:19 UTC Comment hidden (obsolete)
Comment 22 Mark Tompsett 2013-12-03 03:34:24 UTC Comment hidden (obsolete)
Comment 23 Mark Tompsett 2013-12-03 03:35:27 UTC Comment hidden (obsolete)
Comment 24 Mark Tompsett 2013-12-03 04:31:53 UTC
Comment on attachment 23252 [details] [review]
Bug 9055: Mandumah.com's multiple branch search, multiple branch security mod

I think my description is a little cleaner, but I've left the patch which I think is the one to rebase, and debug. Sadly, it failed to give me results:

opac
search
happy - all libraries
result count: (note the number)
advanced search
keyword = happy
go to the library dropdown and select all then click search.
BUSTED!

All and Securehome settings busted. Preferhome works with my current system preference settings. Have fun debugging.
Comment 25 Mark Tompsett 2013-12-03 21:42:35 UTC
Bug 11334 may also affect this.
Comment 26 Martin Renvoize (ashimema) 2014-12-31 06:53:35 UTC
At over a year since the last comment, is anyone still using/working on this?
Comment 27 Marc Véron 2016-10-03 16:51:50 UTC
Still valid?
Comment 28 Martin Renvoize (ashimema) 2016-10-04 06:26:13 UTC
I reckon lets kill it off Marc, I got not reply two years ago on this one.
Comment 29 Marc Véron 2016-10-04 08:03:44 UTC
Set to RESOLVED WONTFIX because there is no activity since 2013.
Feel free to reactivate if necessary.