[D7] Wunderlist

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

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.

Please set the correct title for this page:

Title:
[Dx] Your project name
Use [D6], [D7] or [D8] to specify which Drupal version your project uses.
e.g. [D7] Unicorn Integration

A git clone command: yours requires authentication. Please get correct command from "Version control tab"

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

No: Does not follow the guidelines for in-project documentation and/or the README Template. Please fill "INTRODUCTION" and other sections.

Code long/complex enough for review

Yes: Follows the guidelines for project length and complexity.

Coding style & Drupal API usage

1. (*) Major: require_once in first lines can break some modules work. You can attach this using "files[]" in .info file
2. All urls to wunderlist site are hardcoded. If the service changes api url you needs to rewrite all module code

This review uses the Project Application Review Template.

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

No: Does not follow the guidelines for in-project documentation and/or the README Template. Although you specify that the OAuth2 Client module is required, any required modules need to be under a 'REQUIREMENTS' heading. If you make it match the README Template Template you'll be good to go.

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

Everything looks to be in order.

This review uses the Project Application Review Template.

README improvements alone are surely not application blockers, anything else that you found or should this be RTBC instead?

Manual review:
1) delete .gitignore
2) add hook_help()
3) add hook_uninstall() and delete all variables.

Hey falc0, manual review here:

Continuing with cbanman's review, README.txt looks better know, although maybe it's better if you put the Requirements section before the Installation one, as stated in the README template.

It's also recommended that you include hook_install() in your .install file, take a look at the recommended practices.

You should also consider to align your project page a little bit more like the Project page template.

And, another suggestion, for your Wunderlist class, you may want to take a look at the Wunderlist PHP SDK project for your future implementations in the module :)

Hope it helps.

Cheers!

@pablitt: that are nice tips for improvements but surely not application blockers, anything else that you found or should this be RTBC instead?

Sorry, I should have mentioned that the last two suggestions were only suggestions, not blockers.

I think the only blocker one is the hook_install() one.

Sorry for the confusion!

Hi @falc0!, this looks really good now!

Looks pretty much RTBC to me :)

README.txt/README.md:
Your Readme is missing Views requirement
Also you need to specify a correct configuration step-by-step guide:
"this link" on your settings page doesn't work and gives a bad request.
You have to specify that people needs to create a new application and require a new API key / secret key at wunderlist before they can see their posts.
Also a link to run cron would be appreciated.

"The module right now only allows to see your listings of your Wunderlist tasks" instead of:
Wunderlist module allows listing of your Wunderlist tasks.

Hi @falc0,

Now it looks much better :)

manual review:

1. wunderlist_help(): the check_plain() and filter_xss_admin() are not needed here since no user provided text is involved, your README does not contain XSS attacks.
2. wunderlist_cron(): clearing all caches every 10 minutes when my cron runs? That looks like a serious performance issue. Why do you need to do that? Can't you check for updates when the actual page is viewed?
3. wunderlist_admin_form(): looks like you are using mt_rand() for something security related here for authorization? "Caution: This function does not generate cryptographically secure values, and should not be used for cryptographic purposes." from http://php.net/manual/en/function.mt-rand.php . Why do you need $state at all here? Please add a comment.
4. wunderlist_list_save(): use watchdog_exception() instead of watchdog() here. Also elsewhere.
5. wunderlist.api.inc: why do you need cURL and cannot use drupal_http_request()? Please add a comment.
6. I could not trigger XSS by using a malicious task name such as <script>alert('XSS');</script>, so that looks good security wise.

So the biggest problem with the module is the performance issue in hook_cron(), I think you should refresh the list on demand in a more efficient fashion. But that is not an application blocker, so ...

Thanks for your contribution, falc0!

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:

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

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.