Bug 8918

a test plan and screenshots please for signoff(In reply to comment #1)
> Created attachment 12810 [details] [review]
> Bug 8918: ILS-DI: Calculate rank when placing hold

This needs a test plan.

Comment on attachment 12810 [details] [review]
Bug 8918: ILS-DI: Calculate rank when placing hold

Srikanth Dhondi, thanks for your commit. When you upload a signed-off patch, don't forget to obsolete the initial one.
If you use git bz, just use the -e flag =
git bz attach -e 8918 HEAD
for example
If you don't use git bz, it's a little bit tricky to find, look at patch "Detail" (on the right of the patch), then click on "edit details", it's on the left

QA comment
 * passes koha-qa.pl
 * add a sub that is local to ILSDI/Service.pm, sounds fair (I'm just surprised that this sub was not needed before/outside from ILDSI...)
 * a question : Julian, did you notice that the
-    my $rank;
+    my $rank = _get_reserve_next_rank($biblionumber);
is followed a few lines later by:
    $rank = '0' unless C4::Context->preference('ReservesNeedReturns');

Have you checked that it works as expected in all cases ? (just asking, I haven't proved there's a problem)

(In reply to comment #5)
> Have you checked that it works as expected in all cases ? (just asking, I
> haven't proved there's a problem)

Not in *all* cases because I just can't imagine *all* test cases here, but I tested different scenario.
I agree that _get_reserve_next_rank should move from C4::ILSDI.
About the syspref ReservesNeedReturns, I'm not sure of what it deserves, but the sure thing is that rank should not be calculated if ReservesNeedReturns = 0.

I'll send the patch soon.

Julian: Could you provide a test plan please?

Test plan:

- Get a valid biblionumber and two borrowernumbers that can hold this biblio record

On master:
- Go to
http://opac.your-library/cgi-bin/koha/ilsdi.pl?service=HoldTitle&patron_id=BORROWERNUMBER1&bib_id=BIBLIONUMBER&request_location=127.0.0.1
and to
http://opac.your-library/cgi-bin/koha/ilsdi.pl?service=HoldTitle&patron_id=BORROWERNUMBER2&bib_id=BIBLIONUMBER&request_location=127.0.0.1
- Then check the holds on your biblio record. Under column 'Priority', you should see 'Waiting' for both reserves.
- Cancel these holds

Apply the patch:
- Go to
http://opac.your-library/cgi-bin/koha/ilsdi.pl?service=HoldTitle&patron_id=BORROWERNUMBER1&bib_id=BIBLIONUMBER&request_location=127.0.0.1
and to
http://opac.your-library/cgi-bin/koha/ilsdi.pl?service=HoldTitle&patron_id=BORROWERNUMBER2&bib_id=BIBLIONUMBER&request_location=127.0.0.1
- Then check the holds on your biblio record. Under column 'Priority', you should now see '1' for the first borrowernumber and '2' for the other.

Apply the same test plan for HoldItem service.

Hi Julian,

I started looking at this, but I am a bit confused about the ReservesNeedReturns preference - can you maybe explain me in a sentence or 2 how it works and why we should not calculate the rank if it's used? Thanks!

Hi Katrin,

according to syspref description, ReservesNeedReturns = 0 means "Automatically mark holds as found and waiting when a hold is placed specifically on them and they are already checked in."

Marking a hold as found and waiting implies to set priority/rank to 0 and to set the flag 'found' to 'W'.
That's why we don't have to calculate the next rank in this case.

QA: Exploring the wonderous world of Koha ILSDI now :)

I am including a QA followup patch, just sideways related to this report, but the pref description of ReservesNeedReturns could use the slightly clearer comment that it only works on item level holds.

In the same run I update the description of ILS-DI:AuthorizedIPs. Note that I am not that enthusiastic about the ':' in pref names there. But the desc needs the comment that you should separate with , and not use spaces. Note that another report probably should widen that: allow spaces, commans and colons between those ip's and test with a regex [';\s] etc.

QA Comment:
This needs still some attention.

Tested e.g. ilsdi.pl?service=HoldItem&patron_id=1&bib_id=196917&item_id=199724 with ReservesNeedReturns enabled.
Result: reserves record includes rank 0 but found is NULL. This combination is not allowed.
Note I stumbled over it by accident, the holding branch was different from home branch.. But you should handle this transit case also.

I saw already some code moving from Services.pm to Reserves. Note that I still have the impressions that there is too much reserves code in HoldTitle and even more in HoldItem.

Failed QA

To test:
 - Try to hold a title with ILS-DI
 - Try to hold an item with ILS-DI (with syspref ReservesNeedReturns on and off)
 - Try to hold a title from the staff interface
 - Try to hold an item from the staff interface (with syspref ReservesNeedReturns on and off)
 - Try to hold a title from OPAC
 - Try to hold an item from OPAC (with syspref ReservesNeedReturns on and off

For each test, check that the rank is correctly set.
If ReservesNeedReturns is off and you hold a specific item, check that reserves.found is correctly to set to 'W' (waiting to be pulled) or 'T' (in transit), depending on the chosen pickup location

Applying: Bug 8918: ILS-DI: Calculate rank when placing hold
Using index info to reconstruct a base tree...
M	C4/ILSDI/Services.pm
M	C4/Reserves.pm
M	koha-tmpl/intranet-tmpl/prog/en/modules/admin/preferences/circulation.pref
Falling back to patching base and 3-way merge...
Auto-merging koha-tmpl/intranet-tmpl/prog/en/modules/admin/preferences/circulation.pref
Auto-merging C4/Reserves.pm
CONFLICT (content): Merge conflict in C4/Reserves.pm
Auto-merging C4/ILSDI/Services.pm
Patch failed at 0001 Bug 8918: ILS-DI: Calculate rank when placing hold
The copy of the patch that failed is found in:
   /home/christopher/git/koha/.git/rebase-apply/patch
When you have resolved this problem, run "git am --continue".
If you prefer to skip this patch, run "git am --skip" instead.
To restore the original branch and stop patching, run "git am --abort".

All patches rebased on master

Patch applied cleanly, go forth and signoff

Patch applied cleanly, go forth and signoff

In order to get these patches further, I would recommend adding a test plan to the commit messages of each individual patch. And please include some unit tests (in a followup) for the new or adjusted routines in modules to increase the chance of getting these patches pushed. Thank you very much.

Rebasing first..

benjamin.rokseth@kul.oslo.kommune.no changed status to Signed off?

Could you please clarify?

(In reply to M. de Rooy from comment #36)
> benjamin.rokseth@kul.oslo.kommune.no changed status to Signed off?
> 
> Could you please clarify?

I appreciate your interest in this report, Benjamin. But I take the liberty to reset the status to Needs Signoff in lack of evidence that these two patches have been tested and are ready for QA.

Note that I was looking at them too and I will still continue with them. But the changes proposed are certainly not trivial and concern an area (Circulation/Holds) which is rather complex and not bug free. At this moment I actually feel that this report is not yet ready for signoff, but this needs some documentation too. To be continued..

No worries Marcel! Rookie in patch signoffs, and strugglig behind company firewalls, so wasn't able to signoff via git-bz. Also noting that you're still working on this, so will keep hands off this one for now. Enough of others waiting signoff... 

Glad you're working on the public APIs, by the way, would love to see some work on a JSON API for Holds, Reservations, etc. in the line of :

https://github.com/biblibre/koha-restful
&
https://github.com/clrh/koha-restful

All the best!
Benjamin

Julian:
Would you mind if I split this report into two pieces? 

I would rather make a distinction between
1) The original bug in HoldTitle and HoldItem for ILS-DI
2) Making changes in AddReserves

Marcel, do what you want with patches if you think it can help them to be in master ;)

