[D7] Site Alerts

There are some errors reported by automated review tools, did you already check them? See http://pareview.sh/pareview/httpgitdrupalorgsandboxgi40k2219655git

We are currently quite busy with all the project applications and we prefer projects with a review bonus. Please help reviewing and put yourself on the high priority list, then we will take a look at your project right away :-)

Also, you should get your friends, colleagues or other community members involved to review this application. Let them go through the review checklist and post a comment that sets this issue to "needs work" (they found some problems with the project) or "reviewed & tested by the community" (they found no major flaws).

I'm a robot and this is an automated message from Project Applications Scraper.

I installed module and tried to Add site monitor and got error message as below:

You need to enter a search text for text and regex checks.

I think you should put required for Search text field if it's necessary or remove verification for empty field.

Please see my attachment for more detail.

A required field without a "*" is a negative point for UX.

In in that case, it would reduce the confusing for user by set "Status check" is the default option rather than Text match/Regex match first. A recommend solution is set visibility for field search_text base on the selection of field type.

Case Status check - hide Search text field
Case Text match/Regex match - show Search text field

Hope this help,

Hi,
i read the code and it seems good for me.
For more readability, i would prefer ading a new php variable instead of using the ternary conditional statement inside drupal_mail.
!empty($site_alerts_monitor->recipient) ? $site_alerts_monitor->recipient : variable_get('site_mail', '')

it seems good for me.
Thanks

Possibly blocking, duplication:

1. There are many site monitoring tool modules already available. This is a relatively simple implementation of monitoring, but it could still be a patch to an existing module. There are several listed here: https://www.drupal.org/node/898296. Please describe how your module is different or why a user would choose your module instead of one of the others. Module duplication and fragmentation is a huge problem on drupal.org and we prefer collaboration over competition. If you disagree or are unable to work with another module for whatever reason, please get back to us and set this back to "needs review".

Non-blocking issues:

• In site_alerts_trigger_alerts(), you can check for the last_checked and frequency and only return results that need a new check, instead of doing it later in php
• You can rewrite the if/else at the bottom of that function as $site_alerts_monitor->status = $alive ? 1 : 0
• In site_alerts_is_alive(), can you use url() with absolute => TRUE to create the string instead of preg_match?
• Also in that function, the if/elses can be condensed, for ex, return ($response->code == 200), you're always returning the value of the if condition anyway
• You might want to include some validation when the user creates a new site check entity, make sure the url is good before using it later on

Checked for security, licensing, Drupal API, individual account, no major issues found.

In addition to the existing site monitoring modules, there is already https://www.drupal.org/sandbox/cecrs/2340909. #2341009: [D7] Site Alert has been RTBC for > 3 months. Assuming these project reviews are handled in order (which I know is not safe to assume), it would be unnecessarily confusing to approve both site_alert and site_alerts.

Well, confusing names for sure, but as far as I can tell

• This module (Site Alerts) is about alerting an admin via email when a url fails to respond properly, checked at cron run times
• That module (Site Alert) is about the site admin creating a side-wide alert message to inform users

So I think we're safe from duplication between those two, though their names are very similar.

Closing due to lack of activity. If you are still working on this application, you should fix all known problems and then set the status to "Needs review". (See also the project application workflow).

I'm a robot and this is an automated message from Project Applications Scraper.