[D7] Pop-up announcement

There are some errors reported by automated review tools, did you already check them? See http://ventral.org/pareview/httpgitdrupalorgsandboxsergey-serov1986160git

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.

In addition to comment #1, you're using the master branch. You should switch to a version specific branch, see http://drupal.org/empty-git-master. You also need to add the git clone command to your summary.

Hi Sergey, instead of using hardcoded HTML in function popup_announcement_create_announcement() you should use theme function providing possibility to override it or at least attach JS as Drupal behavior.
It's also possible using theme functions in JS files, http://drupal.org/node/171213.

Hi, Sergey.

file popup_announcement.install:

• There is no need to use variable_set() in popup_announcement_install(). variable_get() will return default value, even if the variable does not exist.
• Add indexes in popup_announcement_schema().

Since the major code problems have been pointed out I'll focus on documentation.

Clarify the following in the Read Me file and on the project page:

• That it provides only a single message/pop-up multiple times
• That it may not work properly if blocks are cached
• That it is added to the site as a block
• The user must go to the Blocks page to configure the message
• Add a Configuration section to the Read Me file with steps

In the configuration form replace
Number of visits when announcement will be appear
with
Number of visits when announcement will appear.

Only numbers separate by ,
with
Only numbers separate by commas.

My humble opinion:
I feel that this functionality should be implemented on the client side rather than querying the database. For example, a js script can check a visit count cookie and activate the welcome message, such as I did here: http://www.duckydeals.com

I didn't find any technical issues with your module. But I do think it would be much more user friendly it the popup message could be edited in a text editor, instead of just plain text.

Good module.
I installed it and configure it. I although tested theme function to change background and it worked well.

Here are some comments :

.install l.76 : "was requsted" >>> "requested"

.module :
Why do you use time() function and not Drupal constant REQUEST_TIME ?
(see https://drupal.org/node/190417#comment-6697632)

suggestion

- in module configuration form it would be great that if we let empty the field "Number of visits when announcement will appear" the pop-up appears at every visit.

Hi Sergey-Serov:

Nice module - I can see this being quite useful.
An automatic code review of your module showed no errors.
Looking at your module manually:

popup_announcement.install

• In function popup_announcement_uninstall() you should call variable_del() to delete the variables you set and not run a delete query on the variable database.

popup_announcement.module

• In function theme_popup_announcement_create_announcement(), you should use a template file to store the HTML and not hard-code it into the function.

Other than those two issues I can't see anything else of serious concern.

[manual review]

• README.txt could has some spelling errors (announcement in line 25)
• README.txt says that block caching might cause issues with this module's functionality. Is this still true even though the block is set to *DRUPAL_NO_CACHE*? If so, might be worth investigation which scenarios could cause this. Or at the very least, list them in the readme.
• popup_announcement.js @file comment should say "Primary" instead of "Primarily"
• minor code styling inssues in popup_announcement.js (spacing in properties in lin 15 and 18).
• after much clicking and configuration, clearing session table, etc. I was unable to get this module to show any overlay. In console, I was able to verify that the JS behavior and settings weren't being added. Not sure what I'm missing here. Even tried forcing it to display on every page load. Block caching was disabled as well. Seems related to condtions in popup_announcement_create_block()

I think this module has a some good use cases and I'd likely use it if I could get it working. Nice work, looking forward to clearing up how to get this running.

Add command for git clone

In popup_announcement.install instead of setting initial values for your variables, just provide a default when you call variable_get() in popup_announcement_block_configure(). You have that for the height and width, but you can do it for the other variables too. Also provide defaults in popup_announcement_create_block().

Instead of exploding strings like "1,2,5" to manage your settings, consider using a plain array with serialize() and unserialize() instead.

No major problems found.

----
Top Shelf Modules - Enterprise modules from the community for the community.

Thanks for your contribution, sergey-serov!

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.

As noted by other reviewers, while it may be nicer from the developer perspective to set a variable and not have to include its default in variable_get(), variable_get() should always have a default. In theory one should be able to delete everything in the variables table and the site will still run on defaults, and in practice someone may delete some variables. So please remove the variable_set in install and stick defaults into variable_get(). This gets much nicer in Drupal 8!

Here are some recommended readings to help with excellent maintainership:

• Dries' post on Responsible maintainers
• Best practices for creating and maintaining projects
• Maintaining a drupal.org project with Git
• Commit messages - providing history and credit
• Release naming conventions.
• Helping maintainers in the issue queues (here you get to ask fans of your project to help you!)

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.

And thanks to the awesome reviewers: linwiz, rafal.enden, Dalay, geraldmelendez, K3vin, federiko_, chrisfree, _timpatrick, and kscheirer!