Bug 14048 - Change RefundLostItemFeeOnReturn to be branch specific
Summary: Change RefundLostItemFeeOnReturn to be branch specific
Status: CLOSED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: Circulation (show other bugs)
Version: Main
Hardware: All All
: P5 - low enhancement (vote)
Assignee: Tomás Cohen Arazi
QA Contact: Jonathan Druart
URL:
Keywords:
Depends on:
Blocks: 21654
  Show dependency treegraph
 
Reported: 2015-04-22 15:49 UTC by Kyle M Hall
Modified: 2019-05-24 20:47 UTC (History)
13 users (show)

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


Attachments
Bug 14048: Add RefundLostItemFeeRule table and classes (9.75 KB, patch)
2016-06-03 14:55 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 14048: Unit tests (10.95 KB, patch)
2016-06-03 14:55 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 14048: Add syspref and atomic update (5.06 KB, patch)
2016-06-03 14:55 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 14048: Add a refund rules setting form in smart-rules (5.46 KB, patch)
2016-06-03 14:55 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 14048: Hook new rules into C4::Circulation (2.55 KB, patch)
2016-06-03 14:55 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 14048: (QA followup) Rules should be defined for the chosen branch (or all) (9.41 KB, patch)
2016-06-07 19:08 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 14048: (followup) move atomicupdate to SQL-based (4.78 KB, patch)
2016-06-07 19:11 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 14048 [QA Followup] (3.54 KB, patch)
2016-06-09 16:48 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 14048: [QA Followup] (2.82 KB, patch)
2016-06-09 18:39 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 14048: Add RefundLostItemFeeRule table and classes (10.90 KB, patch)
2016-06-12 12:51 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 14048: Unit tests (10.26 KB, patch)
2016-06-12 12:51 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 14048: Add syspref and atomic update (5.94 KB, patch)
2016-06-12 12:51 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 14048: Add a refund rules setting form in smart-rules (7.30 KB, patch)
2016-06-12 12:52 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 14048: Hook new rules into C4::Circulation (2.55 KB, patch)
2016-06-12 12:52 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 14048: [QA Followup] (2.61 KB, patch)
2016-06-12 12:52 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 14048: (QA followup) Fix for TooMany.t (694 bytes, patch)
2016-06-12 12:52 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 14048: Add syspref and atomic update (5.87 KB, patch)
2016-06-12 12:55 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 14048 [QA Followup] - Delete only the removed system preference (1.12 KB, patch)
2016-06-13 11:18 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 14048: Add RefundLostItemFeeRule table and classes (10.90 KB, patch)
2016-06-15 19:07 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 14048: Unit tests (10.26 KB, patch)
2016-06-15 19:07 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 14048: Add a refund rules setting form in smart-rules (7.30 KB, patch)
2016-06-15 19:08 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 14048: Hook new rules into C4::Circulation (2.58 KB, patch)
2016-06-15 19:08 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 14048: [QA Followup] (2.61 KB, patch)
2016-06-15 19:08 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 14048: Add syspref and atomic update (5.87 KB, patch)
2016-06-15 19:08 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 14048 [QA Followup] - Delete only the removed system preference (1.12 KB, patch)
2016-06-15 19:08 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 14048: (QA followup) Update syspref name for HEA (1.22 KB, patch)
2016-06-15 19:19 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 14048: (QA followup) Fix typo on syspref value (3.45 KB, patch)
2016-06-17 13:40 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 14048: (followup) Use the original holding branch (1.59 KB, patch)
2016-06-21 18:22 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 14048: Add RefundLostItemFeeRule table and classes (10.95 KB, patch)
2016-06-21 18:29 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 14048: Unit tests (10.31 KB, patch)
2016-06-21 18:29 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 14048: Add a refund rules setting form in smart-rules (7.36 KB, patch)
2016-06-21 18:29 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 14048: Hook new rules into C4::Circulation (2.64 KB, patch)
2016-06-21 18:29 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 14048: [QA Followup] (2.67 KB, patch)
2016-06-21 18:29 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 14048: Add syspref and atomic update (5.93 KB, patch)
2016-06-21 18:29 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 14048 [QA Followup] - Delete only the removed system preference (1.17 KB, patch)
2016-06-21 18:29 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 14048: (QA followup) Update syspref name for HEA (1.27 KB, patch)
2016-06-21 18:29 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 14048: (QA followup) Fix typo on syspref value (3.50 KB, patch)
2016-06-21 18:29 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 14048: (followup) Use the original holding branch (1.65 KB, patch)
2016-06-21 18:29 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 14048: Add RefundLostItemFeeRule table and classes (11.11 KB, patch)
2016-06-23 16:55 UTC, Nicole C. Engard
Details | Diff | Splinter Review
Bug 14048: Unit tests (10.47 KB, patch)
2016-06-23 16:56 UTC, Nicole C. Engard
Details | Diff | Splinter Review
Bug 14048: Add a refund rules setting form in smart-rules (7.52 KB, patch)
2016-06-23 16:56 UTC, Nicole C. Engard
Details | Diff | Splinter Review
Bug 14048: Hook new rules into C4::Circulation (2.80 KB, patch)
2016-06-23 16:56 UTC, Nicole C. Engard
Details | Diff | Splinter Review
Bug 14048: [QA Followup] (2.83 KB, patch)
2016-06-23 16:56 UTC, Nicole C. Engard
Details | Diff | Splinter Review
Bug 14048: Add syspref and atomic update (6.08 KB, patch)
2016-06-23 16:56 UTC, Nicole C. Engard
Details | Diff | Splinter Review
Bug 14048 [QA Followup] - Delete only the removed system preference (1.33 KB, patch)
2016-06-23 16:57 UTC, Nicole C. Engard
Details | Diff | Splinter Review
Bug 14048: (QA followup) Update syspref name for HEA (1.43 KB, patch)
2016-06-23 16:57 UTC, Nicole C. Engard
Details | Diff | Splinter Review
Bug 14048: (QA followup) Fix typo on syspref value (3.66 KB, patch)
2016-06-23 16:57 UTC, Nicole C. Engard
Details | Diff | Splinter Review
Bug 14048: (followup) Use the original holding branch (1.81 KB, patch)
2016-06-23 16:57 UTC, Nicole C. Engard
Details | Diff | Splinter Review
Bug 14048: Add RefundLostItemFeeRule table and classes (11.11 KB, patch)
2016-06-24 16:18 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 14048: Unit tests (10.47 KB, patch)
2016-06-24 16:18 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 14048: Add a refund rules setting form in smart-rules (7.52 KB, patch)
2016-06-24 16:19 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 14048: Hook new rules into C4::Circulation (3.19 KB, patch)
2016-06-24 16:19 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 14048: [QA Followup] (3.16 KB, patch)
2016-06-24 16:19 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 14048: Add syspref and atomic update (6.08 KB, patch)
2016-06-24 16:19 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 14048 [QA Followup] - Delete only the removed system preference (1.33 KB, patch)
2016-06-24 16:19 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 14048: (QA followup) Update syspref name for HEA (1.43 KB, patch)
2016-06-24 16:19 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 14048: (QA followup) Fix typo on syspref value (3.66 KB, patch)
2016-06-24 16:19 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 14048: (followup) Use the original holding branch (1.80 KB, patch)
2016-06-24 16:19 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 14048: (QA folloup) C4::Context->preference should be used (1001 bytes, patch)
2016-06-25 15:37 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 14048: Use mock_preference in tests (4.21 KB, patch)
2016-06-26 07:23 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 14048: (QA followup) Default to 1 if no default rule (4.37 KB, patch)
2016-06-27 13:20 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 14048: Add RefundLostItemFeeRule table and classes (11.18 KB, patch)
2016-07-01 14:47 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 14048: Unit tests (10.54 KB, patch)
2016-07-01 14:47 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 14048: Add a refund rules setting form in smart-rules (7.59 KB, patch)
2016-07-01 14:47 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 14048: Hook new rules into C4::Circulation (3.26 KB, patch)
2016-07-01 14:47 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 14048: [QA Followup] (3.23 KB, patch)
2016-07-01 14:47 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 14048: Add syspref and atomic update (6.15 KB, patch)
2016-07-01 14:47 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 14048 [QA Followup] - Delete only the removed system preference (1.40 KB, patch)
2016-07-01 14:48 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 14048: (QA followup) Update syspref name for HEA (1.50 KB, patch)
2016-07-01 14:48 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 14048: (QA followup) Fix typo on syspref value (3.73 KB, patch)
2016-07-01 14:48 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 14048: (followup) Use the original holding branch (1.88 KB, patch)
2016-07-01 14:48 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 14048: (QA folloup) C4::Context->preference should be used (1.05 KB, patch)
2016-07-01 14:48 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 14048: Use mock_preference in tests (4.28 KB, patch)
2016-07-01 14:48 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 14048: (QA followup) Default to 1 if no default rule (4.44 KB, patch)
2016-07-01 14:48 UTC, Jonathan Druart
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Kyle M Hall 2015-04-22 15:49:50 UTC
Some library systems would like to be able to control the behavior of RefundLostItemFeeOnReturn

