Bug 3150 - Move emails for sending cart and list contents into notices tool
Summary: Move emails for sending cart and list contents into notices tool
Status: Needs Signoff
Alias: None
Product: Koha
Classification: Unclassified
Component: Notices (show other bugs)
Version: master
Hardware: All All
: P5 - low enhancement (vote)
Assignee: Aleisha Amohia
QA Contact:
URL:
Keywords: release-notes-needed
: 7983 22931 23882 26786 (view as bug list)
Depends on: 27266
Blocks: 7001
  Show dependency treegraph
 
Reported: 2009-04-23 02:40 UTC by Liz Rea
Modified: 2021-05-05 04:41 UTC (History)
14 users (show)

See Also:
Change sponsored?: Sponsored
Patch complexity: Large patch
Text to go in the release notes:
Version(s) released in:


Attachments
Bug 3150: WIP (15.67 KB, patch)
2020-12-03 02:48 UTC, Aleisha Amohia
Details | Diff | Splinter Review
Bug 3150: WIP (20.06 KB, patch)
2020-12-21 02:12 UTC, Aleisha Amohia
Details | Diff | Splinter Review
Bug 3150: WIP (17.48 KB, patch)
2020-12-21 21:10 UTC, Aleisha Amohia
Details | Diff | Splinter Review
Bug 3150: Move emails for sending cart and list contents to notices (119.78 KB, patch)
2020-12-22 01:38 UTC, Aleisha Amohia
Details | Diff | Splinter Review
Bug 3150: Move emails for sending cart and list contents to notices (119.83 KB, patch)
2021-02-24 05:12 UTC, David Nind
Details | Diff | Splinter Review
Bug 3150: Move emails for sending cart and list contents to notices (119.70 KB, patch)
2021-03-16 11:46 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 3150: Move emails for sending cart and list contents to notices (109.43 KB, patch)
2021-05-05 04:41 UTC, Aleisha Amohia
Details | Diff | Splinter Review
Bug 3150: (follow-up) HTML filtering TT notices and removing old files (135.93 KB, patch)
2021-05-05 04:41 UTC, Aleisha Amohia
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Chris Cormack 2010-05-21 01:07:03 UTC


---- Reported by wizzyrea@gmail.com 2009-04-23 14:40:27 ----

This is currently uneven in application. Holds notifications are editable through the web interface, but cart/list/shelf/suggestion response emails are governed by templates.



--- Bug imported by chris@bigballofwax.co.nz 2010-05-21 01:07 UTC  ---

This bug was previously known as _bug_ 3150 at http://bugs.koha.org/cgi-bin/bugzilla3/show_bug.cgi?id=3150

Actual time not defined. Setting to 0.0
CC member kdr@ckls.org does not have an account here

Comment 1 Liz Rea 2012-02-20 16:32:01 UTC
We should add these templates to the notices system once it is pushed.
Comment 2 Katrin Fischer 2013-01-28 12:52:16 UTC
The one nice thing about the emails being in the templates is, that they are translatable. Maybe we could have an option to send cart/shelf list mails depending on chosen templates?
Comment 3 Katrin Fischer 2016-12-28 14:11:12 UTC
We had a lot of libraries ask about this by now - I didn't like the idea at the beginning because it would mean losing translatability. But there seems to be some recent work on making notices translatable - so that would be great.

Adding other bugs as see alsos:
Bug 17762 - Ability to translate notices
Comment 4 Katrin Fischer 2017-01-04 09:45:48 UTC
*** Bug 7983 has been marked as a duplicate of this bug. ***
Comment 5 Katrin Fischer 2019-05-04 20:08:34 UTC
The only hardcoded notifications at the moment that I can think of that are not dealt with on other bugs (ILL, slips for payment + fines) are cart and list. Changing the bug title to reflect that.

We'll need to make sure they work with the TranslateNotices system preference.
Comment 6 Katrin Fischer 2019-08-18 21:22:59 UTC
*** Bug 22931 has been marked as a duplicate of this bug. ***
Comment 7 Katrin Fischer 2019-11-24 12:53:37 UTC
*** Bug 23882 has been marked as a duplicate of this bug. ***
Comment 8 Katrin Fischer 2020-10-22 18:56:29 UTC
*** Bug 26786 has been marked as a duplicate of this bug. ***
Comment 9 Aleisha Amohia 2020-12-03 02:48:16 UTC
Created attachment 114127 [details] [review]
Bug 3150: WIP

