Drupal 7 module

Module Description

The Site Alert module is a lightweight solution for allowing site administrators to easily place a sidewide alert on their site, for example for maintenance downtime, or any general informational message. Alerts have start and end date/times, and can be assigned a severity level. Messages are refreshed by ajax and not subject to site caching, so changes made in the ui will be automatically displayed to users without necessitating a cache clear. The module provides a custom pane (recommended) and a block for sites not using panels.

Features

  • Simple admin interface
  • Start date allows for pre-scheduled alerts
  • End date allows for automatic expiration of no longer relevant alerts
  • Ajax menu callback means alerts can be updated on the fly
  • Provides both a content pane and a block for total flexibility in placement

Links

Link to Sandbox: https://www.drupal.org/sandbox/cecrs/2340909
Link to Git Instructions: https://www.drupal.org/project/2340909/git-instructions

Similar Modules

User Alert - https://www.drupal.org/project/user_alert
This module was much more complex administratively then we wanted, which was a simple way to place an alert on the site for all pages and all users.

Alerts - https://www.drupal.org/project/alerts
Works by attaching an alert to individual nodes. No way for a site administrator to create a site wide alert.

Sitewide Alert Message - https://www.drupal.org/project/sitewide_msg
This was the closest in functionality to what we needed, but uses php poller and an iframe, and we preferred an ajax/jquery solution.

Review Bonus Reviews
https://www.drupal.org/node/2373551#comment-9477685

Comments

cecrs’s picture

cecrs commented
Issue summary: View changes
rgkimball’s picture

rgkimball commented
Status: Needs review » Needs work

There were a few problems with coding standards per: http://pareview.sh/pareview/httpgitdrupalorgsandboxcecrs2340909git

cecrs’s picture

cecrs commented
Status: Needs work » Needs review

All pareview issues have been resolved.

PA robot’s picture

PA robot commented

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.

cravecode’s picture

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

Looks good to me.
Pretty neat little module, good luck!

kreynen’s picture

kreynen commented
Status: Reviewed & tested by the community » Needs work

I'd really like to see #2374169: Better Menu Path fixed before this is promoted.

That's really the only functional issue I found. I added a few Feature requests that I think would improve the module, but it's already better than the alternatives listed. If you are interested in #2374223: Consider using an Entity for Alerts instead of single config, I'm happy to provide a patch for that as well. Using Entities for the alerts would increase the complexity, but I think the additional features make it well worth the additional code.

The only other suggestion I have is to add a similar module section to your project page and list the other modules you've listed here.

Otherwise, great work!

Unfortunately you are also going to need jump through the hoops of the review bonus process. There are project requests that have been sitting the queue as RTBC for > 8 months!

rgkimball’s picture

rgkimball commented

Returning from my original comment to give this an actual review. I'll start by saying I agree with kreynen; alerts ought to be entities (but I realize that's asking for a much more complex module than what's currently here). If alerts were configured as entities, future extensions of this module could allow you to configure alerts for specific user roles, or even a Rules integration. I was also going to mention that the config page should not be a top level menu item, but it looks like that's been covered in the issue queue as well.

I like that your module is very straightforward - it would be easy for any non-technical admin to configure and start using immediately. The first thing that jumped out at me was that you aren't using a native text format for your comment field. Forcing alerts to plain text probably makes them easier to render, but less configurable for a site admin. Let Drupal decide what tags ought to be allowed in your content field instead. Adding text format options to your comment field, should you choose to, would simply be a matter of changing one line of your $form['site_alert_content'] array:'#type' => 'text_format'.

Other than that, everything else I noticed as a potential issue has already been addressed.

One thing that might be helpful is adding a simple close button for alerts. If you don't set it up to remember user sessions, this should be fairly easy to add, and I'd be happy to submit a patch for this if necessary.

cecrs’s picture

cecrs commented
Status: Needs work » Needs review

- rgkimball and kreynen

Thanks for taking the time to review the module! I've committed the menu path patch. I considered the question of converting it to an entity, and while it would make the module a good deal more flexible, my specific purpose in contributing the module was the current lack of a super lightweight solution. I'm not 100% set against the idea, but my feeling is that there are other modules already that offer a more complex approach, and nothing that's truly lightweight. I'm open to further discussion, however.

rgkimball - Both clients that the module was originally written for specifically requested that there be a very limited set of tags allowable in alerts. Given that not all sites use the default text formats, I didn't want to hardcode it to a specific format that might not be there, and adding a text format programmatically was unwanted cruft, so I went with a hardcoded set of allowed tags. I'll have to think about that a bit more. I would however be very grateful for a patch for a close button, although I'm not sure off the top of my head what the behavior is going to be on ajax refresh...

-Cecily

kreynen’s picture

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

@cecrs it makes sense to table the entity discussion until the original project is approved. Maybe once you've jumped through the rest of project review hoops we can discuss that again. I'd be open to putting in some time to add this as a sub-module so the module can be used instead of the basic configuration so the main module stayed very simple.

Adding the template version of the review and updating the RBTC. Again, unless you (or someone willing to share their PAR bonus points with you) do the 3 reviews of other projects in the queue, this applications could be stuck at RBTC for > 8 months. Good luck with the rest of the process!

Automated Review

Only issue remaining is the lack of tests...

No automated test cases were found, did you consider writing Simpletests or PHPUnit tests? This is not a requirement but encouraged for professional software development.

Manual Review

Individual user account
Yes: Follows the guidelines for individual user accounts.
No duplication
Yes: Does not cause module duplication and/or fragmentation. Advantages over similar modules clearly defined.
Master Branch
Yes: Follows the guidelines for master branch.
Licensing
Yes: Follows the licensing requirements.
3rd party assets/code
Yes: Follows the guidelines for 3rd party assets/code.
README.txt/README.md
Yes: Follows the guidelines for in-project documentation and/or the README Template.
Code long/complex enough for review
Yes: Follows the guidelines for project length and complexity.
Secure code
Yes: Meets the security requirements.
Coding style & Drupal API usage
[List of identified issues in no particular order. Use (*) and (+) to indicate an issue importance. Replace the text below by the issues themselves:
  1. (*) Major finding, needs work
  2. (+) Release blocker
  3. Just a recommendation
  4. ...]

The starred items (*) are fairly big issues and warrant going back to Needs Work. Items marked with a plus sign (+) are important and should be addressed before a stable project release. The rest of the comments in the code walkthrough are recommendations.

If added, please don't remove the security tag, we keep that for statistics and to show examples of security problems.

This review uses the Project Application Review Template.

cecrs’s picture

cecrs commented
Issue summary: View changes
kscheirer’s picture

kscheirer
Primary language English
Location Vallejo
commented
Title: Site Alert » [D7] Site Alert
Status: Reviewed & tested by the community » Fixed

Non-blocking issues:

  • You should variable_del() all your variables in a hook_uninstall()
  • Any user-facing text should be passed through t(), like in site_alert_admin, and without html tags
  • You can simplify site_alert_get_alert() greatly, you only want to do any work after you've checked the time vs begin/expire. Also consider using REQUEST_TIME instead of time(), since it will stay constant for the life of the request.

Thanks for your contribution, cecrs!

I updated your account so you can promote this to a full project and also create new projects as either a sandbox or a "full" project.

Here are some recommended readings to help with excellent maintainership:

You can find lots more contributors chatting on IRC in #drupal-contribute. So, come hang out and stay involved!

Thanks, also, for your patience with the review process. Anyone is welcome to participate in the review process. Please consider reviewing other projects that are pending review. I encourage you to learn more about that process and join the group of reviewers.

Thanks to the dedicated reviewer(s) as well.

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.