Bug 18915 - Creating a checkout note (patron note) sends an incomplete email message
Summary: Creating a checkout note (patron note) sends an incomplete email message
Status: RESOLVED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: OPAC (show other bugs)
Version: master
Hardware: All All
: P5 - low major (vote)
Assignee: Aleisha Amohia
QA Contact: Marcel de Rooy
URL:
Whiteboard:
Keywords:
Depends on: 14224
Blocks:
  Show dependency treegraph
 
Reported: 2017-07-09 22:23 UTC by Aleisha Amohia
Modified: 2018-06-04 20:15 UTC (History)
11 users (show)

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


Attachments
Bug 18915: Fix checkout note email message (3.00 KB, patch)
2017-07-09 22:42 UTC, Aleisha Amohia
Details | Diff | Splinter Review
Bug 18915: Fix checkout note email message (7.92 KB, patch)
2017-08-27 23:23 UTC, Aleisha Amohia
Details | Diff | Splinter Review
Bug 18915: Giving CHECKOUT_NOTE access to issues table (907 bytes, patch)
2017-12-17 23:39 UTC, Aleisha Amohia
Details | Diff | Splinter Review
Bug 18915: Renaming svc/patron_notes to svc/checkout_notes (1.59 KB, patch)
2017-12-17 23:48 UTC, Aleisha Amohia
Details | Diff | Splinter Review
Bug 18915: Fix checkout note email message (7.98 KB, patch)
2018-01-09 15:49 UTC, sandboxes@biblibre.com
Details | Diff | Splinter Review
Bug 18915: Giving CHECKOUT_NOTE access to issues table (1.07 KB, patch)
2018-01-09 15:49 UTC, sandboxes@biblibre.com
Details | Diff | Splinter Review
Bug 18915: Renaming svc/patron_notes to svc/checkout_notes (1.78 KB, patch)
2018-01-09 15:50 UTC, sandboxes@biblibre.com
Details | Diff | Splinter Review
Bug 18915: Fix checkout note email message (8.08 KB, patch)
2018-01-12 10:35 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 18915: Giving CHECKOUT_NOTE access to issues table (1.17 KB, patch)
2018-01-12 10:35 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 18915: Renaming svc/patron_notes to svc/checkout_notes (1.87 KB, patch)
2018-01-12 10:35 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 18915: (QA follow-up) Add note for installs after 17.05 (1.70 KB, patch)
2018-01-12 10:35 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 18915: Add CHECKOUT_NOTE notice template for other languages (33.72 KB, patch)
2018-01-15 16:43 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 Aleisha Amohia 2017-07-09 22:23:27 UTC
Taken from bug 14224:

(Marcel de Rooy from comment #122 )
> <<borrowers.firstname>> <<borrowers.surname>> has added a note to the item
> <<biblio.item>> - <<biblio.author>> (<<biblio.biblionumber>>).
> 
> What is biblio.item supposed to print here?
> I got this mail:
> M De Rooy has added a note to the item  - Schwartz, Randal L (23).
> 
> And must say that it might not be that clear..
> Do we need biblio.title and an item number ?

Have tested and can confirm that this bug exists.
Comment 1 Aleisha Amohia 2017-07-09 22:42:32 UTC
Created attachment 64937 [details] [review]
Bug 18915: Fix checkout note email message

This patch changes patron_note to checkout_note in the appropriate
places, and ensures the email gets the title of the item.

To test:
1) Apply patch and update database
2) View the message_queue table in mysql
3) Check out an item if haven't already
4) Go to OPAC and set a checkout note for an item
5) View message_queue table and confirm it the title is included in the
email and all instances of 'patron note' have been replaced with
'checkout note'
6) Disable javascript in browser
7) repeat steps 4 and 5 and confirm all works as expected

Sponsored-by: Catalyst IT
Comment 2 Marc Véron 2017-08-09 08:05:46 UTC
What happens if a library has modified the note? - Modifications for title and content would get lost with the following statement in 
bug_18915_-_modifying_patron_note_notice.sql 

