Bug 8918 - ILS-DI: HoldTitle and HoldItem do not calculate rank of hold
Summary: ILS-DI: HoldTitle and HoldItem do not calculate rank of hold
Status: CLOSED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: Hold requests (show other bugs)
Version: master
Hardware: All All
: P5 - low normal (vote)
Assignee: Julian Maurice
QA Contact:
URL:
Keywords:
Depends on:
Blocks: 11640
  Show dependency treegraph
 
Reported: 2012-10-15 12:13 UTC by Julian Maurice
Modified: 2015-06-04 23:23 UTC (History)
14 users (show)

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


Attachments
Bug 8918: ILS-DI: Calculate rank when placing hold (1.64 KB, patch)
2012-10-15 12:14 UTC, Julian Maurice
Details | Diff | Splinter Review
[SIGNED-OFF] Bug 8918: ILS-DI: Calculate rank when placing hold (1.71 KB, patch)
2012-11-16 01:42 UTC, Srikanth Dhondi
Details | Diff | Splinter Review
Bug 8918: QA fixes (3.45 KB, patch)
2012-11-20 16:10 UTC, Julian Maurice
Details | Diff | Splinter Review
Bug 8918: QA fixes (3.50 KB, patch)
2013-02-17 07:57 UTC, Chris Cormack
Details | Diff | Splinter Review
8918 QA Followup for pref descriptions (2.28 KB, patch)
2013-04-12 09:35 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 8918: Fix the transit case in HoldItem (660 bytes, patch)
2013-05-02 08:40 UTC, Julian Maurice
Details | Diff | Splinter Review
Bug 8918: Move $rank and $found calculation into AddReserve (8.31 KB, patch)
2013-05-02 08:40 UTC, Julian Maurice
Details | Diff | Splinter Review
[SIGNED-OFF] Bug 8918: ILS-DI: Calculate rank when placing hold (5.40 KB, patch)
2013-05-02 09:24 UTC, Julian Maurice
Details | Diff | Splinter Review
Bug 8918: Fix the transit case in HoldItem (660 bytes, patch)
2013-05-02 09:24 UTC, Julian Maurice
Details | Diff | Splinter Review
Bug 8918: Move $rank and $found calculation into AddReserve (8.31 KB, patch)
2013-05-02 09:24 UTC, Julian Maurice
Details | Diff | Splinter Review
[SIGNED-OFF] Bug 8918: ILS-DI: Calculate rank when placing hold (5.40 KB, patch)
2013-09-23 07:37 UTC, Julian Maurice
Details | Diff | Splinter Review
Bug 8918: Fix the transit case in HoldItem (644 bytes, patch)
2013-09-23 07:37 UTC, Julian Maurice
Details | Diff | Splinter Review
Bug 8918: Move $rank and $found calculation into AddReserve (8.31 KB, patch)
2013-09-23 07:38 UTC, Julian Maurice
Details | Diff | Splinter Review
Bug 8918: Calculate hold priority in AddReserve (10.93 KB, patch)
2013-11-05 15:27 UTC, Julian Maurice
Details | Diff | Splinter Review
Bug 8918: Add a unit test for GetReserveNextRank (1.20 KB, patch)
2013-11-05 15:27 UTC, Julian Maurice
Details | Diff | Splinter Review
Bug 8918: Calculate hold priority in AddReserve (10.96 KB, patch)
2014-01-29 14:46 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 8918: Add a unit test for GetReserveNextRank (1.43 KB, patch)
2014-01-29 14:46 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 8918: Fix reserve priority in ILS-DI (5.42 KB, patch)
2014-01-30 12:31 UTC, Marcel de Rooy
Details | Diff | Splinter Review
8918 QA Followup for pref descriptions (2.28 KB, patch)
2014-01-30 12:31 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 8918: Add a unit test for CalculatePriority (1.43 KB, patch)
2014-01-30 12:41 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 8918: Add a unit test for CalculatePriority (1.48 KB, patch)
2014-01-30 12:42 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 8918: Fix reserve priority in ILS-DI (5.50 KB, patch)
2014-01-30 12:49 UTC, Marcel de Rooy
Details | Diff | Splinter Review
8918 QA Followup for pref descriptions (2.28 KB, patch)
2014-01-30 12:52 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 8918: Add a unit test for CalculatePriority (1.53 KB, patch)
2014-01-30 12:52 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 8918: Follow-up for some more unit tests for CalculatePriority (3.17 KB, patch)
2014-01-30 13:21 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 8918: Follow-up for some more unit tests for CalculatePriority (3.21 KB, patch)
2014-01-30 13:22 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 8918: Follow-up for some more unit tests for CalculatePriority (3.25 KB, patch)
2014-01-31 09:14 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 8918: Fix reserve priority in ILS-DI (5.46 KB, patch)
2014-02-06 14:46 UTC, Julian Maurice
Details | Diff | Splinter Review
8918 QA Followup for pref descriptions (2.33 KB, patch)
2014-02-06 14:46 UTC, Julian Maurice
Details | Diff | Splinter Review
Bug 8918: Add a unit test for CalculatePriority (1.54 KB, patch)
2014-02-06 14:47 UTC, Julian Maurice
Details | Diff | Splinter Review
Bug 8918: Follow-up for some more unit tests for CalculatePriority (3.27 KB, patch)
2014-02-06 14:47 UTC, Julian Maurice
Details | Diff | Splinter Review
Bug 8918: t/db_dependant/Reserves.t fails if marcflavour is UNIMARC (888 bytes, patch)
2014-02-06 14:47 UTC, Julian Maurice
Details | Diff | Splinter Review
Bug 8918: t/db_dependent/Reserves.t fails if marcflavour is UNIMARC (1.07 KB, patch)
2014-02-07 11:13 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 8918: Follow-up for use of AddBiblio in Reserves.t with UNIMARC (1.65 KB, patch)
2014-02-07 11:14 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 8918: Fix reserve priority in ILS-DI (5.51 KB, patch)
2014-02-07 16:25 UTC, Kyle M Hall
Details | Diff | Splinter Review
8918 QA Followup for pref descriptions (2.35 KB, patch)
2014-02-07 16:25 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 8918: Add a unit test for CalculatePriority (1.60 KB, patch)
2014-02-07 16:25 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 8918: Follow-up for some more unit tests for CalculatePriority (3.32 KB, patch)
2014-02-07 16:25 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 8918: t/db_dependent/Reserves.t fails if marcflavour is UNIMARC (1.09 KB, patch)
2014-02-07 16:25 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 8918: Follow-up for use of AddBiblio in Reserves.t with UNIMARC (1.67 KB, patch)
2014-02-07 16:25 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 Julian Maurice 2012-10-15 12:13:07 UTC
ILS-DI services HoldTitle and HoldItem do not calculate rank before placing hold, so rank is always 0 (which is not good).

