Description
Kyle M Hall (khall)
2015-04-22 15:49:50 UTC
--Kyle, There seems to be a conflict in this test plan for our consortium purposes. HomeOrHolding is set to "the library the item is checked out from" in order to make it possible for the holding library to charge fines and feeds for their patrons according to their own rules--particularly libraries that do not charge fines. However, it is the home libraries that want to be able to decide whether or not a lost item charge gets forgiven if a lost item makes it back to their library. It is completely possible that I do not understand what you are planning to do. The plan of attack looks good to me. I am just wondering if the field branches.refund_lost_item_fee_on_return makes sense. Don't you think it would be better to set this rule in the "Circulation and fine rules" page (admin/smart-rules.pl)? It would be easy to change the behavior for all libraries (if only a default rule is defined). (In reply to Jonathan Druart from comment #2) > The plan of attack looks good to me. > I am just wondering if the field branches.refund_lost_item_fee_on_return > makes sense. > Don't you think it would be better to set this rule in the "Circulation and > fine rules" page (admin/smart-rules.pl)? It would be easy to change the > behavior for all libraries (if only a default rule is defined). I was wondering that myself Jonathan. I'll look into it, seems like a good idea. I think Sheila has a good point - maybe the dependence on HomeOrHolding is not the best option here and we need something that is a bit more flexible? Only other thing is maybe the column name - refund_lost_item_fee_on_return is quite a bit to write, maybe just refund_lost_on_return? :) (In reply to Katrin Fischer from comment #4) > I think Sheila has a good point - maybe the dependence on HomeOrHolding is > not the best option here and we need something that is a bit more flexible? > > Only other thing is maybe the column name - refund_lost_item_fee_on_return > is quite a bit to write, maybe just refund_lost_on_return? :) (In reply to Jonathan Druart from comment #2) > The plan of attack looks good to me. > I am just wondering if the field branches.refund_lost_item_fee_on_return > makes sense. > Don't you think it would be better to set this rule in the "Circulation and > fine rules" page (admin/smart-rules.pl)? It would be easy to change the > behavior for all libraries (if only a default rule is defined). Jonathan, I believe moving this to the circulation rules is at odds with what Sheila is looking for. The only way to make both work will be to add a new system preference to choose which circulation rule to select. Something like RefundLostOnReturnControl. The options could be CheckinLibrary, PatronLibrary, ItemHomeBranch and ItemHoldingbranch. What do you think? Given this, should we still make this a circulation rule setting? (In reply to Katrin Fischer from comment #4) > I think Sheila has a good point - maybe the dependence on HomeOrHolding is > not the best option here and we need something that is a bit more flexible? See above for a new idea! > Only other thing is maybe the column name - refund_lost_item_fee_on_return > is quite a bit to write, maybe just refund_lost_on_return? :) Sure! Created attachment 52016 [details] [review] Bug 14048: Add RefundLostItemFeeRule table and classes This patch introduces new classes for handling refund lost item fee rules. It introduces a new table for storing this rules. It is designed so it is possible to define a global rule, and then branch-specific ones. The specific is prefered if available. This behaviour is fully tested by unit tests introduced by the following patches. This cannot be tested on its own. Sponsored-by: DoverNet Sponsored-by: South-East Kansas Library System Sponsored-by: SWITCH Library Consortium Created attachment 52017 [details] [review] Bug 14048: Unit tests This patch introduces unit tests for the feature. To test: - Have all patches from this bug applied - Run: $ prove -v t/db_dependent/RefundLostItemFeeRule.t => SUCCESS: All tests pass! Sponsored-by: DoverNet Sponsored-by: South-East Kansas Library System Sponsored-by: SWITCH Library Consortium Created attachment 52018 [details] [review] Bug 14048: Add syspref and atomic update This patch introduces the changes to the sysprefs and the atomic update that creates the new table. It picks the syspref value for RefundLostItemFeeOnReturn and uses it to create a default (branchcode='*') rule on the new table. Sponsored-by: DoverNet Sponsored-by: South-East Kansas Library System Sponsored-by: SWITCH Library Consortium Created attachment 52019 [details] [review] Bug 14048: Add a refund rules setting form in smart-rules This patch introduces the CRUD UI for managing the refund lost item fee rules on the general 'Circulation and fines rules' page. Verify that rules can be added and changed (by adding a new one for the same branch). The default rule is locked so it cannot be deleted. Sponsored-by: DoverNet Sponsored-by: South-East Kansas Library System Sponsored-by: SWITCH Library Consortium Created attachment 52020 [details] [review] Bug 14048: Hook new rules into C4::Circulation This patch makes AddIssue and AddReturn use the new implementation The behaviour should respect the specification. Sponsored-by: DoverNet Sponsored-by: South-East Kansas Library System Sponsored-by: SWITCH Library Consortium I picked what was discussed here and implemented the feature using ideas from Jonathan, Kyle and Sheila. I added a table for configuring rules for each branch, and/or globally. This is done in the 'Circulation and fines rules' page. I also added the RefundLostOnReturnControl syspref Sheila and Kyle proposed giving the libraries more granularity on how the rules are processed. The introduced code is fully tested by new unit tests. Please test, and I'll be waiting for your feedback! Created attachment 52169 [details] [review] Bug 14048: (QA followup) Rules should be defined for the chosen branch (or all) The original patches pull all defined rules for edition in the "Circulation and fines rules" page. This was not the intended behaviour for smart-rules.pl Instead, the modified rules should be constrained to the chosen branch (or the general default rule). This patch changes the templates and front-end scripts so they behave like that. To test: - Apply the patch - Open smart-rules.pl => SUCCESS: The text and refund rules section correctly refers to default global rules. The policy can be set to Yes and No, and works as expected. - Pick a specific branch => SUCCESS: The rule refers to the specific branch => SUCCESS: It detects the default rule is picked => SUCCESS: On changing the rules values are correctly retrieved if entering again to the page. => SUCCESS: The 'Use default' text is suffixed with the correct value set by the user in the 'All libraries' scenario. => Sign off :-D Sponsored-by: DoverNet Sponsored-by: South-East Kansas Library System ponsored-by: SWITCH Library Consortium Created attachment 52170 [details] [review] Bug 14048: (followup) move atomicupdate to SQL-based Somehow, the atomic update wasn't working. I'll fill a different bug for that. In the meantime this dev should move on as it doesn't strictly need the .perl atomic update. Created attachment 52202 [details] [review] Bug 14048 [QA Followup] Created attachment 52203 [details] [review] Bug 14048: [QA Followup] Created attachment 52286 [details] [review] Bug 14048: Add RefundLostItemFeeRule table and classes This patch introduces new classes for handling refund lost item fee rules. It introduces a new table for storing this rules. It is designed so it is possible to define a global rule, and then branch-specific ones. The specific is prefered if available. This behaviour is fully tested by unit tests introduced by the following patches. This cannot be tested on its own. Sponsored-by: DoverNet Sponsored-by: South-East Kansas Library System Sponsored-by: SWITCH Library Consortium Created attachment 52287 [details] [review] Bug 14048: Unit tests This patch introduces unit tests for the feature. To test: - Have all patches from this bug applied - Run: $ prove -v t/db_dependent/RefundLostItemFeeRule.t => SUCCESS: All tests pass! Sponsored-by: DoverNet Sponsored-by: South-East Kansas Library System Sponsored-by: SWITCH Library Consortium Created attachment 52288 [details] [review] Bug 14048: Add syspref and atomic update This patch introduces the changes to the sysprefs and the atomic update that creates the new table. It picks the syspref value for RefundLostItemFeeOnReturn and uses it to create a default (branchcode='*') rule on the new table. Sponsored-by: DoverNet Sponsored-by: South-East Kansas Library System Sponsored-by: SWITCH Library Consortium Created attachment 52289 [details] [review] Bug 14048: Add a refund rules setting form in smart-rules This patch introduces the CRUD UI for managing the refund lost item fee rules on the general 'Circulation and fines rules' page. Verify that rules can be added and changed. Rules are correctly associated to the chosen branch (or the general default rule). To test: - Apply the patch - Open smart-rules.pl => SUCCESS: The text and refund rules section correctly refers to default global rules. The policy can be set to Yes and No, and works as expected. - Pick a specific branch => SUCCESS: The rule refers to the specific branch => SUCCESS: It detects the default rule is picked => SUCCESS: On changing the rules values are correctly retrieved if entering again to the page. => SUCCESS: The 'Use default' text is suffixed with the correct value set by the user in the 'All libraries' scenario. => Sign off :-D Sponsored-by: DoverNet Sponsored-by: South-East Kansas Library System Sponsored-by: SWITCH Library Consortium Created attachment 52290 [details] [review] Bug 14048: Hook new rules into C4::Circulation This patch makes AddIssue and AddReturn use the new implementation The behaviour should respect the specification. Sponsored-by: DoverNet Sponsored-by: South-East Kansas Library System Sponsored-by: SWITCH Library Consortium Created attachment 52291 [details] [review] Bug 14048: [QA Followup] Created attachment 52292 [details] [review] Bug 14048: (QA followup) Fix for TooMany.t Created attachment 52293 [details] [review] Bug 14048: Add syspref and atomic update This patch introduces the changes to the sysprefs and the atomic update that creates the new table. It picks the syspref value for RefundLostItemFeeOnReturn and uses it to create a default (branchcode='*') rule on the new table. Sponsored-by: DoverNet Sponsored-by: South-East Kansas Library System Sponsored-by: SWITCH Library Consortium Created attachment 52306 [details] [review] Bug 14048 [QA Followup] - Delete only the removed system preference (In reply to Kyle M Hall from comment #25) > Created attachment 52306 [details] [review] [review] > Bug 14048 [QA Followup] - Delete only the removed system preference Hehe, I don't know why I didn't notice the second line was removed, see the commented Perl code! Created attachment 52444 [details] [review] Bug 14048: Add RefundLostItemFeeRule table and classes This patch introduces new classes for handling refund lost item fee rules. It introduces a new table for storing this rules. It is designed so it is possible to define a global rule, and then branch-specific ones. The specific is prefered if available. This behaviour is fully tested by unit tests introduced by the following patches. This cannot be tested on its own. Sponsored-by: DoverNet Sponsored-by: South-East Kansas Library System Sponsored-by: SWITCH Library Consortium Created attachment 52445 [details] [review] Bug 14048: Unit tests This patch introduces unit tests for the feature. To test: - Have all patches from this bug applied - Run: $ prove -v t/db_dependent/RefundLostItemFeeRule.t => SUCCESS: All tests pass! Sponsored-by: DoverNet Sponsored-by: South-East Kansas Library System Sponsored-by: SWITCH Library Consortium Created attachment 52446 [details] [review] Bug 14048: Add a refund rules setting form in smart-rules This patch introduces the CRUD UI for managing the refund lost item fee rules on the general 'Circulation and fines rules' page. Verify that rules can be added and changed. Rules are correctly associated to the chosen branch (or the general default rule). To test: - Apply the patch - Open smart-rules.pl => SUCCESS: The text and refund rules section correctly refers to default global rules. The policy can be set to Yes and No, and works as expected. - Pick a specific branch => SUCCESS: The rule refers to the specific branch => SUCCESS: It detects the default rule is picked => SUCCESS: On changing the rules values are correctly retrieved if entering again to the page. => SUCCESS: The 'Use default' text is suffixed with the correct value set by the user in the 'All libraries' scenario. => Sign off :-D Sponsored-by: DoverNet Sponsored-by: South-East Kansas Library System Sponsored-by: SWITCH Library Consortium Created attachment 52447 [details] [review] Bug 14048: Hook new rules into C4::Circulation This patch makes AddIssue and AddReturn use the new implementation The behaviour should respect the specification. Sponsored-by: DoverNet Sponsored-by: South-East Kansas Library System Sponsored-by: SWITCH Library Consortium Created attachment 52448 [details] [review] Bug 14048: [QA Followup] Created attachment 52449 [details] [review] Bug 14048: Add syspref and atomic update This patch introduces the changes to the sysprefs and the atomic update that creates the new table. It picks the syspref value for RefundLostItemFeeOnReturn and uses it to create a default (branchcode='*') rule on the new table. Sponsored-by: DoverNet Sponsored-by: South-East Kansas Library System Sponsored-by: SWITCH Library Consortium Created attachment 52450 [details] [review] Bug 14048 [QA Followup] - Delete only the removed system preference Created attachment 52451 [details] [review] Bug 14048: (QA followup) Update syspref name for HEA Tested well for syspref RefundLostOnReturnControl with values "homebranch" and "check-in library" When using "item holding branch" I receive the error below: Software error: ItemHoldingbranch requires the param Created attachment 52482 [details] [review] Bug 14048: (QA followup) Fix typo on syspref value (In reply to Nick Clemens from comment #35) > Tested well for syspref RefundLostOnReturnControl with values "homebranch" > and "check-in library" > > When using "item holding branch" I receive the error below: > Software error: ItemHoldingbranch requires the param Nick, it was due to a typo. It should be fixed by the latest followup. Retesting, it looks like the itemlost check is done after the checkin, this means the item holding branch has been updated to the checkin location, so in the case of 'item holding branch' rule we aren't using the holding branch before th eitem was checked in which is what I think is intended (i.e. we should use where the was last held/checked out, not the current transaction branch) Created attachment 52658 [details] [review] Bug 14048: (followup) Use the original holding branch This patch picks the item's holding branch *before* it gets fixed by using the checkin library instead. This way the RefundLostOnReturnControl syspref set to ItemHoldingBranch is respected (otherwise, as Nick explained this behaves just like if CheckinLibrary was set) Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io> Created attachment 52659 [details] [review] Bug 14048: Add RefundLostItemFeeRule table and classes This patch introduces new classes for handling refund lost item fee rules. It introduces a new table for storing this rules. It is designed so it is possible to define a global rule, and then branch-specific ones. The specific is prefered if available. This behaviour is fully tested by unit tests introduced by the following patches. This cannot be tested on its own. Sponsored-by: DoverNet Sponsored-by: South-East Kansas Library System Sponsored-by: SWITCH Library Consortium Signed-off-by: Nick Clemens <nick@bywatersolutions.com> Created attachment 52660 [details] [review] Bug 14048: Unit tests This patch introduces unit tests for the feature. To test: - Have all patches from this bug applied - Run: $ prove -v t/db_dependent/RefundLostItemFeeRule.t => SUCCESS: All tests pass! Sponsored-by: DoverNet Sponsored-by: South-East Kansas Library System Sponsored-by: SWITCH Library Consortium Signed-off-by: Nick Clemens <nick@bywatersolutions.com> Created attachment 52661 [details] [review] Bug 14048: Add a refund rules setting form in smart-rules This patch introduces the CRUD UI for managing the refund lost item fee rules on the general 'Circulation and fines rules' page. Verify that rules can be added and changed. Rules are correctly associated to the chosen branch (or the general default rule). To test: - Apply the patch - Open smart-rules.pl => SUCCESS: The text and refund rules section correctly refers to default global rules. The policy can be set to Yes and No, and works as expected. - Pick a specific branch => SUCCESS: The rule refers to the specific branch => SUCCESS: It detects the default rule is picked => SUCCESS: On changing the rules values are correctly retrieved if entering again to the page. => SUCCESS: The 'Use default' text is suffixed with the correct value set by the user in the 'All libraries' scenario. => Sign off :-D Sponsored-by: DoverNet Sponsored-by: South-East Kansas Library System Sponsored-by: SWITCH Library Consortium Signed-off-by: Nick Clemens <nick@bywatersolutions.com> Created attachment 52662 [details] [review] Bug 14048: Hook new rules into C4::Circulation This patch makes AddIssue and AddReturn use the new implementation The behaviour should respect the specification. Sponsored-by: DoverNet Sponsored-by: South-East Kansas Library System Sponsored-by: SWITCH Library Consortium Signed-off-by: Nick Clemens <nick@bywatersolutions.com> Created attachment 52663 [details] [review] Bug 14048: [QA Followup] Signed-off-by: Nick Clemens <nick@bywatersolutions.com> Created attachment 52664 [details] [review] Bug 14048: Add syspref and atomic update This patch introduces the changes to the sysprefs and the atomic update that creates the new table. It picks the syspref value for RefundLostItemFeeOnReturn and uses it to create a default (branchcode='*') rule on the new table. Sponsored-by: DoverNet Sponsored-by: South-East Kansas Library System Sponsored-by: SWITCH Library Consortium Signed-off-by: Nick Clemens <nick@bywatersolutions.com> Created attachment 52665 [details] [review] Bug 14048 [QA Followup] - Delete only the removed system preference Signed-off-by: Nick Clemens <nick@bywatersolutions.com> Created attachment 52666 [details] [review] Bug 14048: (QA followup) Update syspref name for HEA Signed-off-by: Nick Clemens <nick@bywatersolutions.com> Created attachment 52667 [details] [review] Bug 14048: (QA followup) Fix typo on syspref value Signed-off-by: Nick Clemens <nick@bywatersolutions.com> Created attachment 52668 [details] [review] Bug 14048: (followup) Use the original holding branch This patch picks the item's holding branch *before* it gets fixed by using the checkin library instead. This way the RefundLostOnReturnControl syspref set to ItemHoldingBranch is respected (otherwise, as Nick explained this behaves just like if CheckinLibrary was set) Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io> Signed-off-by: Nick Clemens <nick@bywatersolutions.com> Created attachment 52748 [details] [review] Bug 14048: Add RefundLostItemFeeRule table and classes This patch introduces new classes for handling refund lost item fee rules. It introduces a new table for storing this rules. It is designed so it is possible to define a global rule, and then branch-specific ones. The specific is prefered if available. This behaviour is fully tested by unit tests introduced by the following patches. This cannot be tested on its own. Sponsored-by: DoverNet Sponsored-by: South-East Kansas Library System Sponsored-by: SWITCH Library Consortium Signed-off-by: Nick Clemens <nick@bywatersolutions.com> Signed-off-by: Jason Robb <jrobb@sekls.org> Signed-off-by: Jennifer Schmidt <jschmidt@switchinc.org> Signed-off-by: Margaret Thrasher <p.thrasher@dover.nh.gov> Created attachment 52749 [details] [review] Bug 14048: Unit tests This patch introduces unit tests for the feature. To test: - Have all patches from this bug applied - Run: $ prove -v t/db_dependent/RefundLostItemFeeRule.t => SUCCESS: All tests pass! Sponsored-by: DoverNet Sponsored-by: South-East Kansas Library System Sponsored-by: SWITCH Library Consortium Signed-off-by: Nick Clemens <nick@bywatersolutions.com> Signed-off-by: Jason Robb <jrobb@sekls.org> Signed-off-by: Jennifer Schmidt <jschmidt@switchinc.org> Signed-off-by: Margaret Thrasher <p.thrasher@dover.nh.gov> Created attachment 52750 [details] [review] Bug 14048: Add a refund rules setting form in smart-rules This patch introduces the CRUD UI for managing the refund lost item fee rules on the general 'Circulation and fines rules' page. Verify that rules can be added and changed. Rules are correctly associated to the chosen branch (or the general default rule). To test: - Apply the patch - Open smart-rules.pl => SUCCESS: The text and refund rules section correctly refers to default global rules. The policy can be set to Yes and No, and works as expected. - Pick a specific branch => SUCCESS: The rule refers to the specific branch => SUCCESS: It detects the default rule is picked => SUCCESS: On changing the rules values are correctly retrieved if entering again to the page. => SUCCESS: The 'Use default' text is suffixed with the correct value set by the user in the 'All libraries' scenario. => Sign off :-D Sponsored-by: DoverNet Sponsored-by: South-East Kansas Library System Sponsored-by: SWITCH Library Consortium Signed-off-by: Nick Clemens <nick@bywatersolutions.com> Signed-off-by: Jason Robb <jrobb@sekls.org> Signed-off-by: Jennifer Schmidt <jschmidt@switchinc.org> Signed-off-by: Margaret Thrasher <p.thrasher@dover.nh.gov> Created attachment 52751 [details] [review] Bug 14048: Hook new rules into C4::Circulation This patch makes AddIssue and AddReturn use the new implementation The behaviour should respect the specification. Sponsored-by: DoverNet Sponsored-by: South-East Kansas Library System Sponsored-by: SWITCH Library Consortium Signed-off-by: Nick Clemens <nick@bywatersolutions.com> Signed-off-by: Jason Robb <jrobb@sekls.org> Signed-off-by: Jennifer Schmidt <jschmidt@switchinc.org> Signed-off-by: Margaret Thrasher <p.thrasher@dover.nh.gov> Created attachment 52752 [details] [review] Bug 14048: [QA Followup] Signed-off-by: Nick Clemens <nick@bywatersolutions.com> Signed-off-by: Jason Robb <jrobb@sekls.org> Signed-off-by: Jennifer Schmidt <jschmidt@switchinc.org> Signed-off-by: Margaret Thrasher <p.thrasher@dover.nh.gov> Created attachment 52753 [details] [review] Bug 14048: Add syspref and atomic update This patch introduces the changes to the sysprefs and the atomic update that creates the new table. It picks the syspref value for RefundLostItemFeeOnReturn and uses it to create a default (branchcode='*') rule on the new table. Sponsored-by: DoverNet Sponsored-by: South-East Kansas Library System Sponsored-by: SWITCH Library Consortium Signed-off-by: Nick Clemens <nick@bywatersolutions.com> Signed-off-by: Jason Robb <jrobb@sekls.org> Signed-off-by: Jennifer Schmidt <jschmidt@switchinc.org> Signed-off-by: Margaret Thrasher <p.thrasher@dover.nh.gov> Created attachment 52754 [details] [review] Bug 14048 [QA Followup] - Delete only the removed system preference Signed-off-by: Nick Clemens <nick@bywatersolutions.com> Signed-off-by: Jason Robb <jrobb@sekls.org> Signed-off-by: Jennifer Schmidt <jschmidt@switchinc.org> Signed-off-by: Margaret Thrasher <p.thrasher@dover.nh.gov> Created attachment 52755 [details] [review] Bug 14048: (QA followup) Update syspref name for HEA Signed-off-by: Nick Clemens <nick@bywatersolutions.com> Signed-off-by: Jason Robb <jrobb@sekls.org> Signed-off-by: Jennifer Schmidt <jschmidt@switchinc.org> Signed-off-by: Margaret Thrasher <p.thrasher@dover.nh.gov> Created attachment 52756 [details] [review] Bug 14048: (QA followup) Fix typo on syspref value Signed-off-by: Nick Clemens <nick@bywatersolutions.com> Signed-off-by: Jason Robb <jrobb@sekls.org> Signed-off-by: Jennifer Schmidt <jschmidt@switchinc.org> Signed-off-by: Margaret Thrasher <p.thrasher@dover.nh.gov> Created attachment 52757 [details] [review] Bug 14048: (followup) Use the original holding branch This patch picks the item's holding branch *before* it gets fixed by using the checkin library instead. This way the RefundLostOnReturnControl syspref set to ItemHoldingBranch is respected (otherwise, as Nick explained this behaves just like if CheckinLibrary was set) Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io> Signed-off-by: Nick Clemens <nick@bywatersolutions.com> Signed-off-by: Jason Robb <jrobb@sekls.org> Signed-off-by: Jennifer Schmidt <jschmidt@switchinc.org> Signed-off-by: Margaret Thrasher <p.thrasher@dover.nh.gov> Applying: Bug 14048: Hook new rules into C4::Circulation Using index info to reconstruct a base tree... M C4/Circulation.pm Falling back to patching base and 3-way merge... Auto-merging C4/Circulation.pm CONFLICT (content): Merge conflict in C4/Circulation.pm Created attachment 52835 [details] [review] Bug 14048: Add RefundLostItemFeeRule table and classes This patch introduces new classes for handling refund lost item fee rules. It introduces a new table for storing this rules. It is designed so it is possible to define a global rule, and then branch-specific ones. The specific is prefered if available. This behaviour is fully tested by unit tests introduced by the following patches. This cannot be tested on its own. Sponsored-by: DoverNet Sponsored-by: South-East Kansas Library System Sponsored-by: SWITCH Library Consortium Signed-off-by: Nick Clemens <nick@bywatersolutions.com> Signed-off-by: Jason Robb <jrobb@sekls.org> Signed-off-by: Jennifer Schmidt <jschmidt@switchinc.org> Signed-off-by: Margaret Thrasher <p.thrasher@dover.nh.gov> Created attachment 52836 [details] [review] Bug 14048: Unit tests This patch introduces unit tests for the feature. To test: - Have all patches from this bug applied - Run: $ prove -v t/db_dependent/RefundLostItemFeeRule.t => SUCCESS: All tests pass! Sponsored-by: DoverNet Sponsored-by: South-East Kansas Library System Sponsored-by: SWITCH Library Consortium Signed-off-by: Nick Clemens <nick@bywatersolutions.com> Signed-off-by: Jason Robb <jrobb@sekls.org> Signed-off-by: Jennifer Schmidt <jschmidt@switchinc.org> Signed-off-by: Margaret Thrasher <p.thrasher@dover.nh.gov> Created attachment 52837 [details] [review] Bug 14048: Add a refund rules setting form in smart-rules This patch introduces the CRUD UI for managing the refund lost item fee rules on the general 'Circulation and fines rules' page. Verify that rules can be added and changed. Rules are correctly associated to the chosen branch (or the general default rule). To test: - Apply the patch - Open smart-rules.pl => SUCCESS: The text and refund rules section correctly refers to default global rules. The policy can be set to Yes and No, and works as expected. - Pick a specific branch => SUCCESS: The rule refers to the specific branch => SUCCESS: It detects the default rule is picked => SUCCESS: On changing the rules values are correctly retrieved if entering again to the page. => SUCCESS: The 'Use default' text is suffixed with the correct value set by the user in the 'All libraries' scenario. => Sign off :-D Sponsored-by: DoverNet Sponsored-by: South-East Kansas Library System Sponsored-by: SWITCH Library Consortium Signed-off-by: Nick Clemens <nick@bywatersolutions.com> Signed-off-by: Jason Robb <jrobb@sekls.org> Signed-off-by: Jennifer Schmidt <jschmidt@switchinc.org> Signed-off-by: Margaret Thrasher <p.thrasher@dover.nh.gov> Created attachment 52838 [details] [review] Bug 14048: Hook new rules into C4::Circulation This patch makes AddIssue and AddReturn use the new implementation The behaviour should respect the specification. Sponsored-by: DoverNet Sponsored-by: South-East Kansas Library System Sponsored-by: SWITCH Library Consortium Signed-off-by: Nick Clemens <nick@bywatersolutions.com> Signed-off-by: Jason Robb <jrobb@sekls.org> Signed-off-by: Jennifer Schmidt <jschmidt@switchinc.org> Signed-off-by: Margaret Thrasher <p.thrasher@dover.nh.gov> Created attachment 52839 [details] [review] Bug 14048: [QA Followup] Signed-off-by: Nick Clemens <nick@bywatersolutions.com> Signed-off-by: Jason Robb <jrobb@sekls.org> Signed-off-by: Jennifer Schmidt <jschmidt@switchinc.org> Signed-off-by: Margaret Thrasher <p.thrasher@dover.nh.gov> Created attachment 52840 [details] [review] Bug 14048: Add syspref and atomic update This patch introduces the changes to the sysprefs and the atomic update that creates the new table. It picks the syspref value for RefundLostItemFeeOnReturn and uses it to create a default (branchcode='*') rule on the new table. Sponsored-by: DoverNet Sponsored-by: South-East Kansas Library System Sponsored-by: SWITCH Library Consortium Signed-off-by: Nick Clemens <nick@bywatersolutions.com> Signed-off-by: Jason Robb <jrobb@sekls.org> Signed-off-by: Jennifer Schmidt <jschmidt@switchinc.org> Signed-off-by: Margaret Thrasher <p.thrasher@dover.nh.gov> Created attachment 52841 [details] [review] Bug 14048 [QA Followup] - Delete only the removed system preference Signed-off-by: Nick Clemens <nick@bywatersolutions.com> Signed-off-by: Jason Robb <jrobb@sekls.org> Signed-off-by: Jennifer Schmidt <jschmidt@switchinc.org> Signed-off-by: Margaret Thrasher <p.thrasher@dover.nh.gov> Created attachment 52842 [details] [review] Bug 14048: (QA followup) Update syspref name for HEA Signed-off-by: Nick Clemens <nick@bywatersolutions.com> Signed-off-by: Jason Robb <jrobb@sekls.org> Signed-off-by: Jennifer Schmidt <jschmidt@switchinc.org> Signed-off-by: Margaret Thrasher <p.thrasher@dover.nh.gov> Created attachment 52843 [details] [review] Bug 14048: (QA followup) Fix typo on syspref value Signed-off-by: Nick Clemens <nick@bywatersolutions.com> Signed-off-by: Jason Robb <jrobb@sekls.org> Signed-off-by: Jennifer Schmidt <jschmidt@switchinc.org> Signed-off-by: Margaret Thrasher <p.thrasher@dover.nh.gov> Created attachment 52844 [details] [review] Bug 14048: (followup) Use the original holding branch This patch picks the item's holding branch *before* it gets fixed by using the checkin library instead. This way the RefundLostOnReturnControl syspref set to ItemHoldingBranch is respected (otherwise, as Nick explained this behaves just like if CheckinLibrary was set) Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io> Signed-off-by: Nick Clemens <nick@bywatersolutions.com> Signed-off-by: Jason Robb <jrobb@sekls.org> Signed-off-by: Jennifer Schmidt <jschmidt@switchinc.org> Signed-off-by: Margaret Thrasher <p.thrasher@dover.nh.gov> Patches rebased. I have squashed the patches for the review, so I won't use the splinter: 1/ my $item_holding_branch = $item->{ holdingbranch }; => would be good to have a comment here to explain the last patch. 2/ Koha::Config::SysPrefs->find should not be used, C4::Context->preference should continue to be used everywhere, to take advantage of the syspref cache. 3/ $schema->resultset('RefundLostItemFeeRule')->search()->delete; should use Koha::RefundLostItemFeeRules 4/ Tests only cover RefundLostOnReturnControl = 'CheckinLibrary' (In reply to Jonathan Druart from comment #72) > I have squashed the patches for the review, so I won't use the splinter: > > 1/ my $item_holding_branch = $item->{ holdingbranch }; > => would be good to have a comment here to explain the last patch. I thought the commit message would be enough. Do u require a comment on the code? > 2/ Koha::Config::SysPrefs->find should not be used, C4::Context->preference > should continue to be used everywhere, to take advantage of the syspref > cache. Ok, followup coming. > 3/ $schema->resultset('RefundLostItemFeeRule')->search()->delete; > should use Koha::RefundLostItemFeeRules This is done on the tests, instead of the usual DELETE FROM ... The problem with Koha::RefundLostItemFeeRules is that it explicitly forbis deleting the default rule. I'd leave it as it is. > 4/ Tests only cover RefundLostOnReturnControl = 'CheckinLibrary' This is incorrect: subtest 'Koha::RefundLostItemFeeRules::_choose_branch() tests' => Tries the three possible values subtest 'Koha::RefundLostItemFeeRules::should_refund() tests' => Tries the three possible values Created attachment 52876 [details] [review] Bug 14048: (QA folloup) C4::Context->preference should be used Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io> Created attachment 52883 [details] [review] Bug 14048: Use mock_preference in tests (In reply to Tomás Cohen Arazi from comment #73) > (In reply to Jonathan Druart from comment #72) > > I have squashed the patches for the review, so I won't use the splinter: > > > > 1/ my $item_holding_branch = $item->{ holdingbranch }; > > => would be good to have a comment here to explain the last patch. > > I thought the commit message would be enough. Do u require a comment on the > code? Would be good, but not required. Forget it > > 2/ Koha::Config::SysPrefs->find should not be used, C4::Context->preference > > should continue to be used everywhere, to take advantage of the syspref > > cache. > > Ok, followup coming. I added one also to fix the tests. > > 3/ $schema->resultset('RefundLostItemFeeRule')->search()->delete; > > should use Koha::RefundLostItemFeeRules > > This is done on the tests, instead of the usual DELETE FROM ... The problem > with Koha::RefundLostItemFeeRules is that it explicitly forbis deleting the > default rule. I'd leave it as it is. Correct, and I was wondering if it would not be easier to assume that default is 1 if no rule exist. It will permit to remove the sql file in the mysql/mandatory dir (and then no need to manage it from the installer) and no need to overwrite delete. Why do you think? > > 4/ Tests only cover RefundLostOnReturnControl = 'CheckinLibrary' > > This is incorrect: > > subtest 'Koha::RefundLostItemFeeRules::_choose_branch() tests' > => Tries the three possible values > > subtest 'Koha::RefundLostItemFeeRules::should_refund() tests' > => Tries the three possible values I have a small request, could we move Koha::RefundLostItemFeeRule to a more refined namespace? I think it would be better long term. I'm thinking Koha::Circulation::Rules::LostItemFeeRule but am open to other suggestions. I'd also be happy to write the followup for that myself if you find that acceptable. Created attachment 52894 [details] [review] Bug 14048: (QA followup) Default to 1 if no default rule This patch answers Jonathan's request to simplify this patchset a bit. It removes the need for the .sql file, as _default_rule now defaults to a fixed value (1, which was set by the removed .sql file). This allowed to remove the overloaded ->delete method. The tests have been adjusted to reflect this changes, including tests for the new 'default'-if-absent situation. Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io> (In reply to Kyle M Hall from comment #77) > I have a small request, could we move Koha::RefundLostItemFeeRule to a more > refined namespace? I think it would be better long term. I'm thinking > Koha::Circulation::Rules::LostItemFeeRule but am open to other suggestions. > I'd also be happy to write the followup for that myself if you find that > acceptable. Kyle, I don't have a strong opinion on this. I think you can write that followup if this passes QA. Could I get this follow-up before passing it? As Kyle suggest, it may be better to use Koha::Circulation::Rules::LostItemFeeRule instead of Koha::Circulation::Rules::RefundLostItemFeeRule but in that case, should not we rename the table too? And actually should not we rename it to something less specific, with a column named refund_lost_item_fee? (In reply to Jonathan Druart from comment #80) > Could I get this follow-up before passing it? > As Kyle suggest, it may be better to use > Koha::Circulation::Rules::LostItemFeeRule instead of > Koha::Circulation::Rules::RefundLostItemFeeRule but in that case, should not > we rename the table too? I'd rather pick Koha::Circulation::Rules::RefundLostItemFee > And actually should not we rename it to something less specific, with a > column named refund_lost_item_fee? Pianohacker is working on a circulation rules refactoring. I'm pretty sure some global re-engineering is needed to have a better solution. So I'd avoid making this bug responsible for such thing. (In reply to Tomás Cohen Arazi from comment #81) > (In reply to Jonathan Druart from comment #80) > > Could I get this follow-up before passing it? > > As Kyle suggest, it may be better to use > > Koha::Circulation::Rules::LostItemFeeRule instead of > > Koha::Circulation::Rules::RefundLostItemFeeRule but in that case, should not > > we rename the table too? > > I'd rather pick Koha::Circulation::Rules::RefundLostItemFee Sounds good! I'll get that followup written asap! > > > And actually should not we rename it to something less specific, with a > > column named refund_lost_item_fee? > > Pianohacker is working on a circulation rules refactoring. I'm pretty sure > some global re-engineering is needed to have a better solution. So I'd avoid > making this bug responsible for such thing. Agreed. The circ rules revamp will make merge all these tables together, so I see little need long term for an additional complication. I think this is fine for the time being. Created attachment 53017 [details] [review] Bug 14048: Add RefundLostItemFeeRule table and classes This patch introduces new classes for handling refund lost item fee rules. It introduces a new table for storing this rules. It is designed so it is possible to define a global rule, and then branch-specific ones. The specific is prefered if available. This behaviour is fully tested by unit tests introduced by the following patches. This cannot be tested on its own. Sponsored-by: DoverNet Sponsored-by: South-East Kansas Library System Sponsored-by: SWITCH Library Consortium Signed-off-by: Nick Clemens <nick@bywatersolutions.com> Signed-off-by: Jason Robb <jrobb@sekls.org> Signed-off-by: Jennifer Schmidt <jschmidt@switchinc.org> Signed-off-by: Margaret Thrasher <p.thrasher@dover.nh.gov> Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org> Created attachment 53018 [details] [review] Bug 14048: Unit tests This patch introduces unit tests for the feature. To test: - Have all patches from this bug applied - Run: $ prove -v t/db_dependent/RefundLostItemFeeRule.t => SUCCESS: All tests pass! Sponsored-by: DoverNet Sponsored-by: South-East Kansas Library System Sponsored-by: SWITCH Library Consortium Signed-off-by: Nick Clemens <nick@bywatersolutions.com> Signed-off-by: Jason Robb <jrobb@sekls.org> Signed-off-by: Jennifer Schmidt <jschmidt@switchinc.org> Signed-off-by: Margaret Thrasher <p.thrasher@dover.nh.gov> Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org> Created attachment 53019 [details] [review] Bug 14048: Add a refund rules setting form in smart-rules This patch introduces the CRUD UI for managing the refund lost item fee rules on the general 'Circulation and fines rules' page. Verify that rules can be added and changed. Rules are correctly associated to the chosen branch (or the general default rule). To test: - Apply the patch - Open smart-rules.pl => SUCCESS: The text and refund rules section correctly refers to default global rules. The policy can be set to Yes and No, and works as expected. - Pick a specific branch => SUCCESS: The rule refers to the specific branch => SUCCESS: It detects the default rule is picked => SUCCESS: On changing the rules values are correctly retrieved if entering again to the page. => SUCCESS: The 'Use default' text is suffixed with the correct value set by the user in the 'All libraries' scenario. => Sign off :-D Sponsored-by: DoverNet Sponsored-by: South-East Kansas Library System Sponsored-by: SWITCH Library Consortium Signed-off-by: Nick Clemens <nick@bywatersolutions.com> Signed-off-by: Jason Robb <jrobb@sekls.org> Signed-off-by: Jennifer Schmidt <jschmidt@switchinc.org> Signed-off-by: Margaret Thrasher <p.thrasher@dover.nh.gov> Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org> Created attachment 53020 [details] [review] Bug 14048: Hook new rules into C4::Circulation This patch makes AddIssue and AddReturn use the new implementation The behaviour should respect the specification. Sponsored-by: DoverNet Sponsored-by: South-East Kansas Library System Sponsored-by: SWITCH Library Consortium Signed-off-by: Nick Clemens <nick@bywatersolutions.com> Signed-off-by: Jason Robb <jrobb@sekls.org> Signed-off-by: Jennifer Schmidt <jschmidt@switchinc.org> Signed-off-by: Margaret Thrasher <p.thrasher@dover.nh.gov> Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org> Created attachment 53021 [details] [review] Bug 14048: [QA Followup] Signed-off-by: Nick Clemens <nick@bywatersolutions.com> Signed-off-by: Jason Robb <jrobb@sekls.org> Signed-off-by: Jennifer Schmidt <jschmidt@switchinc.org> Signed-off-by: Margaret Thrasher <p.thrasher@dover.nh.gov> Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org> Created attachment 53022 [details] [review] Bug 14048: Add syspref and atomic update This patch introduces the changes to the sysprefs and the atomic update that creates the new table. It picks the syspref value for RefundLostItemFeeOnReturn and uses it to create a default (branchcode='*') rule on the new table. Sponsored-by: DoverNet Sponsored-by: South-East Kansas Library System Sponsored-by: SWITCH Library Consortium Signed-off-by: Nick Clemens <nick@bywatersolutions.com> Signed-off-by: Jason Robb <jrobb@sekls.org> Signed-off-by: Jennifer Schmidt <jschmidt@switchinc.org> Signed-off-by: Margaret Thrasher <p.thrasher@dover.nh.gov> Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org> Created attachment 53023 [details] [review] Bug 14048 [QA Followup] - Delete only the removed system preference Signed-off-by: Nick Clemens <nick@bywatersolutions.com> Signed-off-by: Jason Robb <jrobb@sekls.org> Signed-off-by: Jennifer Schmidt <jschmidt@switchinc.org> Signed-off-by: Margaret Thrasher <p.thrasher@dover.nh.gov> Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org> Created attachment 53024 [details] [review] Bug 14048: (QA followup) Update syspref name for HEA Signed-off-by: Nick Clemens <nick@bywatersolutions.com> Signed-off-by: Jason Robb <jrobb@sekls.org> Signed-off-by: Jennifer Schmidt <jschmidt@switchinc.org> Signed-off-by: Margaret Thrasher <p.thrasher@dover.nh.gov> Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org> Created attachment 53025 [details] [review] Bug 14048: (QA followup) Fix typo on syspref value Signed-off-by: Nick Clemens <nick@bywatersolutions.com> Signed-off-by: Jason Robb <jrobb@sekls.org> Signed-off-by: Jennifer Schmidt <jschmidt@switchinc.org> Signed-off-by: Margaret Thrasher <p.thrasher@dover.nh.gov> Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org> Created attachment 53026 [details] [review] Bug 14048: (followup) Use the original holding branch This patch picks the item's holding branch *before* it gets fixed by using the checkin library instead. This way the RefundLostOnReturnControl syspref set to ItemHoldingBranch is respected (otherwise, as Nick explained this behaves just like if CheckinLibrary was set) Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io> Signed-off-by: Nick Clemens <nick@bywatersolutions.com> Signed-off-by: Jason Robb <jrobb@sekls.org> Signed-off-by: Jennifer Schmidt <jschmidt@switchinc.org> Signed-off-by: Margaret Thrasher <p.thrasher@dover.nh.gov> Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org> Created attachment 53027 [details] [review] Bug 14048: (QA folloup) C4::Context->preference should be used Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io> Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org> Created attachment 53028 [details] [review] Bug 14048: Use mock_preference in tests Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org> Created attachment 53029 [details] [review] Bug 14048: (QA followup) Default to 1 if no default rule This patch answers Jonathan's request to simplify this patchset a bit. It removes the need for the .sql file, as _default_rule now defaults to a fixed value (1, which was set by the removed .sql file). This allowed to remove the overloaded ->delete method. The tests have been adjusted to reflect this changes, including tests for the new 'default'-if-absent situation. Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io> Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org> Pushed to master for the 16.11 release, thanks for your work! There is one occurrence left: C4/Installer.pm: push @fnames, C4::Context->config('intranetdir') . "/installer/data/mysql/mandatory/refund_lost_item_fee_rules.sql"; Please fix! (In reply to Jonathan Druart from comment #97) > There is one occurrence left: > C4/Installer.pm: push @fnames, C4::Context->config('intranetdir') . > "/installer/data/mysql/mandatory/refund_lost_item_fee_rules.sql"; > > Please fix! See bug 21654, thanks! |