I feel like this change of behaviour needs to be optional and needs a bit more thought.

Would it be desirable to only cc the guarantor if the guarantee has a real email address? Should an email address on the guarantee record be ignored or take precedence?

I think we won't get around a pref here as people have been using relationships in different ways.

<h1>Something went wrong !</h1>Applying: Bug 12532 - Redirect guarantee email to guarantor
Using index info to reconstruct a base tree...
Falling back to patching base and 3-way merge...
Auto-merging C4/Letters.pm
Auto-merging C4/Members.pm
Auto-merging installer/data/mysql/sysprefs.sql
CONFLICT (content): Merge conflict in installer/data/mysql/sysprefs.sql
Auto-merging installer/data/mysql/updatedatabase.pl
Auto-merging koha-tmpl/intranet-tmpl/prog/en/modules/admin/preferences/patrons.pref
CONFLICT (content): Merge conflict in koha-tmpl/intranet-tmpl/prog/en/modules/admin/preferences/patrons.pref
Failed to merge in the changes.

Bug 12532 - Redirect guarantee email to guarantor

35313 - Bug 12532 - Redirect guarantee email to guarantor

Apply? [(y)es, (n)o, (i)nteractive] y
Applying: Bug 12532 - Redirect guarantee email to guarantor
Using index info to reconstruct a base tree...
Falling back to patching base and 3-way merge...
Auto-merging C4/Members.pm
Auto-merging installer/data/mysql/sysprefs.sql
Auto-merging installer/data/mysql/updatedatabase.pl
CONFLICT (content): Merge conflict in installer/data/mysql/updatedatabase.pl
Failed to merge in the changes.
Patch failed at 0001 Bug 12532 - Redirect guarantee email to guarantor
When you have resolved this problem run "git bz apply --continue".
If you would prefer to skip this patch, instead run "git bz apply --skip".
To restore the original branch and stop patching run "git bz apply --abort".
Patch left in /tmp/Bug-12532---Redirect-guarantee-email-to-guarantor-BQ1gJr.patch

conflit fixed

Comment on attachment 36752 [details] [review]
Bug 12532 - Redirect guarantee email to guarantor

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

::: C4/Members.pm
@@ +1562,5 @@
> +    my $enableRedirectGuaranteeEmail =  C4::Context->preference("EnableRedirectGuaranteeEmail");
> +    if (!$enableRedirectGuaranteeEmail){
> +        $borrowerNumberToSendEmail = $borrowernumber;
> +        $guarantorid = undef;
> +    }

This logic works, but it is not clear to the person who reads code.

@@ +1574,5 @@
> +        my $guarantor = C4::Members::GetMember( 'borrowernumber' => $guarantorid );
> +        if ( $guarantor ){
> +            $to_address = $guarantor->{ $which_address };
> +        }
> +   }

Mixing which_address comparisons with guarantorid logic in the same if structure level is not good coding style in my opinion.

::: installer/data/mysql/sysprefs.sql
@@ +116,4 @@
>  ('EnableBorrowerFiles','0',NULL,'If enabled, allows librarians to upload and attach arbitrary files to a borrower record.','YesNo'),
>  ('EnableOpacSearchHistory','1','YesNo','Enable or disable opac search history',''),
>  ('EnableSearchHistory','0','','Enable or disable search history','YesNo'),
> +('EnableRedirectGuaranteeEmail', '0', NULL, 'Enable the ability to redirect guarantee email messages to guarantor.', 'YesNo'),

R comes before S. :)

If you modify routines in C4 or Koha namespaces, you need to make sure there is test coverage for your changes.

Failed to add tests to ./t/db_dependent/Members.t reflecting changes in C4::Members::GetNoticeEmailAddress.

Failed to add tests to ./t/db_dependent/Letters.t reflecting changes in the C4::Letters::_send_message_by_email routine, which is called by SendQueuedMessages.

These tests would need to include things like intentional failures based on specific system preferences.

I tried testing, patches applied and update ran fine but both tests failed and emails were not redirected.

