|
Description
Nick Clemens (kidclamp)
2018-12-21 19:06:35 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' Nick, don't make your QAM grumpy, assign your bugs! ;) (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? (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. 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 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' Created attachment 88003 [details] [review] Bug 22044: Correct syspref name (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. (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 (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 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 :) 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> Created attachment 88122 [details] [review] Bug 22044: Correct syspref name Created attachment 88123 [details] [review] Bug 22044: Fix sysprefs.sql description to use circ column description Awesome work all! Pushed to master for 19.05 Pushed to 18.11.x for 18.11.05 cant clean apply this patch set to 18.05.x |