This is not ready yet!
Comment 10 Aleisha Amohia 2020-12-17 00:38:53 UTC
I'm currently working on this, and a big goal is to make it work as a notice with the template toolkit syntax. I believe this depends on moving some of the old C4/Biblio.pm subroutines (GetMarcAuthors etc) into Koha/Biblio.pm. That work would be substantial and needs to be done first, in separate bugs.
Comment 11 Katrin Fischer 2020-12-17 09:35:27 UTC
Could we imagine replacing the old subroutines by a more generic approach of accessing the information in the MARC record instead of moving them? 

Thinking of something like:

[% record.245a %]
We can actually do something similar in CSV profiles, but it appears sadly undocumented in the manual: Bug 12404 (the help file with the documentation was removed, but some examples in the comment of the bug)

I think if we don't match the current output 100% that would actually be a good thing as the old routines sometimes display subfields that you don't really want to see in the notices, and it's all hard coded and not nice to use or change or format. So we will likely have more issues to deal with later on always coming back to these methods.

And it could be really useful for other notices as well (later).
Comment 12 Jonathan Druart 2020-12-17 09:59:51 UTC
Hello Aleisha,

This is a very interesting part to move to the notice templates!

However I am expecting it to be tricky, as you saw already :)

Before starting to implement anything I would suggest you to come with a plan. I can provide you early feedback to see if we are starting with the best solution.

In my opinion we should only pass Koha::Objects-based variables to the template. Reusing the existing variables will make us start with obsolete code (it will be harder to improve it in the future as the libraries will have this TT code in their templates).