(In reply to Nick Clemens from comment #13)
> I tried testing, patches applied and update ran fine but both tests failed
> and emails were not redirected.

What tests failed exactly? (With the #)
I too had the tests failing at first. For me the failure was caused by missing patron categories and branches. (db_dependent, indeed)

I tried my test plan again.
The .t ran fine and the emails were redirected properly.

Please note the emails are redirected as they are sent, so you need to run the 
process_message_queue.pl cronjob.
The to_address field in the database does not indicate if the email is redirected or not.

Retested, all works as expected, must have been an issue on my end.

The only confusion I see here is in checking the Notices tab on a patron's account doesn't show where the message went.  A library should know if they have this enabled, and should be able to figure out emails were redirected, but just makes me pause.  Not a blocker but wanted to mention.

(In reply to Katrin Fischer from comment #2)
> I feel like this change of behaviour needs to be optional and needs a bit
> more thought.
> 
> Would it be desirable to only cc the guarantor if the guarantee has a real
> email address? Should an email address on the guarantee record be ignored or
> take precedence?

Could we get an answer on this comment please?
I was writing the same question before reading back the older comments: Why don't you simply cc the guarantor?

(In reply to Jonathan Druart from comment #17)
> (In reply to Katrin Fischer from comment #2)
> > I feel like this change of behaviour needs to be optional and needs a bit
> > more thought.
> > 
> > Would it be desirable to only cc the guarantor if the guarantee has a real
> > email address? Should an email address on the guarantee record be ignored or
> > take precedence?
> 
> Could we get an answer on this comment please?
> I was writing the same question before reading back the older comments: Why
> don't you simply cc the guarantor?

Yes, you can! ;^)

> > I feel like this change of behaviour needs to be optional and needs a bit
> > more thought.

It is optinal now. There is new syspref to enable the redirection (EnableRedirectGuaranteeEmail)

> > Would it be desirable to only cc the guarantor if the guarantee has a real
> > email address?

I agree it would have been another option that works. However, the requirement was to send guarantee's emails to the guarantor. Whether we cc the guarantor or send all guarantee's emails to the guarantor, the expected result is the same : the guarantor get the emails.

> > Should an email address on the guarantee record be ignored or
> > take precedence?

For this development, the guarantee's emails are ignored. The sponsor use it this way and so far, didn't ask to improve it. It would be nice to make it more flexible but I think this improvement should be part of another feature.

François,

Ok it works for me.
But, let me explain a thought :)
Bug 12802 is on its way and will be in conflict with this development (because it changes the behavior of the pref AutoEmailPrimaryAddress and the subroutine GetNoticeEmailAddress).
I am just wondering if the enh introduced by this bug 12532 could not be a new entry in the multivalued pref AutoEmailPrimaryAddresses.
We could have email, emailpro, B_email and "guarantor" (or even "email guarantor", "emailpro guarantor", "B_email guarantor"). This way we could sent the notice to the patron and the guarantor, and fix the special case where the guarantor does not have an email but the guarantee has one.

I am not forcing you to agree with me, it's just a thinking :)

(In reply to Jonathan Druart from comment #19)
> François,
> 
> Ok it works for me.
> But, let me explain a thought :)
> Bug 12802 is on its way and will be in conflict with this development
> (because it changes the behavior of the pref AutoEmailPrimaryAddress and the
> subroutine GetNoticeEmailAddress).
> I am just wondering if the enh introduced by this bug 12532 could not be a
> new entry in the multivalued pref AutoEmailPrimaryAddresses.
> We could have email, emailpro, B_email and "guarantor" (or even "email
> guarantor", "emailpro guarantor", "B_email guarantor"). This way we could
> sent the notice to the patron and the guarantor, and fix the special case
> where the guarantor does not have an email but the guarantee has one.
> 
> I am not forcing you to agree with me, it's just a thinking :)

Indeed, the "guarantor" option could be added to the syspref.
If you agree to add this feature to bug 12802, we can close this one and/or mark it as duplicate.

(In reply to Maxime Beaulieu from comment #20)
> Indeed, the "guarantor" option could be added to the syspref.
> If you agree to add this feature to bug 12802, we can close this one and/or
> mark it as duplicate.

As you can see on bug 12802, I am not in charge of this bug anymore.
And I would be very surprised that it will be included.
I have suggested that to help you to integrate this one, not to make 12802 much harder to integrate.

We won't (In reply to Jonathan Druart from comment #21)
> (In reply to Maxime Beaulieu from comment #20)
> > Indeed, the "guarantor" option could be added to the syspref.
> > If you agree to add this feature to bug 12802, we can close this one and/or
> > mark it as duplicate.
> 
> As you can see on bug 12802, I am not in charge of this bug anymore.
> And I would be very surprised that it will be included.

Then, we will stay focus on this patch. :)

I change the status to "signed off" since it was by Nick Clemens (see http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=12532#c15)

(In reply to Francois Charbonnier from comment #22)
> > As you can see on bug 12802, I am not in charge of this bug anymore.
> > And I would be very surprised that it will be included.

I have absolutely no idea why I have written this last sentence.
I was not wanted to tell that the patches on bug 12802 won't be included/pushed into master.
But certainly more that the new author/maintainer of bug 12802 won't write the changes I have proposed on comment 19.

Sorry for the misunderstanding and please provide an answer.
If the solution seems better to you, please provide a followup on top of bug 12802.

From the discussion above, I would conclude that the author did not yet provide answers to questions raised.
The code looks reasonable but does not apply anymore.

From a functional perspective, it is hard to say (at first glance) if
[a] this approach with a new pref,
[b] the approach of comment19 with bug 12802,
[c] yet another solution (perhaps linked to editing guarantor/guarantee record),
would be the best way to go.
I have my doubts on approach b btw. I would rather leave the guarantor stuff outside firstvalidemail.

Fixed conflicts, the patch should now apply properly. 

Like my coworker said earlier in this discussion, our sponsor is not interested in developping a new preference that allows guarantors to be cc'd. We think this patch is ok on its own and if there happens to be a need for guarantors to be cc'd, the patch for this new feature should be in another bug report.
Thank you.

I think based on this bug's history that it should be "Signed off" at this stage.

I think there are some questions that need to be answered before this can move along:
1) Should this be a blanket setting? Maybe the parent of an 8 year old would want this, but not the parent of a 17 year old
2) Should the generation of the notice be based on the guarantees messaging settings, or the guarantors?
3) Following 2, perhaps guarantors should have the option of receiving notices of guarantees as part of the messaging preferences
4) Wouldn't it be prudent to send the notice to both? It could just CC the guarantor

I think at a minimum this should be an option for each guarantor, and the email should just add them as a CC ( or some other method so that both the guarantee and guarantor receive the notice ). A default setting of on or off could be added to the patron types editor, and a checkbox for receiving guarantees notices could be added to the messaging settings for guarantors on the staff side and the opac.

What do you think of this?

Hello Kyle,

this patch now sends email to both the guarantor and the guarantee. The 
generation of the notice is still based on the guarantee messaging settings.

Mehdi.

Testing:
- EnableRedirectGuaranteeEmail on
- Both guarantor and guarantee have valid email address

Messaging preferences:
- Guarantee: all on
- Guarantor: all on
-Trigger mail (e.g. Checkout), run process_message_queue.pl
Result: Mail goes to both (as expected)

Messaging preferences:
- Guarantee: all on
- Guarantor: all off
- Trigger mail (e.g. Checkout), run process_message_queue.pl
Result: Mail goes to both (as expected)

Messaging preferences:
- Guarantee: all off
- Guarantor: all on
- Trigger mail (e.g. Checkout), run process_message_queue.pl
Result: No mail (as expected)

Messaging preferences:
- Guarantee: all off
- Guarantor: all off
- Trigger mail (e.g. Checkout), run process_message_queue.pl
Result: No mail (as expected)

Running tests:

t/db_dependent/Letters.t
------------------------
#   Failed test 'With EnableRedirectGuaranteeEmail on, Message is sent to the guarantor's email address.'
#   at t/db_dependent/Letters.t line 193.
#          got: 'toto@exemple.com'
#     expected: 'guarantor@example.com'
Fake sendmail at t/db_dependent/Letters.t line 32.

#   Failed test 'With EnableRedirectGuaranteeEmail on, message fails if guarantor has no address.'
#   at t/db_dependent/Letters.t line 198.
#          got: 'sent'
#     expected: 'failed'
Fake sendmail at t/db_dependent/Letters.t line 32.
# Looks like you planned 74 tests but ran 80.
# Looks like you failed 2 tests of 80 run.
t/db_dependent/Letters.t .. Dubious, test returned 2 (wstat 512, 0x200)
Failed 2/74 subtests 

Test Summary Report
-------------------
t/db_dependent/Letters.t (Wstat: 512 Tests: 80 Failed: 8)
  Failed tests:  30-31, 75-80
  Non-zero exit status: 2
  Parse errors: Bad plan.  You planned 74 tests but ran 80.


t/db_dependent/Members.t
------------------------
t/db_dependent/Members.t .. 1/79 "my" variable $guarantee masks earlier declaration in same scope at t/db_dependent/Members.t line 331.
t/db_dependent/Members.t .. 2/79 
#   Failed test 'GetNoticeEmailAddress returns correct value when EnableRedirectGuaranteeEmail is activated'
#   at t/db_dependent/Members.t line 187.
#          got: 'guanrantee@email.test,Marie@work.com'
#     expected: 'Marie@work.com'
t/db_dependent/Members.t .. 79/79 # Looks like you planned 79 tests but ran 81.
# Looks like you failed 1 test of 81 run.
t/db_dependent/Members.t .. Dubious, test returned 1 (wstat 256, 0x100)
Failed 1/79 subtests 

Test Summary Report
-------------------
t/db_dependent/Members.t (Wstat: 256 Tests: 81 Failed: 3)
  Failed tests:  27, 80-81
  Non-zero exit status: 1
  Parse errors: Bad plan.  You planned 79 tests but ran 81.
Files=1, Tests=81,  3 wallclock secs ( 0.04 usr  0.00 sys +  1.96 cusr  0.13 csys =  2.13 CPU)
Result: FAIL

Setting to FQA because of the tests

Created attachment 53699 [details] [review]
Bug 12532 - Redirect guarantee email to guarantor

Fixed merge conflicts.
Both the guarantor and the guarantee will receive the email now.
Also fixed some log errors since the "save" button is the same one use for editing the patron details.
Changed the save button "save" id whene editing only the Patron messaging preferences so we can detect where the request is comming from.

Added unit tests.
Added an atomicupdate file.
Reordered the sysprefs in alphabetical order.
Refactored some code.

TEST PLAN:

I Apply the patch
II Run updatedatabase.pl
III Run db_dependent/Letters.t and db_dependant/Members.t

0) Enable EnhancedMessagingPreferences preference;
0) Enable EnableRedirectGuaranteeEmail preference;
1) Select a patron with child;
2) Enable Item checkout -> email in patron messaging preferences;
3) Select the patron's child;
4) Checkout an item;
5) Valide child checkout message in patron's inbox;

sponsored by the ville de victoriaville

Tested together with follow-up works as expected.
Signed-off-by: Marc <veron@veron.ch>

Created attachment 53700 [details] [review]
Bug 12532 - QA Follow-up

Fixed tests, they now work without any fails

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

Signed-off-by: Marc <veron@veron.ch>

Comment on attachment 53699 [details] [review]
Bug 12532 - Redirect guarantee email to guarantor

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

Hi Maxime and Mehdi,

working on this again:

A) I am confused about some changes that seem unrelated to this feature - can you please explain you explain and/or amend test plan? (see splitter comments below)
B) Is it correct that ACCTDETAILS and RLIST have been dropped from this patch set? (comparing comment#1 to code and syspref description)

C) I'd also like to take into account Kyle's earlier comment#28:

>1) Should this be a blanket setting? Maybe the parent of an 8 year old would want this, but not the parent of a 17 year old
I agree with Kyle here, having this as a global setting will not match the needs of every library. But I think moving from global to more granular should be possible later without any regressions, so for me this is not a blocker.

>2) Should the generation of the notice be based on the guarantees messaging settings, or the guarantors?
It's a good question: Who can decide that the e-mails are forwarded? I don't have a clear idea about this.
Again, not a blocker for me if we'd handle this globally at first. Maybe it could also be a setting under administration -> category codes?

>3) Following 2, perhaps guarantors should have the option of receiving notices of guarantees as part of the messaging preferences
See above :)

>4) Wouldn't it be prudent to send the notice to both? It could just CC the guarantor
I have thought about it and I'd really like to see this change happen. I know more places need to be touched, but it seems to be a much cleaner and clearer solution to me. Also: if we change this later on, we got a behaviour change. Worth to give this more thought from the beginning.

