Bug 12532

Summary: Copy guarantee email to guarantor (or redirect if guarantee has no email set)
Product: Koha Reporter: simith.doliveira
Component: PatronsAssignee: Hammat wele <hammat.wele>
Status: Needs documenting --- QA Contact: Martin Renvoize <martin.renvoize>
Severity: new feature    
Priority: P5 - low CC: black23, caroline.cyr-la-rose, david, eric.begin, fridolin.somers, gmcharlt, hammat.wele, hattara, jesse, jonathan.druart, kati.sillgren, katrin.fischer, kyle.m.hall, kyle, Laura.escamilla, m.de.rooy, martin.renvoize, nick, philippe.blouin, veron, wizzyrea
Version: Main   
Hardware: All   
OS: All   
See Also: http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=12802
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=33223
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=28556
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=35836
Change sponsored?: Sponsored Patch complexity: Small patch
Documentation contact: Documentation submission:
Text to go in the release notes:
This new feature allows libraries to set email notices for guaranteed users to be copied to their guarantors. If the guarantee doesn't have a valid email of their own, then enabling this feature will redirect the guarantee's email notices to the guarantor. A new system preference, `RedirectGuaranteeEmail`, is introduced.
Version(s) released in:
23.11.00
Bug Depends on: 33223    
Bug Blocks: 18397    
Attachments: Bug 12532 - Redirect guarantee email to guarantor email garantor
Bug 12532 - Redirect guarantee email to guarantor
Bug 12532 - Redirect guarantee email to guarantor
Bug 12532 - Redirect guarantee email to guarantor
Bug 12532 - Redirect guarantee email to guarantor
[SIGNED OFF] Bug 12532 - Redirect guarantee email to guarantor
Bug 12532 - Redirect guarantee email to guarantor
Bug 12532 - Redirect guarantee email to guarantor
Bug 11879 - QA Follow-up
Bug 12532 - QA Follow-up
Bug 12532 - Redirect guarantee email to guarantor
Bug 12532 - QA Follow-up
Bug12532 - delete useless code and many change for preference EnableRedirectGuaranteeEmail
Bug 12532 - Redirect guarantee email to guarantor
Bug 12532 - QA Follow-up
Bug 12532 - QA Follow-up
Bug 12532 - Redirect guarantee email to guarantor
Bug 12532 - QA Follow-up
Delete a dupilcated row in systempreferences table
Bug 12532 - Changed occurrences to EnableRedirectGuaranteeEmail for RedirectGuaranteeEmail
Bug 12532 - QA Follow-up
Bug 12532 - Send emails to guarantee and guarantor
Bug 12532 - Send emails to guarantee and guarantor
Bug 12532 - Send emails to guarantee and guarantor
Bug 12532 - Send emails to guarantee and guarantor
Bug 12532 - Send emails to guarantee and guarantor
Bug 12532 - Send emails to guarantee and guarantor
Bug 12532 - Send emails to guarantee and guarantor
Bug 12532 - Send emails to guarantee and guarantor
Bug 12532 - Send emails to guarantee and guarantor
Bug 12532 - (follow up) clean up
Bug 12532 - Send emails to guarantee and guarantor
Bug 12532 - (follow up) clean up
Bug 12532 - Send emails to guarantee and guarantor
Bug 12532 - (follow-up) clean up
Bug 12532 - Send emails to guarantee and guarantor
Bug 12532 - (follow-up) clean up
Bug 12532 - Send emails to guarantee and guarantor
Bug 12532 - (follow-up) clean up
Bug 12532 - Send emails to guarantee and guarantor
Bug 12532 - (follow-up) clean up
Bug 12532 - Send emails to guarantee and guarantor
Bug 12532 - Rebase and correction of the patch by adding the possibility of sending emails to several guarantors
Bug 12532 - Correction of tests involving guarantorid
Bug 12532 - Send emails to guarantee and guarantor
Bug 12532 - (follow-up) clean up
Bug 12532 - Rebase and correction of the patch by adding the possibility of sending emails to several guarantors
Bug 12532 - Correction of tests involving guarantorid
Bug 12532 - Send emails to guarantee and guarantor
Bug 12532 - (follow-up) clean up
Bug 12532 - Correction of tests involving guarantorid
Bug 12532: Send email to guarantee and guarantor;
Bug 12532: Send email to guarantee and guarantor;
Bug 12532: Send email to guarantee and guarantor;
Bug 12532: Send email to guarantee and guarantor
Bug 12532: Send email to guarantee and guarantor
Bug 12532: Send email to guarantee and guarantor
Bug 12532: Send email to guarantee and guarantor
Bug 12532: (QA follow-up) Undo changes to Koha::Email
Bug 12532: (QA follow-up) Clean up notice_email_address
Bug 12532: Send email to guarantee and guarantor
Bug 12532: (QA follow-up) Undo changes to Koha::Email
Bug 12532: (QA follow-up) Clean up notice_email_address
Bug 12532: (follow-up) Send email to guarantor using the CC field
Bug 21272: MARC import should warn about mis-matched branch and item-type fields in 952 during staged import
Bug 21272: Unit test
Bug 12532: Send email to guarantee and guarantor
Bug 12532: (QA follow-up) Undo changes to Koha::Email
Bug 12532: (QA follow-up) Clean up notice_email_address
Bug 12532: (follow-up) Send email to guarantor using the CC field
Bug 12532: (QA follow-up) More cleanup
Bug 12532: (QA follow-up) Remove defunkt test
Bug 12532: (QA follow-up) Add tests for SendQueuedMessages
Bug 12532: (QA follow-up) Add cc_address to message_queue
Bug 12532: DBIC Schema
Bug 12532: (QA follow-up) Fix issues highlighted by unit tests
Bug 12532: Send email to guarantee and guarantor
Bug 12532: (QA follow-up) Undo changes to Koha::Email
Bug 12532: (QA follow-up) Clean up notice_email_address
Bug 12532: (follow-up) Send email to guarantor using the CC field
Bug 12532: (QA follow-up) More cleanup
Bug 12532: (QA follow-up) Remove defunkt test
Bug 12532: (QA follow-up) Add tests for SendQueuedMessages
Bug 12532: (QA follow-up) Add cc_address to message_queue
Bug 12532: Expose CC address in patron notices display
Bug 12532: DBIC Schema
Bug 12532: (QA follow-up) Fix issues highlighted by unit tests
Bug 12532: Fix test
Bug 12532: Send email to guarantee and guarantor
Bug 12532: (QA follow-up) Undo changes to Koha::Email
Bug 12532: (QA follow-up) Clean up notice_email_address
Bug 12532: (follow-up) Send email to guarantor using the CC field
Bug 12532: (QA follow-up) More cleanup
Bug 12532: (QA follow-up) Remove defunkt test
Bug 12532: (QA follow-up) Add tests for SendQueuedMessages
Bug 12532: (QA follow-up) Add cc_address to message_queue
Bug 12532: Expose CC address in patron notices display
Bug 12532: DBIC Schema
Bug 12532: (QA follow-up) Fix issues highlighted by unit tests
Bug 12532: Fix test
Bug 12532: (follow-up) Catch more cases in tests