UPDATE letter SET code = 'CHECKOUT_NOTE', name = 'Checkout note on item set by patron', title = 'Checkout note', content = '<<borrowers.firstname>> <<borrowers.surname>> has added a note to the item <<biblio.title>> - <<biblio.author>> (<<biblio.biblionumber>>).' WHERE code = 'PATRON_NOTE';
Comment 3 Aleisha Amohia 2017-08-18 04:16:53 UTC
(In reply to Marc Véron from comment #2)
> What happens if a library has modified the note? - Modifications for title
> and content would get lost with the following statement in 
> bug_18915_-_modifying_patron_note_notice.sql 
> 
> UPDATE letter SET code = 'CHECKOUT_NOTE', name = 'Checkout note on item set
> by patron', title = 'Checkout note', content = '<<borrowers.firstname>>
> <<borrowers.surname>> has added a note to the item <<biblio.title>> -
> <<biblio.author>> (<<biblio.biblionumber>>).' WHERE code = 'PATRON_NOTE';

I see what you mean, but this patch is fixing the content of the note, so it does need to be changed. Unless you can think of another way to fix the note?
Comment 4 Marc Véron 2017-08-18 07:13:40 UTC
cc Marcel because of the initial comment

Maybe a warning in update notes that contents of note have been reset?
However, IMO librarians would not be happy about.
Comment 5 Katrin Fischer 2017-08-18 10:06:18 UTC
I think in another case, when <<items.fine>> was introduced, Dobrica managed to just replace the broken bit. Could this be an option here? Just replacing biblio.item with biblio.title? The old database update could give an example.
Comment 6 Marcel de Rooy 2017-08-21 12:40:37 UTC
(In reply to Aleisha Amohia from comment #3)
> (In reply to Marc Véron from comment #2)
> > What happens if a library has modified the note? - Modifications for title
> > and content would get lost with the following statement in 
> > bug_18915_-_modifying_patron_note_notice.sql 
> > 
> > UPDATE letter SET code = 'CHECKOUT_NOTE', name = 'Checkout note on item set
> > by patron', title = 'Checkout note', content = '<<borrowers.firstname>>
> > <<borrowers.surname>> has added a note to the item <<biblio.title>> -
> > <<biblio.author>> (<<biblio.biblionumber>>).' WHERE code = 'PATRON_NOTE';
> 
> I see what you mean, but this patch is fixing the content of the note, so it
> does need to be changed. Unless you can think of another way to fix the note?

Aleisha,
Could you fix the note by just a SQL replace for the wrong field?
UPDATE table SET field = REPLACE(field ,from_str, to_str) etc etc
So look for biblio.item only and replace that by biblio.title.
If the library changed the note and fixed it already, there will be no problem.

Please adjust this entry too:
installer/data/mysql/updatedatabase.pl:        VALUES ('circulation', 'PATRON_NOTE', '', 'Patron note on item', '0', 'Patron issue note', '<<borrowers.firstname>> <<borrowers.surname>> has added a note to the item <<biblio.item>> - <<biblio.author>> (<<biblio.biblionumber>>).','email');

And do not forget to add PATRON_NOTE (or CHECKOUT_NOTE) to  installer/data/mysql/en/mandatory/sample_notices.sql
It does not occur yet..

Thanks !
Comment 7 Aleisha Amohia 2017-08-27 23:23:57 UTC
Created attachment 66518 [details] [review]
Bug 18915: Fix checkout note email message

This patch replaces the <<biblio.item>> in the email with
<<biblio.title>> and uses CHECKOUT_NOTE instead of PATRON_NOTE.
This patch also adds the notice to
installer/data/mysql/en/mandatory/sample_notices.sql, and updates the
PATRON_NOTE entry in installer/data/mysql/updatedatabase.pl

To test:
1) Apply patch and update database
2) View the message_queue table in mysql
3) Check out an item if haven't already
4) Go to OPAC and set a checkout note for an item
5) View message_queue table and confirm it the title is included in the
email and all instances of 'patron note' have been replaced with
'checkout note'
6) Disable javascript in browser
7) repeat steps 4 and 5 and confirm all works as expected

Sponsored-by: Catalyst IT
Comment 8 M. Tompsett 2017-11-02 12:42:53 UTC
Comment on attachment 66518 [details] [review]
Bug 18915: Fix checkout note email message

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