>I think at a minimum this should be an option for each guarantor, and the email should just add them as a CC ( or some other method so that both the guarantee and guarantor receive >the notice ). A default setting of on or off could be added to the patron types editor, and a checkbox for receiving guarantees notices could be added to the messaging settings for >guarantors on the staff side and the opac.

@Kyle - would you be ok with this being global and CC as a first step?

D) Some thoughts about wording - you might want to check this with a native speaker :)

- I am in favor of short preference names - I was wondering if we couldn't 
  just leave the "Enable" off: RedirectGuaranteeEmail
- Similar for the preference description: 
  Now: [Do|Don't] enable the ability to redirect guarantee email messages 
  to guarantor. All email message placed in message_queue table will be affected.
  Suggestion: [Enable|Don't enable] redirection of guarantee email messages to the guarantor.
  All email messages send via the message_queue table will be affected.

-------------------------------------------

Some general notes:
- database update - ok
- QA test tools - ok
- tests pass for me - apart from one test failing in 
  Letters.t before and after the patch - ok
- functionality tested for checkout notices with and 
  without guarantor and yes/no pref setting - ok

::: koha-tmpl/intranet-tmpl/prog/en/modules/admin/preferences/patrons.pref
@@ +183,5 @@
> +         - pref: EnableRedirectGuaranteeEmail
> +           choices:
> +               yes: Do
> +               no: "Don't"
> +         - enable the ability to redirect guarantee email messages to guarantor. All email message placed in message_queue table will be affected.

In the initial description of this feature you named ACCTDETAILS and RLIST - those don't go through the message_queue. Looking at the code, I think they are not included?
Which messages are affected?

::: koha-tmpl/intranet-tmpl/prog/en/modules/members/memberentrygen.tt
@@ +82,1 @@
>  

Why those changes?

@@ +1069,5 @@
> +    [% IF ( !opadd ) && ( !opduplicate ) && !destination %]
> +        <script type="text/javascript">
> +            $("#saverecord").attr("id","PatronMmessagingPreferences");
> +        </script>
> +    [% END %]

Same here: why those changes?

::: members/memberentry.pl
@@ +323,5 @@
>      push @errors, "ERROR_login_exist";
>    }
>    
> +  my $password = $input->param('password') || '';
> +  my $password2 = $input->param('password2') || '';

Same here: why those changes?

Created attachment 55985 [details] [review]
Bug12532 - delete useless code and many change for preference EnableRedirectGuaranteeEmail

 messages affected: ODUE, HOLD, DUE, PREDUE, CHECKIN, CHECKOUT

Wanted to test on current master, but first patch does not apply, sorry.

Created attachment 56052 [details] [review]
Bug 12532 - Redirect guarantee email to guarantor

Fixed merge conflicts.
Both the guarantor and the guarantee will receive the email now.
Also fixed some log errors since the "save" button is the same one use for editing the patron details.
Changed the save button "save" id whene editing only the Patron messaging preferences so we can detect where the request is comming from.

Added unit tests.
Added an atomicupdate file.
Reordered the sysprefs in alphabetical order.
Refactored some code.

TEST PLAN:

I Apply the patch
II Run updatedatabase.pl
III Run db_dependent/Letters.t and db_dependant/Members.t

0) Enable EnhancedMessagingPreferences preference;
0) Enable EnableRedirectGuaranteeEmail preference;
1) Select a patron with child;
2) Enable Item checkout -> email in patron messaging preferences;
3) Select the patron's child;
4) Checkout an item;
5) Valide child checkout message in patron's inbox;

sponsored by the ville de victoriaville

Tested together with follow-up works as expected.
Signed-off-by: Marc <veron@veron.ch>

Fix conflict and rebase

Tests on db_dependent/Letters.t and b_dependent/Members.t failed when I tested this. Setting to failed QA for now.
Everything else seemed to work fine though.

Created attachment 59445 [details] [review]
Bug 12532 - QA Follow-up

Fixed tests, they now work without any fails

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

Signed-off-by: Marc <veron@veron.ch>

Created attachment 59446 [details] [review]
Bug 12532 - Redirect guarantee email to guarantor

Fixed merge conflicts.
Both the guarantor and the guarantee will receive the email now.
Also fixed some log errors since the "save" button is the same one use for editing the patron details.
Changed the save button "save" id whene editing only the Patron messaging preferences so we can detect where the request is comming from.

Added unit tests.
Added an atomicupdate file.
Reordered the sysprefs in alphabetical order.
Refactored some code.

TEST PLAN:

I Apply the patch
II Run updatedatabase.pl
III Run db_dependent/Letters.t and db_dependant/Members.t

0) Enable EnhancedMessagingPreferences preference;
0) Enable EnableRedirectGuaranteeEmail preference;
1) Select a patron with child;
2) Enable Item checkout -> email in patron messaging preferences;
3) Select the patron's child;
4) Checkout an item;
5) Valide child checkout message in patron's inbox;

sponsored by the ville de victoriaville

Tested together with follow-up works as expected.
Signed-off-by: Marc <veron@veron.ch>

Created attachment 59447 [details] [review]
Bug 12532 - QA Follow-up

Fixed tests, they now work without any fails

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

Signed-off-by: Marc <veron@veron.ch>

I rebase all patches and fix small problems.

Created attachment 64802 [details] [review]
Bug 12532 - Changed occurrences to EnableRedirectGuaranteeEmail for RedirectGuaranteeEmail

Both were used for the same reason and EnableRedirectGuaranteeEmail doesn't existe in systempreferences table.

Corrected minor error. However, "Bug 12532 - QA Follow-up" give me a Sha1 error so it couldn't be applied.

Created attachment 64824 [details] [review]
Bug 12532 - QA Follow-up

Fixed tests, they now work without any fails.

Because of the sha1 error, I took the content of the original commit "Bug 12532 - QA Follow-up", put it in an other commit and mark the old one has obsolete.
Now the patch apply correctly.

I know I'm not supposed to mess with C4, but the test suite wouldn't run if I didn't expose GetNoticeEmailAddress through EXPORTER.

Kept status 'Signed Off'

You are re-adding subroutines that have been removed from the codebase, this is definitely the wrong way. Use the new methods added to the Koha modules instead.

(In reply to Jonathan Druart from comment #57)
> You are re-adding subroutines that have been removed from the codebase, this
> is definitely the wrong way. Use the new methods added to the Koha modules
> instead.

Do you mean removing GetNoticeEmailAddress from C4 Members entirely and applying the same changes somewhere else?

See bug 19303 and bug 19304

I tester bug, but I get error:
Applying: Bug 12532 - Send emails to guarantee and guarantor
Using index info to reconstruct a base tree...
M	Koha/Patron.pm
M	installer/data/mysql/sysprefs.sql
M	koha-tmpl/intranet-tmpl/prog/en/modules/admin/preferences/patrons.pref
Falling back to patching base and 3-way merge...
error: The following untracked working tree files would be overwritten by merge:
	installer/data/mysql/atomicupdate/bug_12532-RedirectGuaranteeEmail_syspref.sql
Please move or remove them before you can merge.
Aborting
Failed to merge in the changes.
Patch failed at 0001 Bug 12532 - Send emails to guarantee and guarantor

Its on master?

Thanks

Hi Michal, it looks like you are not working form a clean git branch:
The following untracked working tree files would be overwritten by merge:
	installer/data/mysql/atomicupdate/bug_12532-RedirectGuaranteeEmail_syspref.sql

This looks like a leftover from earlier testing.

Ok, I'll try it again.

Thank You for review.


Mike

Created attachment 85135 [details] [review]
Bug 12532 - (follow up) clean up

Hi,

I like the idea of this, however I think from a usability standpoint the guarantor should have an entry in the message queue so they can see all of the notices the library has sent to them, including ones sent to the guarantee. As it stands, the notices are on the guarantee's list, but the additional messages generated aren't in the interface for the guarantor. This could be confusing for users.

Would also like to raise the privacy concerns here, as the guarantee is not notified that their library activity is potentially being closely monitored by the guarantor. I'd like to see a note about that in the system preference, something like "This system preference has the potential to breach the privacy of guaranteed patrons to their guarantors." A note in the email "This message was also sent to $guarantor." Kids, for example, might check out things from the library that they don't want their parents to know about, and could lead to some awkward conversations at the least, and guarantee abuse (or worse) at the worst.

I think this is especially important as we try (or have recently gotten?) to make it possible to make any patron able to have a guarantor.

Please have a think about these issues, I don't think I can sign this off as it is.

Cheers,
Liz

Hi,
status is still "In Discussion"? Or Need Signed Off?

Thank you.

Created attachment 88295 [details] [review]
Bug 12532 - Send emails to guarantee and guarantor

Checks for the syspref "RedirectGuaranteeEmail" in Koha::Patrons::notice_email_address.
When enabled, emails to a patron with a guarantor will be sent to both guarantor and guarantee.

It does not affect values in the message_queue table.

Test plan:

Before applying
1) Search, or create, a patron with guarantor
2) Enable checkout emails
3) Checkout an item. An email should be sent only to the guarantee
4) Apply this patch
5) Run updatedatabase.pl
6) Enable 'RedirectGuaranteeEmail'
7) Go through step 1) to 3). Email should now be sent to both.
8) Run t/db_dependant/Members.t