Description simith.doliveira 2014-07-04 14:58:05 UTC
Our sponsor require an adjustment to redirect guarantee email messages to guarantor. 

All email message placed in message_queue table will be affected: ODUE, HOLD, DUE, PREDUE, CHECKIN, CHECKOUT, RLIST, ACCTDETAILS  

Sponsor by: ville de victoriaville
Comment 1 simith.doliveira 2014-07-04 15:03:39 UTC Comment hidden (obsolete)
Comment 2 Katrin Fischer 2014-07-05 14:41:14 UTC
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?
Comment 3 Katrin Fischer 2014-07-05 14:46:45 UTC
I think we won't get around a pref here as people have been using relationships in different ways.
Comment 4 simith.doliveira 2014-08-12 20:06:27 UTC Comment hidden (obsolete)
Comment 5 Nick Clemens 2015-01-09 22:18:18 UTC
<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.
Comment 6 simith.doliveira 2015-01-15 16:08:08 UTC Comment hidden (obsolete)
Comment 7 Jesse Maseto 2015-03-03 10:10:36 UTC
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
Comment 8 simith.doliveira 2015-03-09 20:05:18 UTC Comment hidden (obsolete)
Comment 9 simith.doliveira 2015-03-09 20:09:03 UTC
conflit fixed
Comment 10 Mark Tompsett 2015-03-31 00:02:37 UTC
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. :)
Comment 11 Mark Tompsett 2015-03-31 01:26:49 UTC
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.
Comment 12 Maxime Beaulieu 2015-06-02 18:51:27 UTC Comment hidden (obsolete)
Comment 13 Nick Clemens 2015-06-04 04:21:45 UTC
I tried testing, patches applied and update ran fine but both tests failed and emails were not redirected.
Comment 14 Maxime Beaulieu 2015-06-05 20:06:51 UTC
(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.
Comment 15 Nick Clemens 2015-06-05 20:46:20 UTC Comment hidden (obsolete)
Comment 16 Nick Clemens 2015-06-05 21:00:59 UTC
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.
Comment 17 Jonathan Druart 2015-06-16 17:02:21 UTC
(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?
Comment 18 Francois Charbonnier 2015-06-23 12:10:41 UTC
(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.
Comment 19 Jonathan Druart 2015-06-23 14:58:54 UTC
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 :)
Comment 20 Maxime Beaulieu 2015-07-21 20:31:21 UTC
(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.
Comment 21 Jonathan Druart 2015-08-04 07:33:53 UTC
(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.
Comment 22 Francois Charbonnier 2016-01-04 20:04:39 UTC
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)
Comment 23 Jonathan Druart 2016-01-05 07:31:28 UTC
(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.
Comment 24 Marcel de Rooy 2016-02-05 09:24:52 UTC
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.
Comment 25 Rémi Mayrand-Provencher 2016-03-03 18:53:18 UTC Comment hidden (obsolete)
Comment 26 Rémi Mayrand-Provencher 2016-03-03 18:56:23 UTC
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.
Comment 27 Owen Leonard 2016-04-04 18:50:26 UTC
I think based on this bug's history that it should be "Signed off" at this stage.
Comment 28 Kyle M Hall 2016-04-28 13:21:02 UTC
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?
Comment 29 Mehdi Hamidi 2016-07-22 15:36:10 UTC
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.
Comment 30 Mehdi Hamidi 2016-07-22 15:37:09 UTC Comment hidden (obsolete)
Comment 31 Marc Véron 2016-07-25 05:20:39 UTC
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
Comment 32 Mehdi Hamidi 2016-07-25 15:04:36 UTC Comment hidden (obsolete)
Comment 33 Mehdi Hamidi 2016-07-25 15:07:22 UTC Comment hidden (obsolete)
Comment 34 Marc Véron 2016-07-25 16:40:25 UTC
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>
Comment 35 Marc Véron 2016-07-25 16:41:40 UTC
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 36 Katrin Fischer 2016-08-24 22:04:55 UTC
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?
Comment 37 Bouzid Fergani 2016-10-03 19:07:59 UTC
Created attachment 55985 [details] [review]
Bug12532 - delete useless code and many change for preference EnableRedirectGuaranteeEmail

 messages affected: ODUE, HOLD, DUE, PREDUE, CHECKIN, CHECKOUT
Comment 38 Marc Véron 2016-10-04 07:48:29 UTC
Wanted to test on current master, but first patch does not apply, sorry.
Comment 39 Bouzid Fergani 2016-10-05 12:59:10 UTC
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>
Comment 40 Bouzid Fergani 2016-10-05 12:59:32 UTC Comment hidden (obsolete)
Comment 41 Bouzid Fergani 2016-10-05 13:02:10 UTC
Fix conflict and rebase
Comment 42 Luke Honiss 2017-01-19 22:16:19 UTC
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.
Comment 43 Bouzid Fergani 2017-01-23 17:17:11 UTC
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>
Comment 44 Bouzid Fergani 2017-01-23 17:18:48 UTC
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>
Comment 45 Bouzid Fergani 2017-01-23 17:19:40 UTC
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>
Comment 46 Bouzid Fergani 2017-01-23 17:22:18 UTC
I rebase all patches and fix small problems.
Comment 47 Philippe Audet-Fortin 2017-05-05 18:20:24 UTC Comment hidden (obsolete)
Comment 48 Philippe Audet-Fortin 2017-07-05 15:26:22 UTC
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.
Comment 49 Philippe Audet-Fortin 2017-07-05 15:44:05 UTC
Corrected minor error. However, "Bug 12532 - QA Follow-up" give me a Sha1 error so it couldn't be applied.
Comment 50 Philippe Audet-Fortin 2017-07-05 15:46:08 UTC Comment hidden (obsolete)
Comment 51 Philippe Audet-Fortin 2017-07-05 19:13:35 UTC
Created attachment 64824 [details] [review]
Bug 12532 - QA Follow-up

Fixed tests, they now work without any fails.
Comment 52 Philippe Audet-Fortin 2017-07-05 19:19:14 UTC
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.
Comment 53 David Bourgault 2017-09-13 13:52:04 UTC Comment hidden (obsolete)
Comment 54 Alex Buckley 2017-10-21 01:08:16 UTC Comment hidden (obsolete)
Comment 55 Charles Farmer 2018-01-23 23:22:19 UTC Comment hidden (obsolete)
Comment 56 Charles Farmer 2018-01-23 23:37:27 UTC
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'
Comment 57 Jonathan Druart 2018-01-30 14:56:40 UTC
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.
Comment 58 Roch D'Amour 2018-02-19 23:47:38 UTC
(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?
Comment 59 Jonathan Druart 2018-02-20 15:15:18 UTC
See bug 19303 and bug 19304
Comment 60 Roch D'Amour 2018-02-21 22:16:29 UTC Comment hidden (obsolete)
Comment 61 Roch D'Amour 2018-03-09 16:24:41 UTC Comment hidden (obsolete)
Comment 62 Charles Farmer 2018-05-06 02:39:41 UTC Comment hidden (obsolete)
Comment 63 Charles Farmer 2018-08-09 01:56:09 UTC Comment hidden (obsolete)
Comment 64 Michal Denar 2018-09-26 12:29:54 UTC
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
Comment 65 Katrin Fischer 2018-09-27 11:45:47 UTC
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.
Comment 66 Michal Denar 2018-09-27 11:59:12 UTC
Ok, I'll try it again.

Thank You for review.


Mike
Comment 67 Blou 2018-10-09 21:05:11 UTC Comment hidden (obsolete)
Comment 68 Mikaël Olangcay Brisebois 2019-02-14 17:32:24 UTC Comment hidden (obsolete)
Comment 69 Mikaël Olangcay Brisebois 2019-02-14 17:32:35 UTC
Created attachment 85135 [details] [review]
Bug 12532 - (follow up) clean up
Comment 70 Liz Rea 2019-04-05 16:08:08 UTC
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
Comment 71 José-Mario Monteiro-Santos 2019-04-11 19:31:20 UTC Comment hidden (obsolete)
Comment 72 José-Mario Monteiro-Santos 2019-04-11 19:31:29 UTC Comment hidden (obsolete)
Comment 73 Mikaël Olangcay Brisebois 2019-04-17 15:53:16 UTC Comment hidden (obsolete)
Comment 74 Mikaël Olangcay Brisebois 2019-04-17 15:53:27 UTC Comment hidden (obsolete)
Comment 75 Mikaël Olangcay Brisebois 2019-04-17 15:56:11 UTC Comment hidden (obsolete)
Comment 76 Mikaël Olangcay Brisebois 2019-04-17 15:56:35 UTC Comment hidden (obsolete)
Comment 77 Mikaël Olangcay Brisebois 2019-04-17 15:57:40 UTC Comment hidden (obsolete)
Comment 78 Mikaël Olangcay Brisebois 2019-04-17 15:57:50 UTC Comment hidden (obsolete)
Comment 79 Michal Denar 2019-04-17 19:01:05 UTC
Hi,
status is still "In Discussion"? Or Need Signed Off?

Thank you.
Comment 80 José-Mario Monteiro-Santos 2019-04-18 12:31:40 UTC
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
Comment 81 José-Mario Monteiro-Santos 2019-04-18 12:31:46 UTC
Created attachment 88296 [details] [review]
Bug 12532 - (follow-up) clean up
Comment 82 Caroline Cyr La Rose 2020-12-09 14:55:45 UTC
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?
Comment 83 Alexis Ripetti 2020-12-16 17:24:59 UTC Comment hidden (obsolete)
Comment 84 Alexis Ripetti 2020-12-16 17:25:18 UTC
Created attachment 114456 [details] [review]
Bug 12532 - Rebase and correction of the patch by adding the possibility of sending emails to several guarantors
Comment 85 Alexis Ripetti 2020-12-16 17:25:25 UTC Comment hidden (obsolete)
Comment 86 Alexis Ripetti 2020-12-16 17:32:41 UTC Comment hidden (obsolete)
Comment 87 Alexis Ripetti 2020-12-16 17:32:50 UTC
Created attachment 114459 [details] [review]
Bug 12532 - (follow-up) clean up
Comment 88 Alexis Ripetti 2020-12-16 17:32:55 UTC
Created attachment 114460 [details]
Bug 12532 - Rebase and correction of the patch by adding the possibility of sending emails to several guarantors
Comment 89 Alexis Ripetti 2020-12-16 17:33:01 UTC
Created attachment 114461 [details] [review]
Bug 12532 - Correction of tests involving guarantorid
Comment 90 David Nind 2021-04-09 19:58:33 UTC
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
Comment 91 The Minh Luong 2022-02-24 20:56:07 UTC
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
Comment 92 The Minh Luong 2022-02-24 20:56:12 UTC
Created attachment 131092 [details] [review]
Bug 12532 - (follow-up) clean up
Comment 93 The Minh Luong 2022-02-24 20:56:18 UTC
Created attachment 131093 [details] [review]
Bug 12532 - Correction of tests involving guarantorid
Comment 94 The Minh Luong 2022-02-24 20:56:25 UTC
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.
Comment 95 Laura Escamilla 2023-04-03 17:30:36 UTC
(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.
Comment 96 Marius 2023-04-06 13:32:37 UTC
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>
Comment 97 David Nind 2023-04-06 19:12:23 UTC
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
Comment 98 Marius 2023-04-21 20:10:00 UTC Comment hidden (obsolete)
Comment 99 Hammat wele 2023-04-25 14:18:06 UTC
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.
Comment 100 Hammat wele 2023-04-25 14:37:11 UTC
(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
Comment 101 David Nind 2023-04-25 18:45:01 UTC
(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
Comment 102 Hammat wele 2023-04-25 20:34:52 UTC
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.
Comment 103 Hammat wele 2023-04-25 20:43:13 UTC
(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
Comment 104 David Nind 2023-04-26 00:47:52 UTC
(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
Comment 105 Hammat wele 2023-04-26 15:04:15 UTC
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.
Comment 106 Hammat wele 2023-04-26 15:32:42 UTC
(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
Comment 107 David Nind 2023-04-26 19:34:55 UTC
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>
Comment 108 David Nind 2023-04-26 19:36:11 UTC
(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
Comment 109 Martin Renvoize 2023-05-10 16:49:20 UTC
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 110 Martin Renvoize 2023-05-10 16:55:38 UTC
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 111 Martin Renvoize 2023-05-10 17:11:25 UTC
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
Comment 112 Martin Renvoize 2023-05-10 17:16:37 UTC
Working on fixes and will PQA with follow-ups.
Comment 113 Martin Renvoize 2023-05-10 18:26:32 UTC
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)
Comment 114 Martin Renvoize 2023-05-10 18:26:37 UTC
Created attachment 151029 [details] [review]
Bug 12532: (QA follow-up) Clean up notice_email_address

Reduce some of the code duplication
Comment 115 Martin Renvoize 2023-05-10 18:28:42 UTC
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.
Comment 116 Martin Renvoize 2023-05-10 18:39:13 UTC
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
Comment 117 Marcel de Rooy 2023-05-11 05:31:04 UTC
(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.
Comment 118 Martin Renvoize 2023-05-11 06:47:09 UTC
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`
Comment 119 Blou 2023-05-11 13:45:13 UTC
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.
Comment 120 Hammat wele 2023-08-25 23:07:37 UTC
(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.
Comment 121 Hammat wele 2023-08-25 23:42:19 UTC
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>
Comment 122 Hammat wele 2023-08-25 23:42:24 UTC
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)
Comment 123 Hammat wele 2023-08-25 23:42:29 UTC
Created attachment 154856 [details] [review]
Bug 12532: (QA follow-up) Clean up notice_email_address

Reduce some of the code duplication
Comment 124 Hammat wele 2023-08-25 23:42:34 UTC
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.
Comment 125 Hammat wele 2023-08-31 19:12:26 UTC Comment hidden (obsolete)
Comment 126 Hammat wele 2023-08-31 19:12:31 UTC Comment hidden (obsolete)
Comment 127 Martin Renvoize 2023-09-18 15:37:36 UTC
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>
Comment 128 Martin Renvoize 2023-09-18 15:37:40 UTC
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>
Comment 129 Martin Renvoize 2023-09-18 15:37:44 UTC
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>
Comment 130 Martin Renvoize 2023-09-18 15:37:48 UTC
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>
Comment 131 Martin Renvoize 2023-09-18 15:37:51 UTC
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>
Comment 132 Martin Renvoize 2023-09-18 15:37:55 UTC
Created attachment 155833 [details] [review]
Bug 12532: (QA follow-up) Remove defunkt test

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 133 Martin Renvoize 2023-09-18 15:37:59 UTC
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
Comment 134 Martin Renvoize 2023-09-18 15:38:03 UTC
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.
Comment 135 Martin Renvoize 2023-09-18 15:38:07 UTC
Created attachment 155836 [details] [review]
Bug 12532: DBIC Schema
Comment 136 Martin Renvoize 2023-09-18 15:38:11 UTC
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.
Comment 137 Martin Renvoize 2023-09-18 15:38:41 UTC
Still got a tiny bit to finnish up here to get over the qa line.. I'll continue tommorow.
Comment 138 Martin Renvoize 2023-09-19 06:49:04 UTC
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>
Comment 139 Martin Renvoize 2023-09-19 06:49:08 UTC
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>
Comment 140 Martin Renvoize 2023-09-19 06:49:13 UTC
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>
Comment 141 Martin Renvoize 2023-09-19 06:49:17 UTC
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>
Comment 142 Martin Renvoize 2023-09-19 06:49:21 UTC
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>
Comment 143 Martin Renvoize 2023-09-19 06:49:25 UTC
Created attachment 155878 [details] [review]
Bug 12532: (QA follow-up) Remove defunkt test

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 144 Martin Renvoize 2023-09-19 06:49:29 UTC
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
Comment 145 Martin Renvoize 2023-09-19 06:49:33 UTC
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.
Comment 146 Martin Renvoize 2023-09-19 06:49:37 UTC
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.
Comment 147 Martin Renvoize 2023-09-19 06:49:41 UTC
Created attachment 155882 [details] [review]
Bug 12532: DBIC Schema
Comment 148 Martin Renvoize 2023-09-19 06:49:45 UTC
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.
Comment 149 Martin Renvoize 2023-09-19 06:53:56 UTC
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.
Comment 150 Jonathan Druart 2023-09-19 09:15:54 UTC
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.
Comment 151 Martin Renvoize 2023-09-19 20:50:24 UTC
Thanks for the follow-up Jonathan. Me being code blind it seems
Comment 152 Martin Renvoize 2023-09-20 14:11:50 UTC
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>
Comment 153 Martin Renvoize 2023-09-20 14:11:54 UTC
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>
Comment 154 Martin Renvoize 2023-09-20 14:11:58 UTC
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>
Comment 155 Martin Renvoize 2023-09-20 14:12:02 UTC
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>
Comment 156 Martin Renvoize 2023-09-20 14:12:06 UTC
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>
Comment 157 Martin Renvoize 2023-09-20 14:12:10 UTC
Created attachment 155939 [details] [review]
Bug 12532: (QA follow-up) Remove defunkt test

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 158 Martin Renvoize 2023-09-20 14:12:14 UTC
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>
Comment 159 Martin Renvoize 2023-09-20 14:12:18 UTC
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>
Comment 160 Martin Renvoize 2023-09-20 14:12:22 UTC
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>
Comment 161 Martin Renvoize 2023-09-20 14:12:26 UTC
Created attachment 155943 [details] [review]
Bug 12532: DBIC Schema

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 162 Martin Renvoize 2023-09-20 14:12:30 UTC
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>
Comment 163 Martin Renvoize 2023-09-20 14:12:34 UTC
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>
Comment 164 Martin Renvoize 2023-09-20 14:12:38 UTC
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>
Comment 165 Martin Renvoize 2023-09-20 14:14:28 UTC
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
Comment 166 Blou 2023-09-20 14:18:33 UTC
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.
Comment 167 Tomás Cohen Arazi 2023-10-10 13:02:10 UTC
Pushed to master for 23.11.

Nice work everyone, thanks!
Comment 168 Fridolin Somers 2023-10-12 20:48:39 UTC
new feature not pushed to 23.05.x
Comment 169 Katrin Fischer 2023-10-28 16:08:45 UTC
*** Bug 35162 has been marked as a duplicate of this bug. ***
Comment 170 Kati Sillgren 2023-10-30 06:44:46 UTC
Thanks for this new feature!