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 :)
Created attachment 41779 [details] [review] Bug 14702: Fix GetReserveFee The code in this sub of Reserves is unneeded and erroneous. The old code checked via biblioitems if there still was an item not issued and if so, it checked reserves: if there was no no hold yet for the biblio, the reserve fee went to zero. (Strange !) So only the second hold would trigger a charge. (But Koha does NOT allow that second hold at this time!) So it actually does nothing! This can/should be drastically simplified: We only call GetReserveFee from AddReserve. So there is an item free and it is possible to place a hold. Since it is currently not possible to place a second hold, and even if it was, we just charge our hold fee from patron categories! NOTE: It somehow seems a bit odd to have hold fee in the patron categories. Maybe it should be added to itemtypes (on another report)? This is a fix that changes behavior! A follow-up will print a warning for libraries upgrading. Test plan: [1] Add a hold for a patron without hold fee. Check the patron account if there is no charge for the new hold. [2] Add a hold for a ptron with hold fee. Check account again.
Created attachment 41780 [details] [review] Bug 14702: Unit test for GetReservesFee Test plan: Run the test!
Created attachment 41781 [details] [review] Bug 14702: Database revision for GetReserveFee change This patch adds a warning when upgrading. Test plan: [1] Have a patron category with a hold fee. [2] Run the webinstaller upgrade. Check for the warning.
hmm maybe it still needs some attention for the biblio level hold
s/IFNULL/COALESCE
(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..
(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.
Created attachment 41853 [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. 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. 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?
Created attachment 41854 [details] [review] Bug 14702: Unit tests for ChargeReservesFee Test plan: Run the test: t/db_dependent/Reserves_ChargeReserveFee.t
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..
Created attachment 41856 [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. 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. 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?
Created attachment 41857 [details] [review] Bug 14702: Unit tests for ChargeReservesFee Test plan: Run the test: t/db_dependent/Reserves_ChargeReserveFee.t
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?
(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..
(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. :)
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.
(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.
(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.
(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).
(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.
Created attachment 42005 [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?
Created attachment 42006 [details] [review] Bug 14702: Unit tests for GetReserveFee and ChargeReservesFee Test plan: Run the test: t/db_dependent/Reserves_Fee.t
Created attachment 42015 [details] [review] Bug 14702: Refactor GetReserveFee Test plan worked, did also my own tests. Code was also doing the same things as before. Signed-off-by: Joonas Kylmälä <j.kylmala@gmail.com>
Marcel, in unit test could you add the license in the top of the file.
(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 ?
Created attachment 42016 [details] [review] Bug 14702: Unit tests for GetReserveFee and ChargeReservesFee Test plan: Run the test: t/db_dependent/Reserves_Fee.t
(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..
Created attachment 42017 [details] [review] Bug 14702: Unit tests for GetReserveFee and ChargeReservesFee Test plan worked, code looks now great with the license! Signed-off-by: Joonas Kylmälä <j.kylmala@gmail.com>
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.
(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 :)
Created attachment 42153 [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 Kylmälä <j.kylmala@gmail.com>
Created attachment 42154 [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.
Created attachment 42155 [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>
Created attachment 42156 [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.
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>
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>
Created attachment 42167 [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>
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>
Patches pushed to master. Thanks Marcel!