From 984cbf4a168ef98314ce7ed6322909258e74dbbd Mon Sep 17 00:00:00 2001 From: Francis Charette Migneault Date: Thu, 12 Oct 2023 09:27:01 -0400 Subject: [PATCH] fix issues from PR review --- tests/functional/test_cli.py | 5 +- tests/test_notify.py | 113 ++++++++++++++++++++++++++++++++++- weaver/cli.py | 5 +- weaver/notify.py | 4 +- 4 files changed, 120 insertions(+), 7 deletions(-) diff --git a/tests/functional/test_cli.py b/tests/functional/test_cli.py index e42d9cfb5..65a467005 100644 --- a/tests/functional/test_cli.py +++ b/tests/functional/test_cli.py @@ -624,11 +624,12 @@ def test_execute_subscribers(self): assert mocked_requests.call_args_list[1].kwargs["json"] == expect_outputs # results JSON # first argument None is 'from_addr' not configured, this is allowed if provided by 'From' email header + test_proc_byte = self.test_process["Echo"] assert mocked_emails.call_count == 2, "Should not have sent both failed/success email notifications" assert mocked_emails.call_args_list[0].args[:2] == (None, subscribers["inProgressEmail"]) - assert b"Job test-client-Echo Started" in mocked_emails.call_args_list[0].args[-1] + assert f"Job {test_proc_byte} Started".encode() in mocked_emails.call_args_list[0].args[-1] assert mocked_emails.call_args_list[1].args[:2] == (None, subscribers["successEmail"]) - assert b"Job test-client-Echo Succeeded" in mocked_emails.call_args_list[1].args[-1] + assert f"Job {test_proc_byte} Succeeded".encode() in mocked_emails.call_args_list[1].args[-1] # NOTE: # For all below '<>_auto_resolve_vault' test cases, the local file referenced in the Execute request body diff --git a/tests/test_notify.py b/tests/test_notify.py index 45ca70ef6..0b786aa8e 100644 --- a/tests/test_notify.py +++ b/tests/test_notify.py @@ -1,4 +1,6 @@ +import contextlib import os +import pathlib import smtplib import tempfile import uuid @@ -6,8 +8,9 @@ import mock import pytest +from weaver import WEAVER_MODULE_DIR from weaver.datatype import Job -from weaver.notify import decrypt_email, encrypt_email, notify_job_email +from weaver.notify import decrypt_email, encrypt_email, notify_job_email, resolve_email_template from weaver.status import Status @@ -148,3 +151,111 @@ def test_notify_job_email_custom_template(): "", f"Job: {test_url}/processes/{test_job.process}/jobs/{test_job.id}", ]) + + +@pytest.mark.parametrize( + ["settings", "test_process", "test_status", "tmp_default", "expect_result"], + [ + ({}, None, None, None, 4), + # directory exists, but none of the supported mako variants found under it + ({"weaver.wps_email_notify_template_dir": tempfile.gettempdir()}, None, None, None, IOError), + ({"weaver.wps_email_notify_template_dir": "/DOES_NOT_EXIST"}, None, None, None, 4), + ({"weaver.wps_email_notify_template_dir": ""}, None, None, None, 4), + ({"weaver.wps_email_notify_template_dir": "", + "weaver.wps_email_notify_template_default": "RANDOM"}, None, None, None, 4), + ({"weaver.wps_email_notify_template_dir": ""}, None, None, None, 4), + ({"weaver.wps_email_notify_template_dir": "", + "weaver.wps_email_notify_template_default": "test-default.mako"}, None, None, "random.mako", 4), + ({"weaver.wps_email_notify_template_dir": ""}, None, None, "test-default.mako", 4), + ({"weaver.wps_email_notify_template_dir": ""}, None, None, "test-default.mako", 4), + ({"weaver.wps_email_notify_template_dir": "", + "weaver.wps_email_notify_template_default": "test-default.mako"}, None, None, "default.mako", 3), + ({"weaver.wps_email_notify_template_dir": "", + "weaver.wps_email_notify_template_default": "test-default.mako"}, None, None, "test-default.mako", 2), + ( + {"weaver.wps_email_notify_template_dir": "", + "weaver.wps_email_notify_template_default": "test-default.mako"}, + "random-process", + None, + "test-default.mako", + 2 + ), + ( + {"weaver.wps_email_notify_template_dir": "", + "weaver.wps_email_notify_template_default": "test-default.mako"}, + "random-process", + Status.SUCCEEDED, + "test-default.mako", + 2 + ), + ( + {"weaver.wps_email_notify_template_dir": "", + "weaver.wps_email_notify_template_default": "test-default.mako"}, + "tmp-process", + Status.STARTED, + "test-default.mako", + 2 + ), + ( + {"weaver.wps_email_notify_template_dir": "", + "weaver.wps_email_notify_template_default": "test-default.mako"}, + "tmp-process", + None, + "test-default.mako", + 2 + ), + ( + {"weaver.wps_email_notify_template_dir": "", + "weaver.wps_email_notify_template_default": "test-default.mako"}, + "tmp-process", + Status.SUCCEEDED, + "test-default.mako", + 2 + ), + ( + {"weaver.wps_email_notify_template_dir": "", + "weaver.wps_email_notify_template_default": "test-default.mako"}, + "tmp-process", + Status.STARTED, + "test-default.mako", + 1 + ), + ] +) +def test_resolve_email_template(settings, test_process, test_status, tmp_default, expect_result): + # process name and job status important to evaluate expected mako file resolution + tmp_process = "tmp-process" + tmp_status = Status.STARTED + + with contextlib.ExitStack() as tmp_stack: + tmp_dir = pathlib.Path(tmp_stack.enter_context(tempfile.TemporaryDirectory())) + if settings.get("weaver.wps_email_notify_template_dir") == "": + settings["weaver.wps_email_notify_template_dir"] = tmp_dir + + tmp_proc_dir = tmp_dir / tmp_process + os.makedirs(tmp_proc_dir, exist_ok=True) + tmp_file0 = tmp_proc_dir / f"{tmp_process}.mako" + tmp_file0.touch() + tmp_file1 = tmp_dir / f"{tmp_status}.mako" + tmp_file1.touch() + tmp_file2 = tmp_dir / f"{tmp_default}.mako" + tmp_file3 = tmp_dir / "default.mako" + default_file = os.path.join(WEAVER_MODULE_DIR, "wps_restapi/templates/notification_email_example.mako") + + ordered_possible_matches = [ + tmp_file0, # {tmp_dir}/{process}/{status}.mako + tmp_file1, # {tmp_dir}/{process}.mako + tmp_file2, # {tmp_dir}/{default}.mako + tmp_file3, # {tmp_dir}/default.mako + default_file, # weaver default mako + ] + + test_job = Job(task_id=uuid.uuid4(), process=test_process, status=test_status) + try: + found_template = resolve_email_template(test_job, settings) + found_template_index = ordered_possible_matches.index(found_template.filename) + assert isinstance(expect_result, int), f"Test expected to raise {expect_result} but did not raise." + assert found_template_index == expect_result + except Exception as exc: + assert issubclass(expect_result, Exception), f"Test did not expect an error, but raised {exc!s}." + assert isinstance(exc, expect_result), f"Test expected {expect_result}, but raised {exc!s} instead." diff --git a/weaver/cli.py b/weaver/cli.py index 1b171c4c5..440af8b0f 100644 --- a/weaver/cli.py +++ b/weaver/cli.py @@ -1960,7 +1960,7 @@ class SubscriberAction(argparse.Action): """ Action that will validate that the input argument references a valid subscriber argument. - If valid, the returned value with be an updated subscriber definition. + If valid, the returned value will be an updated subscriber definition. All arguments using ``action=SubscriberType`` should include a ``dest="."`` parameter that will map the ``subscriber`` value under a dictionary ``holder`` that will be passed to the :class:`argparse.Namespace`. """ @@ -1991,7 +1991,7 @@ def validate(self, option, value): else: raise NotImplementedError(f"Cannot parse option: '{option}'") if not re.match(pattern, value): - raise argparse.ArgumentError(self, f"Value '{value} is a valid subscriber argument for '{option}'.") + raise argparse.ArgumentError(self, f"Value '{value}' is not a valid subscriber argument for '{option}'.") def add_subscribers_params(parser): @@ -2051,6 +2051,7 @@ def add_subscribers_params(parser): metavar="URL", dest="subscribers.successUri", help=( + "Send an HTTP callback request to this URL if the job execution completed successfully.\n\n" "The request body will contain the JSON representation of the job results." ) ) diff --git a/weaver/notify.py b/weaver/notify.py index 01a0bb252..4c5df65a6 100644 --- a/weaver/notify.py +++ b/weaver/notify.py @@ -51,7 +51,7 @@ def resolve_email_template(job, settings): else: default_setting = "weaver.wps_email_notify_template_default" default_default = "default.mako" - default_name = settings.get(default_setting, default_default) + default_name = settings.get(default_setting) or default_default process_name = f"{job.process!s}.mako" process_status_name = f"{job.process!s}/{job.status!s}.mako" default_template = os.path.join(template_dir, default_name) @@ -84,7 +84,7 @@ def notify_job_email(job, to_email_recipient, container): port = settings.get("weaver.wps_email_notify_port") ssl = asbool(settings.get("weaver.wps_email_notify_ssl", True)) - if not smtp_host or not port: + if not smtp_host or not port: # pragma: no cover # only raise to warn service manager # note: don't expose the values to avoid leaking them in logs raise ValueError( "The email server configuration is missing or incomplete. "