Commerce Giftwrap

Here's an automated review of coding format: http://ventral.org/pareview/httpgitdrupalorgsandboxsplatio1334264git
Mainly whitespace issues at the end of lines. You can setup many IDE's to trim these automatically. Let me know what program you are using to code and I might be able to direct you to configuration pages.

You should have the description in .info file on one row. Now it is on two rows and because of this the module shows under Other instead of Commerce (contrib).
In checkout when I have entered my message which is 'Testing' and press submit I get this error:
EntityMetadataWrapperException: Unknown data property commerce_giftwrap_message. in EntityStructureWrapper->getPropertyInfo() (line 339 of /var/www/commerce.dev/profiles/commerce_kickstart/modules/entity/includes/entity.wrapper.inc).

I enabled module, used default settings for gift wrap message and then made order in normal way.

Sorry I meant to do a full review but got tied up. Your code looks very good. Unfortunately I too am getting the same error message as reported by @iler on a clean install using all the latest required modules.

Some other tiny things to consider:

commerce_giftwrap_delete_giftwrap_line_items()
- $line_item_id does not have a default value if there are none; before you check if its empty. Whilst you don't need to initialise variables it is good practice to do so.
- You also have a check for the $line_item_id in a 'foreach' statement. If you only expect one value you should 'break' the 'foreach' loop when you get one. This happens is a couple of places. Why waste the extra cycles when you have what you want.

