Bug 12426 - Allow resending of emails from the notices tab in the patron account
Summary: Allow resending of emails from the notices tab in the patron account
Status: CLOSED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: Notices (show other bugs)
Version: master
Hardware: All All
: P5 - low enhancement (vote)
Assignee: Lari Taskula
QA Contact: Marcel de Rooy
URL:
Keywords:
Depends on:
Blocks: 14723 14843 14855 18439
  Show dependency treegraph
 
Reported: 2014-06-15 14:12 UTC by Katrin Fischer
Modified: 2017-12-07 22:16 UTC (History)
12 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:


Attachments
Bug 12426: Allow resending of messages from the notices tab in the patron account (3.55 KB, patch)
2015-09-03 12:03 UTC, Lari Taskula
Details | Diff | Splinter Review
[Signed-off] Bug 12426: Allow resending of messages from the notices tab in the patron account (3.74 KB, patch)
2015-09-03 15:34 UTC, Marc Véron
Details | Diff | Splinter Review
Bug 12426: Allow resending of messages from the notices tab in the patron account (5.43 KB, patch)
2015-09-04 13:51 UTC, Lari Taskula
Details | Diff | Splinter Review
Bug 12426: Allow resending of messages from the notices tab in the patron account (5.40 KB, patch)
2015-09-04 14:52 UTC, Lari Taskula
Details | Diff | Splinter Review
[Signed-off] Bug 12426: Allow resending of messages from the notices tab in the patron account (5.57 KB, patch)
2015-09-05 06:19 UTC, Marc Véron
Details | Diff | Splinter Review
Bug 12426: Simplify the code adding a new subroutine GetMessage (6.73 KB, patch)
2015-09-14 10:39 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 12426: Allow resend for sent messages (5.42 KB, patch)
2015-09-16 15:43 UTC, Lari Taskula
Details | Diff | Splinter Review
Bug 12426: Allow resend for sent messages (5.45 KB, patch)
2015-09-16 16:00 UTC, Lari Taskula
Details | Diff | Splinter Review
Bug 12426: Allow resend for sent messages (5.22 KB, patch)
2015-09-16 16:35 UTC, Lari Taskula
Details | Diff | Splinter Review
Bug 12426: Allow resend for sent messages (4.23 KB, patch)
2015-09-16 16:49 UTC, Lari Taskula
Details | Diff | Splinter Review
[Signed-off] Bug 12426: Allow resending of messages from the notices tab in the patron account (5.54 KB, patch)
2016-02-02 18:55 UTC, M. Tompsett
Details | Diff | Splinter Review
Bug 12426: Simplify the code adding a new subroutine GetMessage (6.65 KB, patch)
2016-02-02 18:56 UTC, M. Tompsett
Details | Diff | Splinter Review
Bug 12426: Allow resend for sent messages (4.23 KB, patch)
2016-02-02 18:56 UTC, M. Tompsett
Details | Diff | Splinter Review
Bug 12426: Allow resending of messages from the notices tab in the patron account (5.55 KB, patch)
2016-02-03 14:04 UTC, Marc Véron
Details | Diff | Splinter Review
Bug 12426: Simplify the code adding a new subroutine GetMessage (6.65 KB, patch)
2016-02-03 14:05 UTC, Marc Véron
Details | Diff | Splinter Review
Bug 12426: Allow resend for sent messages (4.42 KB, patch)
2016-02-03 14:10 UTC, Marc Véron
Details | Diff | Splinter Review
Bug 12426: Allow resending of messages from the notices tab in the patron account (5.55 KB, patch)
2016-02-12 10:31 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 12426: Simplify the code adding a new subroutine GetMessage (6.75 KB, patch)
2016-02-12 10:32 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 12426: Allow resend for sent messages (4.42 KB, patch)
2016-02-12 10:32 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 12426: [QA Follow-up] Clear to_address to force update (1.83 KB, patch)
2016-02-15 08:29 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 12426: Allow resending of messages from the notices tab in the patron account (5.54 KB, patch)
2016-03-03 03:44 UTC, M. Tompsett
Details | Diff | Splinter Review
Bug 12426: Simplify the code adding a new subroutine GetMessage (6.65 KB, patch)
2016-03-03 03:44 UTC, M. Tompsett
Details | Diff | Splinter Review
Bug 12426: Allow resend for sent messages (4.23 KB, patch)
2016-03-03 03:44 UTC, M. Tompsett
Details | Diff | Splinter Review
Bug 12426: [QA Follow-up] Clear to_address to force update (1.78 KB, patch)
2016-03-03 03:44 UTC, M. Tompsett
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Katrin Fischer 2014-06-15 14:12:19 UTC
Add a link to allow resending a notice from the notices tab in the patron account.

