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).

Oops - I applied it wrong :)  Will try again.

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

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

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)

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 ?

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.

back to needs signoff then ?

I also have added 87 and 5788 as dependancies

Updating Version : This ENH will be for Koha 3.8

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)

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

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!

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.

Thx for the explanation. I think I will start here then :)

Starting tests.

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.

The database update gives no feedback at all. 
Please add some 'print' with feedback about the updates done.

Hate to say so, but looking at your patch it's still moving branchcode in kohastructure.

The database update worked nicely. I will continue testing and you could fix kohastructure with a follow up patch?

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.

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.

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.

My apologies, that one should go to 5787.

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?

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.

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.

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)

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

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.

Ok, Srdjan, wait and see.
thx

*** Bug 8372 has been marked as a duplicate of this bug. ***

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

Integral patch for 5786, 5787, 5788

Is there a reason this was signed off, then marked does not apply?

Because between the time I signed off and Kyle looked at it new patches were pushed that made this patch not apply anymore.

*** Bug 5787 has been marked as a duplicate of this bug. ***

*** Bug 5788 has been marked as a duplicate of this bug. ***

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 :)

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.

pplying: bug_5786, bug_5787, bug_5788: Holds rewrite
error: patch failed: C4/Auth.pm:412
error: C4/Auth.pm: patch does not apply

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

> 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

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.

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.

I'm making it Signed Off. Functionality should not be changed, there's a small possibility of HTML issues

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

Hi Paul,
Want me to do anything?

Hi Srdjan, it looks like Paul found a real problem with this - can you confirm and maybe fix it?

(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 ;-)

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.

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?

(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.

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

(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..

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

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?

*** Bug 5434 has been marked as a duplicate of this bug. ***

(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..

So shall we just remove the ReservesMaxPickUpDelay bit?

*** Bug 9974 has been marked as a duplicate of this bug. ***

(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

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".

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 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?

Generally I'm happy with this patch.. though a comment on the above FIXME would hasten my signing off.

Rebased patches after Auth.pm tidy in bug 13499

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.

Passing QA on this.. would be lovely to see it in.

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.

(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 :)

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.

Created attachment 35550 [details]
Screenshot of the opac hold page with itemtype set to 'Forced'

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 ;)

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.

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

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.

What Martin said also makes sense to me. I think we can have it like that.

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.

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

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'

(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.

Patches pushed to master.

Thanks Srdjan!

I uggest updating this to 'new feature' as it's going to be a nice highlight in the umcoming release :)

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.

This might get reverted if not fixed fast - please take a look asap!

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

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

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.

I'd like to avoid any meltdowns :)

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>

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

> 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 :)

Hm, do we still need a bug report for the visibility check of the hold buttons?