The patch will calculate this rank by counting the number of holds for a biblio which are not waiting to be pulled ($found ne 'W')
Comment 1 Julian Maurice 2012-10-15 12:14:18 UTC Comment hidden (obsolete)
Comment 2 Mason James 2012-11-13 04:33:03 UTC
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 3 Srikanth Dhondi 2012-11-16 01:42:50 UTC Comment hidden (obsolete)
Comment 4 Paul Poulain 2012-11-20 14:38:48 UTC
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
Comment 5 Paul Poulain 2012-11-20 14:54:14 UTC
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)
Comment 6 Julian Maurice 2012-11-20 15:58:42 UTC
(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.
Comment 7 Julian Maurice 2012-11-20 16:10:57 UTC Comment hidden (obsolete)
Comment 8 Chris Cormack 2013-02-17 07:57:54 UTC Comment hidden (obsolete)
Comment 9 Jonathan Druart 2013-03-15 11:20:05 UTC
Julian: Could you provide a test plan please?
Comment 10 Julian Maurice 2013-03-18 15:28:53 UTC
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.
Comment 11 Katrin Fischer 2013-03-20 10:34:49 UTC
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!
Comment 12 Julian Maurice 2013-03-20 10:48:37 UTC
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.
Comment 13 Marcel de Rooy 2013-04-12 08:37:37 UTC
QA: Exploring the wonderous world of Koha ILSDI now :)
Comment 14 Marcel de Rooy 2013-04-12 09:35:30 UTC
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.
Comment 15 Marcel de Rooy 2013-04-12 09:35:46 UTC Comment hidden (obsolete)
Comment 16 Marcel de Rooy 2013-04-12 10:03:02 UTC
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
Comment 17 Julian Maurice 2013-05-02 08:40:15 UTC Comment hidden (obsolete)
Comment 18 Julian Maurice 2013-05-02 08:40:20 UTC Comment hidden (obsolete)
Comment 19 Julian Maurice 2013-05-02 08:45:41 UTC
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
Comment 20 Julian Maurice 2013-05-02 09:24:12 UTC Comment hidden (obsolete)
Comment 21 Julian Maurice 2013-05-02 09:24:21 UTC Comment hidden (obsolete)
Comment 22 Julian Maurice 2013-05-02 09:24:27 UTC Comment hidden (obsolete)
Comment 23 I'm just a bot 2013-09-18 09:51:32 UTC
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".
Comment 24 Julian Maurice 2013-09-23 07:37:45 UTC Comment hidden (obsolete)
Comment 25 Julian Maurice 2013-09-23 07:37:56 UTC Comment hidden (obsolete)
Comment 26 Julian Maurice 2013-09-23 07:38:01 UTC Comment hidden (obsolete)
Comment 27 Julian Maurice 2013-09-23 07:38:22 UTC
All patches rebased on master
Comment 28 I'm just a bot 2013-09-24 05:32:37 UTC
Patch applied cleanly, go forth and signoff
Comment 29 I'm just a bot 2013-10-24 19:31:24 UTC
Patch applied cleanly, go forth and signoff
Comment 30 Marcel de Rooy 2013-11-01 16:06:25 UTC
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.
Comment 31 Julian Maurice 2013-11-05 15:27:13 UTC Comment hidden (obsolete)
Comment 32 Julian Maurice 2013-11-05 15:27:24 UTC Comment hidden (obsolete)
Comment 33 Marcel de Rooy 2014-01-29 14:46:39 UTC Comment hidden (obsolete)
Comment 34 Marcel de Rooy 2014-01-29 14:46:48 UTC Comment hidden (obsolete)
Comment 35 Marcel de Rooy 2014-01-29 14:47:25 UTC
Rebasing first..
Comment 36 Marcel de Rooy 2014-01-29 15:22:53 UTC
benjamin.rokseth@kul.oslo.kommune.no changed status to Signed off?

