Bug 7825

Summary: Change OPACItemHolds syspref to be of type Choice, choices being "no", "yes", "force"
Product: Koha Reporter: Srdjan Jankovic <srdjan>
Component: OPACAssignee: Jonathan Druart <jonathan.druart>
Status: CLOSED FIXED QA Contact: Katrin Fischer <katrin.fischer>
Severity: enhancement    
Priority: P5 - low CC: chris, claire.hernandez, gmcharlt, gwilliams, jdemuth, jonathan.druart, katrin.fischer, kyle, m.de.rooy, nengard, paul.poulain, sandboxes, stephane.delaye
Version: master   
Hardware: All   
OS: All   
See Also: http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=5786
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=13478
Change sponsored?: --- Patch complexity: Medium patch
Documentation contact: Documentation submission:
Text to go in the release notes:
Version(s) released in:
Attachments: patch
bug_7825: Sign off Changed OPACItemHolds syspref to be no|tes|force choice
bug_7825: Sign off Changed OPACItemHolds syspref to be no|yes|force choice
Bug 7825: Followup prevent submission without choosing an item
bug_7825: Sign off Changed OPACItemHolds syspref to be no|tes|force choice
Bug 7825: Followup prevent submission without choosing an item
Bug 7825: Followup values are 1, 0 or 'force'
Bug 7825: Followup - Change choices to avoid conflicts with system preference implicit typeing.
[SIGNED-OFF] Bug 7825 - Change OPACItemHolds syspref to be of type Choice, choices being "no", "yes", "force"
[SIGNED-OFF] Bug 7825: Followup prevent submission without choosing an item
Bug 7825: Followup - Change choices to avoid conflicts with system preference implicit typeing.
Bug 7825: Followup - Change choices to avoid conflicts with system preference implicit typing.
bug_7825: Sign off Changed OPACItemHolds syspref to be no|tes|force choice
Bug 7825: Followup prevent submission without choosing an item
Bug 7825: Followup values are 1, 0 or 'force'
FIX some ergonomic issues in opac-reserve
Bug 7825: FIX some ergonomic issues in opac-reserve
Bug 7825: FIX some ergonomic issues in opac-reserve
bug_7825: Sign off Changed OPACItemHolds syspref to be no|tes|force choice
Bug 7825: Followup prevent submission without choosing an item
Bug 7825: Followup values are 1, 0 or 'force'
Bug 7825: FIX some ergonomic issues in opac-reserve
bug_7825: Sign off Changed OPACItemHolds syspref to be no|tes|force choice
Bug 7825: Followup prevent submission without choosing an item
bug_7825: Sign off Changed OPACItemHolds syspref to be no|tes|force choice
Bug 7825: Followup prevent submission without choosing an item
bug_7825: Sign off Changed OPACItemHolds syspref to be no|tes|force choice
Bug 7825: Followup prevent submission without choosing an item
bug_7825: Sign off Changed OPACItemHolds syspref to be no|tes|force choice
Bug 7825: Followup prevent submission without choosing an item
Bug 7825: Updatedatabase should only change the type and options values.
bug_7825: Sign off Changed OPACItemHolds syspref to be no|tes|force choice
Bug 7825: Followup prevent submission without choosing an item
bug_7825: Sign off Changed OPACItemHolds syspref to be no|tes|force choice
Bug 7825: Followup prevent submission without choosing an item
Bug 7825: Updatedatabase should only change the type and options values.
Bug 7825: Changed OPACItemHolds syspref to be no|tes|force choice
Bug 7825: Changed OPACItemHolds syspref to be no|tes|force choice
Bug 7825: Changed OPACItemHolds syspref to be no|yes|force choice - bootstrap
Bug 7825: Clarify the yes and no values
Bug 7825: Clarify the yes and no values
Bug 7825: FIX the force value for the bootstrap theme
[PASSED QA] Bug 7825: Changed OPACItemHolds syspref to be no|yes|force choice
[PASSED QA] Bug 7825: Changed OPACItemHolds syspref to be no|yes|force choice - bootstrap
[PASSED QA] Bug 7825: Clarify the yes and no values
[PASSED QA] Bug 7825: FIX the force value for the bootstrap theme
[PASSED QA] Bug 7825: FIX the force value for the bootstrap theme
Bug 7825: QA followup - preselection of items
Bug 7825: QA followup - preselection of items (bootstrap)
[PASSED QA] Bug 7825: QA followup - preselection of items
[PASSED QA] Bug 7825: QA followup - preselection of items (bootstrap)
Bug 7825: Changed OPACItemHolds syspref to be no|yes|force choice
Bug 7825: Changed OPACItemHolds syspref to be no|yes|force choice - bootstrap
Bug 7825: Clarify the yes and no values
Bug 7825: FIX the force value for the bootstrap theme
Bug 7825: QA followup - preselection of items
Bug 7825: QA followup - preselection of items (bootstrap)

