Work to be done: * Change OPACItemHolds syspref to be of type Choice, choices being "no", "yes", "force" * opac-reserve.pl: change if (OPACItemHolds) logic to if (OPACItemHolds eq "yes") * opac-reserve.tt: - change OPACItemHolds tests to take in account new values as appropriate - if OPACItemHolds is "force" do not display "Place On" column, and automatically open "Select a specific copy" table How to test: * Set OPACItemHolds to "no". Check that OPAC holds behave as it is now (no "Place On" column, no "Select a specific copy" table, making a reserve is successful) * Set OPACItemHolds to "yes". Check that OPAC holds behave as it is now ( "Place On" column is there, "Select a specific copy" table shows when "A specific copy" is selected, making a reserve is successful) * Set OPACItemHolds to "force". Check that OPAC holds behave as follows: - no "Place On" column is present - "Select a specific copy" table is opened - making a reserve is successful
Created attachment 8641 [details] [review] patch
i test the option force with this sys pref and it's work. It's a request of customers i sign the patch
Created attachment 8645 [details] [review] bug_7825: Sign off Changed OPACItemHolds syspref to be no|tes|force choice Amended opac-reserve.tt accordingly Signed-off-by: Delaye Stephane <stephane.delaye@biblibre.com>
Created attachment 8646 [details] [review] bug_7825: Sign off Changed OPACItemHolds syspref to be no|yes|force choice
My tests show 2 problems: * when you set the syspref to "allow", the user should be able to choose "next available" or any item. I can't have the item list appearing ! * when you set the syspref to "force", it seems good, as you get the items list and no option to say "next available". BUT, you can submit the form without choosing any item, and in this case, it seems that you've placed a "next available" hold. The 3rd option (don't allow) is OK Overall, it may be easy to fix, but for now it does not work. PS: anyone who tests and want to switch back to master = DELETE from systempreferences WHERE variable="OPACItemHolds"; INSERT INTO `systempreferences` (variable,value,explanation,options,type) VALUES('OPACItemHolds','1','Allow OPAC users to place hold on specific items. If OFF, users can only request next available copy.','','YesNo'); will revert you to "without 7825 applied" If you just switch to master branch, things go crazy because the syspref is no more 0/1
Created attachment 12881 [details] [review] Bug 7825: Followup prevent submission without choosing an item
(In reply to comment #5) > My tests show 2 problems: > * when you set the syspref to "allow", the user should be able to choose > "next available" or any item. I can't have the item list appearing ! I can't reproduce, "it works for me" > * when you set the syspref to "force", it seems good, as you get the items > list and no option to say "next available". BUT, you can submit the form > without choosing any item, and in this case, it seems that you've placed a > "next available" hold. I think my patch solves this problem.
i want test in a sandbox (biblibre) but i have the message 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/opac-tmpl/prog/en/modules/opac-reserve.tt CONFLICT (content): Merge conflict in koha-tmpl/opac-tmpl/prog/en/modules/opac-reserve.tt
Created attachment 12883 [details] [review] bug_7825: Sign off Changed OPACItemHolds syspref to be no|tes|force choice Amended opac-reserve.tt accordingly Signed-off-by: Delaye Stephane <stephane.delaye@biblibre.com>
Created attachment 12884 [details] [review] Bug 7825: Followup prevent submission without choosing an item
Created attachment 12885 [details] [review] Bug 7825: Followup values are 1, 0 or 'force'
if the force mode is active in 'OPACItemHolds' there must be one item is selected by default in this screen ../cgi-bin/koha/opac-reserve.pl?biblionumbers=... and in the table 'Select a specific copy'
(In reply to comment #12) > if the force mode is active in 'OPACItemHolds' there must be one item is > selected by default in this screen > > ../cgi-bin/koha/opac-reserve.pl?biblionumbers=... > > and in the table 'Select a specific copy' I add my word to what stephane says, because he just showed me the problem: if you set the value to "force" and validate your hold without selecting an item, the hold is placed ... 1st available, which should be forbidden by the "Force" syspref. The easiest solution to this problem is to select the 1st item in the list of items (that will avoid one click to all holds placed on records with just one item !)
No disrespect, I understand the motivation, but I deliberately created 7854 in order to be able to avoid 0, 1, force. I could have done it that way myself, but it is wrong and perpetuates a deficiency in the config system
(In reply to comment #14) > No disrespect, I understand the motivation, but I deliberately created 7854 > in order to be able to avoid 0, 1, force. I could have done it that way > myself, but it is wrong and perpetuates a deficiency in the config system Hi Srdjan! The values 'yes' and 'no' can't be choosen for systempreferences. When you change the value of OPACItemHolds using the interface (preferences.pl), only 'force' is keep. 'yes' and 'no' are respectively replaced with 1 and ''. So you can't use the comparaison with 'yes' in the template. This behaviour is due to the YAML::Syck::ImplicitTyping flag set to on (see http://search.cpan.org/~toddr/YAML-Syck-1.21/lib/YAML/Syck.pm#$YAML::Syck::ImplicitTyping). The simpliest way is to change 'yes' and 'no' with 'allow' and 'noallow' for example.
Ho ok, it's what you explain in the Bug 7854 :)
Created attachment 12915 [details] [review] Bug 7825: Followup - Change choices to avoid conflicts with system preference implicit typeing.
> The simpliest way is to change 'yes' and 'no' with 'allow' and 'noallow' for > example. I completely agree. Trying to override the implicit typing just to use 'yes' and 'no' seems a bit silly and overly complicated. I've written a followup to do just that ( coincidentally, I used 'allow' and 'noallow' without knowing you had suggested the same thing! ). Jonathan, I hope it's ok that I've obsoleted your one patch, as this patch is essentially a replacement for that one.
Created attachment 12920 [details] [review] [SIGNED-OFF] Bug 7825 - Change OPACItemHolds syspref to be of type Choice, choices being "no", "yes", "force" Signed-off-by: Delaye Stephane <stephane.delaye@biblibre.com> Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Created attachment 12921 [details] [review] [SIGNED-OFF] Bug 7825: Followup prevent submission without choosing an item Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Created attachment 12922 [details] [review] Bug 7825: Followup - Change choices to avoid conflicts with system preference implicit typeing.
Created attachment 12923 [details] [review] Bug 7825: Followup - Change choices to avoid conflicts with system preference implicit typing.
(In reply to comment #18) > > The simpliest way is to change 'yes' and 'no' with 'allow' and 'noallow' for > > example. > > I completely agree. Trying to override the implicit typing just to use 'yes' > and 'no' seems a bit silly and overly complicated. > > I've written a followup to do just that ( coincidentally, I used 'allow' and > 'noallow' without knowing you had suggested the same thing! ). > > Jonathan, I hope it's ok that I've obsoleted your one patch, as this patch > is essentially a replacement for that one. I disagree, implicit typing is wrong, and the real fix is what Srdjan outlined on bug 7854.
(In reply to comment #23) > I disagree, implicit typing is wrong, and the real fix is what Srdjan > outlined on bug 7854. Hi Chris! I agree with you, implicit typing is not what we want. But the Bug 7854 is open for 6 months and no one proposed a patch. So we have 3 (more?) solutions: 1/ Block this patch and wait for a patch for Bug 7854 (and maybe have side effect, we don't know). 2/ Keep ImplicitTyping for YesNo and remove it for others types. 3/ Considering that a YesNo admits a boolean value rather than a Choice type, it is not logical to have Yes, No and a third value (Yes and No should cover 100% of choices, but here we have a third choice) I think the option 2 is not persuasive. It requires a new layer to parse YAML. For me it sounds good to have not Yes, No and a third choice. And this patch could be in master before Bug 7854.
No answer and no discussion, so I switch back to Needs SO. Feedback welcomed :)
Created attachment 13305 [details] [review] bug_7825: Sign off Changed OPACItemHolds syspref to be no|tes|force choice Amended opac-reserve.tt accordingly Signed-off-by: Delaye Stephane <stephane.delaye@biblibre.com>
Created attachment 13306 [details] [review] Bug 7825: Followup prevent submission without choosing an item
Created attachment 13307 [details] [review] Bug 7825: Followup values are 1, 0 or 'force'
rebased patches
if in the system preference OPACItemHolds the option "force" is selected when the borrower make a hold the first item in the table "select a specific copy" must be checked by default.
Created attachment 13340 [details] [review] FIX some ergonomic issues in opac-reserve
The last patch fixes some ergonomic issues: - Preselect an item (force and allow option) - Add a toggle function in order to show or hide the items block
Created attachment 13341 [details] [review] Bug 7825: FIX some ergonomic issues in opac-reserve
Patch tested with a sandbox, by Delaye Stephane <stephane.delaye@biblibre.com>
Created attachment 13350 [details] [review] Bug 7825: FIX some ergonomic issues in opac-reserve http://bugs.koha-community.org/show_bug.cgi?id=7825 Signed-off-by: Delaye Stephane <stephane.delaye@biblibre.com>
I think this new setting will have to be incorporated in bug 5786 - depending on what gets pushed first.
Rebasing this patch is really a pain. It is signed off and the Bug 5786 is blocked in discussion. So I think we don't have to wait for it.
5786 has been on and off the
Bug 5786 has been around since 2011 - it hurts me to see that this important development is stuck in discussion. But I was not trying to say that this has to wait, only pointing out a conflict that will arise at some point that needs to be addressed.
Problem with third patch: Applying: Bug 7825: Followup values are 1, 0 or 'force' /usr/share/koha/testclone/.git/rebase-apply/patch:32: tab in indent. [% IF OPACItemHolds == '1' or OPACItemHolds == 'force' %] fatal: sha1 information is lacking or useless (koha-tmpl/opac-tmpl/prog/en/modules/opac-reserve.tt). Repository lacks necessary blobs to fall back on 3-way merge. Cannot fall back to three-way merge.
(In reply to comment #40) > Problem with third patch: > > Applying: Bug 7825: Followup values are 1, 0 or 'force' > /usr/share/koha/testclone/.git/rebase-apply/patch:32: tab in indent. > [% IF OPACItemHolds == '1' or OPACItemHolds == > 'force' %] > fatal: sha1 information is lacking or useless > (koha-tmpl/opac-tmpl/prog/en/modules/opac-reserve.tt). > Repository lacks necessary blobs to fall back on 3-way merge. > Cannot fall back to three-way merge. Hi Marcel, I cannot reproduce: $ git reset --hard origin/master $ git bz apply 7825 Bug 7825 - Change OPACItemHolds syspref to be of type Choice, choices being "no", "yes", "force" bug_7825: Sign off Changed OPACItemHolds syspref to be no|tes|force choice Apply? [yn] y Applying: bug_7825: Sign off Changed OPACItemHolds syspref to be no|tes|force choice Bug 7825: Followup prevent submission without choosing an item Apply? [yn] y Applying: Bug 7825: Followup prevent submission without choosing an item Bug 7825: Followup values are 1, 0 or 'force' Apply? [yn] y Applying: Bug 7825: Followup values are 1, 0 or 'force' Bug 7825: FIX some ergonomic issues in opac-reserve Apply? [yn] y Applying: Bug 7825: FIX some ergonomic issues in opac-reserve
Sorry, Jonathan. I retried on current master, with git bz now: Applying: Bug 7825: Followup values are 1, 0 or 'force' /usr/share/koha/testclone/.git/rebase-apply/patch:32: tab in indent. [% IF OPACItemHolds == '1' or OPACItemHolds == 'force' %] fatal: sha1 information is lacking or useless (koha-tmpl/opac-tmpl/prog/en/modules/opac-reserve.tt). Repository lacks necessary blobs to fall back on 3-way merge. Cannot fall back to three-way merge. Patch failed at 0001 Bug 7825: Followup values are 1, 0 or 'force' Does not apply
Created attachment 15032 [details] [review] bug_7825: Sign off Changed OPACItemHolds syspref to be no|tes|force choice Amended opac-reserve.tt accordingly Signed-off-by: Delaye Stephane <stephane.delaye@biblibre.com>
Created attachment 15033 [details] [review] Bug 7825: Followup prevent submission without choosing an item
Created attachment 15034 [details] [review] Bug 7825: Followup values are 1, 0 or 'force'
Created attachment 15035 [details] [review] Bug 7825: FIX some ergonomic issues in opac-reserve http://bugs.koha-community.org/show_bug.cgi?id=7825 Signed-off-by: Delaye Stephane <stephane.delaye@biblibre.com>
(In reply to comment #42) > Sorry, Jonathan. I retried on current master, with git bz now: > > Applying: Bug 7825: Followup values are 1, 0 or 'force' > /usr/share/koha/testclone/.git/rebase-apply/patch:32: tab in indent. > [% IF OPACItemHolds == '1' or OPACItemHolds == > 'force' %] > fatal: sha1 information is lacking or useless > (koha-tmpl/opac-tmpl/prog/en/modules/opac-reserve.tt). > Repository lacks necessary blobs to fall back on 3-way merge. > Cannot fall back to three-way merge. > Patch failed at 0001 Bug 7825: Followup values are 1, 0 or 'force' > > Does not apply Marcel, I just rebased these patchs because of I got a conflict on the updatedatabase.pl file. But I didn't get your error.
Applying: Bug 7825: Followup values are 1, 0 or 'force' /usr/share/koha/testclone/.git/rebase-apply/patch:32: tab in indent. [% IF OPACItemHolds == '1' or OPACItemHolds == 'force' %] fatal: sha1 information is lacking or useless (koha-tmpl/opac-tmpl/prog/en/modules/opac-reserve.tt). Repository lacks necessary blobs to fall back on 3-way merge. Cannot fall back to three-way merge. Patch failed at 0001 Bug 7825: Followup values are 1, 0 or 'force' Could you please check on a fresh/clean git?
Created attachment 15606 [details] [review] bug_7825: Sign off Changed OPACItemHolds syspref to be no|tes|force choice Amended opac-reserve.tt accordingly Signed-off-by: Delaye Stephane <stephane.delaye@biblibre.com>
Created attachment 15607 [details] [review] Bug 7825: Followup prevent submission without choosing an item + Fix some ergonomic issues in opac-reserve Signed-off-by: Delaye Stephane <stephane.delaye@biblibre.com>
Marcel, Same on a fresh install. I rebased the 3 last patches. I hope it will be better like that.
s/rebased/rebased and squashed
Created attachment 18435 [details] [review] bug_7825: Sign off Changed OPACItemHolds syspref to be no|tes|force choice Amended opac-reserve.tt accordingly Signed-off-by: Delaye Stephane <stephane.delaye@biblibre.com>
Created attachment 18436 [details] [review] Bug 7825: Followup prevent submission without choosing an item + Fix some ergonomic issues in opac-reserve Signed-off-by: Delaye Stephane <stephane.delaye@biblibre.com>
Rebased patches...
I can't QA this as its a Catalyst patch, can someone else please look at it
(In reply to comment #56) > I can't QA this as its a Catalyst patch, can someone else please look at it And I proposed a patch + SO from BibLibre, I cannot either.
(In reply to comment #57) > (In reply to comment #56) > > I can't QA this as its a Catalyst patch, can someone else please look at it > > And I proposed a patch + SO from BibLibre, I cannot either. Luckily, we have more people on the QA team :)
CONFLICT (content): Merge conflict in koha-tmpl/opac-tmpl/prog/en/modules/opac-reserve.tt
Created attachment 18878 [details] [review] bug_7825: Sign off Changed OPACItemHolds syspref to be no|tes|force choice Amended opac-reserve.tt accordingly Signed-off-by: Delaye Stephane <stephane.delaye@biblibre.com>
Created attachment 18879 [details] [review] Bug 7825: Followup prevent submission without choosing an item + Fix some ergonomic issues in opac-reserve Signed-off-by: Delaye Stephane <stephane.delaye@biblibre.com>
It looks like there is a value conflict here. The database update is assuming the syspref value of '1' should be converted to 'yes', but the code is still assuming it that it is '1'. I think leaving 'no' as an empty value makes the template code cleaner, but I think it would be better to keep the alteration of '1' to 'yes'. However, it's probably much easier to alter the database update, so I wouldn't fail qa just for keeping '1' instead of 'yes'. I hope this makes sense!
(In reply to comment #62) > It looks like there is a value conflict here. The database update is > assuming the syspref value of '1' should be converted to 'yes', but the code > is still assuming it that it is '1'. > > I think leaving 'no' as an empty value makes the template code cleaner, but > I think it would be better to keep the alteration of '1' to 'yes'. However, > it's probably much easier to alter the database update, so I wouldn't fail > qa just for keeping '1' instead of 'yes'. > > I hope this makes sense! Hum... whaou, not easy to understand :) In fact "yes" and "no" are "transformed" to 1 and "" (see comment 15 and bug 7854). So if we want to have 1 and 0 in DB, we will got "1", "0" and "force" which is not really cleaner I think. We cannot set "yes", "no" and "force" in the DB and to test strings ("yes", "no", "force") in the template.
Thanks for the clarity! I should have remembered that! Kyle (In reply to comment #63) > (In reply to comment #62) > > It looks like there is a value conflict here. The database update is > > assuming the syspref value of '1' should be converted to 'yes', but the code > > is still assuming it that it is '1'. > > > > I think leaving 'no' as an empty value makes the template code cleaner, but > > I think it would be better to keep the alteration of '1' to 'yes'. However, > > it's probably much easier to alter the database update, so I wouldn't fail > > qa just for keeping '1' instead of 'yes'. > > > > I hope this makes sense! > > Hum... whaou, not easy to understand :) > In fact "yes" and "no" are "transformed" to 1 and "" (see comment 15 and bug > 7854). > So if we want to have 1 and 0 in DB, we will got "1", "0" and "force" which > is not really cleaner I think. > We cannot set "yes", "no" and "force" in the DB and to test strings ("yes", > "no", "force") in the template.
Created attachment 18903 [details] [review] bug_7825: Sign off Changed OPACItemHolds syspref to be no|tes|force choice Amended opac-reserve.tt accordingly Signed-off-by: Delaye Stephane <stephane.delaye@biblibre.com> Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Created attachment 18904 [details] [review] Bug 7825: Followup prevent submission without choosing an item + Fix some ergonomic issues in opac-reserve Signed-off-by: Delaye Stephane <stephane.delaye@biblibre.com> Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
I was a bit premature passing QA ( sorry! ). Passes qa tests. The commit messages are basically non-existent so I would suggest someone fix up the commit messages. The big issue is still the database values for this are '1', '0' ( actually empty string, but no need to split hairs ), and 'force', but the database update is still modifying them to be 'yes' or 'no'. I think the simple solution would be to remove the database update, which is superfluous, and breaks the functionality.
Actually, I'm wrong, the database updated is needed to change it to a 'choice' with the choice values, but the actual preference value should not be updated. > I think the simple solution would be to remove the database update, which is > superfluous, and breaks the functionality.
(In reply to comment #68) > Actually, I'm wrong, the database updated is needed to change it to a > 'choice' with the choice values, but the actual preference value should not > be updated. > > > I think the simple solution would be to remove the database update, which is > > superfluous, and breaks the functionality. Yes!
Created attachment 18920 [details] [review] Bug 7825: Updatedatabase should only change the type and options values. The value field should not be change. We have to keep '1' for 'yes' and '' for 'no'.
Switch back to NSO but maybe it is not necessary to retest of the feature just for the DB change.
The db change does not alter the behavior of the patch set, so I don't believe it needs to be signed off separately. It's more of a qa followup. (In reply to comment #71) > Switch back to NSO but maybe it is not necessary to retest of the feature > just for the DB change.
Created attachment 18924 [details] [review] bug_7825: Sign off Changed OPACItemHolds syspref to be no|tes|force choice Amended opac-reserve.tt accordingly Signed-off-by: Delaye Stephane <stephane.delaye@biblibre.com> Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Created attachment 18925 [details] [review] Bug 7825: Followup prevent submission without choosing an item + Fix some ergonomic issues in opac-reserve Signed-off-by: Delaye Stephane <stephane.delaye@biblibre.com> Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Created attachment 18926 [details] [review] bug_7825: Sign off Changed OPACItemHolds syspref to be no|tes|force choice Amended opac-reserve.tt accordingly Signed-off-by: Delaye Stephane <stephane.delaye@biblibre.com> Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Created attachment 18927 [details] [review] Bug 7825: Followup prevent submission without choosing an item + Fix some ergonomic issues in opac-reserve Signed-off-by: Delaye Stephane <stephane.delaye@biblibre.com> Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Created attachment 18928 [details] [review] Bug 7825: Updatedatabase should only change the type and options values. The value field should not be change. We have to keep '1' for 'yes' and '' for 'no'. Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Everything looks good, though we still have a lack of a decent commit message. Perhaps this will do: Currently, the Koha system preference OPACItemHolds allows a library system to choose to allow patrons to place bib and item level holds from the OPAC, or to allow patrons only to place bib level holds only from the opac. This patch set adds a third option, "force", which *requires* that patrons place item level holds from the opac, with no option of making a bib level hold. This patch modifies the system preference OPACItemHolds to be of the type "Choice", with the choices being "no", "yes", "force". The patch does not alter the currently set value of OPACItemHolds for database updates, only adding the additional option. Test Plan: 1) Set OPACItemHolds to "no". Check that OPAC holds behave as it is now (no "Place On" column, no "Select a specific copy" table, making a reserve is successful) 2) Set OPACItemHolds to "yes". Check that OPAC holds behave as it is now ( "Place On" column is there, "Select a specific copy" table shows when "A specific copy" is selected, making a reserve is successful) 3) Set OPACItemHolds to "force". Check that OPAC holds behave as follows: - no "Place On" column is present - "Select a specific copy" table is opened - making a reserve is successful
Patch no longer applies; merge conflict is not straightforward to resolve.
Created attachment 22635 [details] [review] Bug 7825: Changed OPACItemHolds syspref to be no|tes|force choice Currently, the Koha system preference OPACItemHolds allows a library system to choose to allow patrons to place bib and item level holds from the OPAC, or to allow patrons only to place bib level holds only from the opac. This patch set adds a third option, "force", which *requires* that patrons place item level holds from the opac, with no option of making a bib level hold. This patch modifies the system preference OPACItemHolds to be of the type "Choice", with the choices being "no", "yes", "force". The patch does not alter the currently set value of OPACItemHolds for database updates, only adding the additional option. Test Plan: 1) Set OPACItemHolds to "no". Check that OPAC holds behave as it is now (no "Place On" column, no "Select a specific copy" table, making a reserve is successful) 2) Set OPACItemHolds to "yes". Check that OPAC holds behave as it is now ( "Place On" column is there, "Select a specific copy" table shows when "A specific copy" is selected, making a reserve is successful) 3) Set OPACItemHolds to "force". Check that OPAC holds behave as follows: - no "Place On" column is present - "Select a specific copy" table is opened - making a reserve is successful This patch is a squashed patch of the 3 following patches: Amended opac-reserve.tt accordingly Signed-off-by: Delaye Stephane <stephane.delaye@biblibre.com> Bug 7825: Followup prevent submission without choosing an item + Fix some ergonomic issues in opac-reserve Signed-off-by: Delaye Stephane <stephane.delaye@biblibre.com> Bug 7825: Updatedatabase should only change the type and options values. The value field should not be change. We have to keep '1' for 'yes' and '' for 'no'. Original patch is from Srdjan Jankovic.
(In reply to Galen Charlton from comment #79) > Patch no longer applies; merge conflict is not straightforward to resolve. Not easy to rebase, bug 10836 changed a big part of the html stucture. Switch back to Needs Signoff :-( I squashed all 3 patches, I did not manage to rebase them cleanly.
Patch tested with a sandbox, by Koha team AMU <koha.aixmarseille@gmail.com>
Created attachment 22877 [details] [review] Bug 7825: Changed OPACItemHolds syspref to be no|tes|force choice Currently, the Koha system preference OPACItemHolds allows a library system to choose to allow patrons to place bib and item level holds from the OPAC, or to allow patrons only to place bib level holds only from the opac. This patch set adds a third option, "force", which *requires* that patrons place item level holds from the opac, with no option of making a bib level hold. This patch modifies the system preference OPACItemHolds to be of the type "Choice", with the choices being "no", "yes", "force". The patch does not alter the currently set value of OPACItemHolds for database updates, only adding the additional option. Test Plan: 1) Set OPACItemHolds to "no". Check that OPAC holds behave as it is now (no "Place On" column, no "Select a specific copy" table, making a reserve is successful) 2) Set OPACItemHolds to "yes". Check that OPAC holds behave as it is now ( "Place On" column is there, "Select a specific copy" table shows when "A specific copy" is selected, making a reserve is successful) 3) Set OPACItemHolds to "force". Check that OPAC holds behave as follows: - no "Place On" column is present - "Select a specific copy" table is opened - making a reserve is successful This patch is a squashed patch of the 3 following patches: Amended opac-reserve.tt accordingly Signed-off-by: Delaye Stephane <stephane.delaye@biblibre.com> Bug 7825: Followup prevent submission without choosing an item + Fix some ergonomic issues in opac-reserve Signed-off-by: Delaye Stephane <stephane.delaye@biblibre.com> Bug 7825: Updatedatabase should only change the type and options values. The value field should not be change. We have to keep '1' for 'yes' and '' for 'no'. Original patch is from Srdjan Jankovic. Signed-off-by: Koha team AMU <koha.aixmarseille@gmail.com>
Works for ccsr and prog, but not bootstrap. One other comment: when forcing item holds, it would probably be best to display the list of items be default so the patron knows which specific copy has been selected for the hold.
Hm, may be a misunderstanding - I think forcing the holds means that you can't place a bib level hold, but only an item level hold?
Created attachment 23631 [details] [review] Bug 7825: Changed OPACItemHolds syspref to be no|yes|force choice - bootstrap Same test plan as previous patch but with the bootstrap theme set.
(In reply to Kyle M Hall from comment #84) > One other comment: when forcing item holds, it would probably be best to > display the list of items be default so the patron knows which specific copy > has been selected for the hold. I would prefer not to modify the tt file since it is signed off. I don't think it could be considered as blocker.
Comment on attachment 22877 [details] [review] Bug 7825: Changed OPACItemHolds syspref to be no|tes|force choice Review of attachment 22877 [details] [review]: ----------------------------------------------------------------- Hi Jonathan, in your comment you wrote: The value field should not be change. We have to keep '1' for 'yes' and '' for 'no'. But I still see the yes/no in the database update and syspref part? ::: installer/data/mysql/sysprefs.sql @@ +228,4 @@ > ('OpacHiddenItems','','','This syspref allows to define custom rules for hiding specific items at opac. See docs/opac/OpacHiddenItems.txt for more informations.','Textarea'), > ('OpacHighlightedWords','1','','If Set, then queried words are higlighted in OPAC','YesNo'), > ('OpacHoldNotes','0','','Show hold notes on OPAC','YesNo'), > +('OPACItemHolds','yes','no|yes|force','Allow OPAC users to place hold on specific items. If No, users can only request next available copy. If Yes, users can choose between next available and specific copy. If Force, users *must* choose a specific copy.','Choice'), Should be 0 and 1? ::: installer/data/mysql/updatedatabase.pl @@ +7745,5 @@ > > + > +$DBversion = "3.13.00.XXX"; > +if ( C4::Context->preference("Version") < TransformToNum($DBversion) ) { > + $dbh->do("UPDATE systempreferences SET type = 'Choice', options = 'no|yes|force' WHERE variable = 'OPACItemHolds'"); Should be 0 and 1?
I will wait for a follow-up and then continue on this as soon as I can.
Created attachment 24218 [details] [review] Bug 7825: Clarify the yes and no values The value of the pref should be '1' (for yes) or '0' (for no). The options could be kept as 'yes' and 'no': since the ImplicitTyping is set, yes/no would become '1/0'. But I think it is preferable not to use the yes/no value in order not to introduce a bug here if ImplicitTyping is unset later.
Thx Jonathan, back to testing this.
Hi Jonathan, I fixed the database update to be 15 and include the bug number, but now I am stuck: I can't get force to behave as described in the bootstrap theme. It behaves like 'no', not allowing me to set item level holds. I have checked in the database the value for the pref is really set to 'force' and have tried reloading the Javascript. Jonathan, can you please check and also include the other 2 changes if another patch is needed?
(In reply to Katrin Fischer from comment #92) > Hi Jonathan, > I fixed the database update to be 15 and include the bug number, but now I > am stuck: I can't get force to behave as described in the bootstrap theme. > It behaves like 'no', not allowing me to set item level holds. > I have checked in the database the value for the pref is really set to > 'force' and have tried reloading the Javascript. Katrin, I am very sorry! I was sure to test all cases but it seems I missed something. Given that it is a specific case and the code is different from the prog theme, I am sure I have tested it! Anyway, it could not work with this code... I will provide a followup. > Jonathan, can you please check and also include the other 2 changes if > another patch is needed? Yes.
Created attachment 24270 [details] [review] Bug 7825: Clarify the yes and no values The value of the pref should be '1' (for yes) or '0' (for no). The options could be kept as 'yes' and 'no': since the ImplicitTyping is set, yes/no would become '1/0'. But I think it is preferable not to use the yes/no value in order not to introduce a bug here if ImplicitTyping is unset later.
Created attachment 24271 [details] [review] Bug 7825: FIX the force value for the bootstrap theme If the pref was set to 'force', the item selection was not shown. .copiesrow should be hidden before showing specific #copiesrow_ID
Created attachment 24538 [details] [review] [PASSED QA] Bug 7825: Changed OPACItemHolds syspref to be no|yes|force choice Currently, the Koha system preference OPACItemHolds allows a library system to choose to allow patrons to place bib and item level holds from the OPAC, or to allow patrons only to place bib level holds only from the opac. This patch set adds a third option, "force", which *requires* that patrons place item level holds from the opac, with no option of making a bib level hold. This patch modifies the system preference OPACItemHolds to be of the type "Choice", with the choices being "no", "yes", "force". The patch does not alter the currently set value of OPACItemHolds for database updates, only adding the additional option. Test Plan: 1) Set OPACItemHolds to "no". Check that OPAC holds behave as it is now (no "Place On" column, no "Select a specific copy" table, making a reserve is successful) 2) Set OPACItemHolds to "yes". Check that OPAC holds behave as it is now ( "Place On" column is there, "Select a specific copy" table shows when "A specific copy" is selected, making a reserve is successful) 3) Set OPACItemHolds to "force". Check that OPAC holds behave as follows: - no "Place On" column is present - "Select a specific copy" table is opened - making a reserve is successful This patch is a squashed patch of the 3 following patches: Amended opac-reserve.tt accordingly Signed-off-by: Delaye Stephane <stephane.delaye@biblibre.com> Bug 7825: Followup prevent submission without choosing an item + Fix some ergonomic issues in opac-reserve Signed-off-by: Delaye Stephane <stephane.delaye@biblibre.com> Bug 7825: Updatedatabase should only change the type and options values. The value field should not be change. We have to keep '1' for 'yes' and '' for 'no'. Original patch is from Srdjan Jankovic. Signed-off-by: Koha team AMU <koha.aixmarseille@gmail.com> Signed-off-by: Katrin Fischer <Katrin.Fischer.83@web.de> Testing notes on 4th patch.
Created attachment 24539 [details] [review] [PASSED QA] Bug 7825: Changed OPACItemHolds syspref to be no|yes|force choice - bootstrap Same test plan as previous patch but with the bootstrap theme set. Signed-off-by: Katrin Fischer <Katrin.Fischer.83@web.de>
Created attachment 24540 [details] [review] [PASSED QA] Bug 7825: Clarify the yes and no values The value of the pref should be '1' (for yes) or '0' (for no). The options could be kept as 'yes' and 'no': since the ImplicitTyping is set, yes/no would become '1/0'. But I think it is preferable not to use the yes/no value in order not to introduce a bug here if ImplicitTyping is unset later. Signed-off-by: Katrin Fischer <Katrin.Fischer.83@web.de>
Created attachment 24541 [details] [review] [PASSED QA] Bug 7825: FIX the force value for the bootstrap theme If the pref was set to 'force', the item selection was not shown. .copiesrow should be hidden before showing specific #copiesrow_ID Signed-off-by: Katrin Fischer <Katrin.Fischer.83@web.de> Passes all tests and QA script. Works on template level, tested with Boostrap and Prog theme with all 3 possible seetings of OPACItemHolds: 1) no = offer only title level holds 2) yes = offer both title and item level holds 3) force = offer only item level holds Also ran some additional tests on the Boostrap theme with deactivated Javascript. Note: I like this, but I think when "force" is selected the item list needs to be shown by default. I am also not so happy that the first item is automatically selected, but this is the current behaviour without the patches as well.
Created attachment 24542 [details] [review] [PASSED QA] Bug 7825: FIX the force value for the bootstrap theme If the pref was set to 'force', the item selection was not shown. .copiesrow should be hidden before showing specific #copiesrow_ID Signed-off-by: Katrin Fischer <Katrin.Fischer.83@web.de> Passes all tests and QA script. Works on template level, tested with Boostrap and Prog theme with all 3 possible seetings of OPACItemHolds: 1) no = offer only title level holds 2) yes = offer both title and item level holds 3) force = offer only item level holds Also ran some additional tests on the Boostrap theme with deactivated Javascript. Note: I like this, but I think when "force" is selected the item list needs to be shown by default. I am also not happy about preselecting the first item.
I was close to passing this, as you can see from the attached patches, but I am not happy about the force behaviour. I think Kyle already pointed it out in comment 84: When force is selected, the item list is not shown by default. Additionally the first item is preselected. Both points bother me. The user will have no idea which item, which itemtype (short loan? normal loan?) at which branch got selected for the hold to fill. I think this is kind of missing the point of forcing the patron to pick the item to fill the hold. For serials, where it really matters which item you pick, this is even more difficult.
Created attachment 24578 [details] [review] Bug 7825: QA followup - preselection of items This patch changes the default behavior for the 'force' value. If the pref is set to force, no item is preselected and the item list is shown.
Created attachment 24579 [details] [review] Bug 7825: QA followup - preselection of items (bootstrap)
(In reply to Katrin Fischer from comment #101) Hi Katrin, The 2 last patches fix your request.
Created attachment 24591 [details] [review] [PASSED QA] Bug 7825: QA followup - preselection of items This patch changes the default behavior for the 'force' value. If the pref is set to force, no item is preselected and the item list is shown. Signed-off-by: Katrin Fischer <Katrin.Fischer.83@web.de>
Created attachment 24592 [details] [review] [PASSED QA] Bug 7825: QA followup - preselection of items (bootstrap) Signed-off-by: Katrin Fischer <Katrin.Fischer.83@web.de> Passes all tests and QA script. Retested with all 3 themes, works as expected.
Hi Jonathan, thx for taking my suggestions into account. It works as described.
Created attachment 27961 [details] [review] Bug 7825: Changed OPACItemHolds syspref to be no|yes|force choice Currently, the Koha system preference OPACItemHolds allows a library system to choose to allow patrons to place bib and item level holds from the OPAC, or to allow patrons only to place bib level holds only from the opac. This patch set adds a third option, "force", which *requires* that patrons place item level holds from the opac, with no option of making a bib level hold. This patch modifies the system preference OPACItemHolds to be of the type "Choice", with the choices being "no", "yes", "force". The patch does not alter the currently set value of OPACItemHolds for database updates, only adding the additional option. Test Plan: 1) Set OPACItemHolds to "no". Check that OPAC holds behave as it is now (no "Place On" column, no "Select a specific copy" table, making a reserve is successful) 2) Set OPACItemHolds to "yes". Check that OPAC holds behave as it is now ( "Place On" column is there, "Select a specific copy" table shows when "A specific copy" is selected, making a reserve is successful) 3) Set OPACItemHolds to "force". Check that OPAC holds behave as follows: - no "Place On" column is present - "Select a specific copy" table is opened - making a reserve is successful This patch is a squashed patch of the 3 following patches: Amended opac-reserve.tt accordingly Signed-off-by: Delaye Stephane <stephane.delaye@biblibre.com> Bug 7825: Followup prevent submission without choosing an item + Fix some ergonomic issues in opac-reserve Signed-off-by: Delaye Stephane <stephane.delaye@biblibre.com> Bug 7825: Updatedatabase should only change the type and options values. The value field should not be change. We have to keep '1' for 'yes' and '' for 'no'. Original patch is from Srdjan Jankovic. Signed-off-by: Koha team AMU <koha.aixmarseille@gmail.com> Signed-off-by: Katrin Fischer <Katrin.Fischer.83@web.de> Testing notes on 4th patch.
Created attachment 27962 [details] [review] Bug 7825: Changed OPACItemHolds syspref to be no|yes|force choice - bootstrap Same test plan as previous patch but with the bootstrap theme set. Signed-off-by: Katrin Fischer <Katrin.Fischer.83@web.de>
Created attachment 27963 [details] [review] Bug 7825: Clarify the yes and no values The value of the pref should be '1' (for yes) or '0' (for no). The options could be kept as 'yes' and 'no': since the ImplicitTyping is set, yes/no would become '1/0'. But I think it is preferable not to use the yes/no value in order not to introduce a bug here if ImplicitTyping is unset later. Signed-off-by: Katrin Fischer <Katrin.Fischer.83@web.de>
Created attachment 27964 [details] [review] Bug 7825: FIX the force value for the bootstrap theme If the pref was set to 'force', the item selection was not shown. .copiesrow should be hidden before showing specific #copiesrow_ID Signed-off-by: Katrin Fischer <Katrin.Fischer.83@web.de> Passes all tests and QA script. Works on template level, tested with Boostrap and Prog theme with all 3 possible seetings of OPACItemHolds: 1) no = offer only title level holds 2) yes = offer both title and item level holds 3) force = offer only item level holds Also ran some additional tests on the Boostrap theme with deactivated Javascript. Note: I like this, but I think when "force" is selected the item list needs to be shown by default. I am also not happy about preselecting the first item.
Created attachment 27965 [details] [review] Bug 7825: QA followup - preselection of items This patch changes the default behavior for the 'force' value. If the pref is set to force, no item is preselected and the item list is shown. Signed-off-by: Katrin Fischer <Katrin.Fischer.83@web.de>
Created attachment 27966 [details] [review] Bug 7825: QA followup - preselection of items (bootstrap) Signed-off-by: Katrin Fischer <Katrin.Fischer.83@web.de> Passes all tests and QA script. Retested with all 3 themes, works as expected.
Last patch set fix conflicts with bug 11648. Back to Passed QA.
Pushed to master. Thanks, Srdjan and Jonathan!