Bug 5786 - Move AllowOnShelfHolds and OpacItemHolds system preferences to the Circulation Matrix
Summary: Move AllowOnShelfHolds and OpacItemHolds system preferences to the Circulatio...
Status: CLOSED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: Hold requests (show other bugs)
Version: master
Hardware: All All
: P5 - low new feature (vote)
Assignee: Srdjan Jankovic
QA Contact: Marcel de Rooy
URL:
Keywords:
: 5434 5787 5788 8372 9974 (view as bug list)
Depends on:
Blocks: 8362 14792 26287 5609 5787 5788
  Show dependency treegraph
 
Reported: 2011-02-21 19:55 UTC by Chris Cormack
Modified: 2020-08-25 13:37 UTC (History)
16 users (show)

See Also:
Change sponsored?: Sponsored
Patch complexity: ---
Documentation contact:
Documentation submission:
Text to go in the release notes:
Version(s) released in:


Attachments
patch (41.88 KB, patch)
2011-07-22 03:25 UTC, Srdjan Jankovic
Details | Diff | Splinter Review
patch (41.89 KB, patch)
2011-07-27 06:00 UTC, Srdjan Jankovic
Details | Diff | Splinter Review
signed off patch (41.96 KB, patch)
2011-07-27 12:17 UTC, Nicole C. Engard
Details | Diff | Splinter Review
patch (31.20 KB, patch)
2011-12-21 03:35 UTC, Srdjan Jankovic
Details | Diff | Splinter Review
patch (31.20 KB, patch)
2012-01-02 23:24 UTC, Srdjan Jankovic
Details | Diff | Splinter Review
patch (30.80 KB, patch)
2012-01-03 23:54 UTC, Srdjan Jankovic
Details | Diff | Splinter Review
patch (29.94 KB, patch)
2012-01-05 01:48 UTC, Srdjan Jankovic
Details | Diff | Splinter Review
bug_5786, bug_5787, bug_5788: Holds rewrite (57.24 KB, patch)
2012-07-25 03:44 UTC, Srdjan Jankovic
Details | Diff | Splinter Review
[SIGNED-OFF] bug_5786, bug_5787, bug_5788: Holds rewrite (57.39 KB, patch)
2012-07-25 18:50 UTC, Nicole C. Engard
Details | Diff | Splinter Review
bug_5786, bug_5787, bug_5788: Holds rewrite (57.12 KB, patch)
2012-07-30 00:08 UTC, Srdjan Jankovic
Details | Diff | Splinter Review
bug_5786, bug_5787, bug_5788: Holds rewrite (57.29 KB, patch)
2012-07-30 00:14 UTC, Srdjan Jankovic
Details | Diff | Splinter Review
bug_5786, bug_5787, bug_5788: Holds rewrite (57.32 KB, patch)
2012-07-30 12:13 UTC, Kyle M Hall
Details | Diff | Splinter Review
bug_5786, bug_5787, bug_5788: Holds rewrite (76.80 KB, patch)
2012-08-31 06:40 UTC, Srdjan Jankovic
Details | Diff | Splinter Review
bug_5786, bug_5787, bug_5788: Holds rewrite (76.89 KB, patch)
2012-09-05 20:08 UTC, Kyle M Hall
Details | Diff | Splinter Review
bug_5786, bug_5787, bug_5788: Holds rewrite (77.13 KB, patch)
2012-09-19 00:22 UTC, Srdjan Jankovic
Details | Diff | Splinter Review
Bug 5786 follow-up HTML4 coding guideline, removing capitals (1.32 KB, patch)
2012-09-21 16:11 UTC, Paul Poulain
Details | Diff | Splinter Review
bug 5786 follow-up quiet perl -wc (720 bytes, patch)
2012-09-21 16:11 UTC, Paul Poulain
Details | Diff | Splinter Review
bug_5786, bug_5787, bug_5788: Holds rewrite (76.99 KB, patch)
2013-02-25 03:49 UTC, Srdjan Jankovic
Details | Diff | Splinter Review
bug_5786, bug_5787: Holds rewrite (69.47 KB, patch)
2014-12-03 02:59 UTC, Srdjan Jankovic
Details | Diff | Splinter Review
[PASSED QA] Bug 5786 - Move AllowOnShelfHolds system preference to the Circulation Matrix (69.51 KB, patch)
2014-12-19 13:08 UTC, Kyle M Hall
Details | Diff | Splinter Review
[PASSED QA] Bug 5786 [QA Followup] (7.74 KB, patch)
2014-12-19 13:08 UTC, Kyle M Hall
Details | Diff | Splinter Review
[PASSED QA] Bug 5786 [QA Followup] (7.56 KB, patch)
2014-12-19 13:15 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 5786 - Move AllowOnShelfHolds system preference to the Circulation Matrix (69.76 KB, patch)
2015-01-26 17:23 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 5786 [QA Followup] (7.61 KB, patch)
2015-01-26 17:23 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 5786 - Move AllowOnShelfHolds system preference to the Circulation Matrix (69.76 KB, patch)
2015-01-26 17:43 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 5786 [QA Followup] (7.61 KB, patch)
2015-01-26 17:43 UTC, Martin Renvoize
Details | Diff | Splinter Review
Screenshot of the opac hold page with itemtype set to 'Forced' (37.88 KB, image/png)
2015-01-27 06:50 UTC, Katrin Fischer
Details
Bug 5786 - Move AllowOnShelfHolds and OPACItemHolds system prefs to the Circulation Matrix (69.34 KB, patch)
2015-01-28 02:43 UTC, Srdjan Jankovic
Details | Diff | Splinter Review
Bug 5786 [QA Followup] (7.60 KB, patch)
2015-01-28 02:44 UTC, Srdjan Jankovic
Details | Diff | Splinter Review
Bug 5786 - Move AllowOnShelfHolds and OPACItemHolds system prefs to the Circulation Matrix (69.51 KB, patch)
2015-01-29 03:13 UTC, Srdjan Jankovic
Details | Diff | Splinter Review
Bug 5786 [QA Followup] (7.60 KB, patch)
2015-01-29 03:14 UTC, Srdjan Jankovic
Details | Diff | Splinter Review
Bug 5786 - Move AllowOnShelfHolds and OPACItemHolds system prefs to the Circulation Matrix (70.72 KB, patch)
2015-01-29 03:41 UTC, Srdjan Jankovic
Details | Diff | Splinter Review
Bug 5786 [QA Followup] (7.60 KB, patch)
2015-01-29 03:41 UTC, Srdjan Jankovic
Details | Diff | Splinter Review
Bug 5786 - Move AllowOnShelfHolds and OPACItemHolds system prefs to the Circulation Matrix (70.62 KB, patch)
2015-03-03 10:39 UTC, Katrin Fischer
Details | Diff | Splinter Review
Bug 5786 [QA Followup] (7.60 KB, patch)
2015-03-03 10:40 UTC, Katrin Fischer
Details | Diff | Splinter Review
Bug 5786 [QA Followup] (7.68 KB, patch)
2015-03-03 14:02 UTC, Benjamin Rokseth
Details | Diff | Splinter Review
[PASSED-QA] Bug 5786 - Move AllowOnShelfHolds and OPACItemHolds system prefs to the Circulation Matrix (70.66 KB, patch)
2015-03-05 16:00 UTC, Kyle M Hall
Details | Diff | Splinter Review
[PASSED-QA] Bug 5786 [QA Followup] (7.69 KB, patch)
2015-03-05 16:01 UTC, Kyle M Hall
Details | Diff | Splinter Review
[PASSED-QA] Bug 5786 [QA Followup] - Fix language (1.99 KB, patch)
2015-03-05 16:01 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 5786: Correction: Moved holdability check from opac-search.pl to searchResults() (8.65 KB, patch)
2015-03-26 06:36 UTC, Srdjan Jankovic
Details | Diff | Splinter Review
[Signed-off] Bug 5786: Correction: Moved holdability check from opac-search.pl to searchResults() (8.86 KB, patch)
2015-03-26 10:20 UTC, Marc Véron
Details | Diff | Splinter Review
Bug 5786: Correction: Removed erroneous holdability check from opac-search.pl (3.65 KB, patch)
2015-03-27 00:21 UTC, Srdjan Jankovic
Details | Diff | Splinter Review
[SIGNED-OFF] Bug 5786: Correction: Removed erroneous holdability check from opac-search.pl (3.71 KB, patch)
2015-03-30 13:42 UTC, Kyle M Hall
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Chris Cormack 2011-02-21 19:55:09 UTC
This allows this to be restricted by itemtype or patrontype, thus providing a lot more flexibility
Comment 1 Srdjan Jankovic 2011-07-22 03:25:20 UTC Comment hidden (obsolete)
Comment 2 Nicole C. Engard 2011-07-22 16:54:42 UTC
The patch applies, but when I click 'add' on the circ matrix it does not add rules anymore - it just clears out the values I entered and nothing is added (or at least nothing displays).
Comment 3 Nicole C. Engard 2011-07-22 17:02:14 UTC
Oops - I applied it wrong :)  Will try again.
Comment 4 Nicole C. Engard 2011-07-22 17:08:31 UTC
The patch applies and if you add a new rule it works as expected, but to edit rules you need to be able to re-create an existing rule and right now I can't do that. there is no way to edit a rule anymore.  

