Simple Hours [#1535698]

Description

The Simple Hours module allows sites to set and display open and close hours. It can be used for anything that has regular hours, like a store, office, library, welcome center, ticket booth, or call center. If you need to list one thing that opens and closes at certain times, this is the module to do it.

In addition to setting default hours for each day of the week, one may also add exceptions for specific dates, which will override the hours for that day.

The module provides two blocks:

A status block that shows the current open/closed status, including the closing time if open
A listing block that shows the default daily hours in a table and a list of exceptions, if any

The only other similar module that I've found is Office Hours. While that module allows one to enter hours for multiple locations, I did not see a way to enter exceptions, display the hours in blocks (although node_block could be used for that), or, critically, display the current open/close status in a block. Configuration of that module is more involved, and requires the installation of CCK. Simple hours has no dependancies and can be configured for the simple use case of showing open and close hours in a block more quickly.

The module currently only works with Drupal 6.

Links

Project: http://drupal.org/sandbox/jeffam/1532432
Git: http://git.drupal.org/sandbox/jeffam/1532432.git

Code Notes

I ran the module through an automated code review to clean it up, and dealt with the issues I found. However, there are a couple that require explanation:

sites/all/modules/pareview_temp/./test_candidate/simplehours.admin.inc:
+86: [critical] Potential problem: FAPI elements '#title' and '#description' only accept filtered text, be sure to use check_plain(), filter_xss() or similar to ensure your $variable is fully sanitized.
+125: [critical] Potential problem: FAPI elements '#title' and '#description' only accept filtered text, be sure to use check_plain(), filter_xss() or similar to ensure your $variable is fully sanitized.

The variables that are passed to the '#title' element are generated from internal arrays, so they are never user generated and would not need to be sanitized.

Comment	File	Size	Author
	Simple Hours screen shot	192.07 KB	jeffam

Comments

Comment #1

patrickd commented 16 April 2012 at 20:20

welcome,

I did not look into the code but it is not unlikely that drupalcs/pareview finds issues that are actually false positives, they should just warn you to take a good look on it but it's not intelligent enough to actually understand the code.

Nice to see you cleaned up the code, good job! ;)

We do really need more hands in the application queue and highly recommend to get a review bonus so we can come back to your application sooner.

By the way, is the conceptual difference between your module and office hours too big to just put this features into office hours?
Maybe you could open a feature request and ask them whether they think this can be merged with office hours or whether this should be its own module. Please think about this, it's always good to join all forces on one module to make it awesome ;-)

regards

Comment #2

jeffam

English

commented 19 April 2012 at 16:23

Thanks for your quick response.

I guess the honest answer to your question is that the conceptual difference between this module and Office Hours is not too big for these features to be added there. The main benefit of this one is the simplicity in setup, but I can imagine adding a small helper module to Office Hours that would create a new content type, a sample hours node, and maybe a block for each node with an hours field attached to maybe help new users along. Better docs would help there, too.

The features for current open/close status and exceptions could certainly be added to Office Hours as a field formatter and modified widget type, respectively.

I'll take you up on your suggestion to point them here and to think about merging.

Thanks again,

Jeff

Comment #3

patrickd commented 19 April 2012 at 16:31

Status:

Needs review

» Postponed

That's great thanks :)
Let me know how you want to handle it as soon you got an answer from the office hours maintainers.

Comment #4

jeffam

English

commented 19 April 2012 at 16:42

Status:

Postponed

» Needs review

The merge discussion with Office Hours has been started here: #1539768: Merge 'Simple Hours' Sandbox module with 'Office hours'?

Comment #5

olmeta.david commented 10 May 2012 at 12:16

Hello,

Maybe you could add a package name on the .info file.

In your .module file and particularly in your hook_menu(), I'm not sure that the 'title' attribute could take a condition like :

'title' => user_access('access administration pages') ? 'Simple Hours' : 'SImple Hours settings',

To do that, you must have to create a new function (instead of the t() function by default) which do the check.

You could think about the date format too (because for french people for instance, the date format is d/m/Y instead of y-m-d). You can use the DateAPI functions.

Regards,

Comment #6

soncco commented 10 May 2012 at 12:25

'access administration pages' is too generic permission, use your own.

Comment #7

anwar_max

English

commented 17 June 2012 at 12:49

Status:

Needs review

» Needs work

Manual Review:
1) You need to sanitize user provided input before printing it. Please check simplehours_block() function. See http://drupal.org/node/28984
2) Use t() function on line no 70 in simplehours_block() function. $current_exceptions = variable_get('simplehours_exceptions', array()) again, t() missing. Check all your strings in your module.
3) Do not create your own markup like you are create table with markup use http://api.drupal.org/api/drupal/includes!theme.inc/function/theme_table/6
4) Do not create list markup by own use http://api.drupal.org/api/drupal/includes!theme.inc/function/theme_item_...
5) Remove uneccessary comment fron simplehours.admin.inc

Automated Review:
Review of the 6.x-1.x branch:

Run coder to check your style, some issues were found (please check the Drupal coding standards). See attachment.
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.

Please try to resolve http://ventral.org/pareview/httpgitdrupalorgsandboxjeffam1532432git