Sponsored by: Ville de Victoriaville

Created attachment 88296 [details] [review]
Bug 12532 - (follow-up) clean up

What can we do to make this come out of the "In Discussion" limbo? Liz mentionned a privacy issue, should we add a tick box like the display fines to guarantor option?

Created attachment 114456 [details] [review]
Bug 12532 - Rebase and correction of the patch by adding the possibility of sending emails to several guarantors

Created attachment 114459 [details] [review]
Bug 12532 - (follow-up) clean up

Created attachment 114460 [details]
Bug 12532 - Rebase and correction of the patch by adding the possibility of sending emails to several guarantors

Created attachment 114461 [details] [review]
Bug 12532 - Correction of tests involving guarantorid

Patch no longer applies 8-(..

Applying: Bug 12532 - Send emails to guarantee and guarantor
.git/rebase-apply/patch:33: trailing whitespace.
        unless ($to_address) {  
warning: 1 line adds whitespace errors.
Using index info to reconstruct a base tree...
M	C4/Letters.pm
M	Koha/Patron.pm
M	installer/data/mysql/mandatory/sysprefs.sql
M	koha-tmpl/intranet-tmpl/prog/en/modules/admin/preferences/patrons.pref
M	t/db_dependent/Members.t
.git/rebase-apply/patch:33: trailing whitespace.
        unless ($to_address) {  
warning: 1 line adds whitespace errors.
Falling back to patching base and 3-way merge...
Auto-merging t/db_dependent/Members.t
CONFLICT (content): Merge conflict in t/db_dependent/Members.t
Auto-merging koha-tmpl/intranet-tmpl/prog/en/modules/admin/preferences/patrons.pref
Auto-merging installer/data/mysql/mandatory/sysprefs.sql
Auto-merging Koha/Patron.pm
Auto-merging C4/Letters.pm
error: Failed to merge in the changes.
Patch failed at 0001 Bug 12532 - Send emails to guarantee and guarantor

Created attachment 131091 [details] [review]
Bug 12532 - Send emails to guarantee and guarantor

Checks for the syspref "RedirectGuaranteeEmail" in Koha::Patrons::notice_email_address.
When enabled, emails to a patron with a guarantor will be sent to both guarantor and guarantee.

It does not affect values in the message_queue table.

Test plan:

Before applying
1) Search, or create, a patron with guarantor
2) Enable checkout emails
3) Checkout an item. An email should be sent only to the guarantee
4) Apply this patch
5) Run updatedatabase.pl
6) Enable 'RedirectGuaranteeEmail'
7) Go through step 1) to 3). Email should now be sent to both.
8) Run t/db_dependant/Members.t

Sponsored by: Ville de Victoriaville

Bug 12532 - (follow-up) clean up

Created attachment 131092 [details] [review]
Bug 12532 - (follow-up) clean up

Created attachment 131093 [details] [review]
Bug 12532 - Correction of tests involving guarantorid

Created attachment 131094 [details] [review]
Bug 12532: Send email to guarantee and guarantor;

This patch allows guarantors to receive emails sended to their
guarentees. This patch is a rebase of the previous patches.
NOTE: Attachment "Rebase and correction of the patch by adding the possibility of sending emails to several guarantors" generate an sha1 error.
I had to took its content and put it in another commit.

TO TEST:

Before applying:
1) Search, or create, a patron with guarantor.
2) Enable checkout emails for both guarantors and guarantees.
3) Checkout an item. An email should be sent only to the guarantee.
4) Apply the patch.
5) Run updatedatabase.pl
6) Enable 'RedirectGuaranteeEmail'
7) Run misc/cronjobs/process_message_queue.pl
8) Notice that the email should be sended to both the guarantee AND the
guarantor.

(In reply to The Minh Luong from comment #94)
> Created attachment 131094 [details] [review] [review]
> Bug 12532: Send email to guarantee and guarantor;
> 
> This patch allows guarantors to receive emails sended to their
> guarentees. This patch is a rebase of the previous patches.
> NOTE: Attachment "Rebase and correction of the patch by adding the
> possibility of sending emails to several guarantors" generate an sha1 error.
> I had to took its content and put it in another commit.
> 
> TO TEST:
> 
> Before applying:
> 1) Search, or create, a patron with guarantor.
> 2) Enable checkout emails for both guarantors and guarantees.
> 3) Checkout an item. An email should be sent only to the guarantee.
> 4) Apply the patch.
> 5) Run updatedatabase.pl
> 6) Enable 'RedirectGuaranteeEmail'
> 7) Run misc/cronjobs/process_message_queue.pl
> 8) Notice that the email should be sended to both the guarantee AND the
> guarantor.


Emails were only sent to the guarantee and not the guarantor.

Created attachment 149233 [details] [review]
Bug 12532: Send email to guarantee and guarantor;

This patch allows guarantors to receive emails sended to their
guarentees. This patch is a rebase of the previous patches.
NOTE: Attachment "Rebase and correction of the patch by adding the possibility of sending emails to several guarantors" generate an sha1 error.
I had to took its content and put it in another commit.

TO TEST:

Before applying:
1) Search, or create, a patron with guarantor.
2) Enable checkout emails for both guarantors and guarantees.
3) Checkout an item. An email should be sent only to the guarantee.
4) Apply the patch.
5) Run updatedatabase.pl
6) Enable 'RedirectGuaranteeEmail'
7) Run misc/cronjobs/process_message_queue.pl
8) Notice that the email should be sended to both the guarantee AND the
guarantor.

Everything works fine.

Signed-off-by: Marius Mandrescu <marius.mandrescu@inLibro.com>

I have tested (using koha-testing-docker), so that there is a sign-off from someone other than InLibro staff.

It works, but I've failed QA because of the things under 'Notes from testing' - hopefully, once these are fixed the QA process should be easier!

Test plan notes:
1. Add a guarantor to a patron (I added Mary Burton (23529000651225) as a guarantor to Lisa Charles (23529000197047)).
2. For each of the patrons:
   - Add an email address (I used two different email addresses)
   - Update the 'Patron messaging preferences' section so that an email is sent for item checkouts
3. To check the notices sent: [patron account] > Notices (can also check the database: koha-mysql kohadev the select * from message_queue;)
4. To run tests: prove t/db_dependent/Members.t and prove t/db_dependent/Letters.t
5. To test sending emails using a Google account, edit the /etc/koha/sites/kohadev/koha-conf.xml file and add this configuration near the end (where <user_name> = your Google email address; <password> = your APP password, not your Google account password - see Google Help on how to set up an app password):

   <smtp_server>
      <host>smtp.gmail.com</host>
      <port>587</port>
      <timeout>5</timeout>
      <ssl_mode>STARTTLS</ssl_mode>
      <user_name>GOOGLEACCOUNTUSER</user_name>
      <password>GOOGLEAPPPASSWORD</password>
      <debug>1</debug>
   </smtp_server>

Notes from testing:

1. There are whitespace errors when applying the patches. [1]

