Bug 14702 - Refactor GetReserveFee
Summary: Refactor GetReserveFee
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: Marcel de Rooy
QA Contact: Testopia
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2015-08-21 08:45 UTC by Marcel de Rooy
Modified: 2016-06-21 21:40 UTC (History)
6 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 14702: Fix GetReserveFee (5.75 KB, patch)
2015-08-21 11:35 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 14702: Unit test for GetReservesFee (1.74 KB, patch)
2015-08-21 11:35 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 14702: Database revision for GetReserveFee change (1.46 KB, patch)
2015-08-21 11:35 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 14702: Refactor GetReserveFee (7.65 KB, patch)
2015-08-24 12:47 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 14702: Unit tests for ChargeReservesFee (3.73 KB, patch)
2015-08-24 12:47 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 14702: Refactor GetReserveFee (7.46 KB, patch)
2015-08-24 12:53 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 14702: Unit tests for ChargeReservesFee (3.73 KB, patch)
2015-08-24 12:53 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 14702: Refactor GetReserveFee (7.70 KB, patch)
2015-08-27 07:18 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 14702: Unit tests for GetReserveFee and ChargeReservesFee (3.98 KB, patch)
2015-08-27 07:18 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 14702: Refactor GetReserveFee (7.80 KB, patch)
2015-08-27 09:27 UTC, Joonas Kylmälä
Details | Diff | Splinter Review
Bug 14702: Unit tests for GetReserveFee and ChargeReservesFee (4.65 KB, patch)
2015-08-27 09:38 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 14702: Unit tests for GetReserveFee and ChargeReservesFee (4.75 KB, patch)
2015-08-27 10:25 UTC, Joonas Kylmälä
Details | Diff | Splinter Review
Bug 14702: Unit tests for GetReserveFee and ChargeReservesFee (4.84 KB, patch)
2015-09-01 13:54 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 14702: [QA Follow-up] More readable variable names, less queries (2.48 KB, patch)
2015-09-01 13:54 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 14702: Unit tests for GetReserveFee and ChargeReservesFee (4.75 KB, patch)
2015-09-01 13:56 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 14702: [QA Follow-up] More readable variable names, less queries (2.48 KB, patch)
2015-09-01 13:56 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 14702: Refactor GetReserveFee (7.87 KB, patch)
2015-09-01 14:31 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 14702: Unit tests for GetReserveFee and ChargeReservesFee (4.78 KB, patch)
2015-09-01 14:31 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 14702: [QA Follow-up] More readable variable names, less queries (2.51 KB, patch)
2015-09-01 14:31 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 14702: [QA Follow-up] More readable variable names, less queries (2.65 KB, patch)
2015-09-02 07:45 UTC, Joonas Kylmälä
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Marcel de Rooy 2015-08-21 08:45:32 UTC
While working on bug 9809, I discovered some wonderful errors in GetReserveFee and its various calls.
Bug 9809 makes just a small change in this respect; this report should do the rest :)
Comment 1 Marcel de Rooy 2015-08-21 11:35:51 UTC Comment hidden (obsolete)
Comment 2 Marcel de Rooy 2015-08-21 11:35:54 UTC Comment hidden (obsolete)
Comment 3 Marcel de Rooy 2015-08-21 11:35:58 UTC Comment hidden (obsolete)
Comment 4 Marcel de Rooy 2015-08-21 11:38:41 UTC
hmm maybe it still needs some attention for the biblio level hold
Comment 5 Marcel de Rooy 2015-08-21 11:39:49 UTC
s/IFNULL/COALESCE
Comment 6 Marcel de Rooy 2015-08-21 11:43:08 UTC
(In reply to Marcel de Rooy from comment #4)
> hmm maybe it still needs some attention for the biblio level hold

OK
Instead of calling GetReserveFee when we are in AddReserve, we should only charge the hold fee when filling the reserve. So we should move the call to 
ModReserveFill !

I will make some changes next week..
Comment 7 Marcel de Rooy 2015-08-24 12:12:47 UTC
(In reply to Marcel de Rooy from comment #6)
> Instead of calling GetReserveFee when we are in AddReserve, we should only
> charge the hold fee when filling the reserve. So we should move the call to 
> ModReserveFill !

Hm. I will try to keep closer to what the code originally does.
Comment 8 Marcel de Rooy 2015-08-24 12:47:08 UTC Comment hidden (obsolete)
Comment 9 Marcel de Rooy 2015-08-24 12:47:13 UTC Comment hidden (obsolete)
Comment 10 Marcel de Rooy 2015-08-24 12:49:32 UTC
In the end I kept closer to the original "design". So no behavior change!
A closer look revealed that part of the original loop was useless.
Hope that this makes things at least easier to understand..
Comment 11 Marcel de Rooy 2015-08-24 12:53:52 UTC Comment hidden (obsolete)
Comment 12 Marcel de Rooy 2015-08-24 12:53:56 UTC Comment hidden (obsolete)
Comment 13 Joonas Kylmälä 2015-08-26 09:10:45 UTC
Tried to test the patches but got the following:

Applying: Bug 14702: Refactor GetReserveFee
fatal: sha1 information is lacking or useless (C4/Reserves.pm).
Repository lacks necessary blobs to fall back on 3-way merge.
Cannot fall back to three-way merge.
Patch failed at 0001 Bug 14702: Refactor GetReserveFee

Looks like some commits are missing from the master. Are you able to fix this?
Comment 14 Marcel de Rooy 2015-08-26 11:05:56 UTC
(In reply to Joonas Kylmälä from comment #13)
> Tried to test the patches but got the following:
> 
> Applying: Bug 14702: Refactor GetReserveFee
> fatal: sha1 information is lacking or useless (C4/Reserves.pm).
> Repository lacks necessary blobs to fall back on 3-way merge.
> Cannot fall back to three-way merge.
> Patch failed at 0001 Bug 14702: Refactor GetReserveFee
> 
> Looks like some commits are missing from the master. Are you able to fix
> this?

Joonas,
Did you see that this depends on bug 9809? So you should apply those first..
Comment 15 Joonas Kylmälä 2015-08-26 11:11:19 UTC
(In reply to Marcel de Rooy from comment #14)
> Joonas,
> Did you see that this depends on bug 9809? So you should apply those first..

Didn't even know that there was that kind of an attribute in Bugzilla! Works now, thanks. :)
Comment 16 Joonas Kylmälä 2015-08-26 11:43:28 UTC
Well what I noticed from here is that the $bibitems variable is now unneeded and should be removed from the AddReserve subroutine. I think I will still look into this more and try to fully understand the changes, and that there is nothing left out which might be important and is missing in this new version.
Comment 17 Marcel de Rooy 2015-08-26 11:46:41 UTC
(In reply to Joonas Kylmälä from comment #16)
> Well what I noticed from here is that the $bibitems variable is now unneeded
> and should be removed from the AddReserve subroutine. I think I will still
> look into this more and try to fully understand the changes, and that there
> is nothing left out which might be important and is missing in this new
> version.

True. I opened bug 14711 for that. Should have added a see also.
Comment 18 Joonas Kylmälä 2015-08-27 06:02:33 UTC
(In reply to Marcel de Rooy from comment #8)
>Since AddReserves only calls this sub, we can move the code for charging the
>fee to this sub. I renamed it to ChargeReserveFee.
>There is no change in behavior.

I'd suggest putting the fee calculation to another subroutine because it will help keeping the Koha code more modulated and reusable in future cases (even though it's not many lines (2!?)).

Also the code below the comment "# If not all items are issued and there are no holds: charge no fee" shouldn't work like that. What if someone wants to explicitly have that one item or an item from one specific branch and the library doesn't do inter-library transportations.
Comment 19 Joonas Kylmälä 2015-08-27 06:25:47 UTC
(In reply to Joonas Kylmälä from comment #18)
> Also the code below the comment "# If not all items are issued and there are
> no holds: charge no fee" shouldn't work like that. What if someone wants to
> explicitly have that one item or an item from one specific branch and the
> library doesn't do inter-library transportations.

There is the "Item level holds" attribute where you can decide whether it is or not allowed to set a hold per item and not per record (/cgi-bin/koha/admin/smart-rules.pl).
Comment 20 Marcel de Rooy 2015-08-27 06:30:03 UTC
(In reply to Joonas Kylmälä from comment #18)
> (In reply to Marcel de Rooy from comment #8)
> >Since AddReserves only calls this sub, we can move the code for charging the
> >fee to this sub. I renamed it to ChargeReserveFee.
> >There is no change in behavior.
> 
> I'd suggest putting the fee calculation to another subroutine because it
> will help keeping the Koha code more modulated and reusable in future cases
> (even though it's not many lines (2!?)).
I will move these lines.

> Also the code below the comment "# If not all items are issued and there are
> no holds: charge no fee" shouldn't work like that. What if someone wants to
> explicitly have that one item or an item from one specific branch and the
> library doesn't do inter-library transportations.
This is outside the scope of this report. This report does not change behavior but actually documents what Koha does here. If we want to change behavior, we should open a new report on top of this one.
Comment 21 Marcel de Rooy 2015-08-27 07:18:05 UTC Comment hidden (obsolete)
Comment 22 Marcel de Rooy 2015-08-27 07:18:09 UTC Comment hidden (obsolete)
Comment 23 Joonas Kylmälä 2015-08-27 09:27:06 UTC Comment hidden (obsolete)
Comment 24 Joonas Kylmälä 2015-08-27 09:28:33 UTC
Marcel, in unit test could you add the license in the top of the file.
Comment 25 Marcel de Rooy 2015-08-27 09:35:09 UTC
(In reply to Joonas Kylmälä from comment #24)
> Marcel, in unit test could you add the license in the top of the file.

Does it block your signoff ?
Comment 26 Marcel de Rooy 2015-08-27 09:38:53 UTC Comment hidden (obsolete)
Comment 27 Marcel de Rooy 2015-08-27 09:39:48 UTC
(In reply to Marcel de Rooy from comment #25)
> (In reply to Joonas Kylmälä from comment #24)
> > Marcel, in unit test could you add the license in the top of the file.
> 
> Does it block your signoff ?

I added it now, but note that a lot of unit tests do not contain the copyright block..
Comment 28 Joonas Kylmälä 2015-08-27 10:25:01 UTC Comment hidden (obsolete)
Comment 29 Jonathan Druart 2015-09-01 11:09:24 UTC
Marcel,
Can I suggest to make easier the readability of GetReserveFee?
$cntX are not good variable names.

Moreover, we could restrict the number of queries executed in some cases.
For instance if there are no reserves, no need to execute the count on items and issues.
Comment 30 Marcel de Rooy 2015-09-01 13:04:57 UTC
(In reply to Jonathan Druart from comment #29)
> Marcel,
> Can I suggest to make easier the readability of GetReserveFee?
> $cntX are not good variable names.

The previous version was quite readable indeed :)
Comment 31 Marcel de Rooy 2015-09-01 13:54:51 UTC Comment hidden (obsolete)
Comment 32 Marcel de Rooy 2015-09-01 13:54:56 UTC Comment hidden (obsolete)
Comment 33 Marcel de Rooy 2015-09-01 13:56:32 UTC Comment hidden (obsolete)
Comment 34 Marcel de Rooy 2015-09-01 13:56:36 UTC Comment hidden (obsolete)
Comment 35 Jonathan Druart 2015-09-01 14:31:17 UTC
Created attachment 42165 [details] [review]
Bug 14702: Refactor GetReserveFee

The code of GetReserveFee was not very clear.
What it did was: check if there are some items not issued. If so and there
are no holds, calculate no fee.

While doing so, I moved the code to charge the fee (in AddReserve) to a small
new sub ChargeReserveFee.

There is no change in behavior.
The follow-up patch adds unit tests.

Test plan:
[1] Make sure that a patron category (X) includes a hold fee.
[2] Select a biblio with 2 items.
[3] Issue one item to another patron.
[4] Place a hold on this biblio by patron with category X. No charge?
[5] Cancel the hold from the previous step.
[6] Use another patron to place another hold on this biblio.
[7] Place hold again by patron with category X. Is it charged?
[8] Cancel that hold again. Issue the second item to another patron.
[9] Place hold again by patron with category X. Is it charged again?

Signed-off-by: Joonas Kylmälä <j.kylmala@gmail.com>

Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>
Comment 36 Jonathan Druart 2015-09-01 14:31:23 UTC
Created attachment 42166 [details] [review]
Bug 14702: Unit tests for GetReserveFee and ChargeReservesFee

Test plan:
Run the test: t/db_dependent/Reserves/GetReserveFee.t

Signed-off-by: Joonas Kylmala <j.kylmala@gmail.com>

Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>
Comment 37 Jonathan Druart 2015-09-01 14:31:26 UTC Comment hidden (obsolete)
Comment 38 Joonas Kylmälä 2015-09-02 07:45:51 UTC
Created attachment 42191 [details] [review]
Bug 14702: [QA Follow-up] More readable variable names, less queries

The names are much better now :)
Combined the queries for items and issues.
Only check the number of holds when needed.

Test plan:
Verify the changes here by running the unit test again.

Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>
Signed-off-by: Joonas Kylmälä <j.kylmala@gmail.com>
Comment 39 Tomás Cohen Arazi 2015-09-07 15:12:47 UTC
Patches pushed to master.

Thanks Marcel!