Bug 24378

Summary: Change wording on AUTO_RENEWALS notice in updatedatabase
Product: Koha Reporter: Kelly McElligott <kelly>
Component: NoticesAssignee: Katrin Fischer <katrin.fischer>
Status: CLOSED FIXED QA Contact: Testopia <testopia>
Severity: trivial    
Priority: P5 - low CC: 1joynelson, andrewfh, david, jonathan.druart, katrin.fischer
Version: Main   
Hardware: All   
OS: All   
See Also: https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=23787
Change sponsored?: --- Patch complexity: Small patch
Documentation contact: Documentation submission:
Text to go in the release notes:
Version(s) released in:
20.05.00
Bug Depends on: 23787    
Bug Blocks: 28258    
Attachments: Bug 24378: Correct wording in AUTO_RENEWAL notice
Bug 24378: Reword notice in updatedatabase.pl for grammar and clarity
Bug 24378: atomicupdate for new wording
Bug 24378: Reword notice in updatedatabase.pl for grammar and clarity
Bug 24378: atomicupdate for new wording
Bug 24378: Change wording of auto_renewals notice in updatedatabase
Bug 24378: Change wording of auto_renewals notice in updatedatabase
Bug 24378: Database update to rewrite notice if left unchanged
Bug 24378: Change wording of auto_renewals notice in updatedatabase
Bug 24378: Database update to rewrite notice if left unchanged
Bug 24378: (follow-up) Format due date as due date
Bug 24378: Change wording of auto_renewals notice in updatedatabase
Bug 24378: Database update to rewrite notice if left unchanged
Bug 24378: (follow-up) Format due date as due date
Bug 24378: (QA follow-up) fix minor issues

Description Kelly McElligott 2020-01-08 17:25:42 UTC
On the template of AUTO-RENEWALS the wording is grammatically incorrect:

Currently this line states:

[% 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.



Note:  It should be eb installed.
Comment 1 Andrew Fuerste-Henry 2020-03-07 15:03:00 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"
Comment 2 Andrew Fuerste-Henry 2020-03-07 15:05:05 UTC
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.
Comment 3 Andrew Fuerste-Henry 2020-03-10 00:59:22 UTC
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"
Comment 4 Andrew Fuerste-Henry 2020-03-10 01:12:01 UTC
Created attachment 100417 [details] [review]
Bug 24378: atomicupdate for new wording
Comment 5 David Nind 2020-03-10 19:38:38 UTC
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>
Comment 6 David Nind 2020-03-10 19:38:41 UTC
Created attachment 100480 [details] [review]
Bug 24378: atomicupdate for new wording

Signed-off-by: David Nind <david@davidnind.com>
Comment 7 David Nind 2020-03-10 19:40:35 UTC
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)
Comment 8 Andrew Fuerste-Henry 2020-03-11 00:21:45 UTC
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
Comment 9 Andrew Fuerste-Henry 2020-03-11 00:22:32 UTC
Good catch! Added "number" as suggested.
Comment 10 Jonathan Druart 2020-03-11 15:50:42 UTC
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).
Comment 11 Andrew Fuerste-Henry 2020-03-11 21:21:14 UTC
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?
Comment 12 Jonathan Druart 2020-03-12 06:35:21 UTC
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...
Comment 13 Katrin Fischer 2020-03-16 00:17:29 UTC
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');
Comment 14 Andrew Fuerste-Henry 2020-04-10 15:43:49 UTC
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!
Comment 15 Katrin Fischer 2020-04-13 11:06:37 UTC
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.
Comment 16 Andrew Fuerste-Henry 2020-04-13 11:56:51 UTC
By all means, have at it!
Comment 17 Katrin Fischer 2020-04-13 12:50:13 UTC
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.
Comment 18 Katrin Fischer 2020-04-13 12:50:16 UTC
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.
Comment 19 Katrin Fischer 2020-04-13 13:03:33 UTC
This should be pushed together with bug 23787 - so we take care of this once and for all.
Comment 20 Andrew Fuerste-Henry 2020-04-13 16:36:12 UTC
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>
Comment 21 Andrew Fuerste-Henry 2020-04-13 16:36:18 UTC
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>
Comment 22 Katrin Fischer 2020-05-11 11:09:02 UTC
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.
Comment 23 Jonathan Druart 2020-05-11 11:36:38 UTC
I think the indentation changes in the first patch are wrong.
Comment 24 Jonathan Druart 2020-05-11 13:45:06 UTC
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>
Comment 25 Jonathan Druart 2020-05-11 13:45:13 UTC
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>
Comment 26 Jonathan Druart 2020-05-11 13:45:18 UTC
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>
Comment 27 Jonathan Druart 2020-05-11 13:45:25 UTC
Created attachment 104704 [details] [review]
Bug 24378: (QA follow-up) fix minor issues

Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>
Comment 28 Katrin Fischer 2020-05-11 16:34:41 UTC
Thank you for the follow-up!
Comment 29 Martin Renvoize 2020-05-12 10:22:09 UTC
Nice work everyone!

Pushed to master for 20.05
Comment 30 Joy Nelson 2020-05-13 16:14:22 UTC
missing dependencies - not backported to 19.11.x