[D7] Commerce per user price

Fixed the git clone URL in the issue summary for non-maintainer users.

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.

Hi,

I've been testing your module, read through the source code, and here are some things I noticed.

1. Lowercase module name
name = commerce per user price
You probably want to capitalize each word of the name, so that it looks similarly to the other modules (Commerce per User Price).

2. In your hook_menu():
'access callback' => 'user_access',
You do not need this line since 'user_access' is the default access callback.

3. 'Custom Price Override' role.
In order to apply a custom price to a user, you require it to have your hardcoded role, which may be annoying. If there are no other ways, I suggest you to create this role programmatically in your hook_install() and remove it in hook_uninstall().

4. When attempting to add a price rule, got an error:
Fatal error: Call to undefined function drupal_set_messages() in /var/www/sandbox/sites/all/modules/commerce_per_user_price/commerce_per_user_price.module on line 135
This function's correct name is 'drupal_set_message'.

5. When attempting to submit the custom form price with dummy text values, got an error:
PDOException: SQLSTATE[HY000]: General error: 1366 Incorrect integer value: 'asgfsg' for column 'user_id' at row 1: INSERT INTO {product_price_list} (product_id, user_id, price) VALUES (:db_insert_placeholder_0, :db_insert_placeholder_1, :db_insert_placeholder_2); Array ( [:db_insert_placeholder_0] => sgsag [:db_insert_placeholder_1] => asgfsg [:db_insert_placeholder_2] => fasfsa ) incommerce_per_user_price_form_submit() (line 133 of/var/www/sandbox/sites/all/modules/commerce_per_user_price/commerce_per_user_price.module)
This is actually a security hole since anything from the inputs goes directly to the database. You should add '#element_validate' => array('element_validate_integer_positive') to each commerce_per_user_price_form element. Also, add some advanced validation in 'commerce_per_user_price_form_validate' to make sure that user and product with entered IDs actually exist in the database.
By the way, form keys like 'product-id' or 'user-id' look weird to me. Typically, underscore is used.

6. Wrong hook annotations.

/**
 * Implements hook_form_FORM_ID_alter().
 */
function commerce_per_user_price_commerce_cart_line_item_refresh($line_item, $order_wrapper) {

It's hook_commerce_cart_line_item_refresh.

/**
 * Implements hook_form_FORM_ID_alter().
 */
function commerce_per_user_price_commerce_product_calculate_sell_price_line_item_alter($line_item) {

It's hook_commerce_product_calculate_sell_price_line_item_alter.

Thank you for the code, i tried it and works nice, it really helps me so please continue the good work.
Cheers!

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

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

+1 for this module. I found it today and this seems like a pretty cool module.

I have been looking for a module like this for some time. My developers say the code is good and we are installing it now. Thanks Arvind, just what we wanted.

I will keep you posted how we get on but all looks excellent so far.

Thanks,

Manual Review

Individual user account

Yes: Follows the guidelines for individual user accounts.

No duplication

No: Causes module duplication and/or fragmentation.

• Is this functionality already covered by Commerce Pricelist? https://www.drupal.org/project/commerce_pricelist

Master Branch

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

[List of identified issues in no particular order. Use (*) and (+) to indicate an issue importance. Replace the text below by the issues themselves:

1. (*) Still have the role 'Custom Price Override' necessary to have prices applied. You should use a permission instead or remove entirely
2. (*) admin/commerce/config/custom-price does not show in the store config menu, maybe because of the 'MENU_LOCAL_TASK' type?
3. (*) there should be a user interface for the user price table, how do we know what prices are set?
4. (*) Documentation needs work. See the suggested template as a starting point.

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.

Hello arvind.kinja, I reviewed again.

* I think that you need to remove the access checks in functions commerce_per_user_price_commerce_product_calculate_sell_price_line_item_alter and commerce_per_user_price_commerce_cart_line_item_refresh so that there isn't an access check, but would simply implement the new price if available and default to the normal price if not set by your module. As it stands now, someone would need access to both set these prices in order to be the beneficiary of the custom prices, which I don't think is what you intended.

Documentation formatting is better, but still isn't very clear. If you get this to project status, I'll rewrite for you.

* There is still the question of whether this module duplicates functionality of https://www.drupal.org/project/commerce_pricelist. Do you want to address this?

I'm moving the status to reviewed and tested. Good luck with this.. Again, let me know if you need help with documentation later.

hm, looks like you forgot the review bonus tag? Adding it now.

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

• Coder Sniffer has found some issues with your code (please check the Drupal coding standards).
  

  FILE: /home/klausi/pareview_temp/commerce_per_user_price.module
  ----------------------------------------------------------------------
  FOUND 10 ERRORS AFFECTING 10 LINES
  ----------------------------------------------------------------------
    53 | ERROR | [x] Expected 1 blank line after function; 0 found
    70 | ERROR | [x] Expected 1 blank line after function; 0 found
    88 | ERROR | [x] Expected 1 blank line after function; 0 found
   127 | ERROR | [x] Expected 1 blank line after function; 0 found
   157 | ERROR | [x] Expected 1 blank line after function; 0 found
   177 | ERROR | [x] Expected 1 blank line after function; 0 found
   197 | ERROR | [x] Expected 1 blank line after function; 0 found
   217 | ERROR | [x] Expected 1 blank line after function; 0 found
   228 | ERROR | [x] Expected 1 blank line after function; 0 found
   270 | ERROR | [x] Expected 1 blank line after function; 0 found
  ----------------------------------------------------------------------
  PHPCBF CAN FIX THE 10 MARKED SNIFF VIOLATIONS AUTOMATICALLY
  ----------------------------------------------------------------------
  

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

1. commerce_per_user_price_schema(): foreign keys are missing for documentation purposes, see https://api.drupal.org/api/drupal/modules!system!system.api.php/function...
2. commerce_per_user_price_schema(): prices should never be stored as float. Drupal commerce stores them as int, you should do the same.
3. There is a security vulnerability in this 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: the path /custom_price/%/delete is vulnerable to CSRF exploits. You need to either use a confirmation form or CSRF tokens on GET requests. See http://epiqo.com/de/all-your-pants-are-danger-csrf-explained and https://docs.acquia.com/articles/protecting-your-drupal-module-against-c... .

Fixing tag.

manual review:

1. commerce_per_user_price_form_submit(): do not use drupal_goto() in form submit handlers, use $form_state['redirect'] instead.
2. path user_id/autocomplete: access protection is missing. Who needs to use this callback? I think it should be protected with the "commerce per user price" permission or at least with "view user profiles". Since no access mechanism is defined no one has access accept for user 1. So this is not a security issue, it just means your autocomplete callback will not work for any other user yet. Same for product_id/autocomplete.
3. commerce_per_user_price_form_validate(): doc block is wrong this not a hook, but a validation handler. See https://www.drupal.org/coding-standards/docs#forms
4. commerce_per_user_price_view_price_list(): do not call theme() in a page callback, just return the render array. Drupal core will render it alter for you and it is easier for other modules to alter it.

Although you should definitely fix those issues they are not critical application blockers, otherwise looks RTBC to me.

Assign to Naveen as he might have time to take a final look at this.

no objections for more than a week, so ...

Thanks for your contribution, Arvind!

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.