It does look like Koha is following the rules set - so the only thing that needs fixing is the ability to edit rules - either the old way or a new way.

Nicole
Comment 5 Srdjan Jankovic 2011-07-27 06:00:21 UTC Comment hidden (obsolete)
Comment 6 Srdjan Jankovic 2011-07-27 06:19:11 UTC
Note: this has a database patch.
Be sure to edit the updatedatabase.pl version numbers to make it apply, or alternatively run
 ALTER TABLE issuingrules ADD onshelfholds BOOLEAN;
 UPDATE issuingrules SET onshelfholds=1;
against the database
Comment 7 Nicole C. Engard 2011-07-27 12:17:08 UTC Comment hidden (obsolete)
Comment 8 Paul Poulain 2011-08-04 16:52:35 UTC
This patch duplicates with 5872, that does more than moving only AllowOnShelfHolds at issuingrules level. 5872 also moves hold qty to issuingrules, as well as "enable on shelf hold", "set duration to pick up waiting hold" and "hold quantity"

If 5786 is added into master, 5872 will be discarded definetly : they don't share the same DB column names, and many other incompatibilities problems.

I won't speak again of the history of 5872 (came from 5575, that was a split of too large code submission that was made by us during then pains we faced 1.5 years ago), and don't ask for a specific treatment (even if we spent months on this. Really !) I just warn about the consequences or applying 5786.

My suggestion would be to give 5872 a chance before applying this one. And prefer the 5872 to this one if tests are successful (and let me a chance to fix quickly any problem reported on it)
Comment 9 Paul Poulain 2011-10-06 12:17:35 UTC
Sorry for being long to QA.

In the meantime, the way sysprefs are handled has changed. You just need to add the syspref in $KOHA/installer/data/mysql/sysprefs.sql

Could you update/rebase & resubmit please ?
Comment 10 Srdjan Jankovic 2011-10-07 05:09:11 UTC
This needs to be tested together with 5787 and 5788. I have made a merged patch and left it on git+ssh://git.catalyst.net.nz/git/public/koha.git, branch wr76711_holds_rewrite. I am not sure though who has access there.
Comment 11 Paul Poulain 2011-10-07 09:38:19 UTC
back to needs signoff then ?

I also have added 87 and 5788 as dependancies
Comment 12 Paul Poulain 2011-10-24 11:38:02 UTC
Updating Version : This ENH will be for Koha 3.8
Comment 13 Paul Poulain 2011-10-25 15:05:38 UTC
Bug versionned for master. entries will be made against rel_3_8 once the patch has been applied (see thread about that on koha-devel yesterday)
Comment 14 Katrin Fischer 2011-12-20 20:44:27 UTC
Hi Srdjan, 
can this be tested without applying changes for 5787 and 5788? Or do the other 2 only build on things from this patch so should be tested after this was applied?
I think it will be hard testing all 3 together and a step by step introduction into Koha would be a bit easier.

A lot of things have changes since this patch was added, so I think it needs some work now :(

Problems found:
1) We have a new system for adding new system preferences, only 1 central file has to be changed now. 

2) The updatedatabase adds a new column to issuingrules, but there is no corresponding change to kohastructure.sql
Comment 15 Srdjan Jankovic 2011-12-21 03:35:49 UTC Comment hidden (obsolete)
Comment 16 Katrin Fischer 2011-12-22 16:47:50 UTC
Hi Srdjan, 
I am not sure I understand your explanation - so I will ask more questions :)
Is there a sequence in which your features can be tested without causing conflicts? And can we start on this bug and if we can, what would be the next to look at? Or where should we start for the circ enhancements? Thx!
Comment 17 Srdjan Jankovic 2011-12-22 23:26:19 UTC
You can start with any of the three, just pick one. What I was trying to say is:
* pick one of the three and we get it signed of and pushed
* pick another one, but not before the first one is pushed; it will invariably have conflicts and patch will not apply
* I will rebase it and we get that one signed of and pushed
* repeat for the last one

This is because if you sign of all three there will be serious conflicts, so would need to be retested. This way we are minimising testing effort.
Comment 18 Katrin Fischer 2011-12-23 06:02:49 UTC
Thx for the explanation. I think I will start here then :)
Comment 19 Katrin Fischer 2012-01-02 06:47:38 UTC
Starting tests.
Comment 20 Katrin Fischer 2012-01-02 06:53:59 UTC
Hi Srdjan, 
can you explain why you moved branchcode from first to lastcolumn in kohastructure? This seems not right to me and the database update does not reflect this (unintended?) change.
Comment 21 Katrin Fischer 2012-01-02 07:24:50 UTC
The database update gives no feedback at all. 
Please add some 'print' with feedback about the updates done.
Comment 22 Srdjan Jankovic 2012-01-02 23:24:14 UTC Comment hidden (obsolete)
Comment 23 Katrin Fischer 2012-01-03 07:30:04 UTC
Hate to say so, but looking at your patch it's still moving branchcode in kohastructure.
Comment 24 Katrin Fischer 2012-01-03 07:32:45 UTC
The database update worked nicely. I will continue testing and you could fix kohastructure with a follow up patch?
Comment 25 Srdjan Jankovic 2012-01-03 12:09:26 UTC
branchcode is part of the primary key and it belongs to the top. I'm not sure what the policy is for field moving (application should not care about the field position). I can put it back if moving it is a problem.
Comment 26 Katrin Fischer 2012-01-03 12:30:26 UTC
I don't like the idea much of having a difference in the database structure between older and newer installations of Koha. I am not sure there are other/better reasons for doing or not doing it. Perhaps someone else can comment on this.
Comment 27 Srdjan Jankovic 2012-01-03 23:54:58 UTC Comment hidden (obsolete)
Comment 28 Katrin Fischer 2012-01-04 07:15:15 UTC
Hi srdjan, thx for your new patch. The database updates and changes are all ok now. 
I found another problem though when I was trying to configure my installaton for testing:
Your patch removes the OPACItemHolds system preference from  .../prog/en/modules/admin/preferences/opac.pref.
Comment 29 Srdjan Jankovic 2012-01-05 01:48:41 UTC Comment hidden (obsolete)
Comment 30 Srdjan Jankovic 2012-01-05 01:49:30 UTC
My apologies, that one should go to 5787.
Comment 31 Katrin Fischer 2012-01-09 07:24:50 UTC
Hi Srdjan, 

