Problem/Motivation

Splitting task from #2791619: Test Monitoring Mail with real examples, this issue is created to add the missing hook_requirements checking for the monitoring_mail recipient in the runtime phase.

Proposed resolution

Implement a check for the monitoring_mail recipient in a hook_requirements() in the runtime phase.

Remaining tasks

User interface changes

API changes

Data model changes

CommentFileSizeAuthor
#8 Screen Shot 2016-09-21 at 17.07.39.png11.94 KBtduong
#7 implement_monitoring_mail_requirements-2800051-7.patch4.32 KBginovski
#7 interdiff-2800051-4-7.txt2.4 KBginovski
#4 implement_monitoring_mail_requirements_2800051-4.patch4.29 KBginovski
#4 interdiff-2800051-2-4.txt3.09 KBginovski
#2 implement_monitoring_mail_requirements-2800051-2.patch1.86 KBtduong

Comments

tduong created an issue. See original summary.

tduong’s picture

tduong commented
Status: Active » Needs review
Parent issue: » #2791619: Test Monitoring Mail with real examples
StatusFileSize
new implement_monitoring_mail_requirements-2800051-2.patch1.86 KB

Moved the code from the parent issue.

miro_dietiker’s picture

miro_dietiker
Location Switzerland
commented
Status: Needs review » Needs work
Issue tags: +Needs tests 
+++ b/modules/monitoring_mail/monitoring_mail.install
@@ -0,0 +1,49 @@
+    // @todo hook_requirements: no log = no mail

Remove.

Other than that, this looks fine.
But additionally, hook_requirements are about critical integrity constrains. You want to test what hook_requirements return based on the invalid settings.

ginovski’s picture

ginovski commented
Status: Needs work » Needs review
StatusFileSize
new interdiff-2800051-2-4.txt3.09 KB
new implement_monitoring_mail_requirements_2800051-4.patch4.29 KB

1.Added test.
2.Removed todo.

tduong’s picture

tduong commented
Status: Needs review » Needs work
Issue tags: -Needs tests 
  1. +++ b/modules/monitoring_mail/monitoring_mail.install
@@ -0,0 +1,48 @@
+        '@monitoring_settings' => Url::fromRoute('monitoring.settings')->toString(),

    Maybe would be nice to have this as a clickable link, so this needs to be Url::fromRoute()->setAbsolute()->toString(). 

  2. +++ b/modules/monitoring_mail/tests/src/Kernel/MonitoringMailKernelTest.php
@@ -117,4 +117,46 @@ class MonitoringMailKernelTest extends MonitoringUnitTestBase {
+    // Run monitoring_mail (Requirements) and make sure the message contains
...
+    // Run monitoring_mail (Requirements) again and make sure it has warning
...
+    // Run monitoring_mail (Requirements) again and make sure it has warning

    Please separate these steps you do in the test from the things you need set, just for readability.

Otherwise I guess it's alright.

berdir’s picture

berdir
Primary language German
Location Switzerland
commented

1. No, that has nothing to do with absolute, that just adds the domain. to make it clickable, add a link tag to the translatable string.

ginovski’s picture

ginovski commented
Status: Needs work » Needs review
StatusFileSize
new interdiff-2800051-4-7.txt2.4 KB
new implement_monitoring_mail_requirements-2800051-7.patch4.32 KB

Added blank lines and the link in the monitoring settings.

tduong’s picture

tduong commented
StatusFileSize
new Screen Shot 2016-09-21 at 17.07.39.png11.94 KB

Oh, just checked and I noticed we should add a validation for this (?)

ginovski’s picture

ginovski commented
Related issues: +#2807815: Validate email address for gmail in monitoring_mail, +#2807819: Validate email address (other accounts) in monitoring_mail, +#2807825: Handle multiple recipients for monitoring_mail

Created followups for mail validation and multiple recipients:
https://www.drupal.org/node/2807815
https://www.drupal.org/node/2807819
https://www.drupal.org/node/2807825

tduong’s picture

tduong commented
Status: Needs review » Reviewed & tested by the community

Alright, followups are there, and code looks fine for me.

@Ginovski, next time just remember to use [#issue number] when you post a comment referencing to another issue instead of the link.