commerce_giftwrap_pane_checkout_form($form, $form_state, ...
$form_state should be &$form_state - again you're not using it but it might come into play. You have it right in all the other functions in the checkout_pane.inc file.

Here's my module list:

 Address Field     7.x-1.0-beta2
 Drupal core       7.12
 Drupal Commerce   7.x-1.2
 Chaos tool suite  7.x-1.0-rc1
 Entity API        7.x-1.0-rc1
 Rules             7.x-2.0
 Views             7.x-3.3

Product existed before your module was enabled. Order did not exist.

Same for me as for @tinker. Using latest versions of modules and product existed before this module was enabled but order did not exist.

Hi Splatio, checked out the latest version and the gift wrap message went through without error. There is however another issue to do with the gift wrap price. I was lazy and entered "10". When the checkout completed I noticed that the order charged 10 cents ".10" rather than 10 dollars. This is probably because the addition was expecting a decimal number. I do not know the proper way of doing this but I am pretty sure that commerce will have some form validation functions for currency. You should implement those on the gift wrap settings form.

You should filter_xss($message,array()) the commerce_giftwrap_message text provided by the user on the checkout form to strip out HTML and make sure its safe to use.

Oh and BTW since you are using Netbeans have a look at the following to aid setup to auto follow drupal coding standards:
http://drupal.org/node/1019816
http://drupal.org/project/nb_templates (great for hook templates)

Then you can Source -> Format and it will remove most formatting errors.

Hi @splatio,

Sorry for not being clear about user input validation. This should take place before the input is stored and not just when it is being displayed. This way you always know that processed user input is safe. Since you have a textfiled input without an online editor I am assuming that users should enter plain text and you do not want users to enter any HTML code. check_plain($message) encodes HTML characters so they will not affect output format. So if user inputs <b>Bold text</b> it will not be displayed as bold but will be stored encoded and displayed exactly as input showing the code <b>Bold text</b> in the message. filter_xss($message,array()) strips out all HTML code so if user inputs <b>Bold text</b> it will be stored and displayed as "Bold Text". What you are worried about is <iframe> tags that could hijack the site but I assumed you did not want any HTML tags.

So to ensure safe input without any HTML code it should be in commerce_giftwrap_pane_checkout_form_submit() where you set:

// Original. 
// $line_item_wrapper->commerce_giftwrap_message = $form_state['values']['commerce_giftwrap']['commerce_giftwrap_message'];
// New.
$line_item_wrapper->commerce_giftwrap_message = filter_xss($form_state['values']['commerce_giftwrap']['commerce_giftwrap_message'], array());

If you want to allow certain HTML tags such as bold, italic etc then set array() to contain the allowed tags. e.g. array('b','i') or just exclude array() from the function to use defaults. But since you have a plain textarea input I am assuming you do not want any HTML.

I will check out commerce_giftwrap_pane_settings_form_submit(). I thought this was automatic. If not then your original code of adding $form['#submit'][] = 'commerce_giftwrap_pane_settings_form_submit' in commerce_giftwrap_pane_settings_form() was better then the hook_form_commerce_checkout_pane_settings_form_alter().

Hi splatio,

I found some minor issues with your module.
I'm using the latest commerce_kickstart (http://drupal.org/project/commerce_kickstart).

1. When I use a custom message for Gift Wrapping, I can see it on "Review Order" page. That's fine.
If I click "Go back" to change something on my order, then my custom message is not displayed.
You don't set the #default_value in case there is an existing message available.

2. You can't change your custom message once added!
Let's say you saw on "Review Order" that your custom message is wrong. If you go back and edit the message, when you click Next the new message is not saved.

You could update the Message on _submit if there is an existing GiftWrapping in the line items. Right now, you just skip it.

Everything else looks nice to me.

Updating the status to needs work.

Hi,

I've reviewed your latest changes. You have fixed the issues from #15.

I have another issue with

function commerce_giftwrap_get_message($order) {
  $order_wrapper = entity_metadata_wrapper('commerce_order', $order);
  foreach ($order_wrapper->commerce_line_items as $line_item) {
    // If this line item is a giftwrap line item...
    if ($line_item->type->value() == 'giftwrap') {
      $message = $line_item->commerce_giftwrap_message->value();
      break;
    }
  }
  return $message;
}

If there is no line item for gift wrapping, the $message is not set.
I get:

Notice: Undefined variable: message in commerce_giftwrap_get_message() (line 201 of /var/www/reviews/sites/all/modules/reviews/commerce_giftwrap/commerce_giftwrap.module).

You get that warning on "review" page when you don't want a gift wrapping.

Hi,

Another two issues found.
1. Just disable the module and then enable it again.

FieldException: Attempt to create field name <em class="placeholder">commerce_giftwrap_message</em> which already exists and is active. in field_create_field() (line 85 of /var/www/reviews/modules/field/field.crud.inc).

2. I've uninstalled the module and the variables are still in database. For example this one:

commerce_giftwrap_limit_message

You have to use hook_unisntall and delete them.

Hi

Use the following workflow:
- Enable the module and create one order with a Gift Wrap
- Add products to cart, add gift wrap, go to review page. You will see the Gift Wrap in review.
- Uninstall the commerce_giftwrap module.
- On review page, you will see it the gift wrap with empty label, but the price is still there.
- Go back to cart page, when you click submit you get:

EntityMetadataWrapperException: Unable to get the data property currency_code as the parent data structure is not set. in EntityStructureWrapper->getPropertyValue() (line 442 of /var/www/reviews/profiles/commerce_kickstart/modules/entity/includes/entity.wrapper.inc).

- Try to edit the order that already contains the GiftWrap. You will get another error.

I don't think it is ok to delete the line item type: giftwrap.
I think that is causing the issues because order wrapper is trying to load it.

It looks like this should actually be "needs review" status, since it's the applicant, not the reviewer, who needs more info. In the review queue, the reviewers are the "maintainer," so they won't often look at something marked "maintainer needs more info."

Manual review:

Missing finishing new line

In commerce_giftwrap.install and commerce_giftwrap.module you are missing a finishing new line in the end of the file

Space before parenthesis

On line 67 and 75 It should be if (), not if().

New line between functions

You should ad a new line between each function and its comment to make the code easier to read.

Else, this look RTBC for me.

Sorry for the delay, but you have not listed any reviews of other project applications in your issue summary as strongly recommended in the application documentation.

Review of the 7.x-1.x branch:

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

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. Get a review bonus and we will come back to your application sooner.

manual review:

1. "variable_get('commerce_giftwrap_additional_info', '')": is there a UI to set that variable? Also if you add such a UI make sure that you add proper sanitization when printing this message to avoid XSS issues.
2. Same in commerce_giftwrap_order_tab(): $message is user provided input and needs to be sanitized before printing. See http://drupal.org/node/28984
3. Same in commerce_giftwrap_pane_review().

Otherwise the code looks good, but security issues are application blockers, so I need to bump this back to "needs work".

Took a look, anfd this seems like RTBC for me.

manual review:
"'#markup' => '<h3>Giftwrap Message</h3>',": all user facing text should run through t() for translation.

But that is just a minor issue, so ...

Thanks for your contribution, splatio! Welcome to the community of project contributors on drupal.org.

I've granted you the git vetted user role which will let you promote this to a full project and also create new projects as either sandbox or "full" projects depending on which you feel is best.

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.

As you continue to work on your module, keep in mind: Commit messages - providing history and credit and Release naming conventions.

Thanks to the dedicated reviewer(s) as well.

Hello klausi
I have a doubt about giftwrap message.
I am working on the www.uppermen.com.br now.
I have written the giftwrap message and click the checkout.
But the store owner doesn't have access to the gift message.
It's very kind of you to help me about this issue.
Regards Yakiv

@Yakiv Shumenko. The commerce_giftwrap project has already been approved and published. If you have a problem with the module then report it in the issue queue for the project: https://www.drupal.org/project/issues/commerce_giftwrap
Project main page is : https://www.drupal.org/project/commerce_giftwrap

This issue queue is for project application before it is published.