[D7] Commerce Product Queue

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

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.

First please fix the issues on http://pareview.sh/pareview/httpgitdrupalorgsandboxtajinderminhas2605116git"

FILE: ...iew/pareview_temp/includes/views/CommerceProductQueueRelationship.inc
---------------------------------------------------------------------------
FOUND 2 ERRORS AFFECTING 2 LINES
---------------------------------------------------------------------------
16 | ERROR | Public method name
| | "CommerceProductQueueRelationship::option_definition" is not
| | in lowerCamel format
26 | ERROR | Public method name
| | "CommerceProductQueueRelationship::options_form" is not in
| | lowerCamel format
---------------------------------------------------------------------------

Manual Review:

Please add the @param and @return[only if there is return in the fucntion] to all the functions except hook implementations
See https://www.drupal.org/node/1354

In class CommerceProductQueueRelationship under function query the $alias = $this->table; makes little ambiguous. Can you please add a little comment to such kind of statements in all over the module.

@rakesh.gectcr: the views method names cannot be changed, they are inherited from Views. More comments are always a good idea, but are surely not application blockers. Anything else that you found or should this be RTBC instead?

@klausi,

Other than that nothing much, +1 RTBC

Automated Review

There are some minor issue found in 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.

README.txt/README.md

[No] Kindly provide more details in your readme to get aware the new user with your module. Find more on read me template https://www.drupal.org/node/2181737

Code long/complex enough for review

[Yes] Follows the guidelines for project length and complexity.

Secure code

Yet to test.

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. * Implement hook_help() to help the users to get the basic understanding about your module.
2. * Implement hook_uninstall() to uninstall the tables which are being used in this module.

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.

It is recommended to review at least three modules to get the reviews bonus and keep your module top in priority.

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.

@ tajinder.minhas

I will do review the new changes.

Mean time Can you please try to review another 3-5 project applications. And please update that comment links to issue summary of this.

https://groups.drupal.org/node/427683

Review :-

1. In D7, we don't need hook_uninstall() to remove module defined tables. It is automatically done during module un-installation. So I guess, there is no need to have hook_uninstall for just removing tables.

2. In commerce_product_queue_load, we are kind of doing 'select *' thing which is not good for performance perspective, just try to load only those things which are required.

3. If your title for a page is dynamic, then instead of setting it through drupal_set_title, try 'title callback'

4. I think commerce_product_queue_update_queue and commerce_product_queue_create_queue is redundant code as not being used anywhere.

5. There is no validation to check whether product already added or not while adding to the queue.

6. I guess $form_state is passed by reference.

7. 'Default label for relationship' I guess this needs some updation.

Seems fine to me. Just one thing and I guess not a blocker. Some places, you have used drupal_Set_message(). Please make it consistent.

Also in admin.inc, we have
$val = &$form_state['values']; which is wrong as form_state is already passed by reference so no use of ampersand here.

otherwise looks good to me.

manual review:

1. project page: what is the use case of the module, what problem does it solve? Where is the queue displayed? Is that a queue for admins or for shop end users? Please explain according to https://www.drupal.org/node/997024
2. commerce_product_queue_schema(): the primary key of the second DB table is not defined?
3. commerce_product_queue_get_products(): this will be slow because you do one DB query per item. Use commerce_product_load_multiple() or entity_load() instead.
4. commerce_product_queue_edit_form(): do not run check_plain() on #default_value - the form API does that automatically already. And double escaping is bad. See https://www.drupal.org/node/28984
5. commerce_product_queue_edit_form_submit(): do not call drupal_goto() in a form submit handler, use $form_state['redirect'] instead. Example demonstrated at https://api.drupal.org/api/drupal/includes!form.inc/function/drupal_redi...
6. There is a security issue in the module and as part of our git admin training I'm assigning this to Manjit so that he can take a look. If he does not find anything I'm going to post the vulnerability details in one week. And please don't remove the security tag, we keep that for statistics and to show examples of security problems.

Removing review bonus tag, you can add it again if you have done another 3 reviews of other projects.

Now revealing the security vulnerability:
commerce_product_queue_admin_remove_product(): this is vulnerable to CSRF attacks. When executing user actions that change data in the database you must use the Form API or CSRF tokens in URLs to confirm the intent of the user. See http://epiqo.com/de/all-your-pants-are-danger-csrf-explained and https://docs.acquia.com/articles/protecting-your-drupal-module-against-c... .

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.