Bug 23787 - Add AUTO_RENEWALS in sample_notices.sql
Summary: Add AUTO_RENEWALS in sample_notices.sql
Status: Pushed to master
Alias: None
Product: Koha
Classification: Unclassified
Component: Notices (show other bugs)
Version: master
Hardware: All All
: P5 - low normal (vote)
Assignee: Andrew Fuerste-Henry
QA Contact: Testopia
URL:
Keywords:
Depends on:
Blocks: 18532 23789 24378
  Show dependency treegraph
 
Reported: 2019-10-09 15:51 UTC by Caroline Cyr La Rose
Modified: 2020-05-13 16:08 UTC (History)
7 users (show)

See Also:
Change sponsored?: ---
Patch complexity: Small patch
Who signed the patch off:
Text to go in the release notes:
Version(s) released in:
20.05.00


Attachments
Bug 23787: Add AUTO_RENEWALS in sample_notices.sql (21.24 KB, patch)
2019-10-09 18:22 UTC, Caroline Cyr La Rose
Details | Diff | Splinter Review
Bug 23787: Add AUTO_RENEWALS to it-IT installer (2.51 KB, patch)
2019-11-02 15:21 UTC, Katrin Fischer
Details | Diff | Splinter Review
Bug 23787: Add AUTO_RENEWALS in sample_notices.sql (18.35 KB, patch)
2020-03-10 01:42 UTC, Andrew Fuerste-Henry
Details | Diff | Splinter Review
Bug 23787: add AUTO_RENEWALS to sample_notices (4.19 KB, patch)
2020-03-11 00:46 UTC, Andrew Fuerste-Henry
Details | Diff | Splinter Review
Bug 23787: Add AUTO_RENEWALS to sample_notices (4.24 KB, patch)
2020-03-11 10:35 UTC, David Nind
Details | Diff | Splinter Review
Bug 23787: add AUTO_RENEWALS to sample_notices (2.34 KB, patch)
2020-04-10 16:04 UTC, Andrew Fuerste-Henry
Details | Diff | Splinter Review
Bug 23787: Add AUTO_RENEWALS to sample_notices (2.39 KB, patch)
2020-04-13 13:08 UTC, Katrin Fischer
Details | Diff | Splinter Review
Bug 23787: Add sample AUTO_RENEWALS notice to translatd installers (13.11 KB, patch)
2020-04-13 13:08 UTC, Katrin Fischer
Details | Diff | Splinter Review
Bug 23787: Add AUTO_RENEALS sample notice to es-ES installer (2.05 KB, patch)
2020-04-13 13:08 UTC, Katrin Fischer
Details | Diff | Splinter Review
Bug 23787: Add sample AUTO_RENEWALS notice to translatd installers (13.24 KB, patch)
2020-04-13 16:12 UTC, Andrew Fuerste-Henry
Details | Diff | Splinter Review
Bug 23787: Add AUTO_RENEALS sample notice to es-ES installer (2.12 KB, patch)
2020-04-13 16:12 UTC, Andrew Fuerste-Henry
Details | Diff | Splinter Review
Bug 23787: Add AUTO_RENEWALS to sample_notices (2.49 KB, patch)
2020-04-27 12:16 UTC, Andrew Fuerste-Henry
Details | Diff | Splinter Review
Bug 23787: (follow-up) Format due date as due date (6.90 KB, patch)
2020-05-05 21:38 UTC, Katrin Fischer
Details | Diff | Splinter Review
Bug 23787: Add sample AUTO_RENEWALS notice to translatd installers (13.25 KB, patch)
2020-05-11 10:28 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 23787: Add AUTO_RENEWALS to sample_notices (2.55 KB, patch)
2020-05-11 10:28 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 23787: (follow-up) Format due date as due date (6.97 KB, patch)
2020-05-11 10:28 UTC, Jonathan Druart
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Caroline Cyr La Rose 2019-10-09 15:51:17 UTC
I wanted to translate the AUTO_RENEWALS notice, but it's not in the sample_notices.sql. It is only in the updatedatabase.pl