Sometimes the email address in the patron record is wrong and the library wants to resend the notices after correcting the email address.
Comment 1 Lari Taskula 2015-09-03 12:03:01 UTC Comment hidden (obsolete)
Comment 2 Marc Véron 2015-09-03 15:34:42 UTC Comment hidden (obsolete)
Comment 3 Jonathan Druart 2015-09-03 16:18:32 UTC
Lari,
I really would prefer to see a new subroutine added to C4::Letters covered by tests.
Something like C4::Letters::ResendMessage($message_id).
Comment 4 Lari Taskula 2015-09-04 13:51:48 UTC Comment hidden (obsolete)
Comment 5 Marc Véron 2015-09-04 14:05:08 UTC
Lari, is the first patch obsolete?
Comment 6 Lari Taskula 2015-09-04 14:10:26 UTC
Marc, yes! Sorry, I forgot to do that. I made some modifications by Jonathan's wish.
Comment 7 Lari Taskula 2015-09-04 14:52:17 UTC Comment hidden (obsolete)
Comment 8 Marc Véron 2015-09-05 06:19:29 UTC Comment hidden (obsolete)
Comment 9 Jonathan Druart 2015-09-14 10:39:04 UTC Comment hidden (obsolete)
Comment 10 Jonathan Druart 2015-09-14 10:39:41 UTC
Lari, can I get your opinion on the second patch?
Comment 11 Lari Taskula 2015-09-14 12:00:38 UTC
Jonathan,

I agree we need indeed a way to get messages by id. In fact this will be very useful for me right now because of another bug that requires to get messages by id. 

Simplifying the code in notices.pl is well done. It is a good idea to move the status "failed" check into ResendMessage() so that it cannot be accidentally called for sent messages. I originally thought it would be up to the programmer to take care of the status check, and allow if he is sure he wants to resend a sent message, but I don't really see why anyone would want to do this. Maybe it is best to make the check inside ResendMessage().

Thanks for the help!
Comment 12 Katrin Fischer 2015-09-14 12:29:21 UTC
I think it could be very useful for a library to be able to resent messages that don't have a "failed" status - for example: the library has typoed in the email address. In this cases the email is sent, but bounced back to the library's email address (email account doesn't exist at this provider etc.) and it's up to the library to correct the email address. But the status will be sent.
I have been asked a lot if there is a way to trigger a notice again - which is why I filed this bug :)
Comment 13 Lari Taskula 2015-09-16 09:27:50 UTC
(In reply to Katrin Fischer from comment #12)
> I think it could be very useful for a library to be able to resent messages
> that don't have a "failed" status - for example: the library has typoed in
> the email address. In this cases the email is sent, but bounced back to the
> library's email address (email account doesn't exist at this provider etc.)
> and it's up to the library to correct the email address. But the status will
> be sent.
> I have been asked a lot if there is a way to trigger a notice again - which
> is why I filed this bug :)

That's a very good point. For some reason I had been ignoring this :) With the current patch it is a bit risky, however. As Jonathan mentioned problems with browser history,

