When the user clicks on a subject or an author, the list of subjects (authors) is displayed and the user can choose more than 1 subject (author) to search on this is very convenient when, for example, subjects are : France, history, kingdom, war to do a search on "France + war" easily
Created attachment 3345 [details] [review] Patch from BibLibre
The patch for Bug 5871 contains the CSS changes required to make this bug work properly.
Some comments, after testing: 1. This completely breaks subject searching for users with JavaScript disabled. It should gracefully degrade. 2. The behavior of these menus doesn't follow that of other menus in the OPAC like the Lists menu or the "More searches" menu. If we're going to add this now we really should have it use the YUI Menu library so the interactions are consistent.
If OPACXSLTDetailsDisplay is turned on subjects are listed twice: once in the section output by the XSL and once with the new display.
This bug is mentioned in: Bug 5888: opac-detail.pl: adding of search rebound on author and subject http://lists.koha-community.org/pipermail/koha-patches/2011-March/014116.html
Created attachment 9738 [details] [review] opac-detail subject/author links improvements When the user clicks on a subject or an author, the list of subjects (authors) is displayed and the user can choose more than 1 subject (author) to search on -- Recreated patch against current master - Fixed display when xslt is enabled - Fixed searching when javascript is disabled
I noticed a couple of problems with this patch: * The magnifying glass to link to authority records produces an invalid link for authority-controlled headings. * When you click on a subdivision that appears in more than one subject, the dropdown has that subdivision checked in every single subject in the list, which is confusing. * If you choose all the subdivisions of a particular subject (and only those subdivisions), a user would expect it to use the authid for the search if UseAuthoritiesForTracings is turned on, but it does not, which is confusing and not what I'd expect. * Although comment 6 says "Fixed display when xslt is enabled," it seems that the feature simply doesn't show up at all when XSLT is enabled, which is fine, but should be indicated somewhere, at least in the commit message. Even better would be if the feature could be made to work with XSLT as well, of course. An indication would make it clear to a tester whether the fact that it doesn't work with XSLT means that the patch failed QA, or just means that it works as intended.
(In reply to comment #7) > I noticed a couple of problems with this patch: > * The magnifying glass to link to authority records produces an invalid link > for authority-controlled headings. I'm not sure to understand. Could you provide such a link as an example? > * When you click on a subdivision that appears in more than one subject, the > dropdown has that subdivision checked in every single subject in the list, > which is confusing. > * If you choose all the subdivisions of a particular subject (and only those > subdivisions), a user would expect it to use the authid for the search if > UseAuthoritiesForTracings is turned on, but it does not, which is confusing > and not what I'd expect. Ok I will fix those things. > * Although comment 6 says "Fixed display when xslt is enabled," it seems > that the feature simply doesn't show up at all when XSLT is enabled, which > is fine, but should be indicated somewhere, at least in the commit message. > Even better would be if the feature could be made to work with XSLT as well, > of course. An indication would make it clear to a tester whether the fact > that it doesn't work with XSLT means that the patch failed QA, or just means > that it works as intended. To be clear, this feature is not intended to work when using XSLT for opac-detail.pl. But with XSLT enabled subjects and authors must be listed only once (the problem was subjects were listed twice).
(In reply to comment #8) > (In reply to comment #7) > > I noticed a couple of problems with this patch: > > * The magnifying glass to link to authority records produces an invalid link > > for authority-controlled headings. > I'm not sure to understand. Could you provide such a link as an example? It doesn't include the authid at all. The URL just ends authid= (or something like that... I can't check right at the moment) > > * When you click on a subdivision that appears in more than one subject, the > > dropdown has that subdivision checked in every single subject in the list, > > which is confusing. > > * If you choose all the subdivisions of a particular subject (and only those > > subdivisions), a user would expect it to use the authid for the search if > > UseAuthoritiesForTracings is turned on, but it does not, which is confusing > > and not what I'd expect. > Ok I will fix those things. Thanks. > > * Although comment 6 says "Fixed display when xslt is enabled," it seems > > that the feature simply doesn't show up at all when XSLT is enabled, which > > is fine, but should be indicated somewhere, at least in the commit message. > > Even better would be if the feature could be made to work with XSLT as well, > > of course. An indication would make it clear to a tester whether the fact > > that it doesn't work with XSLT means that the patch failed QA, or just means > > that it works as intended. > To be clear, this feature is not intended to work when using XSLT for > opac-detail.pl. But with XSLT enabled subjects and authors must be listed > only once (the problem was subjects were listed twice). Okay. A note "affects 'regular mode' only" would clarify greatly.
Created attachment 9855 [details] [review] Bug 5888: opac-detail subject/author links improvements When the user clicks on a subject or an author, the list of subjects (authors) is displayed and the user can choose more than 1 subject (author) to search on. Also added a link to opac-authoritiesdetail.pl when possible. Only affects 'Normal view'. Does not affect XSLT display. -- > It doesn't include the authid at all. The URL just ends authid= (or something > like that... I can't check right at the moment) Fixed. > * When you click on a subdivision that appears in more than one subject, the > dropdown has that subdivision checked in every single subject in the list, > which is confusing. Fixed. > * If you choose all the subdivisions of a particular subject (and only those > subdivisions), a user would expect it to use the authid for the search if > UseAuthoritiesForTracings is turned on, but it does not, which is confusing > and not what I'd expect. Fixed but UseAuthoritiesForTracings is only used with XSLT, so I don't use it in this patch. If there is an authid and all subdivisions are checked, then the authid is used. Otherwise the strings are used.
Created attachment 9856 [details] [review] Bug 5888: opac-detail subject/author links improvements When the user clicks on a subject or an author, the list of subjects (authors) is displayed and the user can choose more than 1 subject (author) to search on. Also added a link to opac-authoritiesdetail.pl when possible. Only affects 'Normal view'. Does not affect XSLT display. Signed-off-by: Jared Camins-Esakov <jcamins@cpbibliography.com>
*** Bug 5871 has been marked as a duplicate of this bug. ***
Just a *dumb* question: If I click on a subject or author, wouldn't it be logical to start searching for it directly instead of presenting another menu where I have to click Search again? I understand that I now have the option to select multiple subjects, but if I wanted that, wouldn't I have typed them in the search box already? Also note that I would not prefer to let normal view and xslt view deviate in this regard. Confusing for users?
What do you suggest? Let the link behave like currently in master and add a new link after subjects and authors for displaying this menu? For sure, you can type what you want in the search box, but I think it's easier to check boxes than to type complete names of authorities in a search box. I do not understand your comment about normal view and xslt view. Do you want to enable this feature for xslt view too? If someone uses his own xslt and want this feature, he has to modify his xslt by himself. Anyway, opac users do not use both normal and xslt views, right? How can this be confusing for them?
(In reply to comment #14) > What do you suggest? Let the link behave like currently in master and add a > new link after subjects and authors for displaying this menu? Well, that could be the best of both! If I click on a name, I directly get what I want. If I want to select multiple names/topics, I get your menu. I would be in favor of that; but I do not want to force a personal opinion. Your are welcome to raise this point on the mailing list (before just changing your code) ... > I do not understand your comment about normal view and xslt view. Do you > want to enable this feature for xslt view too? If someone uses his own xslt > and want this feature, he has to modify his xslt by himself. You are right about custom xslt, but if the default xslt could also support this feature somehow, that would be nice. (Note that this could be handled in a followup.) > Anyway, opac users do not use both normal and xslt views, right? How can > this be confusing for them? Correct. Maybe it is more confusing for developers :-)
(In reply to comment #15) > (In reply to comment #14) > > What do you suggest? Let the link behave like currently in master and add a > > new link after subjects and authors for displaying this menu? > Well, that could be the best of both! If I click on a name, I directly get > what I want. If I want to select multiple names/topics, I get your menu. I > would be in favor of that; but I do not want to force a personal opinion. > Your are welcome to raise this point on the mailing list (before just > changing your code) ... I'm not a fan of this option. There is already a magnifying glass when your entry is an authority. Adding another image will result in less readability I think. I think that the option we choose with Nimes (that sponsored the feature) is the best compromise. Note that this enhancement is a tiny one, and if it does not pass QA as the feature is submitted, Nîmes & BibLibre have agreed to abandon it. > > I do not understand your comment about normal view and xslt view. Do you > > want to enable this feature for xslt view too? If someone uses his own xslt > > and want this feature, he has to modify his xslt by himself. > You are right about custom xslt, but if the default xslt could also support > this feature somehow, that would be nice. (Note that this could be handled > in a followup.) I agree this is independant from this patch = the XSLT could also support this feature without this patch. QA comment: * why isn't the javascript in a separate file ? That would: - reduce the network load (js is cached) - ease the introduction of this feature at XSLT level
Julian, With permission I would like to break this patch into two parts: one that links to the authority record, which I think is fairly uncontroversial, and one that adds the cool new subject search pop-up. Any objections? I would, of course, be sure to retain your authorship.
Jared, I have no objections at all. You can do what you want with these patches ;-)
Created attachment 11586 [details] [review] Bug 5888: opac-detail subject/author links improvements When the user clicks on a subject or an author, the list of subjects (authors) is displayed and the user can choose more than 1 subject (author) to search on. Only affects 'Normal view'. Does not affect XSLT display. Signed-off-by: Jared Camins-Esakov <jcamins@cpbibliography.com> Split out of the original patch for bug 5888. The links to the authority records are added by bug 8210. Additional signoff required since I split the patch up.
Woooo how cool is that!
This works exactly as described, it pops up a list you can search on, and the search works. Their appears to be no noticable performance hit on the page load. Passes xt/tt_valid.t xt/author/valid_templates.t and xt/author/translatable-templates.t While we could put the js in its own file. That is a QA decision, I'm happy to do an initial signoff
Created attachment 11592 [details] [review] Bug 5888: opac-detail subject/author links improvements When the user clicks on a subject or an author, the list of subjects (authors) is displayed and the user can choose more than 1 subject (author) to search on. Only affects 'Normal view'. Does not affect XSLT display. Signed-off-by: Jared Camins-Esakov <jcamins@cpbibliography.com> Split out of the original patch for bug 5888. The links to the authority records are added by bug 8210. Signed-off-by: Chris Cormack <chrisc@catalyst.net.nz>
I know this is already signed off, but when I went to take a look myself I couldn't get it to apply: Applying: Bug 5888: opac-detail subject/author links improvements fatal: sha1 information is lacking or useless (koha-tmpl/opac-tmpl/prog/en/css/opac.css). Repository lacks necessary blobs to fall back on 3-way merge. Cannot fall back to three-way merge. Patch failed at 0001 Bug 5888: opac-detail subject/author links improvements
(In reply to comment #23) > I know this is already signed off, but when I went to take a look myself I > couldn't get it to apply: > > Applying: Bug 5888: opac-detail subject/author links improvements > fatal: sha1 information is lacking or useless > (koha-tmpl/opac-tmpl/prog/en/css/opac.css). > Repository lacks necessary blobs to fall back on 3-way merge. > Cannot fall back to three-way merge. > Patch failed at 0001 Bug 5888: opac-detail subject/author links improvements Did you apply the patch for 8210 first? I broke them up into two.
Created attachment 12308 [details] [review] Bug 5888: opac-detail subject/author links improvements When the user clicks on a subject or an author, the list of subjects (authors) is displayed and the user can choose more than 1 subject (author) to search on. Only affects 'Normal view'. Does not affect XSLT display. Signed-off-by: Jared Camins-Esakov <jcamins@cpbibliography.com> Split out of the original patch for bug 5888. The links to the authority records are added by bug 8210. Signed-off-by: Chris Cormack <chrisc@catalyst.net.nz>
> Signed-off-by: Chris Cormack <chrisc@catalyst.net.nz> patch looks good, passing QA… $ koha-qa.pl testing 1 commit(s) (applied to commit c74b72b) * e5b58d3 Bug 5888: opac-detail subject/author links improvements koha-tmpl/opac-tmpl/prog/en/css/opac.css koha-tmpl/opac-tmpl/prog/en/modules/opac-detail.tt * koha-tmpl/opac-tmpl/prog/en/modules/opac-detail.tt OK
QA comment: There is a bug in Author links: if you have, say, 3 authors, and click "All", then the query built is on and an=NNNN and an=MMMM and an=PPPP The initial "on and" is wrong (and comes from the fact that the query is built using the 'All' checked
Created attachment 12343 [details] [review] [SIGNED-OFF] Bug 5888: opac-detail subject/author links improvements Rebased on master.
Created attachment 12344 [details] [review] Bug 5888: Fix building of query in authors links 'all' checkbox was taken into account, resulting in the string 'on and' added to beginning of query.
Sorry, this patch does not apply any more: error: patch failed: koha-tmpl/opac-tmpl/prog/en/modules/opac-detail.tt:360 error: koha-tmpl/opac-tmpl/prog/en/modules/opac-detail.tt: patch does not apply Did you hand edit your patch? It does not apply to blobs recorded in its index. Cannot fall back to three-way merge.
Created attachment 13631 [details] [review] [SIGNED-OFF] Bug 5888: opac-detail subject/author links improvements When the user clicks on a subject or an author, the list of subjects (authors) is displayed and the user can choose more than 1 subject (author) to search on. Also added a link to opac-authoritiesdetail.pl when possible. Only affects 'Normal view'. Does not affect XSLT display. Signed-off-by: Jared Camins-Esakov <jcamins@cpbibliography.com> Split out of the original patch for bug 5888. The links to the authority records are added by bug 8210. Signed-off-by: Chris Cormack <chrisc@catalyst.net.nz>
Created attachment 13632 [details] [review] Bug 5888: Fix building of query in authors links 'all' checkbox was taken into account, resulting in the string 'on and' added to beginning of query.
Patches rebased on master
I can't get this to apply, sorry. I tried first with git bz apply 5888 Bug 5888 - opac-detail subject/author links improvements [SIGNED-OFF] Bug 5888: opac-detail subject/author links improvements Apply? [yn] y Applying: Bug 5888: opac-detail subject/author links improvements error: patch failed: koha-tmpl/opac-tmpl/prog/en/modules/opac-detail.tt:293 error: koha-tmpl/opac-tmpl/prog/en/modules/opac-detail.tt: patch does not apply Patch failed at 0001 Bug 5888: opac-detail subject/author links improvements I also tried with git am -iu3 Applying: Bug 5888: opac-detail subject/author links improvements Using index info to reconstruct a base tree... Falling back to patching base and 3-way merge... Auto-merging koha-tmpl/opac-tmpl/prog/en/css/opac.css Auto-merging koha-tmpl/opac-tmpl/prog/en/modules/opac-detail.tt CONFLICT (content): Merge conflict in koha-tmpl/opac-tmpl/prog/en/modules/opac-detail.tt Failed to merge in the changes. Patch failed at 0001 Bug 5888: opac-detail subject/author links improvements
(In reply to comment #34) > I can't get this to apply, sorry. I tried first with git bz apply 5888 > Auto-merging koha-tmpl/opac-tmpl/prog/en/modules/opac-detail.tt > CONFLICT (content): Merge conflict in > koha-tmpl/opac-tmpl/prog/en/modules/opac-detail.tt > Failed to merge in the changes. > Patch failed at 0001 Bug 5888: opac-detail subject/author links improvements Melia, in this case, you can set the status to "Patch doesn't apply"
Created attachment 13973 [details] [review] [SIGNED-OFF] Bug 5888: opac-detail subject/author links improvements When the user clicks on a subject or an author, the list of subjects (authors) is displayed and the user can choose more than 1 subject (author) to search on. Also added a link to opac-authoritiesdetail.pl when possible. Only affects 'Normal view'. Does not affect XSLT display. Signed-off-by: Jared Camins-Esakov <jcamins@cpbibliography.com> Split out of the original patch for bug 5888. The links to the authority records are added by bug 8210. Signed-off-by: Chris Cormack <chrisc@catalyst.net.nz>
Created attachment 13974 [details] [review] Bug 5888: Fix building of query in authors links 'all' checkbox was taken into account, resulting in the string 'on and' added to beginning of query.
Created attachment 14140 [details] [review] [SIGNED-OFF] Bug 5888: opac-detail subject/author links improvements When the user clicks on a subject or an author, the list of subjects (authors) is displayed and the user can choose more than 1 subject (author) to search on. Also added a link to opac-authoritiesdetail.pl when possible. Only affects 'Normal view'. Does not affect XSLT display. Signed-off-by: Jared Camins-Esakov <jcamins@cpbibliography.com> Split out of the original patch for bug 5888. The links to the authority records are added by bug 8210. Signed-off-by: Chris Cormack <chrisc@catalyst.net.nz> Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Created attachment 14141 [details] [review] [SIGNED-OFF] Bug 5888: Fix building of query in authors links 'all' checkbox was taken into account, resulting in the string 'on and' added to beginning of query. Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
QA: Looking at this one again..
QA Comment on 5888: Still have my doubts on this feature. The usability is IMO the biggest issue. Please note my earlier comment 13. If I just want to search on the author I click on, I do not want to click again on Search. If I have some more authors and subject, the page even needs to be scrolled down before reaching the buttons. This would mean 1 click, 1 scroll and another click instead of the previous 1 click. If I want to search on two subjects, I will type them just as fast in the search box as selecting them in the menu? All together, I would prefer this option to be controlled by a syspref (default disabled), or offer the new menu in a different way. You could e.g. add a last option to the subject line like "Combined subject search" (only if #subjects>1) and present the menu then. Comment 16 suggests that this is less readable. But note that is only one extra link at the end. No large effect on readability, but usability is kept or even improved. Another minor point: If I want to remove the menu again, it should be possible without the need to scroll and click. In the case of more authors and subjects, the Cancel button cannot be reached rightaway. Could you add a Close or X button at the top to remove the menu? Another minor point: It is working on normal view only, but not on xslt. Since all comments are on a functional level, I will set this patch to In Discussion and first send a mail to the QA team.
I think a syspref would make sense to me - libraries are sensitive about such things, so giving them a choice is always good.
Since some people find this feature cool and others not, I follow Katrin : a syspref could be an easy way to gain consensus.
Created attachment 15230 [details] [review] Bug 5888: Followup add a syspref (OPACPopupAuthorsSearch) New syspref OPACPopupAuthorsSearch. If it is disabled, the development has no effect.
Created attachment 15231 [details] [review] Bug 5888: Add label for each entry in the authors list
Feel free to rename the syspref name/description if needed.
Created attachment 15341 [details] [review] Bug 5888: Followup for preference description Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl> Followup added for trivial string change in finalizing QA.
Created attachment 15342 [details] [review] Bug 5888: opac-detail subject/author links improvements When the user clicks on a subject or an author, the list of subjects (authors) is displayed and the user can choose more than 1 subject (author) to search on. Also added a link to opac-authoritiesdetail.pl when possible. Only affects 'Normal view'. Does not affect XSLT display. Signed-off-by: Jared Camins-Esakov <jcamins@cpbibliography.com> Split out of the original patch for bug 5888. The links to the authority records are added by bug 8210. Signed-off-by: Chris Cormack <chrisc@catalyst.net.nz> Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com> Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Created attachment 15343 [details] [review] Bug 5888: Fix building of query in authors links 'all' checkbox was taken into account, resulting in the string 'on and' added to beginning of query. Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com> Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Created attachment 15344 [details] [review] Bug 5888: Followup add a syspref (OPACPopupAuthorsSearch) New syspref OPACPopupAuthorsSearch. If it is disabled, the development has no effect. Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Created attachment 15345 [details] [review] Bug 5888: Add label for each entry in the authors list Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Created attachment 15346 [details] [review] Bug 5888: Followup for preference description Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl> Followup added for trivial string change in finalizing QA.
Created attachment 15347 [details] [review] Bug 5888: Add label for each entry in the authors list Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Created attachment 15348 [details] [review] Bug 5888: Followup for preference description Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl> Added for trivial string change in finalizing QA.
QA Comment: Little struggle with git bz.. With the pref, this looks good to me. Adjusted pref description by adding "subjects" and "opac detail". Passed QA
Additional thought: Could a followup "easily" add this to opac xslt too, or should we further adjust the pref description when only limited to no-xslt view?
This patch has been pushed to master.
*** Bug 5440 has been marked as a duplicate of this bug. ***