[Bug 14702] New: Fix GetReserveFee
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=14702 Bug ID: 14702 Summary: Fix GetReserveFee Change sponsored?: --- Product: Koha Version: master Hardware: All OS: All Status: NEW Severity: normal Priority: P5 - low Component: Hold requests Assignee: koha-bugs@lists.koha-community.org Reporter: m.de.rooy@rijksmuseum.nl QA Contact: testopia@bugs.koha-community.org CC: gmcharlt@gmail.com 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 :) -- You are receiving this mail because: You are watching all bug changes. You are the assignee for the bug.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=14702 Marcel de Rooy <m.de.rooy@rijksmuseum.nl> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|NEW |ASSIGNED Assignee|koha-bugs@lists.koha-commun |m.de.rooy@rijksmuseum.nl |ity.org | -- You are receiving this mail because: You are watching all bug changes. You are the assignee for the bug.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=14702 Marcel de Rooy <m.de.rooy@rijksmuseum.nl> changed: What |Removed |Added ---------------------------------------------------------------------------- See Also| |http://bugs.koha-community. | |org/bugzilla3/show_bug.cgi? | |id=9809 -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=14702 Marcel de Rooy <m.de.rooy@rijksmuseum.nl> changed: What |Removed |Added ---------------------------------------------------------------------------- Depends on| |9809 Referenced Bugs: http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=9809 [Bug 9809] Get rid of reserveconstraints -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=14702 --- Comment #1 from Marcel de Rooy <m.de.rooy@rijksmuseum.nl> --- Created attachment 41779 --> http://bugs.koha-community.org/bugzilla3/attachment.cgi?id=41779&action=edit 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. -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=14702 --- Comment #2 from Marcel de Rooy <m.de.rooy@rijksmuseum.nl> --- Created attachment 41780 --> http://bugs.koha-community.org/bugzilla3/attachment.cgi?id=41780&action=edit Bug 14702: Unit test for GetReservesFee Test plan: Run the test! -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=14702 --- Comment #3 from Marcel de Rooy <m.de.rooy@rijksmuseum.nl> --- Created attachment 41781 --> http://bugs.koha-community.org/bugzilla3/attachment.cgi?id=41781&action=edit 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. -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=14702 Marcel de Rooy <m.de.rooy@rijksmuseum.nl> changed: What |Removed |Added ---------------------------------------------------------------------------- Patch complexity|--- |Small patch Status|ASSIGNED |Needs Signoff -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=14702 --- Comment #4 from Marcel de Rooy <m.de.rooy@rijksmuseum.nl> --- hmm maybe it still needs some attention for the biblio level hold -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=14702 Marcel de Rooy <m.de.rooy@rijksmuseum.nl> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Needs Signoff |ASSIGNED -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=14702 --- Comment #5 from Marcel de Rooy <m.de.rooy@rijksmuseum.nl> --- s/IFNULL/COALESCE -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=14702 --- Comment #6 from Marcel de Rooy <m.de.rooy@rijksmuseum.nl> --- (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.. -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=14702 Marcel de Rooy <m.de.rooy@rijksmuseum.nl> changed: What |Removed |Added ---------------------------------------------------------------------------- CC| |m.de.rooy@rijksmuseum.nl -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=14702 Marcel de Rooy <m.de.rooy@rijksmuseum.nl> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #41779|0 |1 is obsolete| | -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=14702 Marcel de Rooy <m.de.rooy@rijksmuseum.nl> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #41780|0 |1 is obsolete| | -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=14702 Marcel de Rooy <m.de.rooy@rijksmuseum.nl> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #41781|0 |1 is obsolete| | -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=14702 --- Comment #7 from Marcel de Rooy <m.de.rooy@rijksmuseum.nl> --- (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. -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=14702 --- Comment #8 from Marcel de Rooy <m.de.rooy@rijksmuseum.nl> --- Created attachment 41853 --> http://bugs.koha-community.org/bugzilla3/attachment.cgi?id=41853&action=edit 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? -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=14702 --- Comment #9 from Marcel de Rooy <m.de.rooy@rijksmuseum.nl> --- Created attachment 41854 --> http://bugs.koha-community.org/bugzilla3/attachment.cgi?id=41854&action=edit Bug 14702: Unit tests for ChargeReservesFee Test plan: Run the test: t/db_dependent/Reserves_ChargeReserveFee.t -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=14702 Marcel de Rooy <m.de.rooy@rijksmuseum.nl> changed: What |Removed |Added ---------------------------------------------------------------------------- Summary|Fix GetReserveFee |Refactor GetReserveFee -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=14702 Marcel de Rooy <m.de.rooy@rijksmuseum.nl> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|ASSIGNED |Needs Signoff --- Comment #10 from Marcel de Rooy <m.de.rooy@rijksmuseum.nl> --- 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.. -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=14702 Marcel de Rooy <m.de.rooy@rijksmuseum.nl> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #41853|0 |1 is obsolete| | --- Comment #11 from Marcel de Rooy <m.de.rooy@rijksmuseum.nl> --- Created attachment 41856 --> http://bugs.koha-community.org/bugzilla3/attachment.cgi?id=41856&action=edit 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? -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=14702 Marcel de Rooy <m.de.rooy@rijksmuseum.nl> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #41854|0 |1 is obsolete| | --- Comment #12 from Marcel de Rooy <m.de.rooy@rijksmuseum.nl> --- Created attachment 41857 --> http://bugs.koha-community.org/bugzilla3/attachment.cgi?id=41857&action=edit Bug 14702: Unit tests for ChargeReservesFee Test plan: Run the test: t/db_dependent/Reserves_ChargeReserveFee.t -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=14702 Joonas Kylmälä <j.kylmala@gmail.com> changed: What |Removed |Added ---------------------------------------------------------------------------- CC| |j.kylmala@gmail.com --- Comment #13 from Joonas Kylmälä <j.kylmala@gmail.com> --- 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? -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=14702 --- Comment #14 from Marcel de Rooy <m.de.rooy@rijksmuseum.nl> --- (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.. -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=14702 --- Comment #15 from Joonas Kylmälä <j.kylmala@gmail.com> --- (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. :) -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=14702 --- Comment #16 from Joonas Kylmälä <j.kylmala@gmail.com> --- 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. -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=14702 Marcel de Rooy <m.de.rooy@rijksmuseum.nl> changed: What |Removed |Added ---------------------------------------------------------------------------- See Also| |http://bugs.koha-community. | |org/bugzilla3/show_bug.cgi? | |id=14711 --- Comment #17 from Marcel de Rooy <m.de.rooy@rijksmuseum.nl> --- (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. -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=14702 --- Comment #18 from Joonas Kylmälä <j.kylmala@gmail.com> --- (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. -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=14702 --- Comment #19 from Joonas Kylmälä <j.kylmala@gmail.com> --- (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). -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=14702 --- Comment #20 from Marcel de Rooy <m.de.rooy@rijksmuseum.nl> --- (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.
-- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=14702 Marcel de Rooy <m.de.rooy@rijksmuseum.nl> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #41856|0 |1 is obsolete| | --- Comment #21 from Marcel de Rooy <m.de.rooy@rijksmuseum.nl> --- Created attachment 42005 --> http://bugs.koha-community.org/bugzilla3/attachment.cgi?id=42005&action=edit 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? -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=14702 --- Comment #22 from Marcel de Rooy <m.de.rooy@rijksmuseum.nl> --- Created attachment 42006 --> http://bugs.koha-community.org/bugzilla3/attachment.cgi?id=42006&action=edit Bug 14702: Unit tests for GetReserveFee and ChargeReservesFee Test plan: Run the test: t/db_dependent/Reserves_Fee.t -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=14702 Marcel de Rooy <m.de.rooy@rijksmuseum.nl> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #41857|0 |1 is obsolete| | -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=14702 Joonas Kylmälä <j.kylmala@gmail.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #42005|0 |1 is obsolete| | --- Comment #23 from Joonas Kylmälä <j.kylmala@gmail.com> --- Created attachment 42015 --> http://bugs.koha-community.org/bugzilla3/attachment.cgi?id=42015&action=edit 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> -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=14702 --- Comment #24 from Joonas Kylmälä <j.kylmala@gmail.com> --- Marcel, in unit test could you add the license in the top of the file. -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=14702 --- Comment #25 from Marcel de Rooy <m.de.rooy@rijksmuseum.nl> --- (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 ? -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=14702 Marcel de Rooy <m.de.rooy@rijksmuseum.nl> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #42006|0 |1 is obsolete| | --- Comment #26 from Marcel de Rooy <m.de.rooy@rijksmuseum.nl> --- Created attachment 42016 --> http://bugs.koha-community.org/bugzilla3/attachment.cgi?id=42016&action=edit Bug 14702: Unit tests for GetReserveFee and ChargeReservesFee Test plan: Run the test: t/db_dependent/Reserves_Fee.t -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=14702 --- Comment #27 from Marcel de Rooy <m.de.rooy@rijksmuseum.nl> --- (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.. -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=14702 Marcel de Rooy <m.de.rooy@rijksmuseum.nl> changed: What |Removed |Added ---------------------------------------------------------------------------- Depends on|9809 | Referenced Bugs: http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=9809 [Bug 9809] Get rid of reserveconstraints -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=14702 Joonas Kylmälä <j.kylmala@gmail.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #42016|0 |1 is obsolete| | --- Comment #28 from Joonas Kylmälä <j.kylmala@gmail.com> --- Created attachment 42017 --> http://bugs.koha-community.org/bugzilla3/attachment.cgi?id=42017&action=edit 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> -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=14702 Joonas Kylmälä <j.kylmala@gmail.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Needs Signoff |Signed Off -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=14702 Marcel de Rooy <m.de.rooy@rijksmuseum.nl> changed: What |Removed |Added ---------------------------------------------------------------------------- See Also| |http://bugs.koha-community. | |org/bugzilla3/show_bug.cgi? | |id=13592 -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=14702 Jonathan Druart <jonathan.druart@bugs.koha-community.org> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Signed Off |Failed QA CC| |jonathan.druart@bugs.koha-c | |ommunity.org --- Comment #29 from Jonathan Druart <jonathan.druart@bugs.koha-community.org> --- 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. -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=14702 --- Comment #30 from Marcel de Rooy <m.de.rooy@rijksmuseum.nl> --- (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 :) -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=14702 Marcel de Rooy <m.de.rooy@rijksmuseum.nl> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Failed QA |Signed Off -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=14702 Marcel de Rooy <m.de.rooy@rijksmuseum.nl> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #42017|0 |1 is obsolete| | --- Comment #31 from Marcel de Rooy <m.de.rooy@rijksmuseum.nl> --- Created attachment 42153 --> http://bugs.koha-community.org/bugzilla3/attachment.cgi?id=42153&action=edit 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> -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=14702 --- Comment #32 from Marcel de Rooy <m.de.rooy@rijksmuseum.nl> --- Created attachment 42154 --> http://bugs.koha-community.org/bugzilla3/attachment.cgi?id=42154&action=edit 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. -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=14702 Marcel de Rooy <m.de.rooy@rijksmuseum.nl> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #42153|0 |1 is obsolete| | --- Comment #33 from Marcel de Rooy <m.de.rooy@rijksmuseum.nl> --- Created attachment 42155 --> http://bugs.koha-community.org/bugzilla3/attachment.cgi?id=42155&action=edit 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> -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=14702 Marcel de Rooy <m.de.rooy@rijksmuseum.nl> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #42154|0 |1 is obsolete| | --- Comment #34 from Marcel de Rooy <m.de.rooy@rijksmuseum.nl> --- Created attachment 42156 --> http://bugs.koha-community.org/bugzilla3/attachment.cgi?id=42156&action=edit 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. -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=14702 Jonathan Druart <jonathan.druart@bugs.koha-community.org> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Signed Off |Passed QA -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=14702 Jonathan Druart <jonathan.druart@bugs.koha-community.org> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #42015|0 |1 is obsolete| | Attachment #42155|0 |1 is obsolete| | Attachment #42156|0 |1 is obsolete| | --- Comment #35 from Jonathan Druart <jonathan.druart@bugs.koha-community.org> --- Created attachment 42165 --> http://bugs.koha-community.org/bugzilla3/attachment.cgi?id=42165&action=edit 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> -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=14702 --- Comment #36 from Jonathan Druart <jonathan.druart@bugs.koha-community.org> --- Created attachment 42166 --> http://bugs.koha-community.org/bugzilla3/attachment.cgi?id=42166&action=edit 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> -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=14702 --- Comment #37 from Jonathan Druart <jonathan.druart@bugs.koha-community.org> --- Created attachment 42167 --> http://bugs.koha-community.org/bugzilla3/attachment.cgi?id=42167&action=edit 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> -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=14702 Joonas Kylmälä <j.kylmala@gmail.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #42167|0 |1 is obsolete| | --- Comment #38 from Joonas Kylmälä <j.kylmala@gmail.com> --- Created attachment 42191 --> http://bugs.koha-community.org/bugzilla3/attachment.cgi?id=42191&action=edit 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> -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=14702 Fiona Borthwick <fiona.borthwick@ptfs-europe.com> changed: What |Removed |Added ---------------------------------------------------------------------------- CC| |fiona.borthwick@ptfs-europe | |.com -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=14702 Tomás Cohen Arazi <tomascohen@gmail.com> changed: What |Removed |Added ---------------------------------------------------------------------------- CC| |tomascohen@gmail.com Status|Passed QA |Pushed to Master --- Comment #39 from Tomás Cohen Arazi <tomascohen@gmail.com> --- Patches pushed to master. Thanks Marcel! -- You are receiving this mail because: You are watching all bug changes.
participants (1)
-
bugzilla-daemon＠bugs.koha-community.org