From 4c760ba4b8a44f854527534389f0d2057058d5cc Mon Sep 17 00:00:00 2001 From: Dan Wells Date: Thu, 21 Feb 2019 18:24:19 -0500 Subject: [PATCH] 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 --- .../lib/OpenILS/Application/Search/Biblio.pm | 14 ++++---- .../Application/Trigger/Reactor/PreviewEmail.pm | 42 ++++++++++++++++++++++ .../Application/Trigger/Reactor/SendEmail.pm | 4 --- Open-ILS/src/sql/Pg/400.schema.action_trigger.sql | 9 +++++ Open-ILS/src/sql/Pg/950.data.seed-values.sql | 16 ++++----- .../sql/Pg/upgrade/XXXX.schema.AT-def-groups.sql | 17 ++++++--- 6 files changed, 80 insertions(+), 22 deletions(-) create mode 100644 Open-ILS/src/perlmods/lib/OpenILS/Application/Trigger/Reactor/PreviewEmail.pm 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 aef51af739..ea189ddcb3 100644 --- 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 index 0000000000..7c18202655 --- /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 <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 10e32e1a17..9dd76ee454 100644 --- 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 4d9e9e9dac..74a22d59f5 100644 --- 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 9f9e99a1f4..1a9a676379 100644 --- 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 573aabb243..57ddd975ec 100644 --- 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 = $$
@@ -155,6 +163,7 @@ UPDATE action_trigger.event_definition SET template = $$
$$ WHERE hook = 'biblio.format.record_entry.print'; +-- TODO: check for MD5 of old template before clobbering? COMMIT; -- 2.11.0