fix issues from PR review

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

tests/test_notify.py

@@ -1,13 +1,16 @@
+import contextlib
 import os
+import pathlib
 import smtplib
 import tempfile
 import uuid
 
 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": "<TMP_DIR>"}, None, None, None, 4),
+        ({"weaver.wps_email_notify_template_dir": "<TMP_DIR>",
+          "weaver.wps_email_notify_template_default": "RANDOM"}, None, None, None, 4),
+        ({"weaver.wps_email_notify_template_dir": "<TMP_DIR>"}, None, None, None, 4),
+        ({"weaver.wps_email_notify_template_dir": "<TMP_DIR>",
+          "weaver.wps_email_notify_template_default": "test-default.mako"}, None, None, "random.mako", 4),
+        ({"weaver.wps_email_notify_template_dir": "<TMP_DIR>"}, None, None, "test-default.mako", 4),
+        ({"weaver.wps_email_notify_template_dir": "<TMP_DIR>"}, None, None, "test-default.mako", 4),
+        ({"weaver.wps_email_notify_template_dir": "<TMP_DIR>",
+          "weaver.wps_email_notify_template_default": "test-default.mako"}, None, None, "default.mako", 3),
+        ({"weaver.wps_email_notify_template_dir": "<TMP_DIR>",
+          "weaver.wps_email_notify_template_default": "test-default.mako"}, None, None, "test-default.mako", 2),
+        (
+            {"weaver.wps_email_notify_template_dir": "<TMP_DIR>",
+             "weaver.wps_email_notify_template_default": "test-default.mako"},
+            "random-process",
+            None,
+            "test-default.mako",
+            2
+        ),
+        (
+            {"weaver.wps_email_notify_template_dir": "<TMP_DIR>",
+             "weaver.wps_email_notify_template_default": "test-default.mako"},
+            "random-process",
+            Status.SUCCEEDED,
+            "test-default.mako",
+            2
+        ),
+        (
+            {"weaver.wps_email_notify_template_dir": "<TMP_DIR>",
+             "weaver.wps_email_notify_template_default": "test-default.mako"},
+            "tmp-process",
+            Status.STARTED,
+            "test-default.mako",
+            2
+        ),
+        (
+            {"weaver.wps_email_notify_template_dir": "<TMP_DIR>",
+             "weaver.wps_email_notify_template_default": "test-default.mako"},
+            "tmp-process",
+            None,
+            "test-default.mako",
+            2
+        ),
+        (
+            {"weaver.wps_email_notify_template_dir": "<TMP_DIR>",
+             "weaver.wps_email_notify_template_default": "test-default.mako"},
+            "tmp-process",
+            Status.SUCCEEDED,
+            "test-default.mako",
+            2
+        ),
+        (
+            {"weaver.wps_email_notify_template_dir": "<TMP_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") == "<TMP_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."

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="<holder>.<subscriber>"`` 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."
         )
     )

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. "