I continued testing today and the first results are all ok.

But I have a general question to make sure I understand how this is supposed to work:
Shelf holds are NOT allowed
Record has 2 items, 1 is available, 1 is checked out
Should it be possible to place a biblio level hold in that case?
Comment 32 Katrin Fischer 2012-01-09 07:30:33 UTC
It seems that is the way it works on master too - so it should be all ok.

So 1 item checked out is enough to make holds possible.
Comment 33 Christophe Croullebois 2012-03-02 13:00:15 UTC
Hi,
sorry Srdjan it don't apply, some conflicts...
I have missed something maybe.
In the thread, you spoke about three patches, I only tried to apply  the last one here (patch (29.94 KB, patch)2012-01-05 01:48 UTC, Srdjan Jankovic ).
Could you tell me which one I can test ?
Thx in advance.
Comment 34 Katrin Fischer 2012-03-02 13:03:32 UTC
Hi, I had no time to write up the tests I already did and attach them on this bug :( What I tested so far is the database update and functionality for biblio level itypes. Item level itypes would have been the next thing I was planning to look at (talking about the system preference and testing biblio level and item level holds for both). I think the last one is the only you need to test this functionality. Thre are more patched building on top of that and moving more things to the circulation matrix (afaik) so perhaps Srdjan was talking about them. (5785, 5788)
Comment 35 Christophe Croullebois 2012-03-02 14:29:14 UTC
Sry Srdjan,
I have these conflicts:
CONFLICT (content): Merge conflict in installer/data/mysql/updatedatabase.pl
CONFLICT (content): Merge conflict in koha-tmpl/opac-tmpl/prog/en/modules/opac-ISBDdetail.tt
CONFLICT (content): Merge conflict in koha-tmpl/opac-tmpl/prog/en/modules/opac-MARCdetail.tt
CONFLICT (content): Merge conflict in koha-tmpl/opac-tmpl/prog/en/modules/opac-detail.tt
CONFLICT (content): Merge conflict in opac/opac-ISBDdetail.pl
CONFLICT (content): Merge conflict in opac/opac-MARCdetail.pl
Could you check, or explain me what I made wrong ?
thx
Comment 36 Srdjan Jankovic 2012-03-05 03:44:49 UTC
This patch is being qa-ed by Bywater in conjunction with 5787 and 5788, so I suggest we wait and see what comes out of that.
Comment 37 Christophe Croullebois 2012-03-05 08:23:31 UTC
Ok, Srdjan, wait and see.
thx
Comment 38 Katrin Fischer 2012-07-05 14:15:37 UTC
*** Bug 8372 has been marked as a duplicate of this bug. ***
Comment 39 Melia Meggs 2012-07-24 23:25:05 UTC
A new patch coming soon for this!!  I want to provide test plans to facilitate community testing.


Test plan for On Shelf Holds only


Set up the following rules:

Rule 1:
Patron Category 1
Item type 1
On Shelf Holds Not Allowed

Rule 2:
Patron Category 2
Item type 2
On Shelf Holds Allowed


In both the staff client and the OPAC try the following for each rule combo:

Place hold on shelf item
Place hold on checked out item

--------------------

Please test with Bug 5787 as well.


Test Plan for both On Shelf Holds and OPAC Item Holds together


Set up the following rules:

Rule 1:
Patron Category 1
Item type 1
On Shelf Holds Not Allowed
Allow patrons to place item specific holds

Rule 2:
Patron Category 2
Item type 2
On Shelf Holds Allowed
Allow patrons to place item specific holds

Rule 3:
Patron Category 3
Item type 3
On Shelf Holds Allowed
Don't allow patrons to place item specific holds

Rule 4:
Patron Category 4
Item type 4
On Shelf Holds not Allowed
Don't allow patrons to place item specific holds


In both the staff client and the OPAC try the following for each rule combo:

Place hold on shelf item
Place hold on checked out item
Place an item specific hold
Comment 40 Srdjan Jankovic 2012-07-25 03:44:34 UTC Comment hidden (obsolete)
Comment 41 Srdjan Jankovic 2012-07-25 03:54:45 UTC
Integral patch for 5786, 5787, 5788
Comment 42 Nicole C. Engard 2012-07-25 18:50:11 UTC Comment hidden (obsolete)
Comment 43 Chris Cormack 2012-07-26 18:54:19 UTC
Is there a reason this was signed off, then marked does not apply?
Comment 44 Nicole C. Engard 2012-07-27 13:00:47 UTC
Because between the time I signed off and Kyle looked at it new patches were pushed that made this patch not apply anymore.
Comment 45 Srdjan Jankovic 2012-07-30 00:08:22 UTC Comment hidden (obsolete)
Comment 46 Srdjan Jankovic 2012-07-30 00:14:22 UTC Comment hidden (obsolete)
Comment 47 Kyle M Hall 2012-07-30 12:13:41 UTC Comment hidden (obsolete)
Comment 48 Julian Maurice 2012-08-06 14:27:59 UTC
*** Bug 5787 has been marked as a duplicate of this bug. ***
Comment 49 Julian Maurice 2012-08-06 14:28:30 UTC
*** Bug 5788 has been marked as a duplicate of this bug. ***
Comment 50 Marcel de Rooy 2012-08-30 09:55:53 UTC
Some preliminary *teasing* qa remarks:
Patch does not apply anymore.
It seems to contain a lot of whitespace errors.
And I saw some tabs too :)
Comment 51 Srdjan Jankovic 2012-08-31 06:40:48 UTC Comment hidden (obsolete)
Comment 52 Srdjan Jankovic 2012-08-31 06:42:57 UTC
I've created another patch because the first one does not apply (it was a minor updatedatabase conflict).
I have removed tabs and whitespaces.
If you want it handled differently please let me know.
Comment 53 Kyle M Hall 2012-09-05 18:29:33 UTC
pplying: bug_5786, bug_5787, bug_5788: Holds rewrite
error: patch failed: C4/Auth.pm:412
error: C4/Auth.pm: patch does not apply
Comment 54 Kyle M Hall 2012-09-05 18:30:44 UTC
My bad, the old patch was not deprecated.

(In reply to comment #53)
> pplying: bug_5786, bug_5787, bug_5788: Holds rewrite
> error: patch failed: C4/Auth.pm:412
> error: C4/Auth.pm: patch does not apply
Comment 55 Kyle M Hall 2012-09-05 20:08:39 UTC Comment hidden (obsolete)
Comment 56 Mason James 2012-09-16 12:46:01 UTC
> Signed-off-by: Nicole C. Engard <nengard@bywatersolutions.com>
> Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>

patch looks good, passing QA - well done everyone  :)

$ koha-qa.pl 

