[D7] Availability Calendars Show Availability

Hi,

You have a some error on your code.

Please check this: http://pareview.sh/pareview/httpgitdrupalorgsandboxrli2257653git

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.

Hi.

Git link in description is incorrect.
Please change
git clone --branch 7.x-1.x rli@git.drupal.org:sandbox/rli/2257653.git
to
git clone --branch 7.x-1.x http://git.drupal.org/sandbox/rli/2257653.git ac_show_availability

Hello rli,

I passed coder on your module and I just found one not critical problem :

• ac_show_availability.js : severity => normal, review => comment_docblock_fileFile : @file block missing (Drupal Docs) [comment_docblock_file]

You just need to add a file description in your javascript file ac_show_availability.js. You can refer you to JavaScript coding standards

I don't know if it's a best way :

• I have a features request for your module. It will be possible to create a drush command to download and extract the bxslider library and copy it in the good directory ?
• I have another questions about this library. Usually in Drupal 7 we use the Library module to group in a same place all libraries of the website. Have you thought about using this solution ?

Hello rli,

In the line 73 you create the PHP array "$info['drupal.ac_show_availability']".

In the line 77 you have the index "js" with two index inside. The first index is "$module_path . '/ac_show_availability.js'" and the second is empty.

Pareview.sh say that you haven't give a name to the second index. You need to give the name 'data' like in the exemple of the hook_library page.

They said : " the key may be skipped, the value must specify 'type' => 'setting', and the actual settings must be contained in a 'data' element of the value." But they also said in the code example : "// JavaScript settings may use the 'data' key."

Hi,

You use different array definition in file ac_show_availability.module, in line 78 you defined $key => $value array and in line 82 you use autoasigned $key array.

'js' => array(
      $module_path . '/ac_show_availability.js' => array(
        'group' => JS_DEFAULT,
        'weight' => 100,
      ),
      array(
        'type' => 'setting',
        'data' => array(
          'bxslider' => array(
            'options' => $slider_settings,
          ),
        ),
      ),
    ),

Please check it.

Hello,

Before any comment, thanks for this module which makes me discover availability calendar and thanks for your module.

I have just tested the module to see it in actions before reviewing the code.

And I struggle to have a result, I followed the instructions, and there was no result on the page. So I needed to look into the code and found that the start and end date form element didn't showed up.

Because there is a missing dependency to date_popup.

No error, no warning, but no calendar.

Now that I see it in action, I will review the code.

Thanks

General :

1. pareview.sh result : http://pareview.sh/pareview/httpgitdrupalorgsandboxrli2257653git

ac_show_availability.info file :

1. I think it is better to put the dependencies below, separated from name, description, core, package

ac_show_availability.admin.inc :

1. For validation purpose : I think it is easy to use :
   

   '#element_validate' => array('element_validate_number'),
   

2. In practice (no relation with the previous point) : you have to select a content type, save and then choose the filter fields. Ajax displays the fields the first time or when you choose a new content type, but the form is not valid. I think a help text as description would be appreciate.

ac_show_availability.module :

1.       // Check if filter by taxonomy.
          if ($field['type'] == 'taxonomy_term_reference') {
            $form['availability']['ac_show_availability_' . $filter]['#autocomplete_path'] = 'taxonomy/autocomplete/' . $filter;
           }
   

   Thanks I did not know that tip.

2. $sid = db_query('select sid from {availability_calendar_availability} where cid = :cid and date = :date', array(
               ':cid' => $cid,
               ':date' => date("Y-m-d", strtotime($date)))
             )->fetchField();
   

   Why not using a db_select() ?

ac_show_availability.drush.inc :

1. 'description' => dt('Download and install the bxslider plugin.'),
   

   Thanks I didn't know the dt() function.

Thanks for your module.

Hello rli,

I test, its ok, the admin form is functional even changing the content type.

Ok for point in #12.

Pareview.sh still sees 2 other points for comments. And in the .info, the dependencies are not separated from name, description, core, package.

But it is as you want for this coding style points.

Actually the query mentioned in comment #15 is very simple, so db_query is OK there (Dynamic queries).

@rli, thank you for your contribution.

Automated Review

Best practice issues identified by pareview.sh / drupalcs / coder. Yes, http://pareview.sh/pareview/httpgitdrupalorgsandboxrli2257653git reported few minor issues that need to be fix.

FILE: /var/www/drupal-7-pareview/pareview_temp/ac_show_availability.module
--------------------------------------------------------------------------------
FOUND 3 ERRORS AFFECTING 3 LINES
--------------------------------------------------------------------------------
4 | ERROR | Doc comment short description must start with a capital letter
5 | ERROR | Doc comment short description must be on a single line, further
| | text should be a separate paragraph
88 | ERROR | No key specified for array entry; first entry specifies key
--------------------------------------------------------------------------------

Manual Review

Individual user account

Yes: Follows the guidelines for individual user accounts.

No duplication

Yes: Does not cause module duplication and fragmentation.

Master Branch

Yes: Follows the guidelines for master branch.

Licensing

Yes: Follows the licensing requirements

3rd party code

Yes: Follows the guidelines for 3rd party code.

README.txt/README.md

Yes: Follows the guidelines for in-project documentation and the README Template.

Code long/complex enough for review

Yes: Follows the guidelines for project length and complexity.

Secure code

Yes. If "no", list security issues identified.

Coding style & Drupal API usage

1. (*) Unable to enable this module as wrong dependencies dependencies[] = availability_calendar is there in .info file.
2. hook_help() is missing in your module.
3. (+) _ac_show_availability_admin_settings(): This form works inappropriately for some use cases that I tried that need to be fix.

   I access admin/config/content/resource-availability page and choose following option.
   
   
   

   Once I submit this form, filters values get reset that looks wired.

   
   

   One more scenario I found in this, as body field exist in both article and page content type, I am saving this configuration for article content type and after submission if I choose page content type then body field get selected by default even it was selected for article content type.

4. (*) Your module is also dependent on bxslider but you have not required this library at the time of module installation. Due to this, TypeError: $(...).bxSlider is not a function js errors come.

   
   

   Even If I installed bxslider library, it still produces TypeError: $(...).bxSlider is not a function.

5. You should configure configure = admin/config/content/resource-availability your setting form link through .info file to make it directly accessible from module page.
6. (+) I tried to run drush command, this command even deleted existing bxslider library, fix it.
   

   Unable to unzip jquery.bxslider.zip.                                     [error]
   
   bxslider plugin has been installed in sites/all/libraries              [success]

7. (+) _ac_show_availability_admin_settings: All the variables defined in this form like ac_show_availability_content_type, ac_show_availability_filter_fields etc need to be deleted using hook_uninstall() function after uninstallation of module.
8. Instead of using date(), should use format_date.

The starred items (*) are fairly big issues and warrant going back to Needs Work. Items marked with a plus sign (+) are important and should be addressed before a stable project release. The rest of the comments in the code walkthrough are recommendations.

As I am not a git administrator, so I would recommend you, please help to review other project applications to get a review bonus. This will put you on the high priority list, then git administrators will take a look at your project right away :-)

Thanks Again!

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.