Expected outcome: To make the RefundLostItemFeeOnReturn system preference branch specific

Plan of Attack:
1) Add new field 'refund_lost_item_fee_on_return' to the 'branches' table
2) Pre-populate this field with the system preference value for RefundLostItemFeeOnReturn
3) Remove the system preference RefundLostItemFeeOnReturn
4) Replace uses of RefundLostItemFeeOnReturn with a lookup of the value of branches.refund_lost_item_fee_on_return
5) Use HomeOrHolding to control whether the home branch or the holding branch of the item is used to determine this.
6) Reveal the setting in the branches editor
Comment 1 Sheila Kearns 2015-05-11 19:35: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.
Comment 2 Jonathan Druart 2015-05-12 09:32:40 UTC
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).
Comment 3 Kyle M Hall 2015-05-12 17:55:23 UTC
(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.
Comment 4 Katrin Fischer 2015-05-12 19:46:42 UTC
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? :)
Comment 5 Kyle M Hall 2015-05-13 10:51:47 UTC
(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?
Comment 6 Kyle M Hall 2015-05-13 10:52:26 UTC
(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!
Comment 7 Tomás Cohen Arazi 2016-06-03 14:55:11 UTC Comment hidden (obsolete)
Comment 8 Tomás Cohen Arazi 2016-06-03 14:55:17 UTC Comment hidden (obsolete)
Comment 9 Tomás Cohen Arazi 2016-06-03 14:55:22 UTC Comment hidden (obsolete)
Comment 10 Tomás Cohen Arazi 2016-06-03 14:55:27 UTC Comment hidden (obsolete)
Comment 11 Tomás Cohen Arazi 2016-06-03 14:55:38 UTC Comment hidden (obsolete)
Comment 12 Tomás Cohen Arazi 2016-06-03 14:59:42 UTC
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!
Comment 13 Tomás Cohen Arazi 2016-06-07 19:08:32 UTC Comment hidden (obsolete)
Comment 14 Tomás Cohen Arazi 2016-06-07 19:11:15 UTC Comment hidden (obsolete)
Comment 15 Kyle M Hall 2016-06-09 16:48:28 UTC Comment hidden (obsolete)
Comment 16 Tomás Cohen Arazi 2016-06-09 18:39:12 UTC Comment hidden (obsolete)
Comment 17 Tomás Cohen Arazi 2016-06-12 12:51:47 UTC Comment hidden (obsolete)
Comment 18 Tomás Cohen Arazi 2016-06-12 12:51:52 UTC Comment hidden (obsolete)
Comment 19 Tomás Cohen Arazi 2016-06-12 12:51:58 UTC Comment hidden (obsolete)
Comment 20 Tomás Cohen Arazi 2016-06-12 12:52:07 UTC Comment hidden (obsolete)
Comment 21 Tomás Cohen Arazi 2016-06-12 12:52:13 UTC Comment hidden (obsolete)
Comment 22 Tomás Cohen Arazi 2016-06-12 12:52:19 UTC Comment hidden (obsolete)
Comment 23 Tomás Cohen Arazi 2016-06-12 12:52:25 UTC Comment hidden (obsolete)
Comment 24 Tomás Cohen Arazi 2016-06-12 12:55:36 UTC Comment hidden (obsolete)
Comment 25 Kyle M Hall 2016-06-13 11:18:05 UTC Comment hidden (obsolete)
Comment 26 Tomás Cohen Arazi 2016-06-13 12:35:45 UTC
(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!
Comment 27 Tomás Cohen Arazi 2016-06-15 19:07:50 UTC Comment hidden (obsolete)
Comment 28 Tomás Cohen Arazi 2016-06-15 19:07:56 UTC Comment hidden (obsolete)
Comment 29 Tomás Cohen Arazi 2016-06-15 19:08:01 UTC Comment hidden (obsolete)
Comment 30 Tomás Cohen Arazi 2016-06-15 19:08:06 UTC Comment hidden (obsolete)
Comment 31 Tomás Cohen Arazi 2016-06-15 19:08:11 UTC Comment hidden (obsolete)
Comment 32 Tomás Cohen Arazi 2016-06-15 19:08:16 UTC Comment hidden (obsolete)
Comment 33 Tomás Cohen Arazi 2016-06-15 19:08:21 UTC Comment hidden (obsolete)
Comment 34 Tomás Cohen Arazi 2016-06-15 19:19:14 UTC Comment hidden (obsolete)
Comment 35 Nick Clemens 2016-06-16 19:55:04 UTC
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
Comment 36 Tomás Cohen Arazi 2016-06-17 13:40:01 UTC Comment hidden (obsolete)
Comment 37 Tomás Cohen Arazi 2016-06-17 13:41:57 UTC
(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.
Comment 38 Nick Clemens 2016-06-18 00:33:29 UTC
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)
Comment 39 Tomás Cohen Arazi 2016-06-21 18:22:38 UTC Comment hidden (obsolete)
Comment 40 Nick Clemens 2016-06-21 18:29:09 UTC Comment hidden (obsolete)
Comment 41 Nick Clemens 2016-06-21 18:29:15 UTC Comment hidden (obsolete)
Comment 42 Nick Clemens 2016-06-21 18:29:20 UTC Comment hidden (obsolete)
Comment 43 Nick Clemens 2016-06-21 18:29:24 UTC Comment hidden (obsolete)
Comment 44 Nick Clemens 2016-06-21 18:29:29 UTC Comment hidden (obsolete)
Comment 45 Nick Clemens 2016-06-21 18:29:33 UTC Comment hidden (obsolete)
Comment 46 Nick Clemens 2016-06-21 18:29:40 UTC Comment hidden (obsolete)
Comment 47 Nick Clemens 2016-06-21 18:29:46 UTC Comment hidden (obsolete)
Comment 48 Nick Clemens 2016-06-21 18:29:51 UTC Comment hidden (obsolete)
Comment 49 Nick Clemens 2016-06-21 18:29:55 UTC Comment hidden (obsolete)
Comment 50 Nicole C. Engard 2016-06-23 16:55:52 UTC Comment hidden (obsolete)
Comment 51 Nicole C. Engard 2016-06-23 16:56:16 UTC Comment hidden (obsolete)
Comment 52 Nicole C. Engard 2016-06-23 16:56:21 UTC Comment hidden (obsolete)
Comment 53 Nicole C. Engard 2016-06-23 16:56:30 UTC Comment hidden (obsolete)
Comment 54 Nicole C. Engard 2016-06-23 16:56:37 UTC Comment hidden (obsolete)
Comment 55 Nicole C. Engard 2016-06-23 16:56:57 UTC Comment hidden (obsolete)
Comment 56 Nicole C. Engard 2016-06-23 16:57:07 UTC Comment hidden (obsolete)
Comment 57 Nicole C. Engard 2016-06-23 16:57:15 UTC Comment hidden (obsolete)
Comment 58 Nicole C. Engard 2016-06-23 16:57:21 UTC Comment hidden (obsolete)
Comment 59 Nicole C. Engard 2016-06-23 16:57:29 UTC Comment hidden (obsolete)
Comment 60 Jonathan Druart 2016-06-24 15:43:57 UTC
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
Comment 61 Tomás Cohen Arazi 2016-06-24 16:18:36 UTC Comment hidden (obsolete)
Comment 62 Tomás Cohen Arazi 2016-06-24 16:18:46 UTC Comment hidden (obsolete)
Comment 63 Tomás Cohen Arazi 2016-06-24 16:19:02 UTC Comment hidden (obsolete)
Comment 64 Tomás Cohen Arazi 2016-06-24 16:19:11 UTC Comment hidden (obsolete)
Comment 65 Tomás Cohen Arazi 2016-06-24 16:19:18 UTC Comment hidden (obsolete)
Comment 66 Tomás Cohen Arazi 2016-06-24 16:19:26 UTC Comment hidden (obsolete)
Comment 67 Tomás Cohen Arazi 2016-06-24 16:19:35 UTC Comment hidden (obsolete)
Comment 68 Tomás Cohen Arazi 2016-06-24 16:19:42 UTC Comment hidden (obsolete)
Comment 69 Tomás Cohen Arazi 2016-06-24 16:19:50 UTC Comment hidden (obsolete)
Comment 70 Tomás Cohen Arazi 2016-06-24 16:19:58 UTC Comment hidden (obsolete)
Comment 71 Tomás Cohen Arazi 2016-06-24 16:20:39 UTC
Patches rebased.
Comment 72 Jonathan Druart 2016-06-25 13:34:40 UTC
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'
Comment 73 Tomás Cohen Arazi 2016-06-25 15:35:54 UTC
(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
Comment 74 Tomás Cohen Arazi 2016-06-25 15:37:37 UTC Comment hidden (obsolete)
Comment 75 Jonathan Druart 2016-06-26 07:23:39 UTC Comment hidden (obsolete)
Comment 76 Jonathan Druart 2016-06-26 07:28:42 UTC
(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
Comment 77 Kyle M Hall 2016-06-27 13:09:53 UTC
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.
Comment 78 Tomás Cohen Arazi 2016-06-27 13:20:19 UTC Comment hidden (obsolete)
Comment 79 Tomás Cohen Arazi 2016-06-27 13:26:24 UTC
(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.
Comment 80 Jonathan Druart 2016-06-28 13:55:12 UTC
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?
Comment 81 Tomás Cohen Arazi 2016-06-28 14:12:16 UTC
(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.
Comment 82 Kyle M Hall 2016-06-28 23:53:19 UTC
(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.
Comment 83 Jonathan Druart 2016-07-01 14:47:19 UTC
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>
Comment 84 Jonathan Druart 2016-07-01 14:47:29 UTC
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>
Comment 85 Jonathan Druart 2016-07-01 14:47:38 UTC
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>
Comment 86 Jonathan Druart 2016-07-01 14:47:46 UTC
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>
Comment 87 Jonathan Druart 2016-07-01 14:47:53 UTC
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>
Comment 88 Jonathan Druart 2016-07-01 14:47:59 UTC
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>
Comment 89 Jonathan Druart 2016-07-01 14:48:05 UTC
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>
Comment 90 Jonathan Druart 2016-07-01 14:48:11 UTC
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>
Comment 91 Jonathan Druart 2016-07-01 14:48:19 UTC
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>
Comment 92 Jonathan Druart 2016-07-01 14:48:25 UTC
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>
Comment 93 Jonathan Druart 2016-07-01 14:48:32 UTC
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>
Comment 94 Jonathan Druart 2016-07-01 14:48:39 UTC
Created attachment 53028 [details] [review]
Bug 14048: Use mock_preference in tests

Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>
Comment 95 Jonathan Druart 2016-07-01 14:48:46 UTC
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>
Comment 96 Jesse Weaver 2016-07-07 19:36:27 UTC
Pushed to master for the 16.11 release, thanks for your work!
Comment 97 Jonathan Druart 2016-11-15 13:12:28 UTC
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!
Comment 98 Jonathan Druart 2018-10-24 22:49:30 UTC
(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!