testing 1 commit(s) (applied to commit bf79b88)
 * d24fd5c bug_5786, bug_5787, bug_5788: Holds rewrite
      C4/Auth.pm
      C4/ILSDI/Services.pm
      C4/Items.pm
      C4/Reserves.pm
      C4/VirtualShelves/Page.pm
      admin/smart-rules.pl
      admin/systempreferences.pl
      installer/data/mysql/it-IT/necessari/system_preferences.sql
      installer/data/mysql/kohastructure.sql
      installer/data/mysql/sysprefs.sql
      installer/data/mysql/updatedatabase.pl
      installer/html-template-to-template-toolkit.pl
      koha-tmpl/intranet-tmpl/prog/en/modules/admin/preferences/circulation.pref
      koha-tmpl/intranet-tmpl/prog/en/modules/admin/preferences/opac.pref
      koha-tmpl/intranet-tmpl/prog/en/modules/admin/smart-rules.tt
      koha-tmpl/opac-tmpl/prog/en/includes/opac-detail-sidebar.inc
      koha-tmpl/opac-tmpl/prog/en/modules/opac-reserve.tt
      koha-tmpl/opac-tmpl/prog/en/modules/opac-results-grouped.tt
      koha-tmpl/opac-tmpl/prog/en/modules/opac-results.tt
      koha-tmpl/opac-tmpl/prog/en/modules/opac-shelves.tt
      opac/opac-ISBDdetail.pl
      opac/opac-MARCdetail.pl
      opac/opac-detail.pl
      opac/opac-reserve.pl
      opac/opac-search.pl
      reserve/request.pl

* C4/Auth.pm                                                               OK
* C4/ILSDI/Services.pm                                                     OK
* C4/Items.pm                                                              OK
* C4/Reserves.pm                                                           OK
* C4/VirtualShelves/Page.pm                                                OK
* admin/smart-rules.pl                                                     OK
* admin/systempreferences.pl                                               OK
* installer/data/mysql/updatedatabase.pl                                   OK
* installer/html-template-to-template-toolkit.pl                           OK
* opac/opac-ISBDdetail.pl                                                  OK
* opac/opac-MARCdetail.pl                                                  OK
* opac/opac-detail.pl                                                      OK
* opac/opac-reserve.pl                                                     OK
* opac/opac-search.pl                                                      OK
* reserve/request.pl                                                       OK
* koha-tmpl/intranet-tmpl/prog/en/modules/admin/smart-rules.tt             OK
* koha-tmpl/opac-tmpl/prog/en/includes/opac-detail-sidebar.inc             OK
* koha-tmpl/opac-tmpl/prog/en/modules/opac-reserve.tt                      OK
* koha-tmpl/opac-tmpl/prog/en/modules/opac-results-grouped.tt              OK
* koha-tmpl/opac-tmpl/prog/en/modules/opac-results.tt                      OK
* koha-tmpl/opac-tmpl/prog/en/modules/opac-shelves.tt                      OK
Comment 57 Paul Poulain 2012-09-18 20:25:03 UTC
Sorry srdjan, but the patch does not apply anymore, there's a conflict on
CONFLICT (content): Merge conflict in koha-tmpl/opac-tmpl/prog/en/modules/opac-reserve.tt
that is not easy to solve.

Reminder = the Feature Freeze for 3.10 is in a few days.
Comment 58 Katrin Fischer 2012-09-18 20:59:39 UTC
I would love to see this go in, as it will enable a better configuration for libraries with closed stacks. Hoping it will make the deadline.
Comment 59 Srdjan Jankovic 2012-09-19 00:22:47 UTC Comment hidden (obsolete)
Comment 60 Srdjan Jankovic 2012-09-19 00:24:36 UTC
I'm making it Signed Off. Functionality should not be changed, there's a small possibility of HTML issues
Comment 61 Paul Poulain 2012-09-21 16:10:51 UTC
QA comment:

* I've found some glitches with koha-qa.pl, i've fixed them. They were very minor.
* After an intensive testing, I've found a tricky problem, that occurs only under some specific cases. Let me explain:
 Issuing rules are : default library (branchcode='*'), you say 'ShelfHolds = no'
 for a given library, a given itemtype, and any categorycode, you say 'yes' => only 'BOOKS' from 'Library A' can be reserved while on shelf.

In this case, you will get a wrong result from your query:
SELECT opacitemholds,categorycode,itemtype,branchcode FROM issuingrules WHERE
          (issuingrules.categorycode = ? OR issuingrules.categorycode = '*')
        AND
          (issuingrules.itemtype = ? OR issuingrules.itemtype = '*')
        AND
          (issuingrules.branchcode = ? OR issuingrules.branchcode = '*')
        ORDER BY
          issuingrules.categorycode desc,
          issuingrules.itemtype desc,
          issuingrules.branchcode desc

