LP#1749475 Rethink double-duty of SendEmail reactor

Previously, we were trying to use the same reactor, SendEmail to both
generate email previews and also to generate and send emails.  This
behavior was meant to be controlled via $user_data.

This plan is problematic, particularly for grouped events.  It is
possible for events processed by the same hook/definition to have
user_data specifying different behavior, but then for the these events
to be grouped and processed together, creating a random outcome.

We start to see this issue already in our attempts to process $user_data
safely.  We notice this variable can be either an array for grouped
events, or a hash for ungrouped, or empty for cases where it isn't
required.  We could work around that situation with layers of error
checking, but still would end up with an arbitrary value.

Since very little of SendEmail is needed to actual generate the preview,
perhaps it makes the most sense, and is more clear, if we have a
separate reactor for doing the previews.  This commit does that.

Unfortunately, in doing so, we expose another weakness; event
definitions cannot share a template.  If we want the old hook to keep
functioning, we need to duplicate the new template in the old hook and
create an entirely new "preview" hook.  This invites maintenance
problems, but still better than predictability problems.  However, in
this case, we'll just overwrite the old hook, then emulate its behavior
at the API layer.

Signed-off-by: Dan Wells <dbw2@calvin.edu>

diff --git a/Open-ILS/src/perlmods/lib/OpenILS/Application/Search/Biblio.pm b/Open-ILS/src/perlmods/lib/OpenILS/Application/Search/Biblio.pm
index aef51af..ea189dd 100644 (file)
--- a/Open-ILS/src/perlmods/lib/OpenILS/Application/Search/Biblio.pm
+++ b/Open-ILS/src/perlmods/lib/OpenILS/Application/Search/Biblio.pm
@@ -2019,8 +2019,7 @@ sub format_biblio_record_entry {
         subject     => $subject,
         context_org => $holdings_context_org->shortname,
         sort_by     => $bib_sort,
-        sort_dir    => $sort_dir,
-        preview     => $preview
+        sort_dir    => $sort_dir
     };
 
     if ($for_print) {
@@ -2029,10 +2028,13 @@ sub format_biblio_record_entry {
 
     } elsif ($for_email) {
 
-        return $U->fire_object_event(undef, 'biblio.format.record_entry.email', [ $bucket ], $event_context_org, undef, [ $usr_data ])
-            if ($preview);
-
-        $U->create_events_for_hook('biblio.format.record_entry.email', $bucket, $event_context_org, undef, $usr_data, 1);
+        my $preview_event = $U->fire_object_event(undef, 'biblio.format.record_entry.email.preview', [ $bucket ], $event_context_org, undef, [ $usr_data ]);
+        if ($preview) {
+            return $preview_event;
+        } else {
+            # this is for backward compatibility; the non-preview call isn't currently used...
+            $self->method_lookup('open-ils.search.biblio.record.email.send_output')->run($auth, $preview_event->id);
+        }
     }
 
     return undef;

diff --git a/Open-ILS/src/perlmods/lib/OpenILS/Application/Trigger/Reactor/PreviewEmail.pm b/Open-ILS/src/perlmods/lib/OpenILS/Application/Trigger/Reactor/PreviewEmail.pm
new file mode 100644 (file)
index 0000000..7c18202
--- /dev/null
+++ b/Open-ILS/src/perlmods/lib/OpenILS/Application/Trigger/Reactor/PreviewEmail.pm
@@ -0,0 +1,42 @@
+package OpenILS::Application::Trigger::Reactor::PreviewEmail;
+use strict; use warnings;
+use Data::Dumper;
+use OpenSRF::Utils::SettingsClient;
+use OpenSRF::Utils::Logger qw/:logger/;
+$Data::Dumper::Indent = 0;
+use base 'OpenILS::Application::Trigger::Reactor';
+
+sub ABOUT {
+    return <<ABOUT;
+
+The PreviewEmail reactor generates the contents of an email to view and
+potentially send later.
+
+The value at /opensrf/default/email_notify/sender_address is passed into
+the template as the 'default_sender' variable.
+
+No default template is assumed, and all information other than the
+default_sender that the system provides is expected to be gathered by the
+Event Definition through either Environment or Parameter definitions.
+
+ABOUT
+}
+
+sub handler {
+    my $self = shift;
+    my $env = shift;
+
+    my $conf = OpenSRF::Utils::SettingsClient->new;
+    $$env{default_sender} = $conf->config_value('email_notify', 'sender_address');
+
+    my $text = $self->run_TT($env);
+    if (!$text) {
+        return 0;
+    } else {
+        $logger->info("PreviewEmail Reactor: successfully generated email");
+        return 1;
+    }
+}
+
+1;
+

diff --git a/Open-ILS/src/perlmods/lib/OpenILS/Application/Trigger/Reactor/SendEmail.pm b/Open-ILS/src/perlmods/lib/OpenILS/Application/Trigger/Reactor/SendEmail.pm
index 10e32e1..9dd76ee 100644 (file)
--- a/Open-ILS/src/perlmods/lib/OpenILS/Application/Trigger/Reactor/SendEmail.pm
+++ b/Open-ILS/src/perlmods/lib/OpenILS/Application/Trigger/Reactor/SendEmail.pm
@@ -49,10 +49,6 @@ sub handler {
 
     my $text = encode_utf8($self->run_TT($env));
     return 0 if (!$text);
-    if ($$env{user_data} && ref($$env{user_data}) =~ /HASH/ && $$env{user_data}{preview}) {
-        $logger->info("SendEmail Reactor: success in preview mode, not sending email");
-        return 1;
-    }
 
     my $sender = Email::Send->new({mailer => 'SMTP'});
     $sender->mailer_args([Host => $smtp]);

diff --git a/Open-ILS/src/sql/Pg/400.schema.action_trigger.sql b/Open-ILS/src/sql/Pg/400.schema.action_trigger.sql
index 4d9e9e9..74a22d5 100644 (file)
--- a/Open-ILS/src/sql/Pg/400.schema.action_trigger.sql
+++ b/Open-ILS/src/sql/Pg/400.schema.action_trigger.sql
@@ -124,6 +124,15 @@ INSERT INTO action_trigger.reactor (module,description) VALUES
         'description'
     )
 );
