Bug 12532 - Redirect guarantee email to guarantor
Summary: Redirect guarantee email to guarantor
Status: In Discussion
Alias: None
Product: Koha
Classification: Unclassified
Component: Patrons (show other bugs)
Version: master
Hardware: All All
: P5 - low new feature (vote)
Target Milestone: ---
Assignee: Charles Farmer
QA Contact: Testopia
URL:
Whiteboard:
Keywords:
Depends on:
Blocks:
 
Reported: 2014-07-04 14:58 UTC by simith.doliveira
Modified: 2019-04-18 12:31 UTC (History)
22 users (show)

See Also:
Change sponsored?: Sponsored
Patch complexity: Small patch
Bot Control: ---
When did the bot last check this:
Who signed the patch off:
Text to go in the release notes:
Version(s) released in:


Attachments
Bug 12532 - Redirect guarantee email to guarantor email garantor (4.17 KB, patch)
2014-07-04 15:03 UTC, simith.doliveira
Details | Diff | Splinter Review
Bug 12532 - Redirect guarantee email to guarantor (7.62 KB, patch)
2014-08-12 20:06 UTC, simith.doliveira
Details | Diff | Splinter Review
Bug 12532 - Redirect guarantee email to guarantor (7.72 KB, patch)
2015-01-15 16:08 UTC, simith.doliveira
Details | Diff | Splinter Review
Bug 12532 - Redirect guarantee email to guarantor (7.72 KB, patch)
2015-03-09 20:05 UTC, simith.doliveira
Details | Diff | Splinter Review
Bug 12532 - Redirect guarantee email to guarantor (12.63 KB, patch)
2015-06-02 18:51 UTC, Maxime Beaulieu
Details | Diff | Splinter Review
[SIGNED OFF] Bug 12532 - Redirect guarantee email to guarantor (12.68 KB, patch)
2015-06-05 20:46 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 12532 - Redirect guarantee email to guarantor (11.58 KB, patch)
2016-03-03 18:53 UTC, Rémi Mayrand-Provencher
Details | Diff | Splinter Review
Bug 12532 - Redirect guarantee email to guarantor (14.14 KB, patch)
2016-07-22 15:37 UTC, Mehdi Hamidi
Details | Diff | Splinter Review
Bug 11879 - QA Follow-up (2.80 KB, patch)
2016-07-25 15:04 UTC, Mehdi Hamidi
Details | Diff | Splinter Review
Bug 12532 - QA Follow-up (2.80 KB, patch)
2016-07-25 15:07 UTC, Mehdi Hamidi
Details | Diff | Splinter Review
Bug 12532 - Redirect guarantee email to guarantor (14.22 KB, patch)
2016-07-25 16:40 UTC, Marc Véron
Details | Diff | Splinter Review
Bug 12532 - QA Follow-up (2.84 KB, patch)
2016-07-25 16:41 UTC, Marc Véron
Details | Diff | Splinter Review
Bug12532 - delete useless code and many change for preference EnableRedirectGuaranteeEmail (5.56 KB, patch)
2016-10-03 19:07 UTC, Bouzid Fergani
Details | Diff | Splinter Review
Bug 12532 - Redirect guarantee email to guarantor (12.92 KB, patch)
2016-10-05 12:59 UTC, Bouzid Fergani
Details | Diff | Splinter Review
Bug 12532 - QA Follow-up (5.27 KB, patch)
2016-10-05 12:59 UTC, Bouzid Fergani
Details | Diff | Splinter Review
Bug 12532 - QA Follow-up (5.87 KB, patch)
2017-01-23 17:17 UTC, Bouzid Fergani
Details | Diff | Splinter Review
Bug 12532 - Redirect guarantee email to guarantor (13.63 KB, patch)
2017-01-23 17:18 UTC, Bouzid Fergani
Details | Diff | Splinter Review
Bug 12532 - QA Follow-up (5.87 KB, patch)
2017-01-23 17:19 UTC, Bouzid Fergani
Details | Diff | Splinter Review
Delete a dupilcated row in systempreferences table (7.38 KB, patch)
2017-05-05 18:20 UTC, Philippe Audet-Fortin
Details | Diff | Splinter Review
Bug 12532 - Changed occurrences to EnableRedirectGuaranteeEmail for RedirectGuaranteeEmail (1.65 KB, patch)
2017-07-05 15:26 UTC, Philippe Audet-Fortin
Details | Diff | Splinter Review
Bug 12532 - QA Follow-up (5.66 KB, patch)
2017-07-05 19:13 UTC, Philippe Audet-Fortin
Details | Diff | Splinter Review
Bug 12532 - Send emails to guarantee and guarantor (8.46 KB, patch)
2017-09-13 13:52 UTC, David Bourgault
Details | Diff | Splinter Review
Bug 12532 - Send emails to guarantee and guarantor (8.52 KB, patch)
2017-10-21 01:08 UTC, Alex Buckley
Details | Diff | Splinter Review
Bug 12532 - Send emails to guarantee and guarantor (9.29 KB, patch)
2018-01-23 23:22 UTC, Charles Farmer
Details | Diff | Splinter Review
Bug 12532 - Send emails to guarantee and guarantor (7.69 KB, patch)
2018-02-21 22:16 UTC, Roch D'Amour
Details | Diff | Splinter Review
Bug 12532 - Send emails to guarantee and guarantor (7.66 KB, patch)
2018-03-09 16:24 UTC, Roch D'Amour
Details | Diff | Splinter Review
Bug 12532 - Send emails to guarantee and guarantor (7.82 KB, patch)
2018-05-06 02:39 UTC, Charles Farmer
Details | Diff | Splinter Review
Bug 12532 - Send emails to guarantee and guarantor (7.82 KB, patch)
2018-08-09 01:56 UTC, Charles Farmer
Details | Diff | Splinter Review
Bug 12532 - Send emails to guarantee and guarantor (7.74 KB, patch)
2018-10-09 21:05 UTC, Blou
Details | Diff | Splinter Review
Bug 12532 - Send emails to guarantee and guarantor (7.73 KB, patch)
2019-02-14 17:32 UTC, Mikaël Olangcay Brisebois
Details | Diff | Splinter Review
Bug 12532 - (follow up) clean up (2.48 KB, patch)
2019-02-14 17:32 UTC, Mikaël Olangcay Brisebois
Details | Diff | Splinter Review
Bug 12532 - Send emails to guarantee and guarantor (8.20 KB, patch)
2019-04-11 19:31 UTC, José-Mario Monteiro-Santos
Details | Diff | Splinter Review
Bug 12532 - (follow up) clean up (2.47 KB, patch)
2019-04-11 19:31 UTC, José-Mario Monteiro-Santos
Details | Diff | Splinter Review
Bug 12532 - Send emails to guarantee and guarantor (8.19 KB, patch)
2019-04-17 15:53 UTC, Mikaël Olangcay Brisebois
Details | Diff | Splinter Review
Bug 12532 - (follow-up) clean up (1.57 KB, patch)
2019-04-17 15:53 UTC, Mikaël Olangcay Brisebois
Details | Diff | Splinter Review
Bug 12532 - Send emails to guarantee and guarantor (8.19 KB, patch)
2019-04-17 15:56 UTC, Mikaël Olangcay Brisebois
Details | Diff | Splinter Review
Bug 12532 - (follow-up) clean up (1.57 KB, patch)
2019-04-17 15:56 UTC, Mikaël Olangcay Brisebois
Details | Diff | Splinter Review
Bug 12532 - Send emails to guarantee and guarantor (8.19 KB, patch)
2019-04-17 15:57 UTC, Mikaël Olangcay Brisebois
Details | Diff | Splinter Review
Bug 12532 - (follow-up) clean up (1.57 KB, patch)
2019-04-17 15:57 UTC, Mikaël Olangcay Brisebois
Details | Diff | Splinter Review
Bug 12532 - Send emails to guarantee and guarantor (8.20 KB, patch)
2019-04-18 12:31 UTC, José-Mario Monteiro-Santos
Details | Diff | Splinter Review
Bug 12532 - (follow-up) clean up (1.56 KB, patch)
2019-04-18 12:31 UTC, José-Mario Monteiro-Santos
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
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 M. 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 M. 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