Bug 17866 - Change sender for claim and order notices
Summary: Change sender for claim and order notices
Status: CLOSED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: Notices (show other bugs)
Version: master
Hardware: All All
: P5 - low normal (vote)
Assignee: Marcel de Rooy
QA Contact: Testopia
URL:
Keywords:
Depends on:
Blocks: 18368 18413
  Show dependency treegraph
 
Reported: 2017-01-09 14:09 UTC by Marcel de Rooy
Modified: 2017-12-07 22:15 UTC (History)
4 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 17866: Change sender for serial claim notifications (1.72 KB, patch)
2017-01-09 14:15 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 17866: Change sender for serial claim notifications (1.65 KB, patch)
2017-01-09 14:16 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 17866: Change sender for serial claim notifications (1.65 KB, patch)
2017-01-10 08:16 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 17866: Change sender for serial claim notifications (1.66 KB, patch)
2017-01-17 01:25 UTC, Gracie
Details | Diff | Splinter Review
Bug 17866: [QA Follow-up] Add notification for changed sender address (1.19 KB, patch)
2017-03-22 15:52 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 17866: [QA Follow-up] Reword message for missing email address (2.36 KB, patch)
2017-03-23 09:16 UTC, Marcel de Rooy
Details | Diff | Splinter Review
[PUSHED] Bug 17866: Change sender for serial claim notifications (1.73 KB, patch)
2017-03-23 16:36 UTC, Jonathan Druart
Details | Diff | Splinter Review
[PUSHED] Bug 17866: [QA Follow-up] Add notification for changed sender address (1.22 KB, patch)
2017-03-23 16:36 UTC, Jonathan Druart
Details | Diff | Splinter Review
[PUSHED] Bug 17866: [QA Follow-up] Reword message for missing email address (2.39 KB, patch)
2017-03-23 16:36 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 17866: [QA Follow-up] Add forgotten SetVersion call (1.13 KB, patch)
2017-04-03 06:50 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 17866: [QA Follow-up] Change wording of db revision (1.58 KB, patch)
2017-04-03 07:24 UTC, Marcel de Rooy
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Marcel de Rooy 2017-01-09 14:09:21 UTC
Currently, the From field uses the emailaddress of the staff client member from the user context.
This is exceptional in Koha. And might very well, as in our case, be the cause of fraud detection warnings.
We should branch email address or fallback to KohaAdminEmailAddress, as we do (almost) everywhere else.
Comment 1 Marcel de Rooy 2017-01-09 14:15:14 UTC Comment hidden (obsolete)
Comment 2 Marcel de Rooy 2017-01-09 14:16:04 UTC Comment hidden (obsolete)
Comment 3 Marcel de Rooy 2017-01-09 14:17:37 UTC Comment hidden (obsolete)
Comment 4 Marcel de Rooy 2017-01-10 08:16:04 UTC
Created attachment 58721 [details] [review]
Bug 17866: Change sender for serial claim notifications

Currently, the From field uses the emailaddress of the staff client member
from the user context. This is exceptional in Koha. And might very well be,
as in our case, the cause of fraud detection warnings.
We should use branch email address or fallback to KohaAdminEmailAddress,
as we do (almost) everywhere else.

Test plan:
Go to subscription detail in Koha.
Go to Claims.
Select a missing issue and click Send notification.
Verify the sender address in the generated notification. (Make sure that
you receive this mail.)
Comment 5 Gracie 2017-01-17 01:25:26 UTC
Created attachment 59043 [details] [review]
Bug 17866: Change sender for serial claim notifications

Currently, the From field uses the emailaddress of the staff client member
from the user context. This is exceptional in Koha. And might very well be,
as in our case, the cause of fraud detection warnings.
We should use branch email address or fallback to KohaAdminEmailAddress,
as we do (almost) everywhere else.

Test plan:
Go to subscription detail in Koha.
Go to Claims.
Select a missing issue and click Send notification.
Verify the sender address in the generated notification. (Make sure that
you receive this mail.)

Signed-off-by: Grace McKenzie <grace.mcky@gmail.com>
Comment 6 Jonathan Druart 2017-01-17 10:54:24 UTC
Indeed that sounds like a wrong behaviour, but some are using it for ages. Maybe we should warn them that the original behaviour has changed?

