|
Description
Waylon Robertson
2012-11-11 04:24:13 UTC
Created attachment 13377 [details] [review] Multibranch search and security bug 9055 [ENH] Needs some discussion. some things might be done better. The borrower_branches hash area could be subbed instead of being repeated twice. Comments are welcome. 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." Created attachment 13474 [details] [review] Multibranch search and security Okay, work done! Created attachment 13475 [details] [review] Multibranch search and security Patch after rebase 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? (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. 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. 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 Created attachment 21991 [details] [review] Multibranch search/security patch Created attachment 21992 [details] [review] Fix for patch, fixes updatedatabase.pl mistake made by WaylonR Brought the code up to the current master - 3.14 first alpha. 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.
Created attachment 22742 [details] [review] Bug 9055 full patch to Nov 1st 2013 master Patch to resolve M. Thompsett's comments. Created attachment 23014 [details] [review] Bug 9055: Rebased for master as of 19th Nov 2013 Created attachment 23015 [details] [review] Bug 9055: Rebased for master as of 19th Nov 2013 Created attachment 23016 [details] [review] Bug 9055: Rebased for master as of 19th Nov 2013 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 on attachment 22742 [details] [review] Bug 9055 full patch to Nov 1st 2013 master Unobsoleting the initial patch and fixing myself. Created attachment 23038 [details] [review] Bug 9055: Mandumah.com's multiple branch search, multiple branch security mod 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. The three new preferences to set are: - OPAC: SearchableBranches - Patrons: multibranch - Searching: MultiBranchSelect Created attachment 23250 [details] [review] Bug 9055: Mandumah.com's multiple branch search, multiple branch security mod 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. The three new preferences to set are: - OPAC: SearchableBranches - Patrons: multibranch - Searching: MultiBranchSelect Created attachment 23251 [details] [review] Bug 9055: Mandumah.com's multiple branch search, multiple branch security mod 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. The three new preferences to set are: - OPAC: SearchableBranches - Patrons: multibranch - Searching: MultiBranchSelect Created attachment 23252 [details] [review] Bug 9055: Mandumah.com's multiple branch search, multiple branch security mod 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. The three new preferences to set are: - OPAC: SearchableBranches - Patrons: multibranch - Searching: MultiBranchSelect 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. Bug 11334 may also affect this. At over a year since the last comment, is anyone still using/working on this? Still valid? I reckon lets kill it off Marc, I got not reply two years ago on this one. Set to RESOLVED WONTFIX because there is no activity since 2013. Feel free to reactivate if necessary. |