It returns (I don't know why !) the branch='*' rule.

This feature existed previously in biblibre/master, and got lost with some other features during 3.2/3.4
We made it a different way, here is the piece of code:


    # This configuration table defines the order of inheritance. We'll loop over it.
    my @attempts = (
        [ "*",           "*",       "*" ],
        [ "*",           $itemtype, "*" ],
        [ $categorycode, "*",       "*" ],
        [ $categorycode, $itemtype, "*" ],
        [ "*",           "*",       $branchcode ],
        [ "*",           $itemtype, $branchcode ],
        [ $categorycode, "*",       $branchcode ],
        [ $categorycode, $itemtype, $branchcode ],
    );

    # This complex query returns a nested hashref, so we can access a rule using :
    # my $rule = $$rules{$categorycode}{$itemtype}{$branchcode};
    # this will be usefull in the inheritance computation code
    my $rules = $dbh->selectall_hashref(
        "SELECT * FROM issuingrules where branchcode IN ('*',?) and itemtype IN ('*', ?) and categorycode IN ('*',?)",
        [ "branchcode", "itemtype", "categorycode" ],
        undef, ( $branchcode, $itemtype, $categorycode )
    );

    # This block is for inheritance. It loops over rules returned by the
    # previous query. If a value is found in a more specific rule, it replaces
    # the old value from the more generic rule.
    my $oldrule;
    for my $attempt (@attempts) {
        if ( my $rule = $$rules{ @$attempt[2] }{ @$attempt[1] }{ @$attempt[0] } ) {
            if ($oldrule) {
                for ( keys %$oldrule ) {
                    if ( defined $rule->{$_} ) {
                        $oldrule->{$_} = $rule->{$_};
                    }
                }
            } else {
                $oldrule = $rule;
            }
        }
    }

=> we retrieve an array of all applicable rules, then we check each of them. the last one we find is the more precise and should be used ! (it's in "$oldrule", that is not a very good name I agree. It's the "appliedrule" in fact.

Marking failed QA, will attach my follow-ups
Comment 62 Paul Poulain 2012-09-21 16:11:32 UTC Comment hidden (obsolete)
Comment 63 Paul Poulain 2012-09-21 16:11:45 UTC Comment hidden (obsolete)
Comment 64 Srdjan Jankovic 2012-09-24 00:34:14 UTC
Hi Paul,
Want me to do anything?
Comment 65 Katrin Fischer 2012-09-24 05:50:42 UTC
Hi Srdjan, it looks like Paul found a real problem with this - can you confirm and maybe fix it?
Comment 66 Paul Poulain 2012-09-24 19:45:53 UTC
(In reply to comment #65)
> Hi Srdjan, it looks like Paul found a real problem with this - can you
> confirm and maybe fix it?

agreed ;-)
Comment 67 Srdjan Jankovic 2012-09-25 00:53:07 UTC
That did not happen to me:

Server version: 5.5.24-0ubuntu0.12.04.1 (Ubuntu)
mysql> SELECT opacitemholds,categorycode,itemtype,branchcode FROM issuingrules WHERE
    ->           (issuingrules.categorycode = 'Adult' OR issuingrules.categorycode = '*')
    ->         AND
    ->           (issuingrules.itemtype = 'AGN' OR issuingrules.itemtype = '*')
    ->         AND
    ->           (issuingrules.branchcode = 'N' OR issuingrules.branchcode = '*')
    ->         ORDER BY
    ->           issuingrules.categorycode desc,
    ->           issuingrules.itemtype desc,
    ->           issuingrules.branchcode desc;
+---------------+--------------+----------+------------+
| opacitemholds | categorycode | itemtype | branchcode |
+---------------+--------------+----------+------------+
|             1 | *            | AGN      | N          |
|             1 | *            | *        | *          |
+---------------+--------------+----------+------------+
2 rows in set (0.00 sec)

On a general note, we should have a centralised rule picker. I remember writing one some years ago, but it seems to have been lost.
On an even more general note, issuingrules table is just so wrong.
Comment 68 Katrin Fischer 2012-12-13 10:26:45 UTC
Paul, could you take a look at Srdjan's comment please and try to find out why the 2 of you get different results while testing?
Comment 69 Paul Poulain 2012-12-17 17:08:54 UTC
(In reply to comment #68)
> Paul, could you take a look at Srdjan's comment please and try to find out
> why the 2 of you get different results while testing?

Katrin, thx for reviving this patch ! I switch back to signed off because I won't be able to find time for intensive testing in a close future. If someone else want to give a try.
Comment 70 Melia Meggs 2013-01-25 01:01:47 UTC
Hi, just checking on this.  Is someone from the QA team available to QA this  signed off patch?  It has been struggling through the testing process for a good long time now...

Thanks,

Melia
Comment 71 Marcel de Rooy 2013-01-25 08:12:49 UTC
(In reply to comment #70)
> Hi, just checking on this.  Is someone from the QA team available to QA this
> signed off patch?  It has been struggling through the testing process for a
> good long time now...
With Dec 17, it was reaching the top 10 of the queue, but this comment makes it go down again :) The QA team tries to follow the Changed date column in Bugzilla. Unfortunately, every comment impacts that date; it would have been nicer if it was the date of the patch itself.
Will try to remember this bug number to pick it earlier..
Comment 72 Marcel de Rooy 2013-02-22 09:30:38 UTC
Preliminary QA Comment:
I appreciate that already a huge amount of time went in this report for development and .. testing.
With a huge patch like this and a queue of 140 signed off patches, it is very likely to happen that your patch will not apply as unfortunately is the case right now..
Smaller patches under several reports will have a better chance.
I saw a test plan from Melia. That is very good, but does it really cover all changes? Including 341 lines in Reserves module?
Just saying this, because I realize that rebasing this is a nightmare. But testing and QAing either :)

I also note that we have several developments in the queue in the same area. This morning I looked also at 8367 (maxpickupdelay).
It appears that your development is older. Some communication there could prevent further duplication of work? 

Lots of whitespace errors. Could you cleanup when needing to rebase?
CONFLICT (content): Merge conflict in opac/opac-reserve.pl
CONFLICT (content): Merge conflict in koha-tmpl/opac-tmpl/prog/en/modules/opac-reserve.tt
Funny conflict there:
                        [% ELSIF ( bibitemloo.bib_available ) %]
                            No available items.
                        [% ELSE %]
<<<<<<< HEAD
                          [% UNLESS ( bibitemloo.bib_available ) %]
                            <div class="bibmessage">No available items.</div>
CONFLICT (content): Merge conflict in installer/data/mysql/updatedatabase.pl

Looking at your updatedatabase changes, I wonder why you do not remove the pref for maxpickupdelay when adding it to issuing rules? 
Like you do for shelf holds and item holds. This could create confusion..

Setting to Does not apply
Comment 73 Srdjan Jankovic 2013-02-25 03:49:22 UTC Comment hidden (obsolete)
Comment 74 Srdjan Jankovic 2013-02-25 04:08:34 UTC
We tried individual bugs first (after all, that's how we started), but it proved to be merging/testing nightmare, too interwined).
I don't think the problem is

I have submitted the full patch (again) because of the conflicts, and did not know how to create just a patch with conflict resolution (it was a full merge, hence huge). If you want me to do any other kind of patch that will make sign-off easier, please say so.

Re whitespace errors, I could not find any.

ReservesMaxPickUpDelay is bit of a nightmare. I'm not sure what exactly happened there, but seems to be reintroduced or something with 7408. What shall we do with that one?
Comment 75 Katrin Fischer 2013-03-04 10:49:56 UTC
*** Bug 5434 has been marked as a duplicate of this bug. ***
Comment 76 Marcel de Rooy 2013-03-21 08:19:59 UTC
(In reply to comment #74)
> ReservesMaxPickUpDelay is bit of a nightmare. I'm not sure what exactly
> happened there, but seems to be reintroduced or something with 7408. What
> shall we do with that one?

Hard to say. The solution in bug 8367 for MaxPickupDelay seems interesting. In the meantime a lot of patches have been pushed (especially this week), leaving this patch at Does not apply..
Comment 77 Srdjan Jankovic 2013-03-22 02:27:33 UTC
So shall we just remove the ReservesMaxPickUpDelay bit?
Comment 78 Nicole C. Engard 2013-04-02 22:00:17 UTC
*** Bug 9974 has been marked as a duplicate of this bug. ***
Comment 79 Srdjan Jankovic 2014-12-03 02:59:56 UTC Comment hidden (obsolete)
Comment 80 Srdjan Jankovic 2014-12-03 03:03:45 UTC
(In reply to M. de Rooy from comment #76)

> Hard to say. The solution in bug 8367 for MaxPickupDelay seems interesting.
> In the meantime a lot of patches have been pushed (especially this week),
> leaving this patch at Does not apply..

Removed maxpickupdelay issuingrule, it was a rule only without any implementation
Comment 81 Kyle M Hall 2014-12-19 13:08:16 UTC Comment hidden (obsolete)
Comment 82 Kyle M Hall 2014-12-19 13:08:27 UTC Comment hidden (obsolete)
Comment 83 Kyle M Hall 2014-12-19 13:15:03 UTC Comment hidden (obsolete)
Comment 84 Katrin Fischer 2014-12-19 13:26:34 UTC
I am really keen on seeing this in Koha and I know this has been stuck for quite a while, but for the possible implications of it and because it's rather huge, I'd like to get a second sign-off and/or QA opinion.

Not sure which status to set - to avoid confusion I will set "Signed off".
Comment 85 Kyle M Hall 2014-12-19 13:37:36 UTC
I agree, that sounds like a good idea. I think a one more signoff should be sufficient. For that reason I'll set the status to needs signoff. That way everyone knows it's open for testing.

(In reply to Katrin Fischer from comment #84)
> I am really keen on seeing this in Koha and I know this has been stuck for
> quite a while, but for the possible implications of it and because it's
> rather huge, I'd like to get a second sign-off and/or QA opinion.
> 
> Not sure which status to set - to avoid confusion I will set "Signed off".
Comment 86 Martin Renvoize 2015-01-07 08:20:35 UTC
Comment on attachment 34534 [details] [review]
[PASSED QA] Bug 5786 - Move AllowOnShelfHolds system preference to the Circulation Matrix

Review of attachment 34534 [details] [review]:
-----------------------------------------------------------------

::: C4/Items.pm
@@ +174,4 @@
>          ($data->{'serialseq'} , $data->{'publisheddate'}) = $ssth->fetchrow_array();
>      }
>  	#if we don't have an items.itype, use biblioitems.itemtype.
> +    # FIXME this should respect the itypes systempreference

Are we OK with this new FIXME being added?
Comment 87 Martin Renvoize 2015-01-07 08:21:43 UTC
Generally I'm happy with this patch.. though a comment on the above FIXME would hasten my signing off.
Comment 88 Martin Renvoize 2015-01-26 17:23:52 UTC Comment hidden (obsolete)
Comment 89 Martin Renvoize 2015-01-26 17:23:58 UTC Comment hidden (obsolete)
Comment 90 Martin Renvoize 2015-01-26 17:29:19 UTC
Rebased patches after Auth.pm tidy in bug 13499
Comment 91 Martin Renvoize 2015-01-26 17:33:56 UTC
OK.. that's another signoff..  happily tested and can't find any regressions this end.

I'll pop it through the qa script and run the tests again then perhaps add my qa signature too.
Comment 92 Martin Renvoize 2015-01-26 17:43:13 UTC Comment hidden (obsolete)
Comment 93 Martin Renvoize 2015-01-26 17:43:17 UTC Comment hidden (obsolete)
Comment 94 Martin Renvoize 2015-01-26 17:43:58 UTC
Passing QA on this.. would be lovely to see it in.
Comment 95 Katrin Fischer 2015-01-26 23:38:18 UTC
I got confused about the number of sign-offs and QAs, so I thought I'd add another round of testing/QA. 
Overall this works pretty well (yay!), but I found some problems:

CODE REVIEW

1) Change the bug description to better reflect what the patch does:
  It's not only moving AllowOnShelfHolds, but also OpacItemHolds into the
  circulation matrix (DONE)
  Maybe also change the commit message? (trivial)

2) Why is this line removed from updatedatabaes.pl?
$dbh->do("UPDATE `systempreferences` SET type='Integer' WHERE variable='ReservesMaxPickupDelay'"); (?)

3) Why set the issuingrules to 1, after finding out the original setting first?
+    $dbh->do("UPDATE issuingrules SET opacitemholds=1");
Shouldn't it update to $opacitemholds with Y, N or F? (blocker)

4) Add bug number to database update. (trivial)


TESTING

Issuingrules

5) Automatic renewal is a yes/no pull down, on shelf holds is a checkbox. I think to be more consistent we should use one or the other. (?)

