[D7] Commerce RMA

Goldmark,

Please provide all the relevant information at this page. Have a look at point 6 at this page http://drupal.org/node/1011698.

Also fix coding style issues detected by automated tool http://ventral.org/pareview/httpgitdrupalorgsandboxgoldorak1778134git.

Thanks

We are currently quite busy with all the project applications and I can only review projects with a review bonus. Please help me reviewing and I'll take a look at your project right away :-)

Closing due to lack of activity. Feel free to reopen if you are still working on this application.

If you reopen this please keep in mind that we are currently quite busy with all the project applications and I can only review projects with a review bonus. Please help me reviewing and I'll take a look at your project right away :-)

I haven't looked at the automated coding style review, so this might duplicate some of the issues found there (and also won't include many of them, just those that jump out too much ;))

README.txt

A couple of spelling issues and does not need to mention that it's only for Drupal 7 because that's easily understandable from the dependencies. Also, for the two dependencies, it would be nice to provide their url too, not only the project name.

commerce_rma.api.php

Unneeded empty line at the beginning of both functions.

/**
 * Defines return status for line items attached to a return.
 *
 * The return reasons array structure is as follows:

Defines return statusES... attached to a return line item.
The return *statuses* array structure...

And the $return_status from the function body should be $return_statuses, or just $statuses.

I haven't got to the actual module code yet, but do you also provide an alter hook for these two info hooks? If not, I think you should and then add them to this .api file.

commerce_rma.info

The current Drupal Commerce standard says that "package" needs to be "Commerce (contrib)".

You don't need to specify the "version" and "project" keys in there, they are added automatically by the d.o packaging script when you create an official release.

commerce_rma.install

/**
 * Implements hook_install().
 */
function commerce_rma_install() {
  // Set own module mail class.
  variable_set(
    'mail_system',
    array(
      'default-system' => 'DefaultMailSystem',
      'commerce_rma'   => 'CommerceRmaMailSystem',
    ));
}

You're not playing nice with other modules that might have overridden this variable. You should first get the variable, add your modification and then set it.

Same for the uninstall hook.

commerce_rma.module

* @package commerce_rma is not needed in the @file doxygen block.

'label' => t('Return', array(), array('context' => 'a drupal commerce return')),
I would set the context to: "Drupal Commerce return order"

commerce_rma_hook_info() Not all of these hooks are in the .api file.

commerce_rma_label() You should only care about your bundle, for the rest, just do a return commerce_order_label($entity, $entity_type);. Also, I think the function name should be commerce_rma_order_label().

commerce_rma_commerce_return_reasons_info() The doxygen block should be just: "Implements hook_commerce_return_reasons_info().". Same for all the other hook implementations.
You should add more 'space' between the item weights in order to allow other contrib or custom modules to put items between them ('weight' => -50; 'weight' => -45, etc..)

commerce_rma_field_widget_form()
'select[name="commerce_line_items[und][line_items][' . $line_item_id . '][return_reasons]"]' => array( You should use the $langcode parameter here instead of 'und'.

// Add admin .CSS file.
drupal_add_css(drupal_get_path('module', 'commerce_rma') . '/theme/commerce_rma.admin.css');

You should use $element['#attached'] for that.

// Unset the title and description and add it to the line item form.
$language = $widget_form['commerce_unit_price']['#language'];

This $language variable doesn't seem to be used anywhere below its declaration.

commerce_rma_field_widget_validate()

// The return.
$return = $form_state['commerce_order'];

I would call this $commerce_return_order for better clarity. Same in other places where $return is used for a commerce order.

commerce_rma_commerce_return_save() and commerce_rma_commerce_return_delete() I don't see the purpose of these functions.. you are just saving and deleting a commerce_order after all, so you should use the API functions from commerce_order.module.

commerce_rma_get_return_reasons()
A-ha! So you do fire an alter hook :) Please document it in the .api file.

commerce_rma_commerce_order_presave()
variable_get('commerce_return_maximum_days', 0); and variable_get('return_is_activated', FALSE);
These variables should be namespaced with your module name (e.g. 'commerce_rma_return_maximum_days' and 'commerce_rma_return_is_activated')