2. Formatting patch titles - these should be in the format Bug XXXX: The title for the patch AND Bug XXXX: (follow-up) The title for the follow-up (see https://wiki.koha-community.org/wiki/Commit_messages#Subject_line).

3. Patches with the same name - this can be confusing, but I'm not sure of the best way to do this for this bug. Also, as this bug has been around for a while, and has a bit of history, maybe the patches should be consolidated. I would ask on IRC to get expert advice on the best approach to take for this bug (maybe have the main patch, a patch for the database update, and a patch for test changes).

4. Running updatedatabase produced no output on the command line for me - normally it does. The way to do database updates changed a while ago, but I don't know enough about how this works to comment. Maybe ask on IRC or see https://wiki.koha-community.org/wiki/Database_updates However, the new system preference RedirectGuaranteeEmail appears for me.

5. Running the tests fail for: prove t/db_dependent/Members.t and prove t/db_dependent/Letters.t [2]

6. Comment #70 - not sure whether any of the issues raised have been addressed, or whether these should be addressed in a separate bug.

[1] Applying the patch - whitespace error:

Apply? [(y)es, (n)o, (i)nteractive] y
Applying: Bug 12532 - Send emails to guarantee and guarantor
.git/rebase-apply/patch:35: trailing whitespace.
        unless ($to_address) {  
warning: 1 line adds whitespace errors.
Using index info to reconstruct a base tree...
M       C4/Letters.pm
M       Koha/Patron.pm
M       installer/data/mysql/mandatory/sysprefs.sql
M       koha-tmpl/intranet-tmpl/prog/en/modules/admin/preferences/patrons.pref
M       t/db_dependent/Members.t
.git/rebase-apply/patch:35: trailing whitespace.
        unless ($to_address) {  
warning: 1 line adds whitespace errors.
Falling back to patching base and 3-way merge...
Auto-merging t/db_dependent/Members.t
Auto-merging koha-tmpl/intranet-tmpl/prog/en/modules/admin/preferences/patrons.pref
Auto-merging installer/data/mysql/mandatory/sysprefs.sql
Auto-merging Koha/Patron.pm
Auto-merging C4/Letters.pm
Applying: Bug 12532 - (follow-up) clean up
Applying: Bug 12532 - Correction of tests involving guarantorid
Applying: Bug 12532: Send email to guarantee and guarantor;

[2] Test output:

prove t/db_dependent/Members.t
t/db_dependent/Members.t .. 53/53 # Looks like you planned 53 tests but ran 57.
t/db_dependent/Members.t .. Dubious, test returned 255 (wstat 65280, 0xff00)
All 53 subtests passed 

Test Summary Report
-------------------
t/db_dependent/Members.t (Wstat: 65280 Tests: 57 Failed: 4)
  Failed tests:  54-57
  Non-zero exit status: 255
  Parse errors: Bad plan.  You planned 53 tests but ran 57.
Files=1, Tests=57,  2 wallclock secs ( 0.02 usr  0.01 sys +  1.77 cusr  0.42 csys =  2.22 CPU)
Result: FAIL


prove t/db_dependent/Letters.t
t/db_dependent/Letters.t .. 48/87 
    #   Failed test 'SendAlerts is using the mocked send_or_die routine (claimissues)'
    #   at t/db_dependent/Letters.t line 854.
    # didn't find a warning
    # expected to find warning: (?^u:Fake send_or_die)
    # Looks like you planned 14 tests but ran 2.
    # Looks like you failed 1 test of 2 run.

#   Failed test 'Test SMS handling in SendQueuedMessages'
#   at t/db_dependent/Letters.t line 923.
Can't call method "letter_id" on an undefined value at t/db_dependent/Letters.t line 862.
# Looks like your test exited with 11 just after 83.
t/db_dependent/Letters.t .. Dubious, test returned 11 (wstat 2816, 0xb00)
Failed 5/87 subtests 

Test Summary Report
-------------------
t/db_dependent/Letters.t (Wstat: 2816 Tests: 83 Failed: 1)
  Failed test:  83
  Non-zero exit status: 11
  Parse errors: Bad plan.  You planned 87 tests but ran 83.
Files=1, Tests=83,  2 wallclock secs ( 0.02 usr  0.00 sys +  1.74 cusr  0.34 csys =  2.10 CPU)
Result: FAIL

Created attachment 150208 [details] [review]
Bug 12532: Send email to guarantee and guarantor

This patch allows guarantors to receive emails sended to their
guarentees. This patch is a rebase of the previous patches.
I took all the content of previous commit and put it in one commit.

TO TEST:

Before applying:
1) Search, or create, a patron with guarantor.
2) Enable checkout emails for both guarantors and guarantees.
3) Checkout an item. An email should be sent only to the guarantee.
4) Apply the patch.
5) Run updatedatabase.pl
6) Enable 'RedirectGuaranteeEmail'
7) Run misc/cronjobs/process_message_queue.pl
8) Notice that the email should be sended to both the guarantee AND the
guarantor.

(In reply to David Nind from comment #97)
> I have tested (using koha-testing-docker), so that there is a sign-off from
> someone other than InLibro staff.
> 
> Notes from testing:
> 
> 1. There are whitespace errors when applying the patches. [1]
> 
> 2. Formatting patch titles - these should be in the format Bug XXXX: The
> title for the patch AND Bug XXXX: (follow-up) The title for the follow-up
> (see https://wiki.koha-community.org/wiki/Commit_messages#Subject_line).
> 
> 3. Patches with the same name - this can be confusing, but I'm not sure of
> the best way to do this for this bug. Also, as this bug has been around for
> a while, and has a bit of history, maybe the patches should be consolidated.
> I would ask on IRC to get expert advice on the best approach to take for
> this bug (maybe have the main patch, a patch for the database update, and a
> patch for test changes).
> 
> 4. Running updatedatabase produced no output on the command line for me -
> normally it does. The way to do database updates changed a while ago, but I
> don't know enough about how this works to comment. Maybe ask on IRC or see
> https://wiki.koha-community.org/wiki/Database_updates However, the new
> system preference RedirectGuaranteeEmail appears for me.
> 
> 5. Running the tests fail for: prove t/db_dependent/Members.t and prove
> t/db_dependent/Letters.t [2]
> 
> 6. Comment #70 - not sure whether any of the issues raised have been
> addressed, or whether these should be addressed in a separate bug.
 

I've made some fixes to the patch

(In reply to Hammat wele from comment #100)
> I've made some fixes to the patch

Thanks Hammat!

Unfortunately, the patch no longer applies 8-(..

git bz apply 12532

Bug 12532 - Redirect guarantee email to guarantor

150208 - Bug 12532: Send email to guarantee and guarantor

Apply? [(y)es, (n)o, (i)nteractive] y
Applying: Bug 12532: Send email to guarantee and guarantor
.git/rebase-apply/patch:37: trailing whitespace.
        unless ($to_address) {  
warning: 1 line adds whitespace errors.
Using index info to reconstruct a base tree...
M       C4/Letters.pm
M       Koha/Patron.pm
M       installer/data/mysql/mandatory/sysprefs.sql
M       koha-tmpl/intranet-tmpl/prog/en/modules/admin/preferences/patrons.pref
M       t/db_dependent/Letters.t
.git/rebase-apply/patch:37: trailing whitespace.
        unless ($to_address) {  
warning: 1 line adds whitespace errors.
Falling back to patching base and 3-way merge...
Auto-merging t/db_dependent/Letters.t
Auto-merging koha-tmpl/intranet-tmpl/prog/en/modules/admin/preferences/patrons.pref
Auto-merging installer/data/mysql/mandatory/sysprefs.sql
Auto-merging Koha/Patron.pm
Auto-merging C4/Letters.pm
CONFLICT (content): Merge conflict in C4/Letters.pm
error: Failed to merge in the changes.
Patch failed at 0001 Bug 12532: Send email to guarantee and guarantor
hint: Use 'git am --show-current-patch=diff' to see the failed patch
When you have resolved this problem run "git bz apply --continue".
If you would prefer to skip this patch, instead run "git bz apply --skip".
To restore the original branch and stop patching run "git bz apply --abort".
Patch left in /tmp/Bug-12532-Send-email-to-guarantee-and-guarantor-W5BJUI.patch

Created attachment 150217 [details] [review]
Bug 12532: Send email to guarantee and guarantor

This patch allows guarantors to receive emails sended to their
guarentees. This patch is a rebase of the previous patches.
I took all the content of previous commit and put it in one commit.

TO TEST:

Before applying:
1) Search, or create, a patron with guarantor.
2) Enable checkout emails for both guarantors and guarantees.
3) Checkout an item. An email should be sent only to the guarantee.
4) Apply the patch.
5) Run updatedatabase.pl
6) Enable 'RedirectGuaranteeEmail'
7) Run misc/cronjobs/process_message_queue.pl
8) Notice that the email should be sended to both the guarantee AND the
guarantor.

