Description
Hugo Agud
2017-12-13 09:15:44 UTC
Created attachment 69783 [details] [review] Bug 19804: Add new DB column issuingrules.suspension_chargeperiod Created attachment 69784 [details] [review] Bug 19804: DBIC Schema changes Created attachment 69785 [details] [review] Bug 19804: Add a 'Fine charging interval' for suspension days We already have a chargeperiod (Fine charging interval) value which is taken into account for fine ($) for not for the suspension period. This patch adds a new column suspension_chargeperiod (Fine day charging interval) to add the same behaviour when a suspension is calculated. Test plan: Add overdue item and play with the circulation rules (and the calendar). The suspension period must be correctly calculated. Please provide the different tests you made. Created attachment 69786 [details] [review] Bug 19804: Add the new column to the circ rules interface Good morning I have insall the patch into kohadev, I have checked out and checked in several items with different configuration and it works.. I have created a rule of 1 suspension day for every 2 delay Case 1 Cumulative on Calendar without holidays--> works as espected Calendar with holiday --> work as expected Cumulative off Calendar without holidays --> works as expected Calendar with holidays --> works as expected I missed something to check, if not I will sign it asap Created attachment 70404 [details] [review] Bug 19804: Add new DB column issuingrules.suspension_chargeperiod Signed-off-by: Hugo Agud <hagud@orex.es> Created attachment 70405 [details] [review] Bug 19804: DBIC Schema changes Signed-off-by: Hugo Agud <hagud@orex.es> Created attachment 70406 [details] [review] Bug 19804: Add a 'Fine charging interval' for suspension days We already have a chargeperiod (Fine charging interval) value which is taken into account for fine ($) for not for the suspension period. This patch adds a new column suspension_chargeperiod (Fine day charging interval) to add the same behaviour when a suspension is calculated. Test plan: Add overdue item and play with the circulation rules (and the calendar). The suspension period must be correctly calculated. Please provide the different tests you made. Signed-off-by: Hugo Agud <hagud@orex.es> Created attachment 70407 [details] [review] Bug 19804: Add the new column to the circ rules interface Signed-off-by: Hugo Agud <hagud@orex.es> Jonathan, the atomicupdate should set an initial value for existing rules, based on chargeperiod. (In reply to Tomás Cohen Arazi from comment #10) > Jonathan, the atomicupdate should set an initial value for existing rules, > based on chargeperiod. It's done already, "DEFAULT "1"' Some tiny things: - number of tests needs to be 114 for them to pass - the headings at the top and the bottom row of the circulation rules table don't match up - Maybe rename "Fine day charging interval" to "Suspension charging interval" to match a bit more with the other related columns? - Adding a see also for bug 19204 - it's obvious here too, that calendar is ignored. Can you provide a quick follow up? Created attachment 73665 [details] [review] Bug 19804: Add a 'Fine charging interval' for suspension days We already have a chargeperiod (Fine charging interval) value which is taken into account for fine ($) for not for the suspension period. This patch adds a new column suspension_chargeperiod (Fine day charging interval) to add the same behaviour when a suspension is calculated. Test plan: Add overdue item and play with the circulation rules (and the calendar). The suspension period must be correctly calculated. Please provide the different tests you made. Signed-off-by: Hugo Agud <hagud@orex.es> Created attachment 73666 [details] [review] Bug 19804: Add the new column to the circ rules interface Signed-off-by: Hugo Agud <hagud@orex.es> Created attachment 73667 [details] [review] Bug 19804: (follow-up) Fix wording and columns order (In reply to Katrin Fischer from comment #12) > Some tiny things: > > - number of tests needs to be 114 for them to pass Patch amended. > - the headings at the top and the bottom row of the circulation rules table > don't match up > - Maybe rename "Fine day charging interval" to "Suspension charging > interval" to match a bit more with the other related columns? Done, thanks! Hi Joubu, running out of time for testing this morning, but have 2 questions I want to note: 1) For the fine charging interval we multiply: Days overdue / fine charging interval * fine The equivalence would then be: Days overdue / suspension charging interval * suspension in days But that's not quite what this does, is it? 2) I have backdated an item on checkout to be immediately overdue, but on return no restriction was created. Can you add a more detailed test plan? (In reply to Katrin Fischer from comment #17) > Hi Joubu, > > running out of time for testing this morning, but have 2 questions I want to > note: > > 1) For the fine charging interval we multiply: > Days overdue / fine charging interval * fine > > The equivalence would then be: > Days overdue / suspension charging interval * suspension in days > > But that's not quite what this does, is it? Yes it is what it does, take a look at the tests: 1795 # We want to charge 2 days every day, without grace 1796 # With 5 days of overdue: 5 * Z (Z is a typo for 2 I guess..) 1814 # We want to charge 2 days every 2 days, without grace 1815 # With 5 days of overdue: (5 * 2) / 2 1836 # We want to charge 2 days every 3 days, with 1 day of grace 1837 # With 5 days of overdue: ((5-1) / 3 ) * 2 > 2) I have backdated an item on checkout to be immediately overdue, but on > return no restriction was created. Can you add a more detailed test plan? Today is April 6th, I set: Suspension in days (day): 3 Suspension charging interval: 2 Which means: charge 3 days every 2 days. Check an item in with a due date on April 2nd (4 days of overdue) We are expecting: 3*4/2 = 6 days of suspension And I get: "Restricted: Patron's account is restricted until 12/04/2018" \o/ It was me, the devbox had the wrong time and I made a mistake in the circulation conditions... *sigh* - But the good news is the patch working :) Created attachment 73830 [details] [review] Bug 19804: Add new DB column issuingrules.suspension_chargeperiod Signed-off-by: Hugo Agud <hagud@orex.es> Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de> Created attachment 73831 [details] [review] Bug 19804: DBIC Schema changes Signed-off-by: Hugo Agud <hagud@orex.es> Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de> Created attachment 73832 [details] [review] Bug 19804: Add a 'Fine charging interval' for suspension days We already have a chargeperiod (Fine charging interval) value which is taken into account for fine ($) for not for the suspension period. This patch adds a new column suspension_chargeperiod (Fine day charging interval) to add the same behaviour when a suspension is calculated. Test plan: Add overdue item and play with the circulation rules (and the calendar). The suspension period must be correctly calculated. Please provide the different tests you made. Signed-off-by: Hugo Agud <hagud@orex.es> Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de> Created attachment 73833 [details] [review] Bug 19804: Add the new column to the circ rules interface Signed-off-by: Hugo Agud <hagud@orex.es> Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de> Created attachment 73834 [details] [review] Bug 19804: (follow-up) Fix wording and columns order Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de> Pushed to master for 18.05, thanks to everybody involved! Created attachment 73894 [details] [review] Bug 19804: Fix test in GetHardDueDate.t Created attachment 73895 [details] [review] Bug 19804: Fix test in maxsuspensiondays.t We do not want a random value of suspension_chargeperiod from TestBuilder Last two patches pushed to master. |