6) If you checked the checkbox on saving and open the rule for editing, the checkbox is not checked, but it should be. (blocker)

7) I feel like the description and options of the new opacitemholds is hard to interpret, if you don't know about the former behaviour. But not sure how to rename. I feel like item-level holds might be a little more understandable, but not sure. (trivial)


Placing holds (not sure that's understandable to anyone but me...)

8) All - Books: 10 days, reservesallowed 99, onshelfholds = yes
  Record: 4 items, all Books and available, one being notforloan = 'on order'
  There is a positive all-all-all rule.
  Maxreserves is > 0
  item-level_itypes is set to specific item
- opacitemholds = Y = OK, both options are available
- opacitemholds = N = OK, only title level hold available
! opacitemholds = F = NOT OK? display is confusing, as it still shows 
  "Next available item A specific item" but the first without the checkbox
  Tested in 3.18.2 - there "Next available item" is not shown in this case

Summary: All items one itype, forced item level holds - display still offers "Next available" but no checkbox
         I feel like the combination of one itype F and another set to Y (allow bib level) is problematic.
         We need to decide what to do here - allow bib level (activate the checkbox) or remove the mention of it
         from the templates altogether. I tend to do the first. (normal)

9) Working with last setting from above: opacitemholds = F
- onshelfholds = no = OK, no hold allowed
- onshelfholds = no, but with one item checked out now = OK, item level hold on checked out item allowed
- opacitemholds = N = OK, record level hold allowed
- onshelfholds = no, opacitemholds = Y = OK, item level hold on checked out item allowed, record level hold allowed

Summary: Looks ok
  
10) All - Computer files: 10 days, reservesallowed 99, onshelfholds no, opacitemholds no
   All - Books (BK): 10 days, reservesallowed 99, onshelfholds yes, opacitemholds yes
   Adding another item = Computer files (CF)
- OK
   record level hold is allowed
   item level hold is only allowed for BK items 
- OK CF: onshelfholds = yes
   same
- OK CF: opacitemholds = yes
   item level hols is now allowed for CF items also

Summary: Looks ok

11) All - Computer files (CF): 10 days, reservesallowed 99...
   All - Books (BK): 10 days, reservesallowed 99...
   Another checked out CF item is added. 4 BK, 2 CF now, one of each itype checked out
- OK
  BK: onshelfholds = no, opacitemholds = Y
  CF: onshelfholds = no, opacitemholds = Y
  record level hold is allowed.
  only item level holds on checked out items are allowed.

ILS-DI

OK Didn't explode and seemed ok for the few cases tested.
Comment 96 Srdjan Jankovic 2015-01-27 03:06:54 UTC
(In reply to Katrin Fischer from comment #95)

> CODE REVIEW
> 
> 2) Why is this line removed from updatedatabaes.pl?
> $dbh->do("UPDATE `systempreferences` SET type='Integer' WHERE
> variable='ReservesMaxPickupDelay'"); (?)

I don't remember clearly. This is a combination of bugs that could not be done separately, they are too interdependent. This was part of 5788. But it seems that ReservesMaxPickupDelay was never implemented. I am confused. So maybe put it back, although it seems to be a noop?

> 
> 3) Why set the issuingrules to 1, after finding out the original setting
> first?
> +    $dbh->do("UPDATE issuingrules SET opacitemholds=1");
> Shouldn't it update to $opacitemholds with Y, N or F? (blocker)

That's a bug, it should be:
+    $dbh->do("UPDATE issuingrules SET opacitemholds='$opacitemholds'");

> 
> 4) Add bug number to database update. (trivial)

I have no problems fixing that, however this patch has changed and when I do git bz apply I get:
fatal: sha1 information is lacking or useless (C4/Auth.pm).

Any hints?


> 
> 
> TESTING
> 
> Issuingrules
> 
> 5) Automatic renewal is a yes/no pull down, on shelf holds is a checkbox. I
> think to be more consistent we should use one or the other. (?)

Ok, which one? To me tick box makes more sense for yes/no

> 
> 6) If you checked the checkbox on saving and open the rule for editing, the
> checkbox is not checked, but it should be. (blocker)

I can see there was something done in the follow up, but I cannot pick it up (git bz appply above)

> 
> 7) I feel like the description and options of the new opacitemholds is hard
> to interpret, if you don't know about the former behaviour. But not sure how
> to rename. I feel like item-level holds might be a little more
> understandable, but not sure. (trivial)

I can do that

> 
> 
> Placing holds (not sure that's understandable to anyone but me...)
> 
> 8) All - Books: 10 days, reservesallowed 99, onshelfholds = yes
>   Record: 4 items, all Books and available, one being notforloan = 'on order'
>   There is a positive all-all-all rule.
>   Maxreserves is > 0
>   item-level_itypes is set to specific item
> - opacitemholds = Y = OK, both options are available
> - opacitemholds = N = OK, only title level hold available
> ! opacitemholds = F = NOT OK? display is confusing, as it still shows 
>   "Next available item A specific item" but the first without the checkbox
>   Tested in 3.18.2 - there "Next available item" is not shown in this case
> 
> Summary: All items one itype, forced item level holds - display still offers
> "Next available" but no checkbox
>          I feel like the combination of one itype F and another set to Y
> (allow bib level) is problematic.
>          We need to decide what to do here - allow bib level (activate the
> checkbox) or remove the mention of it
>          from the templates altogether. I tend to do the first. (normal)

