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.
Created attachment 58676 [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, 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. 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.) 0003-Bug-17736-Remove-C4-Reserves-GetReservesFromBiblionu.patch
Created attachment 58677 [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, 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. 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.)
Oops, old function
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.)
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>
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.
(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.
(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.
(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
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; }
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.
(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 ;)
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>
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
Since I guess KohaAdminEmailAddress has to be filled, we can just get rid of no_loggedin_user_email
(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 !
(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.
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.
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>
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>
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>
Pushed to master for 17.05, thanks Marcel!
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.
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?
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
Marcel, did you see my comment? There is also already another bug for this: bug 18368.
(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.
(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'
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 :)
Reopening it again :)
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>
Kyle: please push the last patch too.
Followup pushed for 17.05, thanks Marcel!
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.
(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
(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.
As a behaviour change that is not stricly a bug, this won't be pushed to the stable branch 16.11.x.