From 4747bfbea3f4abb66d05a2bd1abed69e28a4aa3d Mon Sep 17 00:00:00 2001 From: Mitch Jackson Date: Sun, 28 Jan 2018 02:41:17 -0600 Subject: [PATCH] RT# 73421 Fixed E-Mail pipeline to obey contact opt-in flags --- FS/FS/cust_main.pm | 96 ++++++++++++++++++++++------- FS/FS/cust_main_Mixin.pm | 38 +++++++++--- FS/FS/msg_template/email.pm | 29 ++++++++- httemplate/misc/email-customers.html | 27 ++++---- httemplate/view/cust_main/contacts_new.html | 2 +- 5 files changed, 146 insertions(+), 46 deletions(-) diff --git a/FS/FS/cust_main.pm b/FS/FS/cust_main.pm index 925eb4e44..b8d8f102f 100644 --- a/FS/FS/cust_main.pm +++ b/FS/FS/cust_main.pm @@ -3022,48 +3022,101 @@ sub invoicing_list_emailonly_scalar { join(', ', $self->invoicing_list_emailonly); } -=item contact_list [ CLASSNUM, ... ] +=item contact_list [ CLASSNUM, DEST_FLAG... ] -Returns a list of contacts (L objects) for the customer. If -a list of contact classnums is given, returns only contacts in those -classes. If the pseudo-classnum 'invoice' is given, returns contacts that -are marked as invoice destinations. If '0' is given, also returns contacts -with no class. +Returns a list of contacts (L objects) for the customer. If no arguments are given, returns all contacts for the customer. +Arguments may contain classnums. When classnums are specified, only +contacts with a matching cust_contact.classnum are returned. When a +classnum of 0 is given, contacts with a null classnum are also included. + +Arguments may also contain the dest flag names 'invoice' or 'message'. +If given, contacts who's invoice_dest and/or invoice_message flags are +not set to 'Y' will be excluded. + =cut sub contact_list { my $self = shift; my $search = { table => 'contact', - select => 'contact.*, cust_contact.invoice_dest', + select => join(', ',( + 'contact.*', + 'cust_contact.invoice_dest', + 'cust_contact.message_dest', + )), addl_from => ' JOIN cust_contact USING (contactnum)', extra_sql => ' WHERE cust_contact.custnum = '.$self->custnum, }; - my @orwhere; + # Bugfix notes: + # Calling methods were relying on this method to use invoice_dest to + # block e-mail messages. Depending on parameters, this may or may not + # have actually happened. + # + # The bug could cause this SQL to be used to filter e-mail addresses: + # + # AND ( + # cust_contact.classnums IN (1,2,3) + # OR cust_contact.invoice_dest = 'Y' + # ) + # + # improperly including everybody with the opt-in flag AND everybody + # in the contact classes + # + # Possibility to introduce new bugs: + # If callers of this method called it incorrectly, and didn't notice + # because it seemed to send the e-mails they wanted. + + # WHERE ... + # AND ( + # ( cust_contact.classnum IN (1,2,3) ) + # OR + # ( cust_contact.classnum IS NULL ) + # + # AND ( + # ( cust_contact.invoice_dest = 'Y' ) + # OR + # ( cust_contact.message_dest = 'Y' ) + # ) + # ) + + my @and_dest; + my @or_classnum; my @classnums; - foreach (@_) { - if ( $_ eq 'invoice' ) { - push @orwhere, 'cust_contact.invoice_dest = \'Y\''; - } elsif ( $_ eq '0' ) { - push @orwhere, 'cust_contact.classnum is null'; + for (@_) { + if ($_ eq 'invoice' || $_ eq 'message') { + push @and_dest, " cust_contact.${_}_dest = 'Y' "; + } elsif ($_ eq '0') { + push @or_classnum, ' cust_contact.classnum IS NULL '; } elsif ( /^\d+$/ ) { push @classnums, $_; } else { - die "bad classnum argument '$_'"; + croak "bad classnum argument '$_'"; } } - if (@classnums) { - push @orwhere, 'cust_contact.classnum IN ('.join(',', @classnums).')'; - } - if (@orwhere) { - $search->{extra_sql} .= ' AND (' . - join(' OR ', map "( $_ )", @orwhere) . - ')'; + push @or_classnum, 'cust_contact.classnum IN ('.join(',',@classnums).')' + if @classnums; + + if (@or_classnum || @and_dest) { # catch, no arguments given + $search->{extra_sql} .= ' AND ( '; + + if (@or_classnum) { + $search->{extra_sql} .= join ' OR ', map {" ($_) "} @or_classnum; + $search->{extra_sql} .= ' AND ( ' if @and_dest; + } + + if (@and_dest) { + $search->{extra_sql} .= join ' OR ', map {" ($_) "} @and_dest; + $search->{extra_sql} .= ' ) ' if @or_classnum; + } + + $search->{extra_sql} .= ' ) '; + + warn "\$extra_sql: $search->{extra_sql} \n" if $DEBUG; } qsearch($search); @@ -5540,4 +5593,3 @@ L, L, schema.html from the base documentation. =cut 1; - diff --git a/FS/FS/cust_main_Mixin.pm b/FS/FS/cust_main_Mixin.pm index 8b6569a74..169e1eb65 100644 --- a/FS/FS/cust_main_Mixin.pm +++ b/FS/FS/cust_main_Mixin.pm @@ -348,10 +348,21 @@ sub cust_search_sql { =item email_search_result HASHREF -Emails a notice to the specified customers. Customers without -invoice email destinations will be skipped. +Emails a notice to the specified customer's contact_email addresses. -Parameters: + +If the user has specified "Invoice recipients" on the send e-mail screen, +contact_email rows containing the invoice_dest flag will be included. +This option is default, if neither 'invoice' nor 'message' are present. + +If the user has specified "Message recipients" on the send e-mail screen, +contact_email rows containing the message_dest flag will be included. + +The selection is indicated by the presence of the text 'message' or +'invoice' within the to_contact_classnum argument. + + +Parameters: =over 4 @@ -386,9 +397,19 @@ Text body =item to_contact_classnum -The customer contact class (or classes, as a comma-separated list) to send -the message to. If unspecified, will be sent to any contacts that are marked -as invoice destinations (the equivalent of specifying 'invoice'). +This field contains a comma-separated list. This list may contain: + +- the text "invoice" indicating emails should only be sent to contact_email + addresses with the invoice_dest flag set +- the text "message" indicating emails should only be sent to contact_email + addresses with the message_dest flag set +- numbers representing classnum id values for email contact classes. + If any classnum are present, emails should only be sent to contact_email + addresses where contact_email.classnum contains one of these classes. + The classnum 0 also includes where contact_email.classnum IS NULL + +If neither 'invoice' nor 'message' has been specified, this method will +behave as if 'invoice' had been selected =back @@ -483,8 +504,8 @@ sub email_search_result { next; # unlinked object; nothing else we can do } -my %to = {}; -if ($to) { $to{'to'} = $to; } + my %to = (); + if ($to) { $to{'to'} = $to; } my $cust_msg = $msg_template->prepare( 'cust_main' => $cust_main, @@ -736,4 +757,3 @@ L, L =cut 1; - diff --git a/FS/FS/msg_template/email.pm b/FS/FS/msg_template/email.pm index 4ae89f056..753fd3d69 100644 --- a/FS/FS/msg_template/email.pm +++ b/FS/FS/msg_template/email.pm @@ -210,6 +210,22 @@ go away in the future. A L (or arrayref of them) to attach to the message. +=item to_contact_classnum + +Set a string containing a comma-separated list. This list may contain: + +- the text "invoice" indicating emails should only be sent to contact_email + addresses with the invoice_dest flag set +- the text "message" indicating emails should only be sent to contact_email + addresses with the message_dest flag set + - numbers representing classnum id values for email contact classes. + If any classnum are present, emails should only be sent to contact_email + addresses where contact_email.classnum contains one of these classes. + The classnum 0 also includes where contact_email.classnum IS NULL + +If neither 'invoice' nor 'message' has been specified, this method will +behave as if 'invoice' had been selected + =cut =back @@ -296,8 +312,16 @@ sub prepare { my $classnum = $opt{'to_contact_classnum'} || ''; my @classes = ref($classnum) ? @$classnum : split(',', $classnum); - # traditional behavior: send to all invoice recipients - @classes = ('invoice') unless @classes; + + # There are two e-mail opt-in flags per contact_email address. + # If neither 'invoice' nor 'message' has been specified, default + # to 'invoice'. + # + # This default supports the legacy behavior of + # send to all invoice recipients + push @classes,'invoice' + unless grep {$_ eq 'invoice' || $_ eq 'message'} @classes; + @to = $cust_main->contact_list_email(@classes); # not guaranteed to produce contacts, but then customers aren't # guaranteed to have email addresses on file. in that case, env_to @@ -625,4 +649,3 @@ L, schema.html from the base documentation. =cut 1; - diff --git a/httemplate/misc/email-customers.html b/httemplate/misc/email-customers.html index fe637abe1..dc53f6d55 100644 --- a/httemplate/misc/email-customers.html +++ b/httemplate/misc/email-customers.html @@ -171,16 +171,18 @@ Template: Send to contacts:
- <& /elements/checkboxes.html, - 'style' => 'display: inline; vertical-align: top', - 'disable_links' => 1, - 'names_list' => \@contact_checkboxes, - 'element_name_prefix' => 'contact_class_', - 'checked_callback' => sub { - my($cgi, $name) = @_; - $name eq 'invoice' #others default to unchecked - }, - &> + <& /elements/checkboxes.html, + 'style' => 'display: inline; vertical-align: top', + 'disable_links' => 1, + 'names_list' => \@contact_checkboxes, + 'element_name_prefix' => 'contact_class_', + 'checked_callback' => sub { + # Called for each checkbox + # Return true to default as checked, false as unchecked + my($cgi, $name) = @_; + $name eq 'message' + }, + &>
% if ($send_to_domain) {
@@ -422,6 +424,8 @@ if ( !$cgi->param('preview') ) { push @contact_classnum, $1; if ( $1 eq 'invoice' ) { push @contact_classname, 'Invoice recipients'; + } elsif ( $1 eq 'message' ) { + push @contact_classname, 'Message recipients'; } else { my $contact_class = FS::contact_class->by_key($1); push @contact_classname, encode_entities($contact_class->classname); @@ -432,7 +436,8 @@ if ( !$cgi->param('preview') ) { } my @contact_checkboxes = ( - [ 'invoice' => { label => 'Invoice recipients' } ] + [ 'message' => { label => 'Message recipients' } ], + [ 'invoice' => { label => 'Invoice recipients' } ], ); foreach my $class (qsearch('contact_class', { disabled => '' })) { diff --git a/httemplate/view/cust_main/contacts_new.html b/httemplate/view/cust_main/contacts_new.html index a28b44934..fe412cc00 100644 --- a/httemplate/view/cust_main/contacts_new.html +++ b/httemplate/view/cust_main/contacts_new.html @@ -10,8 +10,8 @@ <%$th%>Contact <%$th%>Email <%$th%>Send invoices - <%$th%>Self-service <%$th%>Send messages + <%$th%>Self-service % foreach my $phone_type (@phone_type) { <%$th%><% $phone_type->typename |h %> % } -- 2.11.0