Bug 22044 - NoRenewalBeforePrecision should be set by default for new installations
Summary: NoRenewalBeforePrecision should be set by default for new installations
Status: Pushed to Master
Alias: None
Product: Koha
Classification: Unclassified
Component: Architecture, internals, and plumbing (show other bugs)
Version: master
Hardware: All All
: P5 - low normal (vote)
Target Milestone: ---
Assignee: Nick Clemens
QA Contact: Testopia
URL:
Whiteboard:
Keywords:
Depends on:
Blocks:
 
Reported: 2018-12-21 19:06 UTC by Nick Clemens
Modified: 2019-04-17 12:27 UTC (History)
3 users (show)

See Also:
Change sponsored?: ---
Patch complexity: ---
Bot Control: ---
When did the bot last check this:
Who signed the patch off:
Text to go in the release notes:
Version(s) released in:


Attachments
Bug 22044: Set default and add NoRenewalBeforePrecision preference for new/affected installs (3.17 KB, patch)
2018-12-21 19:22 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 22044: Set default and add NoRenewalBeforePrecision preference for new/affected installs (3.23 KB, patch)
2019-04-15 01:44 UTC, Hayley Mapley
Details | Diff | Splinter Review
Bug 22044: Correct syspref name (1.48 KB, patch)
2019-04-15 13:32 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 22044: Set default and add NoRenewalBeforePrecision preference for new/affected installs (3.23 KB, patch)
2019-04-16 22:13 UTC, Katrin Fischer
Details | Diff | Splinter Review
Bug 22044: Correct syspref name (1.48 KB, patch)
2019-04-16 22:13 UTC, Katrin Fischer
Details | Diff | Splinter Review
Bug 22044: Fix sysprefs.sql description to use circ column description (1.53 KB, patch)
2019-04-16 22:13 UTC, Katrin Fischer
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Nick Clemens 2018-12-21 19:06:35 UTC
NoRenewalBeforePrecision was introduced in bug https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=14395 but did not recieve a default value

When not set the pref dropdown default to 'date' however, the behaviour if unset matches 'exact time'

We should set it to 'exact time' by default since it is the default behaviour
Comment 1 Nick Clemens 2018-12-21 19:22:36 UTC
Created attachment 83469 [details] [review]
Bug 22044: Set default and add NoRenewalBeforePrecision preference for new/affected installs

I am not sure the correct answer - for upgraded installs the pref was
set to date, which changed behaviour

For new installs the pref was unset, behaved as if 'exact_time' so we
shouldn't change behaviour?

If the perceived default is date then the code should be changed, or
this shouldn't be a pref at all?