(In reply to Katrin Fischer from comment #11)
> Could we imagine replacing the old subroutines by a more generic approach of
> accessing the information in the MARC record instead of moving them? 
> 
> Thinking of something like:
> 
> [% record.245a %]

Says we are talking about GetMarcUrls, it could be something like:
[% SET marc_record = biblio.metadata.record %]
[% FOR f IN marc_record.field('856') %]
  [%# Then do something for every 856 field
[% END %]

But then you are going to rewrite the code that is in a subroutine to a TT syntax, that will be copy/pasted in different places.
And it's not what we want, it's too error prone (no test coverage, dup of code).

The only long term and viable solution is to move them to the Koha namespace.
Have the Get* subroutines moved to Koha::Biblio methods, have the test coverage, replace the existing occurrences, and finally reuse them from the template notices.
But this task is huge...
Comment 13 Jonathan Druart 2020-12-17 10:58:51 UTC
I would start with passing Koha::Biblios to the template and see how easy/hard it is to replace the existing template file with a notice template using the TT syntax.

[% FOR biblio IN biblios %]

  [%# title and subtitle are easy to retrieve %]

  [%# authors, we loop on the MARC fields %]

  [%# isbn, we need Koha::Biblio->get_isbns %]

  [%# and so on...

[% END %]

First with send shelves at the OPAC.
Comment 14 Katrin Fischer 2020-12-17 11:25:52 UTC
Hi Aleisha, 

I think Joubu's right... after looking at the methods I realized that we need something easier to use for people in the templates and also something that will work with MARC21 and UNIMARC. 

Moving them one by one on separate bugs like you suggested is a good idea. Maybe we could use the feedback on bug 16522 or start with one to make sure we got a good pattern before moving all of them.
Comment 15 Aleisha Amohia 2020-12-17 19:57:08 UTC
Yup... What Joubu has said is just a longer way of saying what I said in my comment :) so glad we're all agreed.
Comment 16 Katrin Fischer 2020-12-17 20:05:44 UTC
(In reply to Aleisha Amohia from comment #15)
> Yup... What Joubu has said is just a longer way of saying what I said in my
> comment :) so glad we're all agreed.

Took me a little bit to get there :)
Comment 17 Aleisha Amohia 2020-12-17 23:09:07 UTC
Starting with GetMarcAuthors at Bug 27266
Comment 18 Aleisha Amohia 2020-12-18 01:21:22 UTC
GetMarcNotes at Bug 27268
Comment 19 Aleisha Amohia 2020-12-18 02:35:35 UTC
GetMarcSubjects at Bug 27269
Comment 20 Aleisha Amohia 2020-12-18 02:53:05 UTC
GetMarcSeries at Bug 27270
Comment 21 Aleisha Amohia 2020-12-18 02:53:43 UTC
GetMarcUrls at Bug 27271
Comment 22 Aleisha Amohia 2020-12-18 02:58:33 UTC
GetItemsInfo at Bug 27272
Comment 23 Jonathan Druart 2020-12-18 09:28:04 UTC
(In reply to Aleisha Amohia from comment #20)
> GetMarcSeries at Bug 27270

This is not used and is not needed from send-basket and send-shelves scripts. (ie. no "MARCSERIES" in templates)
Comment 24 Jonathan Druart 2020-12-18 09:29:32 UTC
(In reply to Aleisha Amohia from comment #18)
> GetMarcNotes at Bug 27268

No occurrences of "GetMarcNotes" from send-basket and send-shelves scripts.
Comment 25 Jonathan Druart 2020-12-18 09:46:04 UTC
(In reply to Aleisha Amohia from comment #17)
> Starting with GetMarcAuthors at Bug 27266

Are you sure you will want to reuse that from the template notice?
It seems that to answer bug 17868 you will only need to loop over the MARC subfields (for instance GetMarcAuthors is building links we don't need).

Additional questions: should not we take advantage of the move to fix the inconsistencies in the return values?

I don't think "just moving" code from C4 to Koha is sufficient.
Comment 26 Katrin Fischer 2020-12-18 10:03:28 UTC
(In reply to Jonathan Druart from comment #25)
> (In reply to Aleisha Amohia from comment #17)
> > Starting with GetMarcAuthors at Bug 27266
> 
> Are you sure you will want to reuse that from the template notice?
> It seems that to answer bug 17868 you will only need to loop over the MARC
> subfields (for instance GetMarcAuthors is building links we don't need).
> 
> Additional questions: should not we take advantage of the move to fix the
> inconsistencies in the return values?
> 
> I don't think "just moving" code from C4 to Koha is sufficient.

To further simplify things we could let go of the marcflavour parameter as this only seems to be used in context where the marcflavour of the installation is used.

I think another question is if we are using them only for the notices or to replace all calls to templates - if we want to replace the other calls, we'd need to keep the links etc.
Comment 27 Aleisha Amohia 2020-12-20 20:32:33 UTC
Ah I see what you're both saying. i'll give that a go
Comment 28 Tomás Cohen Arazi 2020-12-20 22:18:30 UTC
In my opinion, we should pass Koha::Object(s) to the template as much as possible. It will be much easier to document the things that are available for rendering, as opposed to the current situation, on which it is not straight forward to know what is available in which context.
Comment 29 Aleisha Amohia 2020-12-21 02:12:43 UTC
Created attachment 114545 [details] [review]
Bug 3150: WIP
Comment 30 Aleisha Amohia 2020-12-21 02:14:28 UTC
I've made some starting changes to the notices to use Koha::Biblio objects mostly (you can see it in the atomic update in the patch).

I need some guidance/help with a few things:
- I wasn't sure where to get the dewey and classification info from
- I wasn't sure which notes field to use (biblioitems or biblio)
- the Author and Subjects subroutines are the hardest for me to refactor because I'm not sure how different libraries use them

Some feedback would be great!
Comment 31 Jonathan Druart 2020-12-21 11:00:29 UTC
(In reply to Aleisha Amohia from comment #30)
> I've made some starting changes to the notices to use Koha::Biblio objects
> mostly (you can see it in the atomic update in the patch).
> 
> I need some guidance/help with a few things:
> - I wasn't sure where to get the dewey and classification info from

It looks like they are always empty.
It's broken since the following commit:
  commit af466ca41ab02150f4f6c60bf6262b079b2e6e85
  Date:   Thu Oct 18 18:53:52 2007 -0500
  start of reworking call number handling and other Biblio changes

IMO we should not reintroduce them on this bug report.

> - I wasn't sure which notes field to use (biblioitems or biblio)

It's coming GetBiblioData
 729     my $query = " SELECT * , biblioitems.notes AS bnotes, itemtypes.notforloan as bi_notforloan, biblio.notes                                                                                                

So it's the 'notes' from biblio.

> - the Author and Subjects subroutines are the hardest for me to refactor
> because I'm not sure how different libraries use them

Hum... Another weirdness.
On opac-sendshelf: MARCSUBJCTS is set but not used in the template, 'subject' is used instead, but never sent from the pl...
and it's the same for the other ones (sendbasket, sendshelf both OPAC and staff interfaces).
I'd drop it as well.

For MARCAUTHORS I'd try and write the equivalent of C4::Biblio::GetMarcAuthors in TT, and see how it goes (10 lines or 30 lines?). If it's too ugly then move to Koha::Biblio->get_MARC_authors (or get_authors_from_MARC?)

> Some feedback would be great!

Don't forget that you are in a quite dirty place. Don't loose time with things that are not working, drop them.
I'd suggest that you focus on the simplest template first, no need to work in parallel on the four ones.

Additional note, in your patch you are not looping at all on a Koha::Biblio set. As it's a WIP maybe you are aware of it, but I prefer to highlight it in case you missed it.
Comment 32 Aleisha Amohia 2020-12-21 21:10:44 UTC
Created attachment 114579 [details] [review]
Bug 3150: WIP
Comment 33 Aleisha Amohia 2020-12-22 01:38:07 UTC
Created attachment 114582 [details] [review]
Bug 3150: Move emails for sending cart and list contents to notices

This patch creates notices using Template Toolkit syntax for sending
emails containing cart and list contents.

To test:
1. Apply Bug 27266
2. Run update database and restart services
3. In the staff client, add multiple items to your cart and to a list
4. Go to your cart and click Send to email the contents
5. Add an email and a comment and click Send
6. Confirm the information shown in the success message is correct
7. In your terminal, log into the database. View the message queue ( i.e. select * from message_queue; ). Confirm that your email has been queued and the content is all correct. Confirm the cart contents has been included as an attachment.
8. Go to your list and click Send list to email the contents
9. Repeat steps 5-7
10. Log into the OPAC
11. Add multiple items to your cart and to a list
12. Repeat steps 4-9
13. By the end, you should have four emails in your message queue. All
of the data about the items should be correct, they should all have
attachments, and be addressed to the correct email address.

Sponsored-by: Bibliotheksservice-Zentrum Baden-Württemberg (BSZ)
Comment 34 David Nind 2021-02-24 05:12:08 UTC
Created attachment 117237 [details] [review]
Bug 3150: Move emails for sending cart and list contents to notices

This patch creates notices using Template Toolkit syntax for sending
emails containing cart and list contents.

To test:
1. Apply Bug 27266
2. Run update database and restart services
3. In the staff client, add multiple items to your cart and to a list
4. Go to your cart and click Send to email the contents
5. Add an email and a comment and click Send
6. Confirm the information shown in the success message is correct
7. In your terminal, log into the database. View the message queue ( i.e. select * from message_queue; ). Confirm that your email has been queued and the content is all correct. Confirm the cart contents has been included as an attachment.
8. Go to your list and click Send list to email the contents
9. Repeat steps 5-7
10. Log into the OPAC
11. Add multiple items to your cart and to a list
12. Repeat steps 4-9
13. By the end, you should have four emails in your message queue. All
of the data about the items should be correct, they should all have
attachments, and be addressed to the correct email address.

Sponsored-by: Bibliotheksservice-Zentrum Baden-Württemberg (BSZ)

Signed-off-by: David Nind <david@davidnind.com>
Comment 35 David Nind 2021-02-24 05:20:50 UTC
Testing notes (koha-testing-docker):
- I was able to test the emails being sent by copying the HTML for the message from the database and pasting it into a Thunderbird email message (Insert > HTML) - everything looked okay to me.
- I wasn't able to properly test the attachment, but I'm going to assume the code creates a valid iso2709 file for attaching, sending, and saving/viewing by the recipient.
- There was some minor things I noticed with the email input forms (the ones that pop up after you click the email buttons in Koha) - the 'Comment' label for the input box follows directly after the email address input box. Depending on the width of the pop up window the label isn't aligned properly to the left, but this behaviour is the same as what it was before the patch was applied.
Comment 36 Martin Renvoize 2021-03-16 11:46:48 UTC
Created attachment 118313 [details] [review]
Bug 3150: Move emails for sending cart and list contents to notices

This patch creates notices using Template Toolkit syntax for sending
emails containing cart and list contents.

To test:
1. Apply Bug 27266
2. Run update database and restart services
3. In the staff client, add multiple items to your cart and to a list
4. Go to your cart and click Send to email the contents
5. Add an email and a comment and click Send
6. Confirm the information shown in the success message is correct
7. In your terminal, log into the database. View the message queue ( i.e. select * from message_queue; ). Confirm that your email has been queued and the content is all correct. Confirm the cart contents has been included as an attachment.
8. Go to your list and click Send list to email the contents
9. Repeat steps 5-7
10. Log into the OPAC
11. Add multiple items to your cart and to a list
12. Repeat steps 4-9
13. By the end, you should have four emails in your message queue. All
of the data about the items should be correct, they should all have
attachments, and be addressed to the correct email address.

Sponsored-by: Bibliotheksservice-Zentrum Baden-Württemberg (BSZ)
Signed-off-by: David Nind <david@davidnind.com>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 37 Martin Renvoize 2021-03-16 11:49:27 UTC
This feels good in testing and QA scripts are passing.

Well done working through this Aleisha, I imagine it was rather painful.

Passing QA

(Caveat applies for dependanct bug, I think get_marc_authors is nicer than get_authors_from_MARC).
Comment 38 Jonathan Druart 2021-04-07 10:25:05 UTC
Why don't you HTML filter the variables in the template notice?
Comment 39 Jonathan Druart 2021-04-07 10:37:32 UTC
Do we still need the following files?

koha-tmpl/intranet-tmpl/prog/en/modules/basket/sendbasket.tt
koha-tmpl/intranet-tmpl/prog/en/modules/virtualshelves/sendshelf.tt
koha-tmpl/opac-tmpl/bootstrap/en/modules/opac-sendbasket.tt
koha-tmpl/opac-tmpl/bootstrap/en/modules/opac-sendshelf.tt
Comment 40 Aleisha Amohia 2021-05-05 04:32:14 UTC
(In reply to Jonathan Druart from comment #38)
> Why don't you HTML filter the variables in the template notice?

The original notices use $raw filtering (sendbasket.tt etc), so that's what I used. I can change them all to HTML.

(In reply to Jonathan Druart from comment #39)
> Do we still need the following files?
> 
> koha-tmpl/intranet-tmpl/prog/en/modules/basket/sendbasket.tt
> koha-tmpl/intranet-tmpl/prog/en/modules/virtualshelves/sendshelf.tt
> koha-tmpl/opac-tmpl/bootstrap/en/modules/opac-sendbasket.tt
> koha-tmpl/opac-tmpl/bootstrap/en/modules/opac-sendshelf.tt

I don't think so, I'll remove them.
Comment 41 Aleisha Amohia 2021-05-05 04:41:50 UTC
Created attachment 120477 [details] [review]
Bug 3150: Move emails for sending cart and list contents to notices

This patch creates notices using Template Toolkit syntax for sending
emails containing cart and list contents.

To test:
1. Apply Bug 27266
2. Run update database and restart services
3. In the staff client, add multiple items to your cart and to a list
4. Go to your cart and click Send to email the contents
5. Add an email and a comment and click Send
6. Confirm the information shown in the success message is correct
7. In your terminal, log into the database. View the message queue ( i.e. select * from message_queue; ). Confirm that your email has been queued and the content is all correct. Confirm the cart contents has been included as an attachment.
8. Go to your list and click Send list to email the contents
9. Repeat steps 5-7
10. Log into the OPAC
11. Add multiple items to your cart and to a list
12. Repeat steps 4-9
13. By the end, you should have four emails in your message queue. All
of the data about the items should be correct, they should all have
attachments, and be addressed to the correct email address.

Sponsored-by: Bibliotheksservice-Zentrum Baden-Württemberg (BSZ)
Signed-off-by: David Nind <david@davidnind.com>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 42 Aleisha Amohia 2021-05-05 04:41:56 UTC
Created attachment 120478 [details] [review]
Bug 3150: (follow-up) HTML filtering TT notices and removing old files