(In reply to Jonathan Druart from comment #9)
> Note that ResendNotice only resends failed messages. This will avoid to
> resend already sent messages (using an url from the browser history for
> instance).

I suggest we change the method of request from GET and make it via AJAX calls to avoid problems caused by browser history. I have been working on REST API for Notices and have done all basic operations including resend. It would be nice to see this feature use the REST API via AJAX calls. Any support for this idea?
Comment 14 Katrin Fischer 2015-09-16 09:54:31 UTC
Hm not sure if I understand the browser history problem. Why would a link in the notices table to resend (with old fashioned reload or Ajax) not work?
Comment 15 Katrin Fischer 2015-09-16 10:04:20 UTC
Mostly asking because the REST API is not there yet - it would be nice if this small but great improvement would not be blocked by the bigger development.
Comment 16 Jonathan Druart 2015-09-16 10:06:25 UTC
(In reply to Lari Taskula from comment #13)

> I suggest we change the method of request from GET and make it via AJAX
> calls to avoid problems caused by browser history. I have been working on
> REST API for Notices and have done all basic operations including resend. It
> would be nice to see this feature use the REST API via AJAX calls. Any
> support for this idea?

(In reply to Katrin Fischer from comment #15)
> Mostly asking because the REST API is not there yet - it would be nice if
> this small but great improvement would not be blocked by the bigger
> development.

Exactly what I was going to answer :)
Comment 17 Jonathan Druart 2015-09-16 10:08:04 UTC
(In reply to Katrin Fischer from comment #14)
> Hm not sure if I understand the browser history problem. Why would a link in
> the notices table to resend (with old fashioned reload or Ajax) not work?

Clicking on the resend link, will call /cgi-bin/koha/script.pl?op=resend&message_id=42 (or equivalent).
If you don't limit the resend to fail messages, a user could unexpectedly recall this url, and resend again the message.
Comment 18 Jonathan Druart 2015-09-16 10:08:32 UTC
So AJAX call or via POST could solved the issue
Comment 19 Lari Taskula 2015-09-16 10:34:21 UTC
Katrin,

As Jonathan mentioned, AJAX would work fine but (with current patch) the following link 

    cgi-bin/koha/members/notices.pl?borrowernumber=1&resendnotice=4

would stay in your browser history. Let's imagine we clicked the link and the message was resent perfectly fine and we want to forget about it. But since it's in the browser history, a staff member can accidentally get back on this page by using the address bar, without realizing that this will resend the message once again.

And yes, I understand your concern - it is very problematic that it would be blocked by the REST API since it's not there yet. So what would be the point to use it for such a small feature?

But here we are very optimistic and excited about the REST API. I think it is a great opportunity to keep pushing it forward by making features already dependant on it. To use this nice feature you would need to use the REST API - great way to encourage other people to check it out and learn about it :)
Comment 20 Katrin Fischer 2015-09-16 10:38:18 UTC
Everyone is very excited - but woudl be good to have at least the base patches in Koha first.
Comment 21 Jonathan Druart 2015-09-16 10:59:42 UTC
(In reply to Lari Taskula from comment #19)
> But here we are very optimistic and excited about the REST API. I think it
> is a great opportunity to keep pushing it forward by making features already
> dependant on it. To use this nice feature you would need to use the REST API
> - great way to encourage other people to check it out and learn about it :)

I personally just mark these patches as BLOCKED since the base is not accepted/validated/etc.
It just makes me think "this work will have to rebase for ages".
Comment 22 Tom Misilo 2015-09-16 11:08:00 UTC
Would a possible compromise be, accept as is without using the REST API and without being able to resend failed messages? With the understanding that a new bug # be created that enhances this one by using the REST API? That way the functionality libraries have been looking for more then a year is added.
Comment 23 Katrin Fischer 2015-09-16 11:38:38 UTC
Hi Misilo, I think failed will only cover a really small percentage. I think making it work for all status using a POST could be a little nicer.
Comment 24 Lari Taskula 2015-09-16 15:43:40 UTC Comment hidden (obsolete)
Comment 25 Lari Taskula 2015-09-16 16:00:24 UTC Comment hidden (obsolete)
Comment 26 Jonathan Druart 2015-09-16 16:11:37 UTC
Comment on attachment 42618 [details] [review]
Bug 12426: Allow resend for sent messages

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

+ Your patch introduces trailing whitespaces.

::: members/notices.pl
@@ +58,5 @@
>      my $message = C4::Letters::GetMessage( $message_id );
>      if ( $message->{borrowernumber} = $borrowernumber ) {
>          C4::Letters::ResendMessage( $message_id );
> +        # redirect to self to avoid form submission on refresh
> +        print $input->redirect("/cgi-bin/koha/members/notices.pl?borrowernumber=$borrowernumber");

Not sure this is useful, what will happen if the form is resubmit?
That will change the status from pending to pending? :)

::: t/db_dependent/Letters.t
@@ +18,4 @@
>  # along with Koha; if not, see <http://www.gnu.org/licenses>.
>  
>  use Modern::Perl;
> +use Test::More tests => 68;

I'd say we can keep the test you have removed, and adapt it.
So we check a message can be resend independently of its status.
Comment 27 Jonathan Druart 2015-09-16 16:14:04 UTC
(I should reread my comments before sending them...)
Comment 28 Lari Taskula 2015-09-16 16:18:15 UTC
(In reply to Jonathan Druart from comment #26)
> Comment on attachment 42618 [details] [review] [review]
> Bug 12426: Allow resend for sent messages
> 
> Review of attachment 42618 [details] [review] [review]:
> -----------------------------------------------------------------
> 
> + Your patch introduces trailing whitespaces.
> 
> ::: members/notices.pl
> @@ +58,5 @@
> >      my $message = C4::Letters::GetMessage( $message_id );
> >      if ( $message->{borrowernumber} = $borrowernumber ) {
> >          C4::Letters::ResendMessage( $message_id );
> > +        # redirect to self to avoid form submission on refresh
> > +        print $input->redirect("/cgi-bin/koha/members/notices.pl?borrowernumber=$borrowernumber");
> 
> Not sure this is useful, what will happen if the form is resubmit?
> That will change the status from pending to pending? :)

Without this, when refreshing the page, the browser will ask for resending the form. Some staff member can click refresh to see if the resent message is now in sent status and accidentally put it for resend once again. When we redirect back to notices.pl, form cannot be resent again (at least this was the behavior on my Firefox) even if he/she clicks refresh. It will simply refresh the page without asking/sending any form data again.

> ::: t/db_dependent/Letters.t
> @@ +18,4 @@
> >  # along with Koha; if not, see <http://www.gnu.org/licenses>.
> >  
> >  use Modern::Perl;
> > +use Test::More tests => 68;
> 
> I'd say we can keep the test you have removed, and adapt it.
> So we check a message can be resend independently of its status.

Okay, I'll update it.
Comment 29 Katrin Fischer 2015-09-16 16:18:39 UTC
Hm one idea - should we exclude pending from being resent?
Comment 30 Lari Taskula 2015-09-16 16:35:06 UTC Comment hidden (obsolete)
Comment 31 Lari Taskula 2015-09-16 16:49:39 UTC Comment hidden (obsolete)
Comment 32 Katrin Fischer 2015-10-22 19:46:33 UTC
Would be really nice to see a sign-off for this asap! Please?
Comment 33 Marc Véron 2015-10-22 20:49:52 UTC
I'm sorry, conflict with first patch:

CONFLICT (content): Merge conflict in t/db_dependent/Letters.t
Comment 34 M. Tompsett 2016-02-02 18:55:55 UTC Comment hidden (obsolete)
Comment 35 M. Tompsett 2016-02-02 18:56:03 UTC Comment hidden (obsolete)
Comment 36 M. Tompsett 2016-02-02 18:56:09 UTC Comment hidden (obsolete)
Comment 37 M. Tompsett 2016-02-02 19:02:53 UTC
Except for moving the $op line, this is a rebase, which applies. I have not signed off. The sign off change is identical, so I left it marked as such.
Comment 38 Marc Véron 2016-02-03 14:04:13 UTC Comment hidden (obsolete)
Comment 39 Marc Véron 2016-02-03 14:05:31 UTC Comment hidden (obsolete)
Comment 40 Marc Véron 2016-02-03 14:10:40 UTC Comment hidden (obsolete)
Comment 41 Marcel de Rooy 2016-02-12 08:44:25 UTC
QA: Looking at this one now.
Comment 42 Marcel de Rooy 2016-02-12 10:31:53 UTC Comment hidden (obsolete)
Comment 43 Marcel de Rooy 2016-02-12 10:32:00 UTC Comment hidden (obsolete)
Comment 44 Marcel de Rooy 2016-02-12 10:32:06 UTC Comment hidden (obsolete)
Comment 45 Marcel de Rooy 2016-02-12 10:41:47 UTC
Perhaps we could also think about clearing the to_address and reading it again (as is done in SendQueuedLetters already).

You could have adjusted the patron's email address (possible trigger for resending an email) and now you want to resend to the new/corrected email address.
Comment 46 Katrin Fischer 2016-02-12 10:52:01 UTC
Ah, I thought it always rechecked before sending it - maybe in another piece of code?
Comment 47 Jonathan Druart 2016-02-12 11:08:03 UTC
(In reply to Marcel de Rooy from comment #45)
> Perhaps we could also think about clearing the to_address and reading it
> again (as is done in SendQueuedLetters already).
> 
> You could have adjusted the patron's email address (possible trigger for
> resending an email) and now you want to resend to the new/corrected email
> address.

That's a very good remark and the expected behavior.
IMO it should be a requirement for this patch to be pushed (as it's very easy to implement).

(In reply to Katrin Fischer from comment #46)
> Ah, I thought it always rechecked before sending it - maybe in another piece
> of code?

Usually the to_address is not set when inserting the message to the message queue.
The _send_message_by_email get the patron's email address only if the message does not have one.
Comment 48 Marcel de Rooy 2016-02-15 08:29:21 UTC Comment hidden (obsolete)
Comment 49 Marcel de Rooy 2016-02-15 08:30:46 UTC
(In reply to Jonathan Druart from comment #47)
> That's a very good remark and the expected behavior.
> IMO it should be a requirement for this patch to be pushed (as it's very
> easy to implement).
OK. Added a follow-up. Thx
Comment 50 Brendan Gallagher 2016-03-02 22:55:38 UTC
Didn't apply cleanly - I know because of the moving target that master is.  Please rebase.  Once it's done - leave me an @later in #koha and I'll jump right on it.  Thank you so much for the time and bother of rebasing.

Brendan
Comment 51 M. Tompsett 2016-03-03 03:44:40 UTC
Created attachment 48570 [details] [review]
Bug 12426: Allow resending of messages from the notices tab in the patron account

This patch adds a link 'Resend' under a notice in 'failed' status
in the Patron's Notices tab.

By clicking the link, we will request notices.pl with parameter
"resendnotice=XXXXX" where XXXXX is message_id. In notices.pl,
we then check whether the given message is actually in 'failed'
status. If so, we use the C4::Letters::ResendMessage(123) to
change the status of the message into 'pending'. This way it
will be processed again by the cronjob process_message_queue.pl.

To test, find/create a Patron that has failed notices in message_queue:
1. Enable EnchancedMessagingPreferences system preference
2. Go to Patrons -> Notices
3. In the Notice column, click the title of the failed message
4. Observe that there is nothing for resending the failed message
5. Apply patch.
6. Reload Notices page and repeat step 3
7. Observe that there is now a link "Resend" in the Status-column
8. Click Resend
9. Observe that the message gets into 'pending' status

Works as expected.
Signed-off-by: Marc Véron <veron@veron.ch>
Comment 52 M. Tompsett 2016-03-03 03:44:46 UTC
Created attachment 48571 [details] [review]
Bug 12426: Simplify the code adding a new subroutine GetMessage

The C4::Letters module does not have a GetMessage subroutine, which
could be quite useful.
This patch adds it and simplifies the code added by the previous patch.
It also adds a few tests and fixes POD typos.

Note that ResendNotice only resends failed messages. This will avoid to
resend already sent messages (using an url from the browser history for
instance).
Comment 53 M. Tompsett 2016-03-03 03:44:52 UTC
Created attachment 48572 [details] [review]
Bug 12426: Allow resend for sent messages

This patch allows to resend both sent and failed messages.

With messages in 'sent' status, we have to be careful not to accidentally send
sent messages again. With the previous patch using GET request, this was likely
to happen because of browser storing the GET parameters.

This patch changes request method from GET to POST. Instead of a simple link,
we now have a form element.

In notices.pl we redirect back to notices.pl, because with POST there is a risk
of resending the message accidentally by form resubmission at refresh.

To test, find/create a Patron that has sent or failed notices in message_queue:
1. Enable EnchancedMessagingPreferences system preference
2. Go to Patrons -> Notices
3. In the Notice column, click the title of the sent or failed message
4. Observe that there is nothing for resending the sent or failed message
5. Apply the patches.
6. Reload Notices page and repeat step 3
7. Observe that there is now a link "Resend" in the Status-column
8. Click Resend
9. Observe that the message gets into 'pending' status
Comment 54 M. Tompsett 2016-03-03 03:44:57 UTC
Created attachment 48573 [details] [review]
Bug 12426: [QA Follow-up] Clear to_address to force update

When resending an email from the Notices tab in Patrons, we would like
to use the recent email address.

Test plan:
[1] Look up a patron with some notices sent.
[2] Adjust the patron email address (watch AutoEmailPrimaryAddress).
[3] Resend the notice. Verify if the new address was used.

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 55 M. Tompsett 2016-03-03 03:51:44 UTC
Sorry, Marcel, my rebase lost your sign-off markers at the end of the files.
Comment 56 Marcel de Rooy 2016-03-03 07:28:54 UTC
Putting this back where it came from (PQA)
Any reason why you moved this back to SO, Mark?
If it was a hard rebase, getting another signoff should imo go via NSO.
Comment 57 M. Tompsett 2016-03-03 12:11:49 UTC
(In reply to Marcel de Rooy from comment #56)
> Putting this back where it came from (PQA)
> Any reason why you moved this back to SO, Mark?
> If it was a hard rebase, getting another signoff should imo go via NSO.

Because I wasn't sure what status I should move it to, but it wasn't hard.
And your sign-offs were lost, because I didn't have them to start, and I forgot to add them.
Comment 58 Brendan Gallagher 2016-03-03 20:17:12 UTC
Pushed to Master - Should be in the May 2016 release.   Thanks!