(In reply to David Nind from comment #101)
> Unfortunately, the patch no longer applies 8-(..

I've rebased the patch, this one should apply now.
Sorry

(In reply to Hammat wele from comment #103)
> (In reply to David Nind from comment #101)
> > Unfortunately, the patch no longer applies 8-(..
> 
> I've rebased the patch, this one should apply now.
> Sorry

No problem, and thanks!

I can confirm that this all now works as per the test plan.

However, the tests fail (even though that wasn't included in the test plan) - they passed before applying the patch.

prove t/db_dependent/Members.t
t/db_dependent/Members.t .. 1/57 
#   Failed test 'notice_email_address returns correct value when AutoEmailPrimaryAddress is emailpro'
#   at t/db_dependent/Members.t line 163.
#          got: 'Marie@email.com'
#     expected: 'Marie@work.com'
t/db_dependent/Members.t .. 35/57 # Looks like you failed 1 test of 57.
t/db_dependent/Members.t .. Dubious, test returned 1 (wstat 256, 0x100)
Failed 1/57 subtests 

Test Summary Report
-------------------
t/db_dependent/Members.t (Wstat: 256 Tests: 57 Failed: 1)
  Failed test:  11
  Non-zero exit status: 1
Files=1, Tests=57,  2 wallclock secs ( 0.02 usr  0.00 sys +  1.84 cusr  0.37 csys =  2.23 CPU)
Result: FAIL

prove t/db_dependent/Letters.t
t/db_dependent/Letters.t .. 48/87 
    #   Failed test 'SendQueuedMessages with limit 1'
    #   at t/db_dependent/Letters.t line 1069.
    # found warning: FAIL: No 'to_address' and INVALID borrowernumber (107) at /kohadevbox/koha/C4/Letters.pm line 1325.
    # found warning: FAIL: No 'to_address' and INVALID borrowernumber (107) at /kohadevbox/koha/C4/Letters.pm line 1325.
    # found warning: FAIL: No 'to_address' and INVALID borrowernumber (107) at /kohadevbox/koha/C4/Letters.pm line 1325.
    # found warning: FAIL: No 'to_address' and INVALID borrowernumber (107) at /kohadevbox/koha/C4/Letters.pm line 1325.
    # found warning: FAIL: No 'to_address' and INVALID borrowernumber (107) at /kohadevbox/koha/C4/Letters.pm line 1325.
    # expected to find warning: (?^u:Fake send_or_die)

    #   Failed test 'Sent 1 message with limit of 1 and 5 unprocessed messages'
    #   at t/db_dependent/Letters.t line 1071.
    #          got: '0'
    #     expected: '1'

    #   Failed test 'SendQueuedMessages with limit 2'
    #   at t/db_dependent/Letters.t line 1076.
    # didn't find a warning
    # expected to find warning: (?^u:Fake send_or_die)
    # expected to find warning: (?^u:Fake send_or_die)

    #   Failed test 'Sent 2 messages with limit of 2 and 4 unprocessed messages'
    #   at t/db_dependent/Letters.t line 1078.
    #          got: '0'
    #     expected: '2'

    #   Failed test 'SendQueuedMessages with limit 3'
    #   at t/db_dependent/Letters.t line 1083.
    # didn't find a warning
    # expected to find warning: (?^u:Fake send_or_die)
    # expected to find warning: (?^u:Fake send_or_die)

    #   Failed test 'Sent 2 messages with limit of 3 and 2 unprocessed messages'
    #   at t/db_dependent/Letters.t line 1085.
    #          got: '0'
    #     expected: '2'

    #   Failed test '5 messages sent'
    #   at t/db_dependent/Letters.t line 1088.
    #          got: '0'
    #     expected: '5'

    #   Failed test 'SendQueuedMessages with limit 2 and domain limits'
    #   at t/db_dependent/Letters.t line 1114.
    # found warning: FAIL: No 'to_address' and INVALID borrowernumber (107) at /kohadevbox/koha/C4/Letters.pm line 1325.
    # found warning: FAIL: No 'to_address' and INVALID borrowernumber (107) at /kohadevbox/koha/C4/Letters.pm line 1325.
    # found warning: Sending messages: domain fake1.domain reached limit of 1/1m at /kohadevbox/koha/Koha/Notice/Util.pm line 97.
    # found warning: Fake send_or_die at t/db_dependent/Letters.t line 43.
    # expected to find warning: (?^u:wrong.net reached limit)
    # expected to find warning: (?^u:Fake send_or_die)
    # expected to find warning: (?^u:fake1.domain reached limit)
    # expected to find warning: (?^u:Fake send_or_die)

    #   Failed test 'Only expecting 2 sent messages'
    #   at t/db_dependent/Letters.t line 1116.
    #          got: '1'
    #     expected: '2'

    #   Failed test 'Still 4 pending'
    #   at t/db_dependent/Letters.t line 1117.
    #          got: '3'
    #     expected: '4'

    #   Failed test 'We saw 6 messages while checking domain limits: so we deferred 4'
    #   at t/db_dependent/Letters.t line 1118.
    #          got: '4'
    #     expected: '6'
    # Looks like you planned 18 tests but ran 12.
    # Looks like you failed 11 tests of 12 run.

#   Failed test 'Test limit parameter for SendQueuedMessages'
#   at t/db_dependent/Letters.t line 1119.
# Looks like you failed 1 test of 87.
t/db_dependent/Letters.t .. Dubious, test returned 1 (wstat 256, 0x100)
Failed 1/87 subtests 

Test Summary Report
-------------------
t/db_dependent/Letters.t (Wstat: 256 Tests: 87 Failed: 1)
  Failed test:  86
  Non-zero exit status: 1
Files=1, Tests=87,  2 wallclock secs ( 0.02 usr  0.01 sys +  1.87 cusr  0.31 csys =  2.21 CPU)
Result: FAIL

Created attachment 150240 [details] [review]
Bug 12532: Send email to guarantee and guarantor

This patch allows guarantors to receive emails sended to their
guarentees. This patch is a rebase of the previous patches.
I took all the content of previous commit and put it in one commit.

TO TEST:

Before applying:
1) Search, or create, a patron with guarantor.
2) For both guarantors and guarantees:
   - Add an email address
   - Update the 'Patron messaging preferences' section so that an email is sent for item checkouts
3) Checkout an item. An email should be sent only to the guarantee.
4) Apply the patch.
5) Run updatedatabase.pl
6) Run prove t/db_dependent/Members.t and prove t/db_dependent/Letters.t
7) Enable 'RedirectGuaranteeEmail'
8) Run misc/cronjobs/process_message_queue.pl
9) Notice that the email should be sended to both the guarantee AND the
guarantor.

(In reply to David Nind from comment #104)
> However, the tests fail (even though that wasn't included in the test plan)
> - they passed before applying the patch.

I've added some fixes, the system preferences AutoEmailPrimaryAddress no more exist it was renamed to EmailFieldPrimary.

Thanks for testing

Created attachment 150256 [details] [review]
Bug 12532: Send email to guarantee and guarantor

This patch allows guarantors to receive emails sended to their
guarentees. This patch is a rebase of the previous patches.
I took all the content of previous commit and put it in one commit.

TO TEST:

Before applying:
1) Search, or create, a patron with guarantor.
2) For both guarantors and guarantees:
   - Add an email address
   - Update the 'Patron messaging preferences' section so that an email is sent for item checkouts
3) Checkout an item. An email should be sent only to the guarantee.
4) Apply the patch.
5) Run updatedatabase.pl
6) Run prove t/db_dependent/Members.t and prove t/db_dependent/Letters.t
7) Enable 'RedirectGuaranteeEmail'
8) Run misc/cronjobs/process_message_queue.pl
9) Notice that the email should be sended to both the guarantee AND the
guarantor.

Signed-off-by: David Nind <david@davidnind.com>