commerce_rma_entity_insert()

// Get the field language.
$language = field_language('commerce_order', $entity, 'commerce_returns_reference');

Not needed, field_get_items() does this already.

commerce_rma_entity_delete()

db_delete('field_data_commerce_returns_reference')
->condition('entity_type', $type)
->condition('bundle', $type)
->condition('commerce_returns_reference_target_id', $entity->order_id)
->execute();

This looks very suspicious, you shouldn't interact with the field storage directly. What if this field uses an alternate storage engine?

commerce_rma_commerce_order_uri()
commerce_order_uri() is not a hook, this cannot really implement it. I suspect you want to the same thing as with commerce_rma_label() so be sure that you follow the susgestion from above as well (only care about your bundle, let the 'parent' function handle the rest).

commerce_rma_ui.info

Same problems as commerce_rma.info

commerce_rma_ui.install

Empty file. If there's nothing you need to implement there, please delete it.

commerce_rma.forms.inc

A couple of doxygen blocks from this file were copied form commerce_order.forms.inc but they weren't updated.

http://drupalcode.org/sandbox/goldorak/1778134.git/blob/refs/heads/7.x-1...

I'm not sure why you want to do this in a module that deals with commerce orders. Wouldn't it be easier to depend or recommend an existing module that deals with HTML emails?

commerce_rma.pages.inc

commerce_rma_confirmation_return_user_form_validate()