::: installer/data/mysql/updatedatabase.pl
@@ +14308,4 @@
>  
>      $dbh->do(q|
>          INSERT IGNORE INTO letter (`module`, `code`, `branchcode`, `name`, `is_html`, `title`, `content`, `message_transport_type`)
> +        VALUES ('circulation', 'CHECKOUT_NOTE', '', 'Checkout note on item set by patron', '0', 'Checkout note', '<<borrowers.firstname>> <<borrowers.surname>> has added a note to the item <<biblio.title>> - <<biblio.author>> (<<biblio.biblionumber>>).','email');

I don't think updatedatabase should be modified like this. This is already done. You add a new DB update which tweaks PATRON_NOTE into CHECKOUT_NOTE, if this is correct logic.

::: opac/svc/patron_notes
@@ +82,4 @@
>                  my $biblionumber = $issue->item->biblionumber;
>                  my $letter = C4::Letters::GetPreparedLetter (
>                      module => 'circulation',
> +                    letter_code => 'CHECKOUT_NOTE',

Why even change the letter code, given that you aren't changing the svc name?
Comment 9 Caroline Cyr La Rose 2017-11-03 18:29:26 UTC
The test plan works. 

However, is it normal that if I add <<issues.note>> in the notice it comes back as blank in the message_queue table?

I checked and my notes are in the issues table.
Comment 10 Aleisha Amohia 2017-12-15 00:33:04 UTC
*** Bug 19772 has been marked as a duplicate of this bug. ***
Comment 11 Aleisha Amohia 2017-12-17 23:39:56 UTC
Created attachment 69839 [details] [review]
Bug 18915: Giving CHECKOUT_NOTE access to issues table

This patch allows the notice to reference the issues table so that the
checkout note itself can be added to the note if wanted, such as in
Comment 9.

Sponsored-by: Catalyst IT
Comment 12 Aleisha Amohia 2017-12-17 23:43:45 UTC
(In reply to M. Tompsett from comment #8)
> ::: installer/data/mysql/updatedatabase.pl
> @@ +14308,4 @@
> >  
> >      $dbh->do(q|
> >          INSERT IGNORE INTO letter (`module`, `code`, `branchcode`, `name`, `is_html`, `title`, `content`, `message_transport_type`)
> > +        VALUES ('circulation', 'CHECKOUT_NOTE', '', 'Checkout note on item set by patron', '0', 'Checkout note', '<<borrowers.firstname>> <<borrowers.surname>> has added a note to the item <<biblio.title>> - <<biblio.author>> (<<biblio.biblionumber>>).','email');
> 
> I don't think updatedatabase should be modified like this. This is already
> done. You add a new DB update which tweaks PATRON_NOTE into CHECKOUT_NOTE,
> if this is correct logic.

I thought this change was necessary for fresh installs. If the atomic update is enough then I'm happy to take this out.

> 
> ::: opac/svc/patron_notes
> @@ +82,4 @@
> >                  my $biblionumber = $issue->item->biblionumber;
> >                  my $letter = C4::Letters::GetPreparedLetter (
> >                      module => 'circulation',
> > +                    letter_code => 'CHECKOUT_NOTE',
> 
> Why even change the letter code, given that you aren't changing the svc name?

Mostly so that the name of the note is consistent with the name of the syspref (AllowCheckoutNotes), therefore, the name of the feature. The filename should also be changed as checkout note is more appropriate than patron note anyway - I think this was just a missed case from the original bug.
Comment 13 Aleisha Amohia 2017-12-17 23:48:23 UTC
Created attachment 69840 [details] [review]
Bug 18915: Renaming svc/patron_notes to svc/checkout_notes

For consistency.
Comment 14 Jonathan Druart 2018-01-02 16:26:23 UTC
QA: Please make sure bug 19772 is fixed.
Comment 15 Aleisha Amohia 2018-01-04 22:43:29 UTC
(In reply to Jonathan Druart from comment #14)
> QA: Please make sure bug 19772 is fixed.

Yes this has been fixed. The problem was I'd missed a case when changing PATRON_NOTE to CHECKOUT_NOTE in svc/checkout_notes.
Comment 16 sandboxes@biblibre.com 2018-01-09 15:49:10 UTC
Patch tested with a sandbox, by Marjorie Vila <marjorie.barry-vila@collecto.ca>
Comment 17 sandboxes@biblibre.com 2018-01-09 15:49:53 UTC
Created attachment 70376 [details] [review]
Bug 18915: Fix checkout note email message

This patch replaces the <<biblio.item>> in the email with
<<biblio.title>> and uses CHECKOUT_NOTE instead of PATRON_NOTE.
This patch also adds the notice to
installer/data/mysql/en/mandatory/sample_notices.sql, and updates the
PATRON_NOTE entry in installer/data/mysql/updatedatabase.pl

To test:
1) Apply patch and update database
2) View the message_queue table in mysql
3) Check out an item if haven't already
4) Go to OPAC and set a checkout note for an item
5) View message_queue table and confirm it the title is included in the
email and all instances of 'patron note' have been replaced with
'checkout note'
6) Disable javascript in browser
7) repeat steps 4 and 5 and confirm all works as expected

Sponsored-by: Catalyst IT

Signed-off-by: Marjorie Vila <marjorie.barry-vila@collecto.ca>
Comment 18 sandboxes@biblibre.com 2018-01-09 15:49:57 UTC
Created attachment 70377 [details] [review]
Bug 18915: Giving CHECKOUT_NOTE access to issues table

