Description
Kelly McElligott
2020-01-08 17:25:42 UTC
Created attachment 100309 [details] [review] Bug 24378: Correct wording in AUTO_RENEWAL notice Test Plan: - install patch - update databse - confirm notice name is now in sentence-case - confirm too-many error text now reads "reached" rather than "reach" Easy fix to change that word, and I also adjusted the notice name while I was in there. But I'm not putting this to needs signoff yet because for some reason this notice isn't actually getting created in master and I'm not sure why. Will get some wiser eyes on it. Created attachment 100416 [details] [review] Bug 24378: Reword notice in updatedatabase.pl for grammar and clarity Test plan: - install patch - update databse - confirm notice name is now in sentence-case - confirm too-many error text now reads "reached" rather than "reach" Created attachment 100417 [details] [review] Bug 24378: atomicupdate for new wording Created attachment 100479 [details] [review] Bug 24378: Reword notice in updatedatabase.pl for grammar and clarity Test plan: - install patch - update databse - confirm notice name is now in sentence-case - confirm too-many error text now reads "reached" rather than "reach" Signed-off-by: David Nind <david@davidnind.com> Created attachment 100480 [details] [review] Bug 24378: atomicupdate for new wording Signed-off-by: David Nind <david@davidnind.com> After signing off, I though maybe that: "You have reached the maximum of checkouts possible." should be: "You have reached the maximum number of checkouts possible." (number added after maximum) Created attachment 100485 [details] [review] Bug 24378: Change wording of auto_renewals notice in updatedatabase Test plan: - install patch - update databse - confirm changes to notice name and wording Good catch! Added "number" as suggested. Andrew, you are adding a new notice template, but one can already exist. You should update existing ones, only if they have not been modified yet, and add it if does not exist yet. It seems that the template does not exist for new install, we should add it to the installer file (installer/data/mysql/en/mandatory/sample_notices.yml). Hi Jonathan! I added it to sample-notices.yaml over in bug 23787. I'd already started work on this one before I found the other. What's the best way to link/merge the two? Right, I had not seen the other bug. I would change the wording before adding it to the installer file. A good plan would be to: * Fix the wording for existing installs (you need to make sure it has not been modified or translated!) * Add the updated template for new installs (yml file) * Add the updated template for existing installs, if not exists yet (atomic update file) Maybe we could drop the first step, what do you think? The query would be quite ugly... I think we should not run an updatedatabase here as suggested as the notice could have been translated or removed on purpose (the library not wanting to send it). We could do a replace on the string if the notice exists instead? There are some examples for this in the updatedatabase.pl file: UPDATE saved_sql SET savedsql = REPLACE(savedsql, 'wthdrawn', 'withdrawn'); Katrin makes a good point about not pushing this out to existing installs as they may have intentionally removed it. With that in mind, I'm going to close this bug. I'll iron out the other bug to get this notice with new wording into fresh installs. Thanks! Another option might be to use a 'REPLACE' in the updatedatabase to just change the text in question if it is still there. And we could still fix the initial updatedatabase statement. If it is ok with you, I might have a go at this later. By all means, have at it! Created attachment 102827 [details] [review] Bug 24378: Change wording of auto_renewals notice in updatedatabase On the template of AUTO-RENEWALS the wording is grammatically incorrect: [% IF checkout.auto_renew_error == 'too_many' %] You have reach the maximum of checkouts possible. This should say [% IF checkout.auto_renew_error == 'too_many' %] You have reached the maximum of checkouts possible. Test plan: - Make sure you have the AUTO_RENEWALS notice with the problematic spelling - install patch - update database - confirm changes to notice name and wording Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de> Kept Andrew's change on the original updatedatabase entry, but removed the database update to be done a little differently in a follow-up patch. Amended test plan. Created attachment 102828 [details] [review] Bug 24378: Database update to rewrite notice if left unchanged The database update makes the same changes as done by Andrew on the old database entry. We check for full sentences left unchanged to minimize issues with changes made by the library. Test plan of first patch applies. This should be pushed together with bug 23787 - so we take care of this once and for all. Created attachment 102871 [details] [review] Bug 24378: Change wording of auto_renewals notice in updatedatabase On the template of AUTO-RENEWALS the wording is grammatically incorrect: [% IF checkout.auto_renew_error == 'too_many' %] You have reach the maximum of checkouts possible. This should say [% IF checkout.auto_renew_error == 'too_many' %] You have reached the maximum of checkouts possible. Test plan: - Make sure you have the AUTO_RENEWALS notice with the problematic spelling - install patch - update database - confirm changes to notice name and wording Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de> Kept Andrew's change on the original updatedatabase entry, but removed the database update to be done a little differently in a follow-up patch. Amended test plan. Signed-off-by: Andrew Fuerste-Henry <andrew@bywatersolutions.com> Created attachment 102872 [details] [review] Bug 24378: Database update to rewrite notice if left unchanged The database update makes the same changes as done by Andrew on the old database entry. We check for full sentences left unchanged to minimize issues with changes made by the library. Test plan of first patch applies. Signed-off-by: Andrew Fuerste-Henry <andrew@bywatersolutions.com> Created attachment 104683 [details] [review] Bug 24378: (follow-up) Format due date as due date Changes the due date formatting as was already done for the new sample files on bug 23787. I think the indentation changes in the first patch are wrong. Created attachment 104701 [details] [review] Bug 24378: Change wording of auto_renewals notice in updatedatabase On the template of AUTO-RENEWALS the wording is grammatically incorrect: [% IF checkout.auto_renew_error == 'too_many' %] You have reach the maximum of checkouts possible. This should say [% IF checkout.auto_renew_error == 'too_many' %] You have reached the maximum of checkouts possible. Test plan: - Make sure you have the AUTO_RENEWALS notice with the problematic spelling - install patch - update database - confirm changes to notice name and wording Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de> Kept Andrew's change on the original updatedatabase entry, but removed the database update to be done a little differently in a follow-up patch. Amended test plan. Signed-off-by: Andrew Fuerste-Henry <andrew@bywatersolutions.com> Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org> Created attachment 104702 [details] [review] Bug 24378: Database update to rewrite notice if left unchanged The database update makes the same changes as done by Andrew on the old database entry. We check for full sentences left unchanged to minimize issues with changes made by the library. Test plan of first patch applies. Signed-off-by: Andrew Fuerste-Henry <andrew@bywatersolutions.com> Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org> Created attachment 104703 [details] [review] Bug 24378: (follow-up) Format due date as due date Changes the due date formatting as was already done for the new sample files on bug 23787. Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org> Created attachment 104704 [details] [review] Bug 24378: (QA follow-up) fix minor issues Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org> Thank you for the follow-up! Nice work everyone! Pushed to master for 20.05 missing dependencies - not backported to 19.11.x |