[D7] SolidOpinion

Automated Review

Passed, no errors or warnings.

Submitted by Anonymous (not verified) on Mon, 10/06/2014 - 09:43

Review of the 7.x-1.x branch (commit bb97dcb):

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 fragmentation.

Master Branch

Yes: Follows the guidelines for master branch.

Licensing

Yes: Follows the licensing requirements

3rd party code

Yes: Follows the guidelines for 3rd party code.

README.txt/README.md

No: You seem to be missing some of the required sections from the README Template.
I'd recommend to use one of the templates and maybe include a TOC and reviews can skim through your README easier once it's the boilerplate.
Follow the guidelines for in-project documentation.

Code long/complex enough for review

Yes: Follows the guidelines for project length and complexity.

Secure code

Yes. If "no", list security issues identified.

Coding style & Drupal API usage

1. (*) I believe you should be using Drupal coding style for your javascript and not using raw javascript.
   In your solidopinion.js file you should be using something like Drupal.behaviours syntax?
   E.g. - https://www.drupal.org/node/1025552
   The Drupal Javascript coding standards
   may help point you in the right direction too.
2. I'd recommend adding more inline and possibly even block comments to your code / functions as it's not really clear to someone who doesn't know the solidopinion API what you are doing when scanning through the code
3. The module description in the info file, I would maybe detail that it replaces Drupal commenting system if it does.
4. I would consider allowing the user to change this password via an admin interface curl_setopt($curl, CURLOPT_USERPWD, "dev:eQ9UmN6WsuY");
5. It's somewhat unusual to display this as a warning "The minimal recommended width for Community Widget to be properly displayed is 200 px." as you cannot change this width via the UI to get rid of the warning.
   This may be better suited as a 'status' message displayed when the widget is enabled, or even in the README for the module.
6. If at all possible I would recommend setting up a demo account on solidopinion for module reviewers and provide the details
   on your project review issue for ease of reviewing.

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.

This review uses the Project Application Review Template.

I'd also recommend removing those Issue tags (aside from your PAReview: review bonus ;) ) from this issue.

Before adding tags read the issue tag guidelines. Do NOT use tags for adding random keywords or duplicating any other fields. Separate terms with a comma, not a space.

Looking good for a 1st review anyway.

You don't want to take over the 'access administration pages' permission, that's bad. Just take it out of your permission hook, it already exists.

Your uninstall hook is deleting a system variable 'site_name', that's bad, don't do it!

You should prefix your variable 'solidopinion_', not 'so_'

You should not be deleting files in your uninstall hook, that's bad.

You don't need to rebuild the menu and shouldn't be redirecting your user on install/enable (for one thing, it breaks the drush install method). I'd recommend not implementing either if you don't need to do anything special - just let Drupal handle the user flow.

I think the inclusion of your widget js should be done in the footer, not sprinkled into the code. I'll see if it works, anyway - checkout drupal_add_js.

Here are three more improvements in a patch:

1. use a custom permission 'administer solidopinion' instead of the administer node permission.
2. enable turning off of commenting per content type. The drupal comments allows you to turn commenting on and off per node, but I think that's probably overkill for most sites, and a lot of extra complications.
3. when you add content to the node via the links, you should add your content rather than just replace anything else that's there (i.e. play nicely with other modules)

Corrected git command.

@SolidOpinion, you have not created this issue at right place. It should be appear in Issues for Drupal.org Project applications list. Please correct this for future review, for now I've reviewed this.

Automated Review

Best practice issues identified by pareview.sh / drupalcs / coder. None

Review of the 7.x-1.x branch (commit 24f7c61):

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

This automated report was generated with PAReview.sh, your friendly project application review script. You can also use the online version to check your project. You have to get a review bonus to get a review from me.

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: Does not follow 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

(*)No: List of security issues identified.

1. solidopinion_node_view(): $shortname = variable_get('solidopinion_shortname', ''); As I understood from the code solidopinion_shortname value is coming through $_REQUEST['shortname'] that is vulnerable to XSS exploits. You need to sanitize this before rendering, make sure to read https://www.drupal.org/node/28984 again. Same applies for solidopinion_block_view(). Can you point me to the place in code where you perform the sanitization? If I'm right please don't remove the security tag, we keep that for statistics and to show examples of security problems.

Coding style & Drupal API usage

List of identified issues in no particular order.

1. solidopinion_node_view(): In general, Drupal.behaviors is preferred over jQuery(document).ready(). In this case you can also pass the node nid using using drupal.settings in js file and use Drupal.behaviors there rather writing js code here.
2. (+) solidopinion_permission(): 'access solidopinion content' doesn't seems to be in use. If not then remove it.
   

   'access solidopinion content' => array(
         'title' => t('Access solidopinion content'),
         'description' => t('Access solidopinion content.'),
       ),

3. solidopinion_help(): why the check_markup() here? You are not printing user provided text here but only the trusted README file? Do you just want to insert newlines with nl2br()?
4. solidopinion_node_view(): In general, using #attached is preferred over drupal_add_css, drupal_add_js, and drupal_add_libray.

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.

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

Not sure it's important but if you follow "create a new issue" link on the "Apply for permission to create full projects" page, the project should be "Drupal.org Project applications".
I have been able to find this ticket only because I globally search for tickets tagged with "PAReview: review bonus". I think other reviewers currently don't see this ticket if they only use the "Project" field.

Multiple Applications

It appears that there have been multiple project applications opened under your username:

Project 1: https://www.drupal.org/node/2351269

Project 2: https://www.drupal.org/node/2347459

As successful completion of the project application process results in the applicant being granted the 'Create Full Projects' permission, there is no need to take multiple applications through the process. Once the first application has been successfully approved, then the applicant can promote other projects without review. Because of this, posting multiple applications is not necessary, and results in additional workload for reviewers ... which in turn results in longer wait times for everyone in the queue. With this in mind, your secondary applications have been marked as 'closed(duplicate)', with only one application left open (chosen at random).

If you prefer that we proceed through this review process with a different application than the one which was left open, then feel free to close the 'open' application as a duplicate, and re-open one of the project applications which had been closed.

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

Removing review bonus tag, you have not done any manual review, you just posted the output of an automated review tool. Make sure to read through the source code of the other projects, as requested on the review bonus page.

PAReview: Individual user account

It seems you are using a non-individual account.
All user accounts are for individuals. Accounts created for more than one user or those using anonymous mail services will be blocked when discovered (see Get a Drupal.org account).
Please note that organization accounts cannot be approved for git commit access. See https://drupal.org/node/1966218 and https://drupal.org/node/1863498 for details on what is/isn't allowed. Please update your user profile so that we don't have to assume that this is a group account.

The user account has a full individual name, so that qualifies as individual account.

Automated Review

Is missing a README.txt you should check out https://www.drupal.org/node/2181737 add make sure you get all the required sections.

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.

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

solidopinion.module
You are using one line comments above some of your functions' comment blocks, you should add that to the comment block's short summary or a new paragraph in the block. (just a recommendation)

Closing due to lack of activity. If you are still working on this application, you should fix all known problems and then set the status to "Needs review". (See also the project application workflow).

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

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.

I should agree with jzasnake about those inline comments.

On admin/config/services/solidopinion it says "To add SolidOpinion comments on your site you should create an integration. Please go to SolidOpinion control panel and create an integration code or you can use existing one."

Then I can log in using this info that you provided on this issue page.

"Demo account for reviewers
http://my.solidopinion.com/
username: demo_account@test.com
password: demo_account"

But what am I supposed to do next? How to "create an integration"? On solidopinion.com I found no mention of integration. Please add clear instructions of how to test.

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.

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