This patch allows the notice to reference the issues table so that the
checkout note itself can be added to the note if wanted, such as in
Comment 9.

Sponsored-by: Catalyst IT

Signed-off-by: Marjorie Barry-Vila <marjorie.barry-vila@collecto.ca>

https://bugs.koha-community.org/show_bug.cgi?id=19801

Signed-off-by: Marjorie Vila <marjorie.barry-vila@collecto.ca>
Comment 19 sandboxes@biblibre.com 2018-01-09 15:50:00 UTC
Created attachment 70378 [details] [review]
Bug 18915: Renaming svc/patron_notes to svc/checkout_notes

For consistency.

Signed-off-by: Marjorie Barry-Vila <marjorie.barry-vila@collecto.ca>

https://bugs.koha-community.org/show_bug.cgi?id=19801

Signed-off-by: Marjorie Vila <marjorie.barry-vila@collecto.ca>
Comment 20 Marcel de Rooy 2018-01-12 10:35:31 UTC
Created attachment 70460 [details] [review]
Bug 18915: Fix checkout note email message

This patch replaces the <<biblio.item>> in the email with
<<biblio.title>> and uses CHECKOUT_NOTE instead of PATRON_NOTE.
This patch also adds the notice to
installer/data/mysql/en/mandatory/sample_notices.sql, and updates the
PATRON_NOTE entry in installer/data/mysql/updatedatabase.pl

To test:
1) Apply patch and update database
2) View the message_queue table in mysql
3) Check out an item if haven't already
4) Go to OPAC and set a checkout note for an item
5) View message_queue table and confirm it the title is included in the
email and all instances of 'patron note' have been replaced with
'checkout note'
6) Disable javascript in browser
7) repeat steps 4 and 5 and confirm all works as expected

Sponsored-by: Catalyst IT

Signed-off-by: Marjorie Vila <marjorie.barry-vila@collecto.ca>

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 21 Marcel de Rooy 2018-01-12 10:35:36 UTC
Created attachment 70461 [details] [review]
Bug 18915: Giving CHECKOUT_NOTE access to issues table

This patch allows the notice to reference the issues table so that the
checkout note itself can be added to the note if wanted, such as in
Comment 9.

Sponsored-by: Catalyst IT

Signed-off-by: Marjorie Barry-Vila <marjorie.barry-vila@collecto.ca>

https://bugs.koha-community.org/show_bug.cgi?id=19801

Signed-off-by: Marjorie Vila <marjorie.barry-vila@collecto.ca>

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 22 Marcel de Rooy 2018-01-12 10:35:40 UTC
Created attachment 70462 [details] [review]
Bug 18915: Renaming svc/patron_notes to svc/checkout_notes

For consistency.

Signed-off-by: Marjorie Barry-Vila <marjorie.barry-vila@collecto.ca>

https://bugs.koha-community.org/show_bug.cgi?id=19801

Signed-off-by: Marjorie Vila <marjorie.barry-vila@collecto.ca>

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 23 Marcel de Rooy 2018-01-12 10:35:46 UTC
Created attachment 70463 [details] [review]
Bug 18915: (QA follow-up) Add note for installs after 17.05

Since the 17.05 upgrade adds the note and the sql file did not yet include
the note, we should add it when it is not there (for example new 17.11s).

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Tested by running the dbrev while there is a letter and while not.
Comment 24 Jonathan Druart 2018-01-15 14:32:43 UTC
(In reply to Aleisha Amohia from comment #15)
> (In reply to Jonathan Druart from comment #14)
> > QA: Please make sure bug 19772 is fixed.
> 
> Yes this has been fixed. The problem was I'd missed a case when changing
> PATRON_NOTE to CHECKOUT_NOTE in svc/checkout_notes.

No it is not fixed, if the notice template does not exist I do receive a success message "Your note about ITEM_TITLE has been saved and sent to the library."
Comment 25 Jonathan Druart 2018-01-15 15:25:20 UTC
Pushed to master for 18.05, thanks to everybody involved!
Comment 26 Jonathan Druart 2018-01-15 16:43:14 UTC
Created attachment 70492 [details] [review]
Bug 18915: Add CHECKOUT_NOTE notice template for other languages

Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>
Comment 27 Nick Clemens 2018-01-23 04:08:15 UTC
Awesome work all!

Pushed to Stable for 17.11.02
Comment 28 Fridolin SOMERS 2018-01-30 08:27:55 UTC
Pushed to 17.05.x for v17.05.09