[D7] Google Shopping Feed

Git clone command for the sandbox is missing in the issue summary, please add it.

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.

It seems that robertoperuzzo forgot to add the link to the sandbox and the git clone command.

This is the link to the sandbox: https://www.drupal.org/sandbox/robertoperuzzo/2339909

And this is the missing git clone command:

git clone --branch 7.x-1.0alpha1 http://git.drupal.org/sandbox/robertoperuzzo/2339909.git google_shopping_feed
cd google_shopping_feed

I've edited this project's application issue to add these missing items.

It looks like a very useful module to add to Drupal Commerce websites.

I'm changing the status to needs review to let the PA robot do its work. A manual review could make sense when PA robot's job is done.

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

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

Assigned it to myself for manual review.

@robertoperuzzo, thanks for providing this module which can be very useful, to test it I had to install Commerce.

Automated Review

Git clone command for the sandbox may not be correct in the issue summary, please correct it so that the PA Robot can conduct an automated test.

Although cannot run the automated review, code sniffer reveals some warnings. For example, the second line in the file doc comment must be “@file”.

I tried to clone this module on my local machine. It prompt me to enter your Drupal.org password.

Issue:
git clone --branch 7.x-1.x robertoperuzzo@git.drupal.org:sandbox/robertoperuzzo/2339909.git google_shopping_feed
Cloning into 'google_shopping_feed'...
robertoperuzzo@git.drupal.org's password:
Permission denied, please try again.
Please make sure to provide the correct git repository link.
Instructions:
You can find the correct git clone command for your sandbox by clicking on the Version control tab, removing the checkbox in front of "Maintainer", and clicking Show. You can then copy-paste the git clone command from the codeblock below "Setting up repository for the first time".
Reference : https://www.drupal.org/node/1011698.

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. / No: List of security issues identified.]

 

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:

Critical issues:

1. (*) Remove unused variables or make use of them. Is the feed currently being submitted to your account?!!!!!
   I think you need to provide the missing settings page for the client’s merchant account, then substitute this code:

       // Create a client for our merchant and log in.
       $client = new GSC_Client(variable_get('aqua_gm_account_id'));
       $client->login(variable_get('aqua_gm_email'), variable_get('aqua_gm_pwd'));
   

     

2. (*) In hook_menu() you have provided a custom access argument, however I could not find any implementation of hook_permission(), or similar,
   to define permission in the module. Please correct me if I’m wrong.
3. (*) When storing variables in the database, consider adding a .install file to remove them when the module is uninstalled. I’ve uninstalled the module and at least three variables were left behind in the database.

Recommendations:

1. When assigning values to variables using variable_get(), you may want to provide a default value.
2. In forms, you should pass the HTML markup through the t() function. For the links use the l() function, and for external links(particularly line 220) use array('attributes'=>array('target'=>'_blank’), …
3. Follow naming practice: remove all references to aqua from all files, and correct package = Aqua in the .info file.
4. I did not see any constructor or methods in your .test file. Is this working? If not, please finish it or remove the file.
5. Also, remove the empty file: google_shopping_feed.inc

   

   

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.

Setting this to Needs work until at least the two critical issues are resolved.
Nice module! Would like to see it promoted.

Please also add dependency to Commerce's sub-modules; at least product and price - without doing so you will get a WSOD when visiting the settings page if one of the sub-modules is not enabled.

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.

Added the standard links to the issue summary.

The git clone command shown on your projecty page (under the heading "Setting up repository for the first time") only works for you. Contributors and reviewers need to use the one I've added to this issue summary.

The requirements (commerce) should be listed on the project page, under the heading "Requirements". Best practice is to make your project page follow the Project page template and it may be a good idea to also read tips for a great project page.

I haven't looked at your code yet, so I leave the status set to "Needs review".

Automated Review

Best practice issues identified by pareview.sh. Please review and fix the errors found in the following files.

FILE: /root/repos/pareviewsh/pareview_temp/google_shopping_feed.module
175 | ERROR | [ ] Concatenating translatable strings is not allowed, use placeholders instead and only one string literal

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

1. Review Drupal Coding Standards for use of blank lines, spaces, and inline commenting. These are minor issues that will not block a release, but adherence to the coding standards is best practice.

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.

Overall the code looks sound. For the error found in google_shopping_feed.module line 175, here's a suggestion.

Change

watchdog('google_shopping_feed', 'token: ' . $token);

To

watchdog('google_shopping_feed', 'token: %token', array('%token' => $token));

This review uses the Project Application Review Template.

@robertoperuzzo - You can now promote this to a full project yourself.

@ robertoperuzzo,
please remove unwanted code form your module file for ex on line 368
// $form[$table_index]["google_shopping_feed_condition_{$table_index}"] =
// array(
// '#type' => 'textfield',
// '#title' => 'condition',
// '#description' => t('Condition or state of the item'),
// '#default_value' => variable_get("google_shopping_feed_condition_
// {$table_index}", array()),
// );

or if you use this code in future use proper comment /** ToDO **/

Thank you for your contribution!

I updated your account so you can opt into security advisory coverage now.

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.

Assigning credits.