Feature: Search by order number for return transactions.#43
Feature: Search by order number for return transactions.#43shabananavas wants to merge 7 commits intoAcroMedia:7.x-2.xfrom
Conversation
wilderuncat
left a comment
wilderuncat
left a comment
There was a problem hiding this comment.
Looks great overall, just a few questions.
| $form['order_search']['search'] = array( | ||
| '#type' => 'button', | ||
| '#value' => t('Go'), | ||
| '#value' => t('Return items from this order'), |
There was a problem hiding this comment.
This seems a little long to me... Maybe something like "Lookup Order" would be more succinct?
There was a problem hiding this comment.
This title was suggested by @travis-bradbury , to make it clear to the user. So might need to get a consensus on this one.
There was a problem hiding this comment.
I'll yield to travis, he is a better authority here than me.
There was a problem hiding this comment.
I'm not saying "Return items from this order" is the best. It is a little long. I just wanted something that describes what the user is trying to do.
| } | ||
|
|
||
| /** | ||
| * Returns a modal form displaying each item in an order along with a checkbox |
There was a problem hiding this comment.
The docblock should have a one-line summary under 80 characters long. Then a longer description underneath. Also, it would be useful to include the $order as a @param
| function commerce_pos_return_select_item_popup(&$form, &$form_state, $order) { | ||
|
|
||
| // Load our generic modal form. | ||
| $form['commerce_pos_return_select_item_modal'] = commerce_pos_form_modal(array( |
There was a problem hiding this comment.
Do you also need to do
form_load_include($form_state, 'inc', 'commerce_pos', 'includes/commerce_pos.common');
before this to make sure it is included?
There was a problem hiding this comment.
Nope, we don't need it. It works right out of the box :)
There was a problem hiding this comment.
I'm a bit curious why it works though... Maybe another form in the same request is including it. But what if this form is called alone? Would it still work? Would it hurt to include it twice? Might be better to be explicit. But I haven't tested it so not sure.
There was a problem hiding this comment.
Yeah, I didn't give too much thought into it. Travis would be able to answer this quickly as he did the restructuring for the generic modal.
| // Go through each item in the order and generate the html for the items | ||
| // display. | ||
| foreach (entity_metadata_wrapper('commerce_order', $order)->commerce_line_items as $delta => $line_item_wrapper) { | ||
| if (in_array($line_item_wrapper->type->value(), commerce_product_line_item_types())) { |
There was a problem hiding this comment.
I've found it best to first check if $line_item_wrapper->type exists before checking the value. I've seen many orders / line items get corrupted and things like this result in a 500 error.
| * This popup shows a checkbox next to each item for the user to add that item | ||
| * to the return transaction. | ||
| * | ||
| * @param $order |
There was a problem hiding this comment.
This is missing parameter type.
@param object $order
That should get travis to start passing this.
3 participants
Issue: https://www.drupal.org/node/2772927