I'm afraid that goes way beyond my understanding. But if you tell me what you want to happen, I can make it happen :)
Comment 97 Katrin Fischer 2015-01-27 06:48:28 UTC
Hi Srdjan,

thx for your quick answer.

(In reply to Srdjan Jankovic from comment #96)
> (In reply to Katrin Fischer from comment #95)

> > 2) Why is this line removed from updatedatabaes.pl?
> > $dbh->do("UPDATE `systempreferences` SET type='Integer' WHERE
> > variable='ReservesMaxPickupDelay'"); (?)
> 
> I don't remember clearly. This is a combination of bugs that could not be
> done separately, they are too interdependent. This was part of 5788. But it
> seems that ReservesMaxPickupDelay was never implemented. I am confused. So
> maybe put it back, although it seems to be a noop?

I am not sure it's a noop. It's part of a real old database update:
$DBversion = "3.01.00.007";
We normally don't touch those, as it might give problems to someone updating from an really old version of Koha.


> > 3) Why set the issuingrules to 1, after finding out the original setting
> > first?
> > +    $dbh->do("UPDATE issuingrules SET opacitemholds=1");
> > Shouldn't it update to $opacitemholds with Y, N or F? (blocker)

> That's a bug, it should be:
> +    $dbh->do("UPDATE issuingrules SET opacitemholds='$opacitemholds'");


> > 4) Add bug number to database update. (trivial)
> 
> I have no problems fixing that, however this patch has changed and when I do
> git bz apply I get:
> fatal: sha1 information is lacking or useless (C4/Auth.pm).

The patches applied cleanly for me on last nights master: 

commit ac3f497f64c1854a275fe894c1070f5888c1c302
Bug 13235: Move onclick attr to javacsript code 

I know this one is really trivial, but as we are going to touch updatedatabase for 3) we can fix this too.


> > TESTING
> > 
> > Issuingrules
> > 
> > 5) Automatic renewal is a yes/no pull down, on shelf holds is a checkbox. I
> > think to be more consistent we should use one or the other. (?)
> 
> Ok, which one? To me tick box makes more sense for yes/no

I will check with some people today how they feel. I agree that the checkbox seems more ergonomic, but the Yes/No pull down reflects what is shown after saving the rule, which seems more consistent to me. 

> > 6) If you checked the checkbox on saving and open the rule for editing, the
> > checkbox is not checked, but it should be. (blocker)
> 
> I can see there was something done in the follow up, but I cannot pick it up
> (git bz appply above)

I think the edit functionality uses Javascript, it might be something in the .tt that needs to be fixed here. The change in the follow-up seems not related.
To test:

1) Add a rule, check the check box.
2) Verify it's saved as 'Yes'
3) Edit the same rule using the link at the end
4) Verify the checkbox is now unchecked (last line of the table)


> > 7) I feel like the description and options of the new opacitemholds is hard
> > to interpret, if you don't know about the former behaviour. But not sure how
> > to rename. I feel like item-level holds might be a little more
> > understandable, but not sure. (trivial)
> 
> I can do that

More a note than a blocker, but it would be nice if we could come up with something easier to understand. Maybe Martin or Kyle can suggest something (or they like it how it is :) ).


> > Placing holds (not sure that's understandable to anyone but me...)
> > 
> > 8) All - Books: 10 days, reservesallowed 99, onshelfholds = yes
> >   Record: 4 items, all Books and available, one being notforloan = 'on order'
> >   There is a positive all-all-all rule.
> >   Maxreserves is > 0
> >   item-level_itypes is set to specific item
> > - opacitemholds = Y = OK, both options are available
> > - opacitemholds = N = OK, only title level hold available
> > ! opacitemholds = F = NOT OK? display is confusing, as it still shows 
> >   "Next available item A specific item" but the first without the checkbox
> >   Tested in 3.18.2 - there "Next available item" is not shown in this case
> > 
> > Summary: All items one itype, forced item level holds - display still offers
> > "Next available" but no checkbox
> >          I feel like the combination of one itype F and another set to Y
> > (allow bib level) is problematic.
> >          We need to decide what to do here - allow bib level (activate the
> > checkbox) or remove the mention of it
> >          from the templates altogether. I tend to do the first. (normal)
> 
> I'm afraid that goes way beyond my understanding. But if you tell me what
> you want to happen, I can make it happen :)

I was worried that might happen - the overall message is: it works quite well :), except for one thing in the display related to the 'forced' option, that I think looks not quite right. Trying to give you a better test plan and will also attach a screenshot in the next step:

1) Create an issuingrule:
   All - itype1 - opacitemholds=Force, onshefholds=yes
2) Create a record with only a itype1 item
3) Place a hold in the OPAC
4) Notice you can't place a 'next available' hold, but the option is
   displayed without the checkbox to check it.

In this case, the 'next available' bit can be hidden. It gets a bit more complicated with a second item:

5) Create a second issuiingrule:
   All - itype2 - opacitemholds = Yes (record level allowed), onshelfholds = yes
6) Add a second item of itype2 to the record
7) Place a hold in the OPAC
8) Notice the display is the same as above

The question here is: If one of the itemtypes allows for record level holds and items of that itype are available for item level holds, should a record level hold be allowed? 

Not totally sure about the behaviour question, would really like another opinion here.
Comment 98 Katrin Fischer 2015-01-27 06:50:08 UTC
Created attachment 35550 [details]
Screenshot of the opac hold page with itemtype set to 'Forced'
Comment 99 Martin Renvoize 2015-01-27 09:12:35 UTC
So.. I've been thinking about the options and header text.. and the best I could come up with is:

th - 'Item level holds'
select - 'Allow/Don't allow/Enforce'

Then as a followup for clarity I might add a tooltip to the select box so upon hover we bring back the syspref description... or maybe just having good documentation is good enough ;)
Comment 100 Martin Renvoize 2015-01-27 10:18:03 UTC
Next comment.. I've been mulling over the issue with display/logic when a bib contains items with conflicting rules..