(In reply to Hammat wele from comment #106)
> (In reply to David Nind from comment #104)
> > However, the tests fail (even though that wasn't included in the test plan)
> > - they passed before applying the patch.
> 
> I've added some fixes, the system preferences AutoEmailPrimaryAddress no
> more exist it was renamed to EmailFieldPrimary.
> 
> Thanks for testing

Thanks for fixing these - everything is now working.

Good luck with the QA!

You may also wish to update the assignee for the bug to yourself.

David

Looking here..

I think it you want to make sure you're catching all locations you need to depend on bug 33223 which systematically goes through the codebase making sure we use notice_email_address consistently.

I'm doing a splinter review now.. there seems to be a lot wrapped up into the commit that I'm not sure it directly related to the bug reported.

Comment on attachment 150256 [details] [review]
Bug 12532: Send email to guarantee and guarantor

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

I love where this is going... but I think it needs a few more little fixes to get it through.

Happy to help work those in.

::: C4/Letters.pm
@@ +1471,5 @@
>        || $message->{to_address} ne $email->email->header('To');
>  
> +    _update_message_from_address($message->{'message_id'},$email->email->header('From') )
> +      if !$message->{from_address}
> +      || $message->{from_address} ne $email->email->header('From');

Not sure how this relates to the core bug here.. and I don't see any updated tests to show what this does either?

::: Koha/Email.pm
@@ +118,4 @@
>          $args->{to} = $params->{to};
>      }
>  
> +    my @emails = split(',', $args->{to});

Interesting.. I thought we already supported multiple comma delimited to addresses.. this feels bad enough that we should have it on it's own bug really... but I'm happy to let that slide to get it in.

::: Koha/Patron.pm
@@ +1439,5 @@
> +                $address .=  $guarantor_address if $guarantor_address;
> +            }
> +        }
> +    }
> +    return $address;

I'd like to see this rebased against bug 33223 given it makes the use of notice_email_address more consistent generally.

::: installer/data/mysql/mandatory/sysprefs.sql
@@ +597,4 @@
>  ('RecallsMaxPickUpDelay','7',NULL,'Define the maximum time a recall can be awaiting pickup','Integer'),
>  ('RecordLocalUseOnReturn','0',NULL,'If ON, statistically record returns of unissued items as local use, instead of return','YesNo'),
>  ('Reference_NFL_Statuses','1|2',NULL,'Contains not for loan statuses considered as available for reference','Free'),
> +('RedirectGuaranteeEmail', '0', NULL, 'Enable the ability to redirect guarantee email messages to guarantor.', 'YesNo'),

Redirect seems like the wrong term here.. we're cc'ing in effect in the end.. so perhaps it should be renamed 'Copy'

Comment on attachment 150256 [details] [review]
Bug 12532: Send email to guarantee and guarantor

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

::: Koha/Email.pm
@@ +118,5 @@
>          $args->{to} = $params->{to};
>      }
>  
> +    my @emails = split(',', $args->{to});
> +    foreach my $email (@emails) {

This isn't required.. Email::Address handles formatted lists of addresses already.

See: https://metacpan.org/pod/Email::Address#parse

Working on fixes and will PQA with follow-ups.

Created attachment 151028 [details] [review]
Bug 12532: (QA follow-up) Undo changes to Koha::Email

The changes here aren't required (Email::Address already deals with
lists of addresses internally)

Created attachment 151029 [details] [review]
Bug 12532: (QA follow-up) Clean up notice_email_address

Reduce some of the code duplication

I'm not so sure that we're hooking this into the right place.. I'm wondering if it may be better placed in Koha::Email->create.. 

Needs a little thought.. I've stuck my QA follow-up's in and will switch back to SO to get some other QA opinions.

Sorry, I also meant to say I think the guarantor email should go in the cc field rather than to field (unless the to field is empty in which case just put guarantor in to).  This complicates things a little but I feel it's the right behaviour

(In reply to Martin Renvoize from comment #115)
> I'm not so sure that we're hooking this into the right place.. I'm wondering
> if it may be better placed in Koha::Email->create.. 

I would prefer to keep ->create clean and move this specific guarantor action a level higher.

I agree Marcel.. It was late last night.

So.. In short, I'd like some explanations from the original author as to why they picked the changes they did.. I still don't understand a number of them.

I think hooking into _send_message_by_email in C4::Letters is starting to feel like it might be the right place... but setting a `cc` rather than additional `to`

Hello Martin,

Thanks for the feedbacks AND the corrections.  Now, as to your latest inquiry, I'm not the one who will answer it.  But I'll give perspective.


This has been coded about 10 years ago.  A request from one of our clients that made a lot of sense to us.  Then and now (frankly, sending email to guarantor is a VERY obvious requirement in the public libraries real world).

But it was coded in a very different Koha, and the patches have tried to follow up in the last decade through a good dozen of our team members.  

The sponsor's money have long dried up, but the ticket is opened and we maintain it with the blind faith one day we won't have to rebase it for our own deliveries (were I to stop putting it on our pile one year and 20 libraries would notice me).


So I don't know WHY things are like what they are.  And I'm certain our current dev is not aware either.  He'll modify what's asked to push it through, with no recoding/reachitecturing considered.

I'm sorry nobody at inLibro can answer the questions you asked.

(In reply to Martin Renvoize from comment #116)
> Sorry, I also meant to say I think the guarantor email should go in the cc
> field rather than to field (unless the to field is empty in which case just
> put guarantor in to).  This complicates things a little but I feel it's the
> right behaviour

(In reply to Martin Renvoize from comment #118)
> I agree Marcel.. It was late last night.
> 
> So.. In short, I'd like some explanations from the original author as to why
> they picked the changes they did.. I still don't understand a number of them.
> 
> I think hooking into _send_message_by_email in C4::Letters is starting to
> feel like it might be the right place... but setting a `cc` rather than
> additional `to`

Hi martin,

The main goal here was to send email to both the guarantor and the guarantee either we use the to or cc field.
But I agree with you, we should use the cc field to send the message to the garantors instead adding garantor addresses to the to field.

Created attachment 154854 [details] [review]
Bug 12532: Send email to guarantee and guarantor

This patch allows guarantors to receive emails sended to their
guarentees. This patch is a rebase of the previous patches.
I took all the content of previous commit and put it in one commit.

TO TEST:

Before applying:
1) Search, or create, a patron with guarantor.
2) For both guarantors and guarantees:
   - Add an email address
   - Update the 'Patron messaging preferences' section so that an email is sent for item checkouts
3) Checkout an item. An email should be sent only to the guarantee.
4) Apply the patch.
5) Run updatedatabase.pl
6) Run prove t/db_dependent/Members.t and prove t/db_dependent/Letters.t
7) Enable 'RedirectGuaranteeEmail'
8) Run misc/cronjobs/process_message_queue.pl
9) Notice that the email should be sended to both the guarantee AND the
guarantor.

Signed-off-by: David Nind <david@davidnind.com>

Created attachment 154855 [details] [review]
Bug 12532: (QA follow-up) Undo changes to Koha::Email

The changes here aren't required (Email::Address already deals with
lists of addresses internally)

Created attachment 154856 [details] [review]
Bug 12532: (QA follow-up) Clean up notice_email_address

Reduce some of the code duplication

Created attachment 154857 [details] [review]
Bug 12532: (follow-up) Send email to guarantor using the CC field

In This patch the CC field is used to send the message to the guarantors. If the «to» field is empty (the guarantee has no address) we send the message «to» the garantors.

Created attachment 155828 [details] [review]
Bug 12532: Send email to guarantee and guarantor

This patch allows guarantors to receive emails sended to their
guarentees. This patch is a rebase of the previous patches.
I took all the content of previous commit and put it in one commit.

TO TEST:

Before applying:
1) Search, or create, a patron with guarantor.
2) For both guarantors and guarantees:
   - Add an email address
   - Update the 'Patron messaging preferences' section so that an email is sent for item checkouts
3) Checkout an item. An email should be sent only to the guarantee.
4) Apply the patch.
5) Run updatedatabase.pl
6) Run prove t/db_dependent/Members.t and prove t/db_dependent/Letters.t
7) Enable 'RedirectGuaranteeEmail'
8) Run misc/cronjobs/process_message_queue.pl
9) Notice that the email should be sended to both the guarantee AND the
guarantor.

Signed-off-by: David Nind <david@davidnind.com>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>

Created attachment 155829 [details] [review]
Bug 12532: (QA follow-up) Undo changes to Koha::Email

The changes here aren't required (Email::Address already deals with
lists of addresses internally)

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>

