Description
Ian Walls
2012-01-02 21:30:43 UTC
Created attachment 8171 [details] [review] patch Please say if any styling should be added I wonder if the holds total should be shown at the top of the holdings list instead of the bottom? It would be hard to spot on a title with lots of items. I also wonder if the holds column in the holdings table should say "Holds on this item." That's a long label, but I'm concerned that it is confusing to patrons to see one number for total holds and another number in the table. For instance, 4 holds total, but only one line in the table says Holds: 1 because only one is an item-level hold? Created attachment 8181 [details]
Screenshot showing total holds count versus item hold count
Created attachment 8430 [details]
screenshot of number of holds when only holds on next available
works fine, nice feature :-) ; maybe number of holds could be more seeable when you only have holds on next available (because the table is empty in this case, see 2nd screenshot) ; and maybe it could be titled "total of holds" instead of "holds" Created attachment 8441 [details]
screenshot 2 of number of holds when only holds on next available
QA comment * the code is OK * the syspref is inconsistent I think: if it's set to OFF, the patron can see how many holds are done when he place the hold (opac-reserve.pl?biblionumber=5), in the priority column. So, from 2 choose 1: - remove the syspref - make the display consistent I'm member of the "group for removal of useless sysprefs", but I think this one is usefull, so i'm for option 2. Please provide a follow-up and i'll be happy to pass QA & push this patch ! Created attachment 8552 [details] [review] follow-up patch Just tested this feature, and found a problem: there are 2 sysprefs related to displaying the hold counter on OPAC: OPACShowHoldsCount and OPACDisplayRequestPriority * 1st of all, it would be better to be consistent on names * 2nd = if you set OPACDisplayRequestPriority=1 and OPACShowHoldsCount=0 the display is broken (you get the <th> but not the <td>, so the table is wrong (I agree it's a strange idea though) * 3rd = overall, i'm wondering if we really need 2 sysprefs, that does almost the same thing. Is there a use case where we want to display the number of requests but not your priority ? I feel it's meaningless = when you put your request, if you know there are already 5 holds, you'll be the 6th one. So shouldn't we just have one ? (and, maybe, remove OPACDisplayRequestPriority) keeping failed QA because of #2 If you agree with #3, #1 becomes useless If you provide a patch for #3 i'll be pleased, otherwise, i'll be OK to push this patch without it and open an entry for removal of OPACDisplayRequestPriority. ( But, of course, I prefer the patch ;-) ) > * 3rd = overall, i'm wondering if we really need 2 sysprefs, that does
> almost the same thing. Is there a use case where we want to display the
> number of requests but not your priority ? I feel it's meaningless = when
> you put your request, if you know there are already 5 holds, you'll be the
> 6th one. So shouldn't we just have one ? (and, maybe, remove
> OPACDisplayRequestPriority)
I think the two sys prefs are doing slightly different things.
OPACShowHoldsCount shows the total # of holds to everyone, logged in or not.
OPACDisplayRequestPriority shows the priority of your hold request, when you're logged in.
I think the difference is when a patron does a search in the OPAC, even not logged in he can see how many holds are already on a particular item. If there are already 50 holds, he may decide he doesn't want to go to the effort of logging in and placing a hold. If all the patron can see is his priority, then he wouldn't know there are 50 holds until after he has already logged in and placed his hold, which he would then need to cancel if he doesn't want to wait in line behind 50 other people.
Melia's right, both prefs are necessary: OPACShowHoldsCount shows how long the queue is. No login required. OPACDisplayRequestPriority shows where you are in the queue, once logged in. When OPACShowHoldsCount shows X, and you do not yet have a hold on the material, you can infer your priority at that moment will be X+1. Once you have a hold, however, you won't be able to see how close you are to getting it filled unless OPACDisplayRequestPriority is on The last two comments seem to come from the assumption that something is being taken away, and I don't see it that way. I think the question is, is there a situation where you would want to show one but not the other? Show total holds count but not the user's priority? Show the user's priority but not the overall hold count? Permissions for both could be combined into a "OPACShowHoldQueueDetails" or something like that. Created attachment 8625 [details] [review] patch This patch fixes the display issue. If you want me to replace sys prefs with "OPACShowHoldQueueDetails" please provide the set of options. The way it works now it would have 3 values: 1) none, 2) number of holds only, 3) both number of holds and priority (In reply to comment #12) > The last two comments seem to come from the assumption that something is > being taken away, and I don't see it that way. I think the question is, is > there a situation where you would want to show one but not the other? Show > total holds count but not the user's priority? Show the user's priority but > not the overall hold count? Permissions for both could be combined into a > "OPACShowHoldQueueDetails" or something like that. that's exactly what I wanted to say ! That's fine with me. Do we have consensus re options? Srdjan, for the three options, I think what you have proposed is good: 1) none 2) number of holds only 3) both number of holds and priority I just need to make sure that if you choose 2) that the patron will be able to see the number of holds regardless of whether he is logged in or not. The main concern of the library that is sponsoring this is showing patrons the # of holds on an item when the patron is NOT logged in. I understand that with 3) obviously the patron can only see his priority if logged in. I will check, but I assume 3) must be correct, cause no login - no user, therefore no priority. Created attachment 8701 [details] [review] patch It looks ok except when one person puts a specific item hold, and another person has a non-specific hold. Can you please have a look and comment. patch Apply? [yn] y Applying: bug_7398: Replaced OPACDisplayRequestPriority syspref with OPACShowHoldQueueDetails /home/nengard/kohaclone/.git/rebase-apply/patch:230: trailing whitespace. $_->{itemnumber} error: patch failed: installer/data/mysql/updatedatabase.pl:5109 error: installer/data/mysql/updatedatabase.pl: patch does not apply Patch failed at 0001 bug_7398: Replaced OPACDisplayRequestPriority syspref with OPACShowHoldQueueDetails 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". Patch left in /tmp/patch-q5mYrL.patch nengard@kohavm:~/kohaclone$ Created attachment 8737 [details] [review] bug_7398: Replaced OPACDisplayRequestPriority syspref with OPACShowHoldQueueDetails New syspref is Choice, options being: - None - Holds number only - Holds number and priority Show holds count on OPAC detail Just an updatedatabase conflict, fixed. Created attachment 9008 [details]
screenshot
I tested this and I see some problems:
1) There is no setting to keep current behaviour "only show priority" is not an available setting. We have no information, number of holds and number of holds and priority. I can imagine some libraries want the information about priority only to show to their patrons, not to everyone. So keeping current behaviour seems important to me.
2) I think the display is confusing if you have item level and title level holds. I am attaching a screenshot. Perhaps it would help to make the descriptions a bit more clear. The column could be named something like "specific item holds"? (Item level holds, holds on this item?) Currently it only says holds. The number at the bottom of the table could pehraps be "Total holds"?
> 1) There is no setting to keep current behaviour "only show priority" is not > an available setting. My thinking was that there's no reason to have "only show priority", ie if we want o show priority we may as well show the queue size. But I may be wrong. I will ad that one if you wish. > 2) The column could be named something like > "specific item holds"? (Item level holds, holds on this item?) Currently it > only says holds. The number at the bottom of the table could pehraps be > "Total holds"? We can do that. Please respond to 1) so I can do (or not) both things at once. Hi Srdjan, yes, please also make the current behaviour a valid option. In my opionino it should also be the default when updating existing installations. Created attachment 9251 [details] [review] patch Created attachment 9439 [details] [review] bug_7398: Replaced OPACDisplayRequestPriority syspref with OPACShowHoldQueueDetails New syspref is Choice, options being: - None - Priority only - Holds number only - Holds number and priority Show holds count on OPAC detail Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com> QA comment: trying to test this patch, I get a nasty = Template process failed: file error - parse error - /home/paul/koha.dev/koha-community/koha-tmpl/opac-tmpl/prog/en/modules/opac-reserve.tt line 314-315: unexpected token (]) [% END ] [% IF showholds && showpriority %] at /home/paul/koha.dev/koha-community/C4/Templates.pm line 127. ( My tests were done with the syspref set to holds_priority ) Could you please fix (probably a missing %) and resubmit ? Created attachment 9733 [details] [review] patch Sorry about that. It happens to me sometimes when I open files remotely. I try to exit file with :x, and somehow that : disappears, whichleaves only x and that deletes the character. Should be more careful. (In reply to comment #28) > Sorry about that. It happens to me sometimes when I open files remotely. I > try to exit file with :x, and somehow that : disappears, whichleaves only x > and that deletes the character. Should be more careful. <troll mode ON> You should first use a tool that is an efficient one, like Emacs <troll mode OFF> PS : just kidding, most of developers use VIM at BibLibre ;-) QA comments: * This patch clean some code indenting or writting, gg ! * perlcritic was complaining before the patch, still complains after the patch, not a reason to failed QA. * There are some additions like: +$template->param( showholds=>$show_holds_count); +$template->param( showpriority=>$show_priority); that could more cleanly be written +$template->param( showholds=>$show_holds_count, showpriority=>$show_priority ); but that's a minor comment, passed QA I always feel like the odd man out. I use joe set to wordstar style as my primary editor. (In reply to comment #29) > (In reply to comment #28) > > Sorry about that. It happens to me sometimes when I open files remotely. I > > try to exit file with :x, and somehow that : disappears, whichleaves only x > > and that deletes the character. Should be more careful. > > <troll mode ON> > You should first use a tool that is an efficient one, like Emacs > <troll mode OFF> > > PS : just kidding, most of developers use VIM at BibLibre ;-) (In reply to comment #31) > I always feel like the odd man out. I use joe set to wordstar style as my > primary editor. > I just write mine in my own blood, with a toothpick :) How do you sterilise a toothpick? (In reply to comment #33) > How do you sterilise a toothpick? I was always told that a real programmer doesn't need to sterilize the toothpick. There's so much cola in their veins nothing can harm them. I am looking at both master and 3.10 and I don't see this new system preference ... was there a problem with this patch and upgrades? Nicole, it is OPACShowHoldQueueDetails. You'll see 4 choices there now. Yes, the release notes need to be updated Created attachment 14138 [details]
release notes updated
Hi Nicole,
Here is the Koha 3.10 enhancements document fixed. I don't know/see how to update koha-community.org website. Do I have to re-upload the pdf & update the link & remove the previous file ? isn't there a versioning management?
For the PDF you do have to re-upload and change the link - for the post itself you just click 'edit' and edit the text of the post and it does keep track of revisions. Nicole |