I 'think' perhaps making the 'Force' pref a dominant one.. i.e. if at least one item attached to a bib has the 'Force' flag, then apply the rule to the bib level (i.e all items beocme 'Forced' and thus we should remove the 'Next available item' options and force the patron to select a specific item.

This is just a gut feeling though really.. I would apreciate another pair of eyes/grey matter thinking around the problem.
Comment 101 Srdjan Jankovic 2015-01-28 02:43:37 UTC Comment hidden (obsolete)
Comment 102 Srdjan Jankovic 2015-01-28 02:44:26 UTC Comment hidden (obsolete)
Comment 103 Srdjan Jankovic 2015-01-28 02:51:01 UTC
I have made all corrections except for the placing a hold logic. Once we have agreed what needs to be done we can either
* do a follow up patch and leave this patch as is, or
* include placing a hold correction and reload
Comment 104 Katrin Fischer 2015-01-28 06:47:26 UTC
Hi Srdjan, thx for the follow-up!

Sorry, we didn't resolve the question about the forced item holds yesterday.

I think what Martin said makes sense to me and I think it's also what I have seen in my testing - when one of the itemtypes requires forced item holds, the biblio level hold is not offered. Being more strict seems better than being less so.

If that's right, then it would leave only a display issue. 

If only item level holds are possible, don't show the options to pick "next available item" or "specific item". In my testing it looks like that's what we don on master currently when only item level holds are allowed.
Comment 105 Gaetan Boisson 2015-01-28 09:42:08 UTC
What Martin said also makes sense to me. I think we can have it like that.
Comment 106 Srdjan Jankovic 2015-01-29 03:13:13 UTC Comment hidden (obsolete)
Comment 107 Srdjan Jankovic 2015-01-29 03:14:22 UTC Comment hidden (obsolete)
Comment 108 Srdjan Jankovic 2015-01-29 03:41:03 UTC Comment hidden (obsolete)
Comment 109 Srdjan Jankovic 2015-01-29 03:41:14 UTC Comment hidden (obsolete)
Comment 110 Srdjan Jankovic 2015-01-29 03:46:52 UTC
I hope this is what you had in mind. If not, please forgive me :)
I'll be away first two weeks in Feb, but I'll attend to any issues ASAP when I get back.
Comment 111 Benjamin Rokseth 2015-03-03 10:26:28 UTC
I'm not able to patch against master. Is there a dependency I'm missing?

fatal: sha1 information is lacking or useless (C4/Auth.pm).
Repository lacks necessary blobs to fall back on 3-way merge.
Cannot fall back to three-way merge.
Patch failed at 0001 Bug 5786 - Move AllowOnShelfHolds and OPACItemHolds system prefs to the Circulation Matrix
Comment 112 Katrin Fischer 2015-03-03 10:39:28 UTC Comment hidden (obsolete)
Comment 113 Katrin Fischer 2015-03-03 10:40:08 UTC Comment hidden (obsolete)
Comment 114 Benjamin Rokseth 2015-03-03 14:02:57 UTC Comment hidden (obsolete)
Comment 115 Benjamin Rokseth 2015-03-03 14:04:38 UTC
Tests OK according to test plan.

Two tests in Circulation.t fail, but that error is already present on master and has nothing to do with these patches:

t/db_dependent/Circulation.t .. 1/59 
#   Failed test 'userenv set'
#   at t/db_dependent/Circulation.t line 99.
#          got: 'surname'
#     expected: 'MPL'

#   Failed test '_GetCircControlBranch returned current branch'
#   at t/db_dependent/Circulation.t line 108.
#          got: 'surname'
#     expected: 'MPL'
Comment 116 Srdjan Jankovic 2015-03-04 00:30:04 UTC
(In reply to Benjamin Rokseth from comment #115)
> Tests OK according to test plan.
> 
> Two tests in Circulation.t fail, but that error is already present on master
> and has nothing to do with these patches:

Yes, I have fixed that elsewhere, will submit a bug/patch shortly.
Comment 117 Kyle M Hall 2015-03-05 16:00:58 UTC Comment hidden (obsolete)
Comment 118 Kyle M Hall 2015-03-05 16:01:18 UTC Comment hidden (obsolete)
Comment 119 Kyle M Hall 2015-03-05 16:01:25 UTC Comment hidden (obsolete)
Comment 120 Tomás Cohen Arazi 2015-03-25 14:29:16 UTC
Patches pushed to master.

Thanks Srdjan!
Comment 121 Katrin Fischer 2015-03-25 18:08:49 UTC
I uggest updating this to 'new feature' as it's going to be a nice highlight in the umcoming release :)
Comment 122 Katrin Fischer 2015-03-25 22:44:43 UTC
Comment on attachment 36617 [details] [review]
[PASSED-QA] Bug 5786 - Move AllowOnShelfHolds and OPACItemHolds system prefs to the Circulation Matrix

Review of attachment 36617 [details] [review]:
-----------------------------------------------------------------

::: opac/opac-search.pl
@@ +602,5 @@
> +            if ($borrower) {
> +                $_->{holdable} =
> +                    IsAvailableForItemLevelRequest($_, $borrower) &&
> +                    OPACItemHoldsAllowed($_, $borrower)
> +                  foreach @newresults;

This totally breaks the OPAC search - resulting in a Perl error: 
No homebranch at /home/katrin/kohaclone/C4/Reserves.pm line 2157.

I think $_ is not what you think - especially with using XSLT results. It looks like the HTML output that is created for display if my tests are correct.

The same code is used in the if condition above.

Using $_ and the foreach at the end also makes this quite a confusing read.
Comment 123 Katrin Fischer 2015-03-25 22:56:52 UTC
This might get reverted if not fixed fast - please take a look asap!
Comment 124 Srdjan Jankovic 2015-03-26 06:36:26 UTC Comment hidden (obsolete)
Comment 125 Srdjan Jankovic 2015-03-26 06:42:40 UTC
I hope this patch is not too heavy. What I did is:
* Reverted opac-search.pl to almost what it was before we started this bug
* changed searchResults() in opac context to take in account issuingrules when setting norequests
Comment 126 Marc Véron 2015-03-26 10:20:34 UTC Comment hidden (obsolete)
Comment 127 Katrin Fischer 2015-03-26 23:49:37 UTC
Hi Srdjan,

spotted some problems with the follow-up:

- With the patch t/db_dependent/Search.t fails 2 tests
- As this change a core routine now, we should have new unit tests for it
- It would help testing if you could include some notes about what the code
  is supposed to do and a little test plan
- I see a mismatch between the display in the result list and the 
  detail page. Example:
  * record with 1 item, 
  * on shelf holds are allowed
  * user is logged in
  - result page: no hold link shows
  - detail page: hold link shows
  I failed to make the place hold link show up :(
  

Good news:
- opac search no longer shows a perl error
- opac detail page - normal, marc and isbd view seem to work correctly
- staff search seems ok
- cataloging search as well
Comment 128 Srdjan Jankovic 2015-03-27 00:21:47 UTC Comment hidden (obsolete)
Comment 129 Srdjan Jankovic 2015-03-27 00:34:02 UTC
I see. Unfortunately, I have no quick solutions for that. I hate doing this, but the only thing I can think of to push this to the release is to cheat.
I have removed the extra check, and it will err on the other side so I presume there will be some cases when things will show on the search screen as holdable, while it will be "No holds available" on the details page.
I suggest we keep it like that, and file a bug to fix holdable checkbox appearing when it shouldn't be.
Otherwise I can try to fix it, but it is unlikely to meet the deadline. And I may go into meltdown if this is pulled out :)

Tell me your thoughts.
Comment 130 Katrin Fischer 2015-03-27 00:41:35 UTC
I'd like to avoid any meltdowns :)
Comment 131 Kyle M Hall 2015-03-30 13:42:39 UTC
Created attachment 37373 [details] [review]
[SIGNED-OFF] Bug 5786: Correction: Removed erroneous holdability check from opac-search.pl

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Comment 132 Tomás Cohen Arazi 2015-03-30 16:20:28 UTC
Followup pushed to master.

I'll be waiting for the new bug report. Please add me to the CC.

Thanks Srdjan!

PS: /me hopes no melting took place :-D
Comment 133 Srdjan Jankovic 2015-03-31 01:14:17 UTC
> PS: /me hopes no melting took place :-D

Not really. Actually it was never an option. It is just when I hear a word that I like I'm so eager to use it :)
Comment 134 Katrin Fischer 2015-08-04 12:49:54 UTC
Hm, do we still need a bug report for the visibility check of the hold buttons?