+INSERT INTO action_trigger.reactor (module,description) VALUES
+(   'PreviewEmail',
+    oils_i18n_gettext(
+        'PreviewEmail',
+        'Preview an email based on a user-defined template',
+        'atreact',
+        'description'
+    )
+);
 
 -- TODO: build a PDF generator
 --INSERT INTO action_trigger.reactor (module,description) VALUES

diff --git a/Open-ILS/src/sql/Pg/950.data.seed-values.sql b/Open-ILS/src/sql/Pg/950.data.seed-values.sql
index 9f9e99a..1a9a676 100644 (file)
--- a/Open-ILS/src/sql/Pg/950.data.seed-values.sql
+++ b/Open-ILS/src/sql/Pg/950.data.seed-values.sql
@@ -11763,10 +11763,10 @@ INSERT INTO action_trigger.cleanup ( module, description ) VALUES (
 );
 
 INSERT INTO action_trigger.hook (key,core_type,description,passive) VALUES (
-        'biblio.format.record_entry.email',
+        'biblio.format.record_entry.email.preview',
         'cbreb', 
         oils_i18n_gettext(
-            'biblio.format.record_entry.email',
+            'biblio.format.record_entry.email.preview',
             'An email has been requested for one or more biblio record entries.',
             'ath',
             'description'
@@ -11804,10 +11804,10 @@ INSERT INTO action_trigger.event_definition (
         31,
         TRUE,
         1,
-        'biblio.record_entry.email',
-        'biblio.format.record_entry.email',
+        'biblio.record_entry.email.preview',
+        'biblio.format.record_entry.email.preview',
         'NOOP_True',
-        'SendEmail',
+        'PreviewEmail',
         'DeleteTempBiblioBucket',
         'DeleteTempBiblioBucket',
         'owner',
@@ -19544,13 +19544,13 @@ INSERT INTO action_trigger.event_def_group_member (grp, name, holdings, event_de
     SELECT 1, 'Full', TRUE, id FROM action_trigger.event_definition WHERE hook = 'biblio.format.record_entry.print';
 
 INSERT INTO action_trigger.event_def_group (id, owner, hook, name)
-    VALUES (2,1,'biblio.format.record_entry.email','Email Record(s)');
+    VALUES (2,1,'biblio.format.record_entry.email.preview','Email Record(s)');
 
 INSERT INTO action_trigger.event_def_group_member (grp, name, event_def)
-    SELECT 2, 'Brief', id FROM action_trigger.event_definition WHERE hook = 'biblio.format.record_entry.email';
+    SELECT 2, 'Brief', id FROM action_trigger.event_definition WHERE hook = 'biblio.format.record_entry.email.preview';
 
 INSERT INTO action_trigger.event_def_group_member (grp, name, holdings, event_def)
-    SELECT 2, 'Full', TRUE, id FROM action_trigger.event_definition WHERE hook = 'biblio.format.record_entry.email';
+    SELECT 2, 'Full', TRUE, id FROM action_trigger.event_definition WHERE hook = 'biblio.format.record_entry.email.preview';
 
 INSERT into config.org_unit_setting_type (name, label, description, datatype) 
 VALUES ( 

diff --git a/Open-ILS/src/sql/Pg/upgrade/XXXX.schema.AT-def-groups.sql b/Open-ILS/src/sql/Pg/upgrade/XXXX.schema.AT-def-groups.sql
index 573aabb..57ddd97 100644 (file)
--- a/Open-ILS/src/sql/Pg/upgrade/XXXX.schema.AT-def-groups.sql
+++ b/Open-ILS/src/sql/Pg/upgrade/XXXX.schema.AT-def-groups.sql
@@ -51,13 +51,20 @@ INSERT INTO action_trigger.event_def_group_member (grp, name, holdings, event_de
     SELECT 1, 'Full', TRUE, id FROM action_trigger.event_definition WHERE hook = 'biblio.format.record_entry.print';
 
 INSERT INTO action_trigger.event_def_group (id, owner, hook, name)
-    VALUES (2,1,'biblio.format.record_entry.email','Email Record(s)');
+    VALUES (2,1,'biblio.format.record_entry.email.preview','Email Record(s)');
 
 INSERT INTO action_trigger.event_def_group_member (grp, name, event_def)
-    SELECT 2, 'Brief', id FROM action_trigger.event_definition WHERE hook = 'biblio.format.record_entry.email';
+    SELECT 2, 'Brief', id FROM action_trigger.event_definition WHERE hook = 'biblio.format.record_entry.email.preview';
 
 INSERT INTO action_trigger.event_def_group_member (grp, name, holdings, event_def)
-    SELECT 2, 'Full', TRUE, id FROM action_trigger.event_definition WHERE hook = 'biblio.format.record_entry.email';
+    SELECT 2, 'Full', TRUE, id FROM action_trigger.event_definition WHERE hook = 'biblio.format.record_entry.email.preview';
+
+INSERT INTO action_trigger.reactor VALUES ('PreviewEmail', 'Preview an email based on a user-defined template');
+
+UPDATE action_trigger.hook SET key = 'biblio.format.record_entry.email.preview' where key = 'biblio.format.record_entry.email';
+
+-- Doing these next two separately, as the second might not apply
+UPDATE action_trigger.event_definition SET name = 'biblio.record_entry.email.preview', reactor = 'PreviewEmail', hook = 'biblio.format.record_entry.email.preview' WHERE hook = 'biblio.format.record_entry.email';
 
 UPDATE action_trigger.event_definition SET template = $$
 [%- USE date -%]
@@ -106,7 +113,8 @@ Item Type: [% item.item_type %]
 
 [% END %]
 [% END %]
-$$ WHERE hook = 'biblio.format.record_entry.email';
+$$ WHERE hook = 'biblio.format.record_entry.email.preview';
+-- TODO: check for MD5 of old template before clobbering?
 
 UPDATE action_trigger.event_definition SET template = $$
 <div>
@@ -155,6 +163,7 @@ UPDATE action_trigger.event_definition SET template = $$
     </ol>
 </div>
 $$ WHERE hook = 'biblio.format.record_entry.print';
+-- TODO: check for MD5 of old template before clobbering?
 
 COMMIT;