Description Srdjan Jankovic 2012-03-26 07:24:58 UTC
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
Comment 1 Srdjan Jankovic 2012-03-27 05:43:49 UTC Comment hidden (obsolete)
Comment 2 delaye 2012-03-27 12:41:53 UTC
i test the option force with this sys pref and it's work. It's a request of customers
i sign the patch
Comment 3 Paul Poulain 2012-03-27 12:42:53 UTC Comment hidden (obsolete)
Comment 4 delaye 2012-03-27 12:45:05 UTC Comment hidden (obsolete)
Comment 5 Paul Poulain 2012-03-29 14:09:47 UTC
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
Comment 6 Jonathan Druart 2012-10-17 09:53:42 UTC Comment hidden (obsolete)
Comment 7 Jonathan Druart 2012-10-17 09:55:14 UTC
(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.
Comment 8 delaye 2012-10-17 10:01:00 UTC
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
Comment 9 Jonathan Druart 2012-10-17 11:39:29 UTC Comment hidden (obsolete)
Comment 10 Jonathan Druart 2012-10-17 11:39:35 UTC Comment hidden (obsolete)
Comment 11 Jonathan Druart 2012-10-17 12:19:16 UTC Comment hidden (obsolete)
Comment 12 delaye 2012-10-17 12:57:34 UTC
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'
Comment 13 Paul Poulain 2012-10-17 13:27:50 UTC
(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 !)
Comment 14 Srdjan Jankovic 2012-10-18 00:13:24 UTC
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
Comment 15 Jonathan Druart 2012-10-18 08:39:58 UTC
(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.
Comment 16 Jonathan Druart 2012-10-18 09:07:54 UTC
Ho ok, it's what you explain in the Bug 7854 :)
Comment 17 Kyle M Hall 2012-10-18 17:30:49 UTC Comment hidden (obsolete)
Comment 18 Kyle M Hall 2012-10-18 17:33:53 UTC
> 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.
Comment 19 Kyle M Hall 2012-10-18 19:53:42 UTC Comment hidden (obsolete)
Comment 20 Kyle M Hall 2012-10-18 19:54:01 UTC Comment hidden (obsolete)
Comment 21 Kyle M Hall 2012-10-18 19:54:12 UTC Comment hidden (obsolete)
Comment 22 Kyle M Hall 2012-10-18 19:55:00 UTC Comment hidden (obsolete)
Comment 23 Chris Cormack 2012-10-18 19:58:02 UTC
(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.
Comment 24 Jonathan Druart 2012-10-22 15:09:14 UTC
(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.
Comment 25 Jonathan Druart 2012-10-29 15:42:45 UTC
No answer and no discussion, so I switch back to Needs SO.

Feedback welcomed :)
Comment 26 Jonathan Druart 2012-11-08 13:34:45 UTC Comment hidden (obsolete)
Comment 27 Jonathan Druart 2012-11-08 13:34:55 UTC Comment hidden (obsolete)
Comment 28 Jonathan Druart 2012-11-08 13:35:01 UTC Comment hidden (obsolete)
Comment 29 Jonathan Druart 2012-11-08 13:35:39 UTC
rebased patches
Comment 30 delaye 2012-11-08 13:58:39 UTC
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.
Comment 31 Jonathan Druart 2012-11-09 11:08:43 UTC Comment hidden (obsolete)
Comment 32 Jonathan Druart 2012-11-09 11:11:58 UTC
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
Comment 33 Jonathan Druart 2012-11-09 11:13:49 UTC Comment hidden (obsolete)
Comment 34 Paul Poulain 2012-11-09 15:21:11 UTC
Patch tested with a sandbox, by Delaye Stephane <stephane.delaye@biblibre.com>
Comment 35 Paul Poulain 2012-11-09 15:21:33 UTC Comment hidden (obsolete)
Comment 36 Katrin Fischer 2012-11-10 20:38:47 UTC
I think this new setting will have to be incorporated in bug 5786 - depending on what gets pushed first.
Comment 37 Jonathan Druart 2012-12-13 10:18:33 UTC
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.
Comment 38 Katrin Fischer 2012-12-13 10:23:37 UTC
5786 has been on and off the
Comment 39 Katrin Fischer 2012-12-13 10:24:48 UTC
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.
Comment 40 Marcel de Rooy 2013-01-20 17:53:26 UTC
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.
Comment 41 Jonathan Druart 2013-01-21 08:23:59 UTC
(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
Comment 42 Marcel de Rooy 2013-02-04 11:13:15 UTC
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
Comment 43 Jonathan Druart 2013-02-04 13:51:09 UTC Comment hidden (obsolete)
Comment 44 Jonathan Druart 2013-02-04 13:51:16 UTC Comment hidden (obsolete)
Comment 45 Jonathan Druart 2013-02-04 13:51:22 UTC Comment hidden (obsolete)
Comment 46 Jonathan Druart 2013-02-04 13:51:27 UTC Comment hidden (obsolete)
Comment 47 Jonathan Druart 2013-02-04 13:52:57 UTC
(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.
Comment 48 Marcel de Rooy 2013-02-22 08:39:59 UTC
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?
Comment 49 Jonathan Druart 2013-02-22 09:36:37 UTC Comment hidden (obsolete)
Comment 50 Jonathan Druart 2013-02-22 09:36:45 UTC Comment hidden (obsolete)
Comment 51 Jonathan Druart 2013-02-22 09:38:59 UTC
Marcel,
Same on a fresh install.
I rebased the 3 last patches. I hope it will be better like that.
Comment 52 Jonathan Druart 2013-02-22 09:39:27 UTC
s/rebased/rebased and squashed
Comment 53 Jonathan Druart 2013-05-28 14:59:07 UTC Comment hidden (obsolete)
Comment 54 Jonathan Druart 2013-05-28 14:59:12 UTC Comment hidden (obsolete)
Comment 55 Jonathan Druart 2013-05-28 14:59:40 UTC
Rebased patches...
Comment 56 Chris Cormack 2013-06-12 09:51:48 UTC
I can't QA this as its a Catalyst patch, can someone else please look at it
Comment 57 Jonathan Druart 2013-06-12 10:07:56 UTC
(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.
Comment 58 Marcel de Rooy 2013-06-12 11:05:05 UTC
(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 :)
Comment 59 Kyle M Hall 2013-06-12 11:46:11 UTC
CONFLICT (content): Merge conflict in koha-tmpl/opac-tmpl/prog/en/modules/opac-reserve.tt
Comment 60 Jonathan Druart 2013-06-12 11:52:04 UTC Comment hidden (obsolete)
Comment 61 Jonathan Druart 2013-06-12 11:52:11 UTC Comment hidden (obsolete)
Comment 62 Kyle M Hall 2013-06-12 12:04:14 UTC
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!
Comment 63 Jonathan Druart 2013-06-12 13:30:09 UTC
(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.
Comment 64 Kyle M Hall 2013-06-12 14:24:43 UTC
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.
Comment 65 Kyle M Hall 2013-06-12 14:58:21 UTC Comment hidden (obsolete)
Comment 66 Kyle M Hall 2013-06-12 14:58:38 UTC Comment hidden (obsolete)
Comment 67 Kyle M Hall 2013-06-12 15:11:29 UTC
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.
Comment 68 Kyle M Hall 2013-06-12 15:13:04 UTC
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.
Comment 69 Jonathan Druart 2013-06-13 09:36:15 UTC
(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!
Comment 70 Jonathan Druart 2013-06-13 09:40:52 UTC Comment hidden (obsolete)
Comment 71 Jonathan Druart 2013-06-13 09:43:24 UTC
Switch back to NSO but maybe it is not necessary to retest of the feature just for the DB change.
Comment 72 Kyle M Hall 2013-06-13 11:16:49 UTC
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.
Comment 73 Kyle M Hall 2013-06-13 11:20:54 UTC Comment hidden (obsolete)
Comment 74 Kyle M Hall 2013-06-13 11:21:13 UTC Comment hidden (obsolete)
Comment 75 Kyle M Hall 2013-06-13 11:21:44 UTC Comment hidden (obsolete)
Comment 76 Kyle M Hall 2013-06-13 11:21:56 UTC Comment hidden (obsolete)
Comment 77 Kyle M Hall 2013-06-13 11:22:10 UTC Comment hidden (obsolete)
Comment 78 Kyle M Hall 2013-06-13 11:30:14 UTC
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
Comment 79 Galen Charlton 2013-10-31 05:45:03 UTC
Patch no longer applies; merge conflict is not straightforward to resolve.
Comment 80 Jonathan Druart 2013-10-31 12:59:46 UTC Comment hidden (obsolete)
Comment 81 Jonathan Druart 2013-10-31 13:01:35 UTC
(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.
Comment 82 Biblibre Sandboxes 2013-11-12 13:43:16 UTC
Patch tested with a sandbox, by Koha team AMU <koha.aixmarseille@gmail.com>
Comment 83 Biblibre Sandboxes 2013-11-12 13:43:40 UTC Comment hidden (obsolete)
Comment 84 Kyle M Hall 2013-12-13 20:06:34 UTC
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.
Comment 85 Katrin Fischer 2013-12-13 21:22:13 UTC
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?
Comment 86 Jonathan Druart 2013-12-18 14:04:49 UTC Comment hidden (obsolete)
Comment 87 Jonathan Druart 2013-12-18 14:08:31 UTC
(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 88 Katrin Fischer 2014-01-11 19:47:42 UTC
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?
Comment 89 Katrin Fischer 2014-01-11 19:48:54 UTC
I will wait for a follow-up and then continue on this as soon as I can.
Comment 90 Jonathan Druart 2014-01-13 08:47:17 UTC Comment hidden (obsolete)
Comment 91 Katrin Fischer 2014-01-13 20:03:25 UTC
Thx Jonathan, back to testing this.
Comment 92 Katrin Fischer 2014-01-13 20:27:29 UTC
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?
Comment 93 Jonathan Druart 2014-01-14 08:48:38 UTC
(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.
Comment 94 Jonathan Druart 2014-01-14 08:49:27 UTC Comment hidden (obsolete)
Comment 95 Jonathan Druart 2014-01-14 08:49:35 UTC Comment hidden (obsolete)
Comment 96 Katrin Fischer 2014-01-18 08:27:45 UTC Comment hidden (obsolete)
Comment 97 Katrin Fischer 2014-01-18 08:28:03 UTC Comment hidden (obsolete)
Comment 98 Katrin Fischer 2014-01-18 08:28:15 UTC Comment hidden (obsolete)
Comment 99 Katrin Fischer 2014-01-18 08:28:25 UTC Comment hidden (obsolete)
Comment 100 Katrin Fischer 2014-01-18 08:35:05 UTC Comment hidden (obsolete)
Comment 101 Katrin Fischer 2014-01-18 08:37:13 UTC
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.
Comment 102 Jonathan Druart 2014-01-20 10:39:11 UTC Comment hidden (obsolete)
Comment 103 Jonathan Druart 2014-01-20 10:39:20 UTC Comment hidden (obsolete)
Comment 104 Jonathan Druart 2014-01-20 10:40:31 UTC
(In reply to Katrin Fischer from comment #101)
Hi Katrin,
The 2 last patches fix your request.
Comment 105 Katrin Fischer 2014-01-21 07:11:44 UTC Comment hidden (obsolete)
Comment 106 Katrin Fischer 2014-01-21 07:11:59 UTC Comment hidden (obsolete)
Comment 107 Katrin Fischer 2014-01-21 07:13:13 UTC
Hi Jonathan, thx for taking my suggestions into account. It works as described.
Comment 108 Jonathan Druart 2014-05-06 09:45:47 UTC
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.
Comment 109 Jonathan Druart 2014-05-06 09:45:53 UTC
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>
Comment 110 Jonathan Druart 2014-05-06 09:45:59 UTC
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>
Comment 111 Jonathan Druart 2014-05-06 09:46:04 UTC
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.
Comment 112 Jonathan Druart 2014-05-06 09:46:13 UTC
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>
Comment 113 Jonathan Druart 2014-05-06 09:46:26 UTC
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.
Comment 114 Jonathan Druart 2014-05-06 09:47:29 UTC
Last patch set fix conflicts with bug 11648.
Back to Passed QA.
Comment 115 Galen Charlton 2014-05-26 01:13:56 UTC
Pushed to master.  Thanks, Srdjan and Jonathan!