Setting it back to Assigned for a short while since I stil want to add a few additional tests.

Julian,
Do you agree with the amendments? Note that I also renamed GetReserveNextRank to CalculatePriority. In the context of the Reserves module priority is used more often than rank. Hopefully, this will create less confusion.
I also noted some points for adjustment in HoldTitle and HoldItem on report 11641.
Using the new routine in AddReserves is moved to report 11640. This makes testing this report much easier and chances of getting it pushed much higher.

All by all, I will sign off on this report now. And give some attention to 11640 later. I leave 11641 for you :)
If you would still see something here that blocks this report, please feel free to change the status again.

Hopefully this will give this report a push in the right direction now..

Still see something. Please wait :)

Resolved a small date comparison problem in the unit test.
Julian: Could you still test this again one time and set status back to Signed off? Thanks.

> Julian: Could you still test this again one time and set status back to Signed off? Thanks.

I added a new patch so I leave the bug status to Needs Signoff

Created attachment 25139 [details] [review]
Bug 8918: Fix reserve priority in ILS-DI

Priority was not calculated when using ILS-DI.
A new routine is added to Reserves.pm that calculates it.
A separate report will make the changes needed in Reserves in order to
use this new routine more generally.
This patch does therefore only affect ILS-DI.

Note: ILS-DI already allows you to generate multiple holds on a biblio or
item for the same patron. This patch does not change that behavior.

