[D7] Newsletter Prompt

Hi PaoloN,

Automatic Review -

http://pareview.sh/pareview/httpsgitdrupalorgsandboxpaolon2596687git

Please fix these errors, will do a manual review in some time.

I wasn't able to enable your module .. I got this error:

PDOException: SQLSTATE[42000]: Syntax error or access violation: 1067 Invalid default value for 'sid': CREATE TABLE {newsletter_prompt} ( `sid` INT unsigned NOT NULL auto_increment DEFAULT 0 COMMENT 'Primary Key: Unique NewsLetter subscriber ID.', `email` VARCHAR(254) NOT NULL DEFAULT '' COMMENT 'The email address collected.', `submitted` INT NOT NULL DEFAULT 0 COMMENT 'Timestamp for when the email was submitted.', `updated` INT NOT NULL DEFAULT 0 COMMENT 'Timestamp for last time the email has been updated.', `locale` VARCHAR(12) NOT NULL DEFAULT '' COMMENT 'The locale where the email has been captured.', `status` TINYINT NOT NULL DEFAULT 0 COMMENT 'Whether the email address is still valid or needs to be removed.', `propagated` TINYINT NOT NULL DEFAULT 0 COMMENT 'Whether the email address has been sent to external providers.', PRIMARY KEY (`sid`), UNIQUE KEY `email` (`email`), INDEX `submitted` (`submitted`), INDEX `updated` (`updated`) ) ENGINE = InnoDB DEFAULT CHARACTER SET utf8 COMMENT 'Table for Newsletter Prompt.'; Array ( ) in db_create_table() (line 2720 of ..

so I couldn't complete the review !!

anyway I did review:

please add you sandbox link to the original post

Automated Review

PASS Automated Review.

Manual Review

Individual user account

[Yes: Follows] the guidelines for individual user accounts.

No duplication

[Yes: Does not cause] module duplication and/or fragmentation.

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. (*) DB error on enabling the module which I posted above this comment
2. Implements hook_help().
3. Readme.txt Add Table of contents in the header

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.

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.

Hello PaoloN :)

I was able this time to enable your module ,, which allowed me to redo the review ..

but here are some notes I found:

1. Implements of hook_menu(). your text of attributes of title and description should be in t() so it could be translated.
2. I tried to show the block but it wont show .. which it should show me what the tpl file got .. am I missing something here! so I couldn't use the module so I can try/test the block.
3. I don't know if this is related to the previous note but ,, the region showed up in top as I select in settings of the module ,, I subscribed with my email but when I browsed the table newsletter_prompt I got 0 result .. isn't where it should store emails as you pointed in your function _newsletter_prompt_drupal_storage?

Thank you ..

Hello PaoloN :)

sorry for this late feedback :(

I installed a fresh Drupal installation .. enabled your module ,, default theme ,, no modules ,, tried to show the block but that didn't work either sorry my friend :( .. I reviewed everything else and everything looks good .. selected to store the email in DB and it did ..

can anyone else try the block thing ! .. it might be from my end .. so I won't change the status of the issue because it needs a second opinion as a review ..

Thank you

Automated Review

Automatic Review on paraview.sh and locally via Coder passed

Manual Review

Individual user account

[Yes: Follows] the guidelines for individual user accounts.

No duplication

[Yes: Does not cause] module duplication and/or fragmentation. However, some similarities might be noted in the Simple Subscription module.

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. There are some typo's in the README, but nothing deal-breaking.

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

No coding issues identified.

This review uses the Project Application Review Template.

Your code looks fantastic. It's very easy to read and you've taken a really simple approach to this sign-up functionality. It was relatively trivial to figure out how your module works just by reading through the code. I was surprised not to find any duplicates for this functionality but glad this one is in the pipeline.

If possible, I'd add your sandbox project link into this ticket as well so that it's easier to review what you've got there for your project documentation so we don't have to dig it out of the README.

As a last note, I also had issues displaying the block. Enabling it doesn't appear to do anything but didn't have a chance to really debug what was going on.

Hey PaoloN,

A few suggestions...

You can use more of the form API to setup the email signup form. For instance, the placeholder you added in JS can be done with
$form['some_element']['#attributes']['placeholder'] = t('Placeholder text');

There is also an "#element_validate" key you could use but I don't think that would save in lines of code or function reuse. I didn't know about a lot of this functionality until I read the From API page closely.

On your JS functions, I typically see them wrapped in a "Drupal.behaviors.functionName" rather than just ran outside of a function.

For the hook you provide, it was hard for me to know what it was or how to use it, but it seems to work fine. Typically, you'll want to put an example hook in newsletter_prompt.api.php with explanation in the docblock of what it does.

For the propagated and status fields in the newsletter_prompt table, I don't see how those values are ever changed. So, I see no point to having them unless you can change them somehow.

The CSS on the top view popup could use some work. I only tried bottom right and top, but the bottom right looked better. When displayed on top the close button wasn't even visible to me on Chrome.

Finally, I would think people might just want a time delay to show the popup as well as having the scroll option. I personally would just use the time delay as some pages would be hard to scroll or the user might not want the below-the-fold-content...also you might want to validate the pixels from top to only include positive values.

These are all suggestions and I see no major flaws, so I'll mark this RTBC.

I will update 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:

• 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 stay involved!

Thank you, 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 go the dedicated reviewer(s) as well.

Congratulations my friend:)