The Commerce Fulfillment module was created to ease the process of order fulfillment using the Drupal Commerce Module.
Allows the user to create packages and shipments, print shipping labels and packing slips, and reorgranise packages and shipments.

Project Page:
https://www.drupal.org/sandbox/gamerwamer/2507083

Git Clone:

git clone --branch 7.x-1.x http://git.drupal.org/sandbox/Gamerwamer/2507083.git drupal_commerce_fulfillment

Drupal Version: 7.x-1.x

Reviews Done:

Comments

EvanSchisler’s picture

Title: [D7] » [D7] Drupal Commerce Fulfillment
Issue summary: View changes
EvanSchisler’s picture

Issue summary: View changes
EvanSchisler’s picture

Issue summary: View changes
EvanSchisler’s picture

Issue summary: View changes
PA robot’s picture

Status: Active » Needs work

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

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.

Vignesh Puliyadi Raja’s picture

Manual Reviews:

1. Change your clone url in summary page
like : git clone --branch 7.x-1.x http://git.drupal.org/sandbox/Gamerwamer/2507083.git drupal_commerce_fulfillment

2. Found warning issues in pareview

FILE: /var/www/drupal-7-pareview/pareview_temp/commerce_fulfillment.module
--------------------------------------------------------------------------
FOUND 0 ERRORS AND 3 WARNINGS AFFECTING 2 LINES
--------------------------------------------------------------------------
104 | WARNING | Variable $view is undefined.
105 | WARNING | Variable $view is undefined.
105 | WARNING | Variable $view is undefined.
--------------------------------------------------------------------------

EvanSchisler’s picture

Issue summary: View changes
EvanSchisler’s picture

Here is my review done by pareview.sh

http://pareview.sh/pareview/httpgitdrupalorgsandboxgamerwamer2507083git

No errors are showing for me after addressing the $view variable.

EvanSchisler’s picture

Status: Needs work » Needs review
EvanSchisler’s picture

Issue summary: View changes
EvanSchisler’s picture

Issue summary: View changes
EvanSchisler’s picture

Issue summary: View changes
EvanSchisler’s picture

Issue tags: +PAreview: review bonus
splendidles’s picture

Manual Review

Individual user account
Follows the guidelines for individual user accounts.
No duplication
Does not cause module duplication and/or fragmentation.
Master Branch
Follows the guidelines for master branch.
Licensing
Follows the licensing requirements.
3rd party assets/code
Follows the guidelines for 3rd party assets/code.
README.txt/README.md
Follows the guidelines for in-project documentation and/or the README Template.
Code long/complex enough for review
Follows the guidelines for project length and complexity.
Secure code
Meets the security requirements. / No: List of security issues identified.

This review uses the Project Application Review Template.

EvanSchisler’s picture

Status: Needs review » Reviewed & tested by the community

Pretty sure this can go to RTBC status after splendidles review?
I am going to change it.
Please let me know if I made a mistake.

mpdonadio’s picture

Status: Reviewed & tested by the community » Needs review

@EvanSchisler, the application workflow is at https://www.drupal.org/node/532400. Someone else needs to set RTBC.

@splendidles, did you see anything to prevent RTBC on this?

klausi’s picture

Assigned: Unassigned » Manjit.Singh
Status: Needs review » Needs work
Issue tags: -PAreview: review bonus +PAreview: security

manual review:

  1. https://www.drupal.org/project/commerce_fulfilment what is the difference to this existing project? Module duplication and fragmentation is a huge problem on drupal.org and we prefer collaboration over competition. Please describe the differences to all existing fulfillment modules on the project page so that users can make an educated choice.
  2. commerce_fulfillment_schema(): could you add the foreign keys to the table such as order_id so that it is clear what relationships you build? See https://www.drupal.org/node/146939
  3. This module has a security vulnerability and as part of our git admin training I'm assigning this to Manjit so that he can take look if he has time. If he does not find anything I'm going to post the vulnerability details in a 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.

EvanSchisler’s picture

Issue summary: View changes
EvanSchisler’s picture

Issue summary: View changes
EvanSchisler’s picture

Issue summary: View changes
Issue tags: +PAreview: review bonus
EvanSchisler’s picture

Project page update highlighting the differences from other Order Fulfillment modules.

Manjit.Singh’s picture

@EvanSchisler Thanks for the Drupal contribution !! I guess

  • 'access callback' => TRUE, in hook_menu().

. Is that a good practice ? Because of this shipping_label and packing_slip is vulnerable.

Please look into the hook_menu functions.

EvanSchisler’s picture

Status: Needs work » Needs review

Thanks for reviewing my module @klausi and @Manjit.Singh .

I have fixed all the issues pointed out and rectified the security issue.

Setting to Needs Review.

klausi’s picture

Assigned: Manjit.Singh » er.pushpinderrana
Status: Needs review » Reviewed & tested by the community

manual review:

  1. commerce_fulfillment_add_product_action_form(): doc block: I don't think this is hook_form(), looks more like a callback for the action system, right? Same for commerce_fulfillment_add_product_action_submit(), there is no hook_submit(). How does Drupal core document the action callbacks? Also elsewhere.
  2. commerce_fulfillment_remove_product_action(): is the $package_id the ID of the commerce_fulfillment_package entity? Then you can just do "entity_metadata_wrapper('commerce_fulfillment_package', $package_id);" which will do the loading for you.
  3. commerce_fulfillment_get_array(): I don't understand this function from the name and the docs. It returns a value sometime? Why is it called _get_array() then? What is in the array that it returns? Entities? Or IDs? This could use a better name and better docs. Maybe commerce_fulfillment_get_package_ids()? You should not mix the logic of two different entity types in one function, it is confusing.
  4. commerce_fulfillment_package_content(): do not call drupal_render() or render() or theme() here, just return a render array. Drupal core will render it later for you and other modules can alter the stuff more easily. See https://www.drupal.org/node/930760
  5. commerce_fulfillment_create_package_form(): doc block is wrong, this is not a hook, this is a form constructor. See https://www.drupal.org/coding-standards/docs#forms
  6. commerce_fulfillment_create_package_form(): "Create a New Package:" is user facing text and must run through t() for translation. Please check all your strings.
  7. commerce_fulfillment--shipping_label.tpl.php: I don' think it makes sense to translate t('@company'), it just adds overhead for translators. Just make sure that the values are properly sanitized and print them without t() I would say. Same in commerce_fulfillment--packing_slip.tpl.php
  8. commerce_fulfillment--shipping_label.tpl.php: contains user facing strings like "Tracking Number:" that should run through t() for translation. Same in commerce_fulfillment--packing_slip.tpl.php
  9. commerce_fulfillment--packing_slip.tpl.php: a template should never call entity loads or make queries. Please prepare all variables that you need before calling theme(), and then pass them to it.
  10. commerce_fulfillment.module: why do you empty classes such as CommerceFulfillmentShipmentEntityController here? Just use EntityAPIController and Entity as class in hook_entity_info() then you don't need them at all?

Although there is quite a bit of API abuse here (especially the query functions in the templates!) those are not absolutely critical application blockers, so I think this is RTBC.

Assigning to er.pushpinderrana as he might have time to take a final look at this.

klausi’s picture

Assigned: er.pushpinderrana » Unassigned
Status: Reviewed & tested by the community » Fixed

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

Thanks for your contribution, EvanSchisler!

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:

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.

EvanSchisler’s picture

Thank you very much for all the suggestions and all the time spent by reviewers!!!

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.