Martin confirmed on IRC that it should be in both.
Comment 1 Caroline Cyr La Rose 2019-10-09 18:22:21 UTC
Created attachment 93937 [details] [review]
Bug 23787: Add AUTO_RENEWALS in sample_notices.sql

This patch adds the AUTO_RENEWALS notice SQL insert query that was only available in the updatedatabase script into all the language-specific sample_notices.sql files in the installer (de-DE, en, es-ES, fr-CA, fr-FR, nb-NO, pl-PL, ru-RU, uk-UA).

To test, make a fresh install in a language listed above, do NOT run updatedatabase, make sure the AUTO_RENEWALS is in the available notices.
Comment 2 Caroline Cyr La Rose 2019-10-09 18:23:12 UTC
I simply copied the INSERT query into all the files. I hope that's okay.
Comment 3 Agustín Moyano 2019-10-23 02:57:58 UTC
Hello Caroline, I believe you should add a script in atomicupdate that inserts this notice in letter table.

AUTO_RENEWALS in updatedatabase.pl is inserted in version $DBversion = '16.12.00.035', so if someone has a version after that, they won't have that notice in the database.

There you should make an "insert ignore" just in case.
Comment 4 Katrin Fischer 2019-11-02 15:21:01 UTC
2 things: When you run xt/sample_noticed.t you can see that it-IT is missing. I am adding it in a follow up.

But another thing: The notice states twice... "The following item...", but there is no item information in the sample text. Should we add something? Also I'd like to make the subject a bit more patron friendly, maybe "Automatic renewals on your account" or similar?

And "notification" would be nicer as "Notification".

I can help with the updatedatabase if necessary!
Comment 5 Katrin Fischer 2019-11-02 15:21:44 UTC
Created attachment 94968 [details] [review]
Bug 23787: Add AUTO_RENEWALS to it-IT installer
Comment 6 Andrew Fuerste-Henry 2020-03-09 13:17:59 UTC
I'm editing the wording of this notice in updatedatabse over in Bug 24378. The English version has some grammatical errors and other issues. We should make sure those changes get reflected in both places.
Comment 7 Andrew Fuerste-Henry 2020-03-10 01:42:36 UTC
Created attachment 100418 [details] [review]
Bug 23787: Add AUTO_RENEWALS in sample_notices.sql

This patch adds the AUTO_RENEWALS notice SQL insert query that was only available in the updatedatabase script into all the language-specific sample_notices.sql files in the installer (de-DE, en, es-ES, fr-CA, fr-FR, it-IT, nb-NO, pl-PL, ru-RU, uk-UA).

To test, make a fresh install in a language listed above, do NOT run updatedatabase, make sure the AUTO_RENEWALS is in the available notices.
Comment 8 Andrew Fuerste-Henry 2020-03-10 01:45:06 UTC
Updated Caroline's work to reflect wording changes I made over in bug 24378, folded it-IT and atomicupdate all into one patch. Fingers crossed I didn't mess it up!
Comment 9 David Nind 2020-03-10 05:27:34 UTC
Patch doesn't apply:

Applying: Bug 23787: Add AUTO_RENEWALS in sample_notices.sql
Using index info to reconstruct a base tree...
A	installer/data/mysql/en/mandatory/sample_notices.sql
Falling back to patching base and 3-way merge...
CONFLICT (modify/delete): installer/data/mysql/en/mandatory/sample_notices.sql deleted in HEAD and modified in Bug 23787: Add AUTO_RENEWALS in sample_notices.sql. Version Bug 23787: Add AUTO_RENEWALS in sample_notices.sql of installer/data/mysql/en/mandatory/sample_notices.sql left in tree.
error: Failed to merge in the changes.
Patch failed at 0001 Bug 23787: Add AUTO_RENEWALS in sample_notices.sql
Comment 10 Andrew Fuerste-Henry 2020-03-10 12:41:08 UTC
Learned an important lesson about grabbing the latest version of master before writing a patch. Will try to get back to this today. Thanks!
Comment 11 Andrew Fuerste-Henry 2020-03-11 00:46:26 UTC
Created attachment 100486 [details] [review]
Bug 23787: add AUTO_RENEWALS to sample_notices