Could you please clarify?
Comment 37 Marcel de Rooy 2014-01-29 15:57:42 UTC
(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..
Comment 38 Benjamin Rokseth 2014-01-29 20:33:02 UTC
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
Comment 39 Marcel de Rooy 2014-01-30 11:06:35 UTC
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
Comment 40 Julian Maurice 2014-01-30 11:12:15 UTC
Marcel, do what you want with patches if you think it can help them to be in master ;)
Comment 41 Marcel de Rooy 2014-01-30 12:31:05 UTC Comment hidden (obsolete)
Comment 42 Marcel de Rooy 2014-01-30 12:31:48 UTC Comment hidden (obsolete)
Comment 43 Marcel de Rooy 2014-01-30 12:41:51 UTC Comment hidden (obsolete)
Comment 44 Marcel de Rooy 2014-01-30 12:42:59 UTC Comment hidden (obsolete)
Comment 45 Marcel de Rooy 2014-01-30 12:49:19 UTC Comment hidden (obsolete)
Comment 46 Marcel de Rooy 2014-01-30 12:52:27 UTC Comment hidden (obsolete)
Comment 47 Marcel de Rooy 2014-01-30 12:52:33 UTC Comment hidden (obsolete)
Comment 48 Marcel de Rooy 2014-01-30 13:02:39 UTC
Setting it back to Assigned for a short while since I stil want to add a few additional tests.
Comment 49 Marcel de Rooy 2014-01-30 13:21:23 UTC Comment hidden (obsolete)
Comment 50 Marcel de Rooy 2014-01-30 13:22:17 UTC Comment hidden (obsolete)
Comment 51 Marcel de Rooy 2014-01-30 13:55:57 UTC
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..
Comment 52 Marcel de Rooy 2014-01-31 08:11:49 UTC
Still see something. Please wait :)
Comment 53 Marcel de Rooy 2014-01-31 09:14:20 UTC Comment hidden (obsolete)
Comment 54 Marcel de Rooy 2014-01-31 09:15:28 UTC
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.
Comment 55 Julian Maurice 2014-02-06 14:46:35 UTC Comment hidden (obsolete)
Comment 56 Julian Maurice 2014-02-06 14:46:52 UTC Comment hidden (obsolete)
Comment 57 Julian Maurice 2014-02-06 14:47:06 UTC Comment hidden (obsolete)
Comment 58 Julian Maurice 2014-02-06 14:47:21 UTC Comment hidden (obsolete)
Comment 59 Julian Maurice 2014-02-06 14:47:30 UTC Comment hidden (obsolete)
Comment 60 Julian Maurice 2014-02-06 14:48:45 UTC
> 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
Comment 61 Marcel de Rooy 2014-02-07 11:13:54 UTC Comment hidden (obsolete)
Comment 62 Marcel de Rooy 2014-02-07 11:14:06 UTC Comment hidden (obsolete)
Comment 63 Kyle M Hall 2014-02-07 16:25:21 UTC
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>
Comment 64 Kyle M Hall 2014-02-07 16:25:27 UTC
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>
Comment 65 Kyle M Hall 2014-02-07 16:25:33 UTC
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>
Comment 66 Kyle M Hall 2014-02-07 16:25:41 UTC
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>
Comment 67 Kyle M Hall 2014-02-07 16:25:46 UTC
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>
Comment 68 Kyle M Hall 2014-02-07 16:25:52 UTC
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>
Comment 69 Galen Charlton 2014-03-10 17:50:08 UTC
Pushed to master.  Thanks, Julian and Marcel!
Comment 70 Fridolin Somers 2014-07-21 11:51:52 UTC
Pushed to 3.14.x, will be in 3.14.07

(i had forget to change this bug status)