[D7] Commerce Availability

Hello,

Thank you for your contribution. Here are some quick issues to get us tarted:

* The git clone is your personal git URL. This is your public Git clone command:
git clone --branch 7.x-1.x http://git.drupal.org/sandbox/inakilz/2163399.git

* You should delete your master branch, according to https://drupal.org/empty-git-master

* In your .module file: module_load_include('inc', 'availability_calendar', 'availability_calendar');
Is it necessary to always include this? Or can you move it to the functions that needs it to save memory?

Also, please take a look at the automated code review results here:
http://pareview.sh/pareview/httpgitdrupalorgsandboxinakilz2163399git

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.

• Remove execution rights from your files.
• Remove master branch: https://drupal.org/empty-git-master
• Fix pareview issues.
• Translate your empty option '-- empty --'
• commerce_availability_rules_action_info() predefine actions to avoid notices.
• In your project application description you have wrong git clone branch and you have your maintenance git clone sentence.

Manual review:

When i try to enable the module with drush i get this error:

Module commerce_availability cannot be enabled because it depends on [error]
the following modules which could not be found: availability_calendar

And i see in your info file you have "dependencies[] = availability_calendar" but i see this isn't a module on drupal.org so you cannot include it here like that.

Removing review bonus tag, you have not done all manual reviews, you just posted the output of an automated review tool. Make sure to read through the source code of the other projects, as requested on the review bonus page.

When finishing your review comment also set the issue status either to "needs work" (you found some problems with the project) or "reviewed & tested by the community" (you found no major flaws).

A very small change in commerce_availability_fill_gaps function

You set $availability = array_merge($availability, $gaps); but then the variable is only used one time.

I suggest you just return the result

#143 - $availability = array_merge($availability, $gaps);
#144 - return $availability;
#143 + return array_merge($availability, $gaps);

• Fix file permissions: http://browse-tutorials.com/snippet/give-permissions-recursively-files-o...
• Why do you use define on all of your variables?

But then it reads the variables in from database on every page load, not when the functions are called.

/**
 * Search for entities names.
 *
 * @param object $field
 *   The entity name for search
 * @param string $entity_type
 *   The entity type for search
 */
function commerce_availability_get_enames(&$field, $entity_type) {
}

• Sentences should end in periods.
• Return parameter not documented.
• Don't use abbreviations in function names, a better name would be "commerce_availability_get_entity_names()".
• The documentation says that $field is an object, but the "stdClass" type hint is missing. The description says it contains the "entity name", which should be a string. A bit further on in the code there is a check if this is an array? This needs to be cleared out.

function commerce_availability_get_enames(&$field, $entity_type) {
  ...
    foreach ($field['bundles'][$entity_type] as $ename) {
      $options[$ename] = $ename;
    }
  ...
}

Don't use abbreviations in variable names. Use $entity_name instead of $ename.

/**
 * Search for line items for an specific field.
 *
 * @param array $form
 *   Nested array of form elements that comprise the form
 * @param array $form_state
 *   A keyed array containing the current state of the form
 */
function commerce_availability_get_lines($form, &$form_state) {
  $ca_name_l_value = isset($form_state['values']['commerce_availability_f_name_l']) ? $form_state['values']['commerce_availability_f_name_l'] : '';
  $field = field_info_field($ca_name_l_value);
  $options = commerce_availability_get_enames($field, 'commerce_line_item');
  $form['line_item_fieldset']['commerce_availability_type_l']['#options'] = $options;
  return $form['line_item_fieldset']['commerce_availability_type_l'];
}

• Function name is confusing, what are "lines"? A better name would be "commerce_availability_get_line_items_from_form()"
• Sentences should end in a period.
• Return parameter is not documented.
• $form_state is passed by reference but it is not altered in any way.

I see these same problems coming back throughout the rest of the admin file. Try to document all return parameters and never use abbreviations in variables or function names. To someone else reading the code it is not obvious what a variable called "$default_ca_f_name_p" is supposed to contain.

No automatic review issues found. Moving on to:

Manual review

• In your git clone command, also include the project's folder name:
  

  git clone --branch 7.x-1.x http://git.drupal.org/sandbox/inakilz/2163399.git commerce_availability
  

• You have a dependency to availability_calendar and states that this projects is based on availability_calendar 4.1. Does it depend on it? If so, add a more detailed dependency in your .info file.
• I don't quite "get" what this module does only by reading the project page.
• Consider using hook_form_BASE_FORM_ID_alter() instead of hook_form_alter() for node forms.
• Providing external @links for hook_HOOK() documentation in docblocks isn't necessary, and is rather distractive. IDEs know where to find this documentation in various .api.php files.
• In commerce_availability.admin.inc you use a lot of variable_get() to variables you already define()d at the top of commerce_availability.module. This kind of makes the whole point of all defines mute? I get that having a central place to lookup variables without specifying its default value is handy, but using define()s for this is… code smell. How about having a single function, say commerce_availability_var($var_name) who's sole purpose is to return either the default or the saved value of it?
• In commerce_availability.js:
  • The first level of indentation in javascript closures is usually ignored.
  • There is a few missing ; after function declarations. E.g.
    

      var commerce_availability_set_availability_dates = function(picker_id, data) {
        Drupal.settings.datePopup[picker_id].settings.available_dates = data;
        commerce_availability_set_first_available_date(new Date(), dateLimit, picker_id);
      }
    

    This should have a ; at the end.

  • There is a syntax error in commerce_availability_check_availability() at line 126. Too many closing '}' before else if.

General comments

This review is based on viewing the code only as I haven't tested this and got any confirmation that it does what it is supposed to, but based on the screenshot and description of the project this seems rather complex for site admins to set up. My gut feel is that this project needs a better description/presentation about what its purpose is. It does a rather specific task in a narrow field of drupal, and therefore requires more detailed explanation.

Closing due to lack of activity. If you are still working on this application, you should fix all known problems and then set the status to "Needs review". (See also the project application workflow).

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