Test plan:
[1] Place multiple holds using ILS-DI HoldTitle service:
    /cgi-bin/koha/ilsdi.pl?service=HoldTitle&patron_id=BORROWERNUMBER&bib_id=BIBLIONUMBER&request_location=test
    Check the priority.
[2] Do the same using HoldItem service:
    /cgi-bin/koha/ilsdi.pl?service=HoldItem&patron_id=BORROWERNUMBER&bib_id=BIBLIONUMBER&item_id=ITEMNUMBER
    Check the priority again.
[3] Use a biblio with multiple items. Place item level holds on both.
    Check in one of these items in another branch. Confirm transfer.
    Check in the other item in the original branch. Confirm hold.
    Now you have a waiting and a transit hold.
    Test HoldTitle and HoldItem service again a few times.
[4] Enable AllowHoldDateInFuture and add a future hold.
    Now test HoldTitle and HoldItem again and check if these holds are
    inserted before the future hold (lower priority).

January 29, 2014: Rebased this patch and amended it to make a distinction
between fixing the ILS-DI bug and using the new routine.
Updated commit message and test plan (marcelr).

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>

Created attachment 25140 [details] [review]
8918 QA Followup for pref descriptions

Updating pref descriptions for ReservesNeedReturns and ILS-DI:AuthorizedIPs.
Just sideway related to this report, but not important enough to separate.

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Signed-off-by: Julian Maurice <julian.maurice@biblibre.com>
Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>

Created attachment 25141 [details] [review]
Bug 8918: Add a unit test for CalculatePriority

Rebased on January 29, 2014 (marcelr)
Added text on the two 'is'-statements.

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Signed-off-by: Julian Maurice <julian.maurice@biblibre.com>
Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>

Created attachment 25142 [details] [review]
Bug 8918: Follow-up for some more unit tests for CalculatePriority

Adding a few unit tests, including the following situations:
Placing a hold when there is a wait.
Placing a hold when there is a future hold.
Calculating priority with future date.

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Signed-off-by: Julian Maurice <julian.maurice@biblibre.com>
Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>

Created attachment 25143 [details] [review]
Bug 8918: t/db_dependent/Reserves.t fails if marcflavour is UNIMARC

Set marcflavour to MARC21 to make tests pass.

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Works for MARC21. But I would prefer a better fix for UNIMARC.
Will send a follow-up for that.

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>

Created attachment 25144 [details] [review]
Bug 8918: Follow-up for use of AddBiblio in Reserves.t with UNIMARC

Changed title and author field for UNIMARC.

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Tested for MARC21, NORMARC and UNIMARC by adding temporary set_preference..

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>

Pushed to master.  Thanks, Julian and Marcel!

Pushed to 3.14.x, will be in 3.14.07

(i had forget to change this bug status)