There is something else: SendALerts calls from serials/claims.pl and acqui/lateorders.pl are checking the error returned and send and error code "no_loggedin_user_email" or "error_claim".
At least the claims.tt template need to be updated to reflect that change.
Comment 7 Marcel de Rooy 2017-01-17 11:14:05 UTC
(In reply to Jonathan Druart from comment #6)
> Indeed that sounds like a wrong behaviour, but some are using it for ages.
> Maybe we should warn them that the original behaviour has changed?
> 
> There is something else: SendALerts calls from serials/claims.pl and
> acqui/lateorders.pl are checking the error returned and send and error code
> "no_loggedin_user_email" or "error_claim".
> At least the claims.tt template need to be updated to reflect that change.

I could add a dbrev with a warning/statement. Should not be a big deal imo btw.

The claims.tt template correction is not in the scope of this report. I only change the sender address. Do not mangle with return values of SendAlerts.
Comment 8 Jonathan Druart 2017-01-17 11:20:41 UTC
(In reply to Marcel de Rooy from comment #7)
> The claims.tt template correction is not in the scope of this report. I only
> change the sender address. Do not mangle with return values of SendAlerts.

Yes of course it does, you are modifying the behaviour of the subroutine, you have to take care of the callers :)
The template will display "No email is configured for your user" whereas it will be displayed when the library does not have an email configured.
Comment 9 Marcel de Rooy 2017-01-17 13:00:12 UTC
(In reply to Gracie from comment #5)
> Created attachment 59043 [details] [review] [review]
> Bug 17866: Change sender for serial claim notifications
> 
> Currently, the From field uses the emailaddress of the staff client member
> from the user context. This is exceptional in Koha. And might very well be,
> as in our case, the cause of fraud detection warnings.
> We should use branch email address or fallback to KohaAdminEmailAddress,
> as we do (almost) everywhere else.
> 
> Test plan:
> Go to subscription detail in Koha.
> Go to Claims.
> Select a missing issue and click Send notification.
> Verify the sender address in the generated notification. (Make sure that
> you receive this mail.)
> 
> Signed-off-by: Grace McKenzie <grace.mcky@gmail.com>

Btw thanks Grace for signoff and Jonathan for follow-up
Comment 10 Marcel de Rooy 2017-03-22 15:31:07 UTC
lateorders.tt seems fine
    if ( $@ ) {
        $template->param(error_claim => $@);
    } elsif ( ref $err and exists $err->{error} and $err->{error} eq "no_email" ) {
        $template->{VARS}->{'error_claim'} = "no_email";
    } elsif ( ref $err and exists $err->{error} and $err->{error} eq "no_order_selected"){
        $template->{VARS}->{'error_claim'} = "no_order_selected";
    } else {
        $template->{VARS}->{'info_claim'} = 1;
    }
Comment 11 Marcel de Rooy 2017-03-22 15:40:41 UTC
serials/claims.pl
    if ( $@ ) {
        $template->param(error_claim => $@);
    } elsif ( ref $err and exists $err->{error} ) {
        if ( $err->{error} eq "no_email" ) {
            $template->param( error_claim => 'no_vendor_email' );
        } elsif ( $err->{error} =~ m|Bad or missing From address| ) {
            $template->param( error_claim => 'no_loggedin_user_email' );
        }
    } else {
        $template->param( info_claim => 1 );

claims.tt
    [% IF error_claim %]
        [% IF error_claim == 'no_vendor_email' %]
            <div class="dialog alert">This vendor has no email defined for late issues.</div>
        [% ELSIF error_claim == 'no_loggedin_user_email' %]
            <div class="dialog alert">No email is configured for your user.</div>
        [% ELSE %]
            <div class="dialog alert">[% error_claim %]</div>
        [% END %]
    [% END %]
    [% IF info_claim %]
        <div class="dialog message">Email has been sent.</div>
    [% END %]

It is a little bit confusing that claims.pl changes the error codes, but it looks like it should be working too.
Comment 12 Marcel de Rooy 2017-03-22 15:43:47 UTC
(In reply to Jonathan Druart from comment #8)
> (In reply to Marcel de Rooy from comment #7)
> > The claims.tt template correction is not in the scope of this report. I only
> > change the sender address. Do not mangle with return values of SendAlerts.
> 
> Yes of course it does, you are modifying the behaviour of the subroutine,
> you have to take care of the callers :)
> The template will display "No email is configured for your user" whereas it
> will be displayed when the library does not have an email configured.

I checked the templates. Looks OK.
But I am still not convinced that if I would have found something, that it should be solved on this report. It would be way out of scope to me.
If I just correct the email address, I am not responsible for wrong error codes that might have been introduced somewhere else. And imo should be solved on a new report too, and preferably by the one who got them in ;)
Comment 13 Marcel de Rooy 2017-03-22 15:52:53 UTC
Created attachment 61473 [details] [review]
Bug 17866: [QA Follow-up] Add notification for changed sender address

See BZ, comment 6, requested by QA.

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 14 Jonathan Druart 2017-03-22 16:24:24 UTC
Marcel, I am not sure to understand.
Without the patch:
If there is no 'From' it means that the logged in user does not have an email address defined. The 'no_loggedin_user_email' code is sent to the template and "No email is configured for your user." is displayed to the user
=> The message makes sense, even if it is not what we want

With the patch:
If there is no 'From' it means that the library does not have an email address defined and KohaAdminEmailAddress is not set.
The 'no_loggedin_user_email' code is sent to the template and "No email is configured for your user." is displayed to the user
=> The message is wrong
Comment 15 Jonathan Druart 2017-03-22 16:26:03 UTC
Since I guess KohaAdminEmailAddress has to be filled, we can just get rid of no_loggedin_user_email
Comment 16 Marcel de Rooy 2017-03-22 16:28:57 UTC
(In reply to Jonathan Druart from comment #14)
> Marcel, I am not sure to understand.
> Without the patch:
> If there is no 'From' it means that the logged in user does not have an
> email address defined. The 'no_loggedin_user_email' code is sent to the
> template and "No email is configured for your user." is displayed to the user
> => The message makes sense, even if it is not what we want
> 
> With the patch:
> If there is no 'From' it means that the library does not have an email
> address defined and KohaAdminEmailAddress is not set.
> The 'no_loggedin_user_email' code is sent to the template and "No email is
> configured for your user." is displayed to the user
> => The message is wrong

OK I am starting to get the point :)
Thanks
Block it again !
Comment 17 Marcel de Rooy 2017-03-23 09:14:58 UTC
(In reply to Jonathan Druart from comment #15)
> Since I guess KohaAdminEmailAddress has to be filled, we can just get rid of
> no_loggedin_user_email

It probably should, but I will leave this test intact and reword the message.
Comment 18 Marcel de Rooy 2017-03-23 09:16:46 UTC
Created attachment 61517 [details] [review]
Bug 17866: [QA Follow-up] Reword message for missing email address

See BZ, comment 14 from Jonathan.

In the exceptional case that branch email address and fallback, i.e.
preference KohaAdminEmailAddress, are both empty or not valid, the error
message should reflect that (of course :)

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Removed branch email and KohaAdminEmailAddress.
Followed the test plan of the first patch and saw the alert.
Comment 19 Jonathan Druart 2017-03-23 16:36:08 UTC
Created attachment 61583 [details] [review]
[PUSHED] Bug 17866: Change sender for serial claim notifications

Currently, the From field uses the emailaddress of the staff client member
from the user context. This is exceptional in Koha. And might very well be,
as in our case, the cause of fraud detection warnings.
We should use branch email address or fallback to KohaAdminEmailAddress,
as we do (almost) everywhere else.

Test plan:
Go to subscription detail in Koha.
Go to Claims.
Select a missing issue and click Send notification.
Verify the sender address in the generated notification. (Make sure that
you receive this mail.)

Signed-off-by: Grace McKenzie <grace.mcky@gmail.com>

Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>
Comment 20 Jonathan Druart 2017-03-23 16:36:13 UTC
Created attachment 61584 [details] [review]
[PUSHED] Bug 17866: [QA Follow-up] Add notification for changed sender address

See BZ, comment 6, requested by QA.

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>

Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>
Comment 21 Jonathan Druart 2017-03-23 16:36:17 UTC
Created attachment 61585 [details] [review]
[PUSHED] Bug 17866: [QA Follow-up] Reword message for missing email address

See BZ, comment 14 from Jonathan.

In the exceptional case that branch email address and fallback, i.e.
preference KohaAdminEmailAddress, are both empty or not valid, the error
message should reflect that (of course :)

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Removed branch email and KohaAdminEmailAddress.
Followed the test plan of the first patch and saw the alert.

Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>
Comment 22 Kyle M Hall 2017-03-31 14:03:38 UTC
Pushed to master for 17.05, thanks Marcel!
Comment 23 Katrin Fischer 2017-04-02 17:20:36 UTC
While the behaviour is a bit odd at the moment, the patch proposes a behaviour change and should not be a sudden surprise in a stable release in my opinion.
Comment 24 Katrin Fischer 2017-04-02 17:25:09 UTC
Btw, this also appears to change the acq claim and order notices if I am reading the code correctly, but it isn't noted in the database update message or bug title. Marcel, could you take a look please?
Comment 25 Marcel de Rooy 2017-04-03 06:50:35 UTC
Created attachment 61774 [details] [review]
Bug 17866: [QA Follow-up] Add forgotten SetVersion call

In db revision 16.12.00.022.

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>


Current status: REOPENED
Comment 26 Katrin Fischer 2017-04-03 07:08:39 UTC
Marcel, did you see my comment? There is also already another bug for this: bug 18368.
Comment 27 Marcel de Rooy 2017-04-03 07:10:47 UTC
(In reply to Katrin Fischer from comment #23)
> While the behaviour is a bit odd at the moment, the patch proposes a
> behaviour change and should not be a sudden surprise in a stable release in
> my opinion.
Looks like a good decision to me.
Comment 28 Marcel de Rooy 2017-04-03 07:14:15 UTC
(In reply to Katrin Fischer from comment #24)
> Btw, this also appears to change the acq claim and order notices if I am
> reading the code correctly, but it isn't noted in the database update
> message or bug title. Marcel, could you take a look please?

$type eq 'claimacquisition' or $type eq 'claimissues' or $type eq 'orderacquisition'
Comment 29 Marcel de Rooy 2017-04-03 07:19:38 UTC
Changed the title. Maybe we should rephrase the dbrev message too. Include the acq claim at least. The other one is a new feature from 16.11, not widely used yet probably. (But you will use it already :)
Comment 30 Marcel de Rooy 2017-04-03 07:24:34 UTC
Reopening it again :)
Comment 31 Marcel de Rooy 2017-04-03 07:24:58 UTC
Created attachment 61776 [details] [review]
Bug 17866: [QA Follow-up] Change wording of db revision

As Katrin noted on the report, we should not limit the db rev message to
the serial claims only. Applies to acquisision claims too.
Note: Strictly speaking, we could also mention the new ACQORDER notice,
but this will not yet be widely used.

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 32 Marcel de Rooy 2017-04-03 07:25:35 UTC
Kyle: please push the last patch too.
Comment 33 Kyle M Hall 2017-04-03 10:31:40 UTC
Followup pushed for 17.05, thanks Marcel!
Comment 34 Jonathan Druart 2017-04-03 15:00:55 UTC
A test fails:
prove t/db_dependent/Letters.t
t/db_dependent/Letters.t .. 1/80 Can't call method "branchemail" on an undefined value at /home/vagrant/kohaclone/C4/Letters.pm line 576.

userenv is not set in tests.
Comment 35 Marc Véron 2017-04-06 07:21:04 UTC
(In reply to Jonathan Druart from comment #34)
> A test fails:
> prove t/db_dependent/Letters.t
> t/db_dependent/Letters.t .. 1/80 Can't call method "branchemail" on an
> undefined value at /home/vagrant/kohaclone/C4/Letters.pm line 576.
> 
> userenv is not set in tests.

Should that go to a new Bug report? - Just stumbled over while testing Bug 12802 - Send notices using several email addresses
Comment 36 Marcel de Rooy 2017-04-06 07:48:10 UTC
(In reply to Marc Véron from comment #35)
> (In reply to Jonathan Druart from comment #34)
> > A test fails:
> > prove t/db_dependent/Letters.t
> > t/db_dependent/Letters.t .. 1/80 Can't call method "branchemail" on an
> > undefined value at /home/vagrant/kohaclone/C4/Letters.pm line 576.
> > 
> > userenv is not set in tests.
> 
> Should that go to a new Bug report? - Just stumbled over while testing Bug
> 12802 - Send notices using several email addresses

Yes, I will put it on a new report.
Comment 37 Katrin Fischer 2017-04-10 21:18:30 UTC
As a behaviour change that is not stricly a bug, this won't be pushed to the stable branch 16.11.x.