if (empty($form_state['values']['confirmation'])
|| !$form_state['values']['confirmation']
) {

Doesn't that mean you actually want an !isset()?

commerce_rma_confirmation_return_user_form_submit()
You define $redirect at the beginning of the function and only use it in the last line. Why not just use it there without declaring a variable.

commerce_rma_redirect_return_user_form_submit()
Here, $redirect is passed through url(), totally not necessary, you just need:
$form_state['redirect'] = 'user/' . $return->uid . '/order/' . $order->order_id . '/returns/add/' . $return->order_id;

commerce_rma.datepicker.js

AFAIK, Drupal behaviors are already launched when the DOM is ready, so jQuery(document).ready( shouldn't be neccessary.

var elements = jQuery('input[name*="return_created"]'); It's safe to use $() here because it's declared as jQuery in the closure.

Update module description.

Multiple Applications

It appears that there have been multiple project applications opened under your username:

Project 1: #1827908: [D7] Commerce RMA
Project 2: #1720336: Commerce Stock Status

As successful completion of the project application process results in the applicant being granted the 'Create Full Projects' permission, there is no need to take multiple applications through the process. Once the first application has been successfully approved, then the applicant can promote other projects without review. Because of this, posting multiple applications is not necessary, and results in additional workload for reviewers ... which in turn results in longer wait times for everyone in the queue. With this in mind, your secondary applications have been marked as 'closed(duplicate)', with only one application left open (chosen at random).

If you prefer that we proceed through this review process with a different application than the one which was left open, then feel free to close the 'open' application as a duplicate, and re-open one of the project applications which had been closed.

Please format your code to match Drupal standarts. Use Coder module or http://ventral.org/ online service to find issues with your code.

1. Use Features instead of manually creating fields and instances.

2. Use hook_menu_local_tasks_alter() properly. As described in documentation:

Alter tabs and actions displayed on the page before they are rendered.

3. commerce_rma_ui_set_breadcrumb() is not a hook and does not need 'Implements hook_set_breadcrumbs().' comment.
Read about hooks here: http://api.drupal.org/api/drupal/includes%21module.inc/group/hooks/7
4. Read documentation for hook_forms(). You use it incorrectly. When you want to display 'commerce_rma_return_form' form simply call

drupal_get_form('commerce_rma_return_form');

5. commerce_rma_ui_return_view():
This is senseless since empty($language) is always true. Use global $language to get the current user's language.

  // Figure out what's user language.
  if (empty($language)) {
    $language = $GLOBALS['language_content']->language;
    if (empty($language)) {
      $language = LANGUAGE_NONE;
    }
  }

6. In the same function, never use theme_render_template() directly. Read documentation about theming: http://api.drupal.org/api/drupal/modules%21system%21system.api.php/funct...
7. In the same function, you should return structured array not a string.
8. In commerce_rma.view_rma_area.tpl.php you should not pass current language into t() function since it already knows it (by the way, you pass it incorrectly). Read documentation here: http://api.drupal.org/api/drupal/includes%21bootstrap.inc/function/t/7
9. commerce_rma.module:772 in commerce_rma_field_widget_validate() function you should not manually unset($row). Such constructions are senseless and clutter code.
Read about PHP variables here: http://php.net/manual/en/language.variables.php
10. In theme_commerce_line_item_return_manager()

    $row = array(
      drupal_render($form['line_items'][$line_item_id]['selected']),
      drupal_render($form['line_items'][$line_item_id]['title']),
      drupal_render($form['line_items'][$line_item_id]['line_item_label']),
      drupal_render($form['line_items'][$line_item_id]['quantity']),
      drupal_render($form['line_items'][$line_item_id]['return_reasons']) . drupal_render($form['line_items'][$line_item_id]['other_reason']),
    );

    if (path_is_admin(current_path())) {
      // Disply the price into the fourth column.
      array_splice($row, 3, 0, array(drupal_render($form['line_items'][$line_item_id]['commerce_unit_price'])));

      // Display the status into the last column.
      array_push($row, drupal_render($form['line_items'][$line_item_id]['return_status']) . drupal_render($form['line_items'][$line_item_id]['return_status_not_comply']));
    }

    $rows[] = $row;

Can be rewritten much simplier:

    $row = array(
      drupal_render($form['line_items'][$line_item_id]['selected']),
      drupal_render($form['line_items'][$line_item_id]['title']),
      drupal_render($form['line_items'][$line_item_id]['line_item_label']),
    );
    if (path_is_admin(current_path())) {
      $row[] = drupal_render($form['line_items'][$line_item_id]['commerce_unit_price']);
    }
    $row[] = drupal_render($form['line_items'][$line_item_id]['quantity']);
    $row[] = drupal_render($form['line_items'][$line_item_id]['return_reasons']) . drupal_render($form['line_items'][$line_item_id]['other_reason']);
    if (path_is_admin(current_path())) {
      $row[] = drupal_render($form['line_items'][$line_item_id]['return_status']) . drupal_render($form['line_items'][$line_item_id]['return_status_not_comply']);
    }
    $rows[] = $row;

11. In commerce_rma_cron():
First read documentation for SelectQuery::condition: http://api.drupal.org/api/drupal/includes%21database%21select.inc/functi...
You should not explicitly pass '=' operator since it is default parament value.
Second, never delete entities from database manually. Use API functions instead (commerce_order_delete() or entity_delete()).
Removing things directly from database makes it inconsistent.

@goldorak fixed a lot of other things in the sandbox at my suggestion, and I think this is really ready to go now :)

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

• Coder Sniffer has found some issues with your code (please check the Drupal coding standards). See attachment.
• DrupalPractice has found some issues with your code, but could be false positives.
  

  
  FILE: /home/klausi/pareview_temp/commerce_rma.module
  --------------------------------------------------------------------------------
  FOUND 0 ERROR(S) AND 1 WARNING(S) AFFECTING 1 LINE(S)
  --------------------------------------------------------------------------------
   722 | WARNING | Form error messages are user facing text and must run through
       |         | t() for translation
  --------------------------------------------------------------------------------
  

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. "if (module_exists('commerce_order')) {": that check is useless since aour module depends on commerce_order, so it is always there.
2. "'access arguments' => array('configure return settings'),": that permissions is not defined with hook_permission()?
3. commerce_rma_configure_order_type(): what are the unset() calls necessary for? Please add a comment.
4. commerce_rma.module: the module file is quite long, I think you can split out some menu callbacks and other stuff that isn't a hook or an important API functions.

Although you should definitively fix those issues they are not critical application blockers, so ...

Thanks for your contribution, goldorak!

I updated your account to let you 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 get 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.