To test:
1 - Have a new system missing this pref
2 - Note when searchign the pref in staff client if appears as 'date'
3 - Check the db to confirm value is 'NULL'
4 - Apply patch and run update
5 - Pref is now set to 'exact_time'
Comment 2 Katrin Fischer 2019-01-16 20:11:16 UTC
Nick, don't make your QAM grumpy, assign your bugs! ;)
Comment 3 Hayley Mapley 2019-04-15 01:36:24 UTC
(In reply to Katrin Fischer from comment #2)
> Nick, don't make your QAM grumpy, assign your bugs! ;)

Hi Nick,

I'm in the process of testing your patch, and am stuck at step 3 - check db to confirm value is 'NULL'. Probably because I'm a newbie, but I can't locate the NoRenewalBeforePrecision that you mention in the systempreferences table to check that it has the value of NULL. The system preference does show up in the staff client. Am I missing something?
Comment 4 Hayley Mapley 2019-04-15 01:44:03 UTC
(In reply to Nick Clemens from comment #1)

> 3 - Check the db to confirm value is 'NULL'

I am guessing that this is instructing us to check whether the preference is actually in the database, rather than whether it's value cell is set to null. If this is the case, I am able to sign off as the test plan results were as expected.
Comment 5 Hayley Mapley 2019-04-15 01:44:29 UTC
Created attachment 87975 [details] [review]
Bug 22044: Set default and add NoRenewalBeforePrecision preference for new/affected installs

I am not sure the correct answer - for upgraded installs the pref was
set to date, which changed behaviour

For new installs the pref was unset, behaved as if 'exact_time' so we
shouldn't change behaviour?

If the perceived default is date then the code should be changed, or
this shouldn't be a pref at all?

To test:
1 - Have a new system missing this pref
2 - Note when searchign the pref in staff client if appears as 'date'
3 - Check the db to confirm value is 'NULL'
4 - Apply patch and run update
5 - Pref is now set to 'exact_time'

Signed-off-by: Hayley Mapley <hayleymapley@catalyst.net.nz>
Comment 6 Julian Maurice 2019-04-15 07:53:20 UTC
Comment on attachment 87975 [details] [review]
Bug 22044: Set default and add NoRenewalBeforePrecision preference for new/affected installs

Review of attachment 87975 [details] [review]:
-----------------------------------------------------------------

::: installer/data/mysql/sysprefs.sql
@@ +310,4 @@
>  ('noissuescharge','5','','Define maximum amount withstanding before check outs are blocked','Integer'),
>  ('NoIssuesChargeGuarantees','','','Define maximum amount withstanding before check outs are blocked','Integer'),
>  ('noItemTypeImages','0',NULL,'If ON, disables item-type images','YesNo'),
> +('NoRenewalBefore','exact_time','date|exact_time','Calculate NoRenewalBefore based on date only or exact time of due date','Choice'),

'NoRenewalBefore' should be 'NoRenewalBeforePrecision'
Comment 7 Nick Clemens 2019-04-15 13:32:47 UTC
Created attachment 88003 [details] [review]
Bug 22044: Correct syspref name
Comment 8 Hayley Mapley 2019-04-15 22:38:13 UTC
(In reply to Nick Clemens from comment #7)
> Created attachment 88003 [details] [review] [review]
> Bug 22044: Correct syspref name

Sorry Nick, the syspref description still references NoRenewalBefore rather than NoRenewalBeforePrecision. Not a big deal so I'll leave as signed off, but would be good for consistency.
Comment 9 Julian Maurice 2019-04-16 10:54:23 UTC
(In reply to Hayley Mapley from comment #8)
> (In reply to Nick Clemens from comment #7)
> > Created attachment 88003 [details] [review] [review] [review]
> > Bug 22044: Correct syspref name
> 
> Sorry Nick, the syspref description still references NoRenewalBefore rather
> than NoRenewalBeforePrecision. Not a big deal so I'll leave as signed off,
> but would be good for consistency.

Not sure if this is a reference to the syspref or to the feature.
Anyway, description in atomic update should match the one in sysprefs.sql
Comment 10 Hayley Mapley 2019-04-16 21:07:20 UTC
(In reply to Julian Maurice from comment #9)
> (In reply to Hayley Mapley from comment #8)
> > (In reply to Nick Clemens from comment #7)
> > > Created attachment 88003 [details] [review] [review] [review] [review]
> > > Bug 22044: Correct syspref name
> > 
> > Sorry Nick, the syspref description still references NoRenewalBefore rather
> > than NoRenewalBeforePrecision. Not a big deal so I'll leave as signed off,
> > but would be good for consistency.
> 
> Not sure if this is a reference to the syspref or to the feature.
> Anyway, description in atomic update should match the one in sysprefs.sql

I am referring to your INSERT statement in the sysprefs.sql:

313 
('NoRenewalBeforePrecision','exact_time','date|exact_time','Calculate NoRenewalBefore based on date only or exact time of due date','Choice'),

It should read 'Calculate NoRenewalBeforePrecision based on...'

Like I say, not a big deal, but would be good for consistency
Comment 11 Katrin Fischer 2019-04-16 22:12:08 UTC
I am with Hayley, NoRenewalBefore is not a thing, the column in sysprefs is labelled "No renewal before" - I'll put forward a follow-up patch :)
Comment 12 Katrin Fischer 2019-04-16 22:13:39 UTC
Created attachment 88121 [details] [review]
Bug 22044: Set default and add NoRenewalBeforePrecision preference for new/affected installs

I am not sure the correct answer - for upgraded installs the pref was
set to date, which changed behaviour

For new installs the pref was unset, behaved as if 'exact_time' so we
shouldn't change behaviour?

If the perceived default is date then the code should be changed, or
this shouldn't be a pref at all?

To test:
1 - Have a new system missing this pref
2 - Note when searchign the pref in staff client if appears as 'date'
3 - Check the db to confirm value is 'NULL'
4 - Apply patch and run update
5 - Pref is now set to 'exact_time'

Signed-off-by: Hayley Mapley <hayleymapley@catalyst.net.nz>
Comment 13 Katrin Fischer 2019-04-16 22:13:43 UTC
Created attachment 88122 [details] [review]
Bug 22044: Correct syspref name
Comment 14 Katrin Fischer 2019-04-16 22:13:46 UTC
Created attachment 88123 [details] [review]
Bug 22044: Fix sysprefs.sql description to use circ column description
Comment 15 Nick Clemens 2019-04-17 12:27:54 UTC
Awesome work all!

Pushed to master for 19.05