Created attachment 155830 [details] [review]
Bug 12532: (QA follow-up) Clean up notice_email_address

Reduce some of the code duplication

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>

Created attachment 155831 [details] [review]
Bug 12532: (follow-up) Send email to guarantor using the CC field

In This patch the CC field is used to send the message to the guarantors. If the «to» field is empty (the guarantee has no address) we send the message «to» the garantors.

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>

Created attachment 155832 [details] [review]
Bug 12532: (QA follow-up) More cleanup

Minor QA cleanup, perltidy, remove Data::Dumper and clarify warn

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>

Created attachment 155833 [details] [review]
Bug 12532: (QA follow-up) Remove defunkt test

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>

Created attachment 155834 [details] [review]
Bug 12532: (QA follow-up) Add tests for SendQueuedMessages

This patch adds unit tests for the changes made to
_send_message_by_email for Guarantor CC

Created attachment 155835 [details] [review]
Bug 12532: (QA follow-up) Add cc_address to message_queue

Add the cc_address field to the message_queue so we may report back that
an email was copied to the guarantor email addresses.

Created attachment 155836 [details] [review]
Bug 12532: DBIC Schema

Created attachment 155837 [details] [review]
Bug 12532: (QA follow-up) Fix issues highlighted by unit tests

* YesNo preferences yield '1' or '0' not 'yes' and 'no'.
* Set message_cc address so we can report later that the message went to
  a cc address.

Still got a tiny bit to finnish up here to get over the qa line.. I'll continue tommorow.

Created attachment 155873 [details] [review]
Bug 12532: Send email to guarantee and guarantor

This patch allows guarantors to receive emails sended to their
guarentees. This patch is a rebase of the previous patches.
I took all the content of previous commit and put it in one commit.

TO TEST:

Before applying:
1) Search, or create, a patron with guarantor.
2) For both guarantors and guarantees:
   - Add an email address
   - Update the 'Patron messaging preferences' section so that an email is sent for item checkouts
3) Checkout an item. An email should be sent only to the guarantee.
4) Apply the patch.
5) Run updatedatabase.pl
6) Run prove t/db_dependent/Members.t and prove t/db_dependent/Letters.t
7) Enable 'RedirectGuaranteeEmail'
8) Run misc/cronjobs/process_message_queue.pl
9) Notice that the email should be sended to both the guarantee AND the
guarantor.

Signed-off-by: David Nind <david@davidnind.com>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>

Created attachment 155874 [details] [review]
Bug 12532: (QA follow-up) Undo changes to Koha::Email

The changes here aren't required (Email::Address already deals with
lists of addresses internally)

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>

Created attachment 155875 [details] [review]
Bug 12532: (QA follow-up) Clean up notice_email_address

Reduce some of the code duplication

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>

Created attachment 155876 [details] [review]
Bug 12532: (follow-up) Send email to guarantor using the CC field

In This patch the CC field is used to send the message to the guarantors. If the «to» field is empty (the guarantee has no address) we send the message «to» the garantors.

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>

Created attachment 155877 [details] [review]
Bug 12532: (QA follow-up) More cleanup

Minor QA cleanup, perltidy, remove Data::Dumper and clarify warn

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>

Created attachment 155878 [details] [review]
Bug 12532: (QA follow-up) Remove defunkt test

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>

Created attachment 155879 [details] [review]
Bug 12532: (QA follow-up) Add tests for SendQueuedMessages

This patch adds unit tests for the changes made to
_send_message_by_email for Guarantor CC

Created attachment 155880 [details] [review]
Bug 12532: (QA follow-up) Add cc_address to message_queue

Add the cc_address field to the message_queue so we may report back that
an email was copied to the guarantor email addresses.

Created attachment 155881 [details] [review]
Bug 12532: Expose CC address in patron notices display

Add the CC address into the patrons notices on the staff client to
highlight that the message was copied to another user.

Created attachment 155882 [details] [review]
Bug 12532: DBIC Schema

Created attachment 155883 [details] [review]
Bug 12532: (QA follow-up) Fix issues highlighted by unit tests

* YesNo preferences yield '1' or '0' not 'yes' and 'no'.
* Set message_cc address so we can report later that the message went to
  a cc address.

I think we're pretty much there now.. however I can't get that darn test to pass.. for some reason the second mock of the new preference doesn't want to take effect in this case so testing for what should happen when the feature is disabled is currently failing. Hopeing another set of QA eyes can spot my test issue and resolve it to get us over the finish line.

Created attachment 155886 [details] [review]
Bug 12532: Fix test

The mock was working. The problem was that the to_address of the notice
was set from the previous send.

A fix would be to update the message, but it seems better to simply
re-enqueue it.

This patch also use Koha::Notice::Messages.

Thanks for the follow-up Jonathan. Me being code blind it seems

Created attachment 155934 [details] [review]
Bug 12532: Send email to guarantee and guarantor

This patch allows guarantors to receive emails sended to their
guarentees. This patch is a rebase of the previous patches.
I took all the content of previous commit and put it in one commit.

TO TEST:

Before applying:
1) Search, or create, a patron with guarantor.
2) For both guarantors and guarantees:
   - Add an email address
   - Update the 'Patron messaging preferences' section so that an email is sent for item checkouts
3) Checkout an item. An email should be sent only to the guarantee.
4) Apply the patch.
5) Run updatedatabase.pl
6) Run prove t/db_dependent/Members.t and prove t/db_dependent/Letters.t
7) Enable 'RedirectGuaranteeEmail'
8) Run misc/cronjobs/process_message_queue.pl
9) Notice that the email should be sended to both the guarantee AND the
guarantor.

Signed-off-by: David Nind <david@davidnind.com>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>

Created attachment 155935 [details] [review]
Bug 12532: (QA follow-up) Undo changes to Koha::Email

The changes here aren't required (Email::Address already deals with
lists of addresses internally)

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>

Created attachment 155936 [details] [review]
Bug 12532: (QA follow-up) Clean up notice_email_address

Reduce some of the code duplication

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>

Created attachment 155937 [details] [review]
Bug 12532: (follow-up) Send email to guarantor using the CC field

In This patch the CC field is used to send the message to the guarantors. If the «to» field is empty (the guarantee has no address) we send the message «to» the garantors.

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>

Created attachment 155938 [details] [review]
Bug 12532: (QA follow-up) More cleanup

Minor QA cleanup, perltidy, remove Data::Dumper and clarify warn

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>

Created attachment 155939 [details] [review]
Bug 12532: (QA follow-up) Remove defunkt test

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>

Created attachment 155940 [details] [review]
Bug 12532: (QA follow-up) Add tests for SendQueuedMessages

This patch adds unit tests for the changes made to
_send_message_by_email for Guarantor CC

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>

Created attachment 155941 [details] [review]
Bug 12532: (QA follow-up) Add cc_address to message_queue

Add the cc_address field to the message_queue so we may report back that
an email was copied to the guarantor email addresses.

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>

Created attachment 155942 [details] [review]
Bug 12532: Expose CC address in patron notices display

Add the CC address into the patrons notices on the staff client to
highlight that the message was copied to another user.

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>

Created attachment 155943 [details] [review]
Bug 12532: DBIC Schema

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>

Created attachment 155944 [details] [review]
Bug 12532: (QA follow-up) Fix issues highlighted by unit tests

* YesNo preferences yield '1' or '0' not 'yes' and 'no'.
* Set message_cc address so we can report later that the message went to
  a cc address.

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>

Created attachment 155945 [details] [review]
Bug 12532: Fix test

The mock was working. The problem was that the to_address of the notice
was set from the previous send.

A fix would be to update the message, but it seems better to simply
re-enqueue it.

This patch also use Koha::Notice::Messages.

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>

Created attachment 155946 [details] [review]
Bug 12532: (follow-up) Catch more cases in tests

This patch added lots more cases to the unit tests to try and catch all
the new functionality.

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>

OK, this took a lot more finagling to make it QA happy than I was hoping.. but the time is spent now and the feature seems good to go to me.

Passing QA

A big big Thank You! Martin.  We have rebased this patch for so long, and it is sooo far from our initial commit...  I'm very happy it finally came through.  You gave it the final, needed, push.

Pushed to master for 23.11.

Nice work everyone, thanks!

new feature not pushed to 23.05.x

*** Bug 35162 has been marked as a duplicate of this bug. ***

Thanks for this new feature!