Libhours

Hi Sean,

it seems, that you are working on a master branch, you should switch to a release branch: http://drupal.org/node/1127732

Ventral.org shows a number of issue with your code, most of them are formatting stuff: http://ventral.org/pareview/httpgitdrupalorgsandboxseanlwatkins1510620git

You should follow also these hints from ventral:
* Remove LICENSE.txt, it will be added by drupal.org packaging automatically.
* Remove "version" from the info file, it will be added by drupal.org packaging automatically.
* Remove all old CVS $Id tags, they are not needed anymore.

You should also add a link to the project description page to the issue and provide a git clone link to make checkout easier.

The community really needs more hands in the application queue and highly recommends to get a review bonus so they (we) will(/can) come back to your application sooner.

I checked the code and it's well written and fully documented. I suggest you try to get a review bonus in order to get approved quicker.

I'd suggest not to use such big arrays as function default parameters, setting a default of array() and then merging it with the default options would probably be better.

like that:

function libhours_admin_location_form(&$form_state, $location = array()) {

  $location_default = array(
    'lid' => 0,
    'name' => '',
    'description' => '',
    'parent' => 0,
  );

  $location = array_merge($location_default, $location);

}

I reviewed most of the code, and find it's well-written in general. I do have some recommendations.

Note that I'm not using any official priorization, and I'm not doing a proper by-the-book code review =) I just ordered things in terms of what I think you should fix first =) I think if the mentioned criticals are done this is probably ready to go.

Critical:

• Install file currently has a syntax error!
• Not escaping user-entered values, possible XSS attack vector. FIX: Add check_plain() when you print out any user-configurable strings (location names, period names, etc). You should add even though this requires certain permissions, since modules with such problems are regularly flagged in security advisories. (See http://drupal.org/writing-secure-code)

Moderate:

• (See attached patch!) Localization support is half-done, Lots of t()s missing.
• Some module features seem to depend on Javascript being enabled. It would be great if it would work with JS switched off.

Minor:

• Some indentation issues (e.g. using 4 spaces instead of 2, etc)
• Uses date() instead of Drupal's format_date() to take the user's Timezone into account.
• DB queries: is "WHERE TRUE" needed? Also, count(1) performs better than count(*).
• Configurable strings (like location/period names) do not have i18n support (in case someone wants to show translations for a certain location).

I tried to actually use the module and while I could configure hours, locations and exceptions, I found that there are no blocks listed in admin/build/block! =) Seems you forgot to add the case 'list': part inside your hook_block() implementation. (I'm guessing you'll have a block available for each location??)

It'd be best if you just installed the module on a new Drupal site from scratch and make sure everything works... maybe you're testing with existing data and you didn't see these problems (like the .install problem) =)

Also, I see you added some check_plain() to output, but there are still some XSS vulnerabilities. You can just go ahead and test them yourselves by, say, adding this bit to location names, descriptions, exception names, etc:

<script>alert('XSS problem!');</script>

For instance, this still triggers a problem on exceptions:

Also, ran a coder.module review and this is what came up... you might also want to fix it!
sites/all/modules/libhours/libhours.module

Good luck!

I forgot to mention you have some uses of db_affected_rows() after doing SELECTs... but I think that db_affected_rows() only returns *changed* rows (so is only useful after UPDATE or INSERT but not SELECT).

don't forget to tag applications having security issues properly
thanks for participation

Possibly related/similar modules:

http://drupal.org/project/office_hours
http://drupal.org/sandbox/jeffam/1532432 (sandbox)

Manual review: the code is very clean generally speaking.

There is some coding style issue in the .install file with your arrays. Please correct them.

Your .api file is very well documented. If it was my module I would suggest to also add @param in your .admin file, I like the idea of "a line of code which is not documented does not exists".

For your tpl file I would suggest to remove all domain-related logic from the html. HTML should only represent the view, easy for a designer to modify.

I can't however set your review as RTBC, I don't know if I am allowed to do that.

It seems that I can.

We have currently exceeded the proposed project application thresholds, so this is on hold for me for now. Get a review bonus and I will review/approve this right away.

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.

There is still a master branch, make sure to set the correct default branch: http://drupal.org/node/1659588 . Then remove the master branch, see also step 6 and 7 in http://drupal.org/node/1127732

Wrong branch name, 6.x-2.0.0 should be 6.x-2.x, see http://drupal.org/node/1015226

Review of the 6.x-2.0.0 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. You have to get a review bonus to get a review from me.

manual review:

1. project page is too short, see http://drupal.org/node/997024
2. Remove all @author and @copyright tags from your code, all code on drupal.org is GPL automatically. You can give credit in README.txt.
3. libhours_init(): why do you need that? please add you javascript and CSS only to pages that are actually generated by your module and where they are needed.
4. libhours_perm(): the "access content" permission is already defined by Drupal core, remove it.
5. libhours_expand_time(): use range() to generate arrays with numbers and remove your loops.
6. libhours_api_process(): use drupal_json() to output JSON. Also elsewhere.
7. libhours_api_error(): never use die(), use drupal_exit() if you must.
8. libhours_admin_location_edit(): why do you need drupal_set_title) here and cannot define that in hook_menu()?
9. libhours_admin_location_form(): "'#default_value' => check_plain($location['name']),": do not use check_plain() on #default_value, the Form API will sanitize is for you already. And double escaping is bad. See http://drupal.org/node/28984
10. libhours_admin_location_form(): same for the select box element, the #options are sanitized by the form API already.
11. libhours_admin_location_form_submit(): use drupal_write_record instead of the manual insert/update queries and you get schema validation for free.

Added project page, git url

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

Updated branch information