To test:
- create a new install
- confirm presence of auto renewal notice
Comment 12 Andrew Fuerste-Henry 2020-03-11 00:47:46 UTC
Adapted to new yaml format. It's my understanding that means one need not copy the notice into all the separate languages, so I only put it in the one yaml file. Please let me know if I'm mistaken in that. Thaanks!
Comment 13 David Nind 2020-03-11 10:35:08 UTC
Created attachment 100526 [details] [review]
Bug 23787: Add AUTO_RENEWALS to sample_notices

To test:
- create a new install
- confirm presence of auto renewal notice

Signed-off-by: David Nind <david@davidnind.com>
Comment 14 Jonathan Druart 2020-03-12 06:36:00 UTC
You should insert the template notice for existing installs only if it does not exist yet.
Comment 15 Katrin Fischer 2020-03-12 07:05:37 UTC
(In reply to Jonathan Druart from comment #14)
> You should insert the template notice for existing installs only if it does
> not exist yet.

I would recommend against adding the notice to existing installations. If you are using auto renewals they will be activated immediately. It might come as an unwanted surprise to libraries that have deleted it on purpose as they don't want to use it. (it seems we added it before, but might still want to handle this differently here?)
Comment 16 Andrew Fuerste-Henry 2020-04-10 16:04:39 UTC
Created attachment 102731 [details] [review]
Bug 23787: add AUTO_RENEWALS to sample_notices

Removed the atomicupdate so we're not adding this to existing installs.

To test:
- create a new install
- confirm presence of auto renewal notice
Comment 17 Katrin Fischer 2020-04-13 13:08:18 UTC
Created attachment 102829 [details] [review]
Bug 23787: Add AUTO_RENEWALS to sample_notices

To test:
- create a new English (en) install
- confirm presence of auto renewal notice

Signed-off-by: David Nind <david@davidnind.com>
Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Comment 18 Katrin Fischer 2020-04-13 13:08:25 UTC
Created attachment 102830 [details] [review]
Bug 23787: Add sample AUTO_RENEWALS notice to translatd installers

This adds the default auto renewal notice to the translated
installers. Translating will be handled by maintainers of the
installers, but this way we don't end up with installations
having a different set of notices than others.

To test:
- Verify the sample_notices.sql files load correctly
or
- Run the web installer for each of those languages and
  verify sample_notices loads correctly.
Comment 19 Katrin Fischer 2020-04-13 13:08:29 UTC
Created attachment 102831 [details] [review]
Bug 23787: Add AUTO_RENEALS sample notice to es-ES installer

Keeping this one separate as there is another bug to move
es-ES to use the new .yml based tanslation process. This
way this patch could be dropped easily if the other
bug makes it in first.

Test plan of second patch applies.
Comment 20 Katrin Fischer 2020-04-13 13:09:10 UTC
Hi Andrew,
Signed off on your patch and made follow-ups for the translated installers. Could you maybe sign-off on those?
Comment 21 Andrew Fuerste-Henry 2020-04-13 16:12:12 UTC
Created attachment 102868 [details] [review]
Bug 23787: Add sample AUTO_RENEWALS notice to translatd installers

This adds the default auto renewal notice to the translated
installers. Translating will be handled by maintainers of the
installers, but this way we don't end up with installations
having a different set of notices than others.

To test:
- Verify the sample_notices.sql files load correctly
or
- Run the web installer for each of those languages and
  verify sample_notices loads correctly.

Signed-off-by: Andrew Fuerste-Henry <andrew@bywatersolutions.com>
Comment 22 Andrew Fuerste-Henry 2020-04-13 16:12:15 UTC
Created attachment 102869 [details] [review]
Bug 23787: Add AUTO_RENEALS sample notice to es-ES installer

Keeping this one separate as there is another bug to move
es-ES to use the new .yml based tanslation process. This
way this patch could be dropped easily if the other
bug makes it in first.

Test plan of second patch applies.

Signed-off-by: Andrew Fuerste-Henry <andrew@bywatersolutions.com>
Comment 23 Andrew Fuerste-Henry 2020-04-13 16:37:34 UTC
Signed off. Thanks, Katrin! Learned some stuff about making different languages go in Koha :)
Comment 24 Jonathan Druart 2020-04-27 10:42:23 UTC
Please double check, using 'en', I see \' when editing the letter:

It\'s too late to renew this item.
Comment 25 Andrew Fuerste-Henry 2020-04-27 12:16:06 UTC
Created attachment 103763 [details] [review]
Bug 23787: Add AUTO_RENEWALS to sample_notices

Removed "\" from "It\'s too late..." -- apparently that doesn't need to get escaped in YAML

To test:
- create a new English (en) install
- confirm presence of auto renewal notice

Signed-off-by: David Nind <david@davidnind.com>
Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Comment 26 Jonathan Druart 2020-04-27 12:35:01 UTC
Don't we want the date to be formatted as a due date?
Comment 27 Katrin Fischer 2020-05-02 13:23:51 UTC
I wanted to fix this, but I can't get the AUTO_RENEWAL notice to generate :(

I was not sure if it was enough to add:

... | $KohaDates  as_due_date => 1 %]

or if it would also need:

[%- USE KohaDates -%]

I our sample notices library we have both versions.
Comment 28 Jonathan Druart 2020-05-05 09:08:46 UTC
It works for me

The following item, [% biblio.title %], has correctly been renewed and is now due on [% checkout.date_due | $KohaDates as_due_date => 1 %]

You don't need the USE KohaDates (it's included by default).

However, I am wondering: it is not possible to have the time part after a renewal, 23:59 is always set. That sounds like a bug, right?

- due date is yesterday at 12:34, renew it, it become another day at 23:59 (with or without auto renewal).
Comment 29 Katrin Fischer 2020-05-05 09:16:14 UTC
I am not sure it's a bug - it would probably require that you define your circ rules in hours instead of days - but not sure if we support "hourly renewal" at all yet. 

What did you set up to get the notice?

I had:
- Set up the notice
- Checked out several items with different due dates today and in the past
- Put a hold on one (to prevent auto_renewal)
- Ran the job... no notice, but renewals took place for some of my items as expected

Patron had an email address. Are any of the messaging settings involved?
Comment 30 Jonathan Druart 2020-05-05 09:33:39 UTC
Did you pass --send-notices to the script?
Comment 31 Katrin Fischer 2020-05-05 09:43:42 UTC
(In reply to Jonathan Druart from comment #30)
> Did you pass --send-notices to the script?

Nope! I'll try again.
Comment 32 Katrin Fischer 2020-05-05 21:38:17 UTC
Created attachment 104385 [details] [review]
Bug 23787: (follow-up) Format due date as due date
Comment 33 Jonathan Druart 2020-05-11 10:28:47 UTC
Created attachment 104675 [details] [review]
Bug 23787: Add sample AUTO_RENEWALS notice to translatd installers

This adds the default auto renewal notice to the translated
installers. Translating will be handled by maintainers of the
installers, but this way we don't end up with installations
having a different set of notices than others.

To test:
- Verify the sample_notices.sql files load correctly
or
- Run the web installer for each of those languages and
  verify sample_notices loads correctly.

Signed-off-by: Andrew Fuerste-Henry <andrew@bywatersolutions.com>

Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>
Comment 34 Jonathan Druart 2020-05-11 10:28:51 UTC
Created attachment 104676 [details] [review]
Bug 23787: Add AUTO_RENEWALS to sample_notices

Removed "\" from "It\'s too late..." -- apparently that doesn't need to get escaped in YAML

To test:
- create a new English (en) install
- confirm presence of auto renewal notice

Signed-off-by: David Nind <david@davidnind.com>
Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>

Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>
Comment 35 Jonathan Druart 2020-05-11 10:28:55 UTC
Created attachment 104677 [details] [review]
Bug 23787: (follow-up) Format due date as due date

Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>
Comment 36 Martin Renvoize 2020-05-12 10:21:58 UTC
Nice work everyone!

Pushed to master for 20.05
Comment 37 Joy Nelson 2020-05-13 16:08:05 UTC
not backported to 19.11.x
...because yaml.