[D7] Popup Advertisements

Automated review

The automated review revealed several issues, please fix these issues.

Manual review

I took a glance at the code and noticed two things:

1) The hook below isn't defined anyware. And preferably these definitions should be placed in an .api.php file.

/**
 * Implements hook_popupad_check_display_conditions().
 */
function popupad_popupad_check_display_conditions($popupad) {
  return !popupad_check_time_cookie();
}

2) The classes in the .module file should be placed in seperate files. (http://drupal.org/node/608152)

Use an .inc file and use files[] in the .info file to extend a class or implement an interface.

That's all for now, once the errors indicated by the automated review are resolved I will check the code more deeply.

Greetings!

We are currently quite busy with all the project applications and I can only review projects with a review bonus. Please help me reviewing and put yourself on the PAReview: review bonus high priority list. Then I'll take a look at your project right away :-)

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

@d.novikov, Tried the module but it did not do anything.

- Downloaded module from git
- Enabled module
- Had Libraries 2.x + already
- Downloaded ColorBox using 'drush colorbox-plugin' which installed 'colorbox' folder into sites/all/libraries
- Configure PopupAd module to Random, delay 1000ms, cookie lifetime 7, rest default
- Created 2 popup entities.
Browsed various non-admin pages and nothing happened. Logged out and tried again.
Do not see popupad.js in source code

Looked at the code format and everything looks good. Good structure, hooks. doc tags.
As reported previously you should truncate the 80 char + lines in the README.txt. I think the CamelCase class names are OK since EntityAPI uses them.

BTW using
Drupal 7.21
Libraries 7.x-2.1
Tried latest Colorbox and 1.x version https://github.com/jackmoore/colorbox/archive/1.x.zip

@d.novikov, Yep that was it. Cookie had been set before Colorbox was installed. I think that identifies an issue.

1. Perhaps you should used $colorbox = libraries_detect('colorbox') then check $colorbox['installed'] to make sure the library exists in popupad_init() and only continue if it does. This could also trigger a warning message on the admin form with directions on how to install colorbox library.
2. There is no validation of user inputs on popupad_admin_settings_form(). User can enter non numeric values for lifetime and delay.
3. Specifying 0 for popup width can create a popup that goes off screen if long sentences are used. If popup is fixed position then user cannot scroll to see the offscreen content. Perhaps a check for max width? This field does not state that its limited to integer which will be used as pixel width. Some people may try to enter widths as percentages.
4. Using entities seems counterintuitive for the user. I would assume the popupad to be a content item so you can add new content item. Having it in 'structures' makes it confusing. Is there a reason you chose entity?

@d.novikov I will be busy for the next 5 days so will not be able to review until end of next week. Good going on the changes and I look forward to testing them out.

I apologize, but if I try to clone your module, I'm asked for a password...

$> git clone --branch 7.x-1.x d.novikov@git.drupal.org:sandbox/d.novikov/1940872.git popupad
Cloning into 'popupad'...
d.novikov@git.drupal.org's password:

Can you help me?

Thanks in advance
Paolo

@pagolo use the public link:
git clone --branch 7.x-1.x http://git.drupal.org/sandbox/d.novikov/1940872.git popup_advertisements

@d.novikov perhaps you should update the git link in #1.

Thank you!
Paolo

Hi, check
http://ventral.org/pareview/httpgitdrupalorgsandboxdnovikov1940872git
and solve all errors/warnings.

Perhaps I missed something, but aside from the name and your documentation I don't see where the "Ads" part comes in. I assume that's what you're using this module to do, but it could be used for many other purposes. Consider a more descriptive name that describes what the module itself does, maybe "Popups Everywhere" or "Popup by Path".

The code seems pretty good though, thanks!

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

@d.novikov If you want to want to change the human readable module name then you should probably change the machine name if it is substantially different. Much easier and better to do this whilst in sandbox.

BTW I don't think module naming is a reason to remove RTBC status but I commend you for doing so. Good work on the module. I have left it 'Needs review' just because you set it there.

Well, the code still looks decent. The name hasn't changed yet, but that's up to you.

Thanks for your contribution, d.novikov!

I updated your account to let you 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:

• 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

You can find lots more contributors chatting on IRC in #drupal-contribute. So, come hang out and get 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.

----
Top Shelf Modules - Crafted, Curated, Contributed.