[D7] Block by date

Please add git clone link in the issue summary.
Please follow this guideline https://www.drupal.org/node/1011698

Please fix the issues reported at http://pareview.sh/pareview/httpgitdrupalorgsandboxrameshbabug2401745git

I feel time zone settings of user and website needs to be considered. As I see in the code it relies on the server timezone.

Hi Ramesh

There are few suggestion/feedback. Please refer below for the same.

1. In info file, please correct the spelling of visibility now it says "visibilty"
2. Same spelling mistake in README.txt file
3. Please add hook_help in the module
4. In the below code you have added date_year_range as -3 to +3, but I think -3 is not required in this context.
$form['visibility']['dates_between']['from_date'] = array(
'#type' => module_exists('date_popup') ? 'date_popup' : 'date_select' ,
'#title' => t('From Date'),
'#date_format' => 'Y-m-d H:i',
'#date_year_range' => '-3:+3',
'#default_value' => isset($block->from_date) ? $block->from_date : variable_get('block_refresh_default_automatic_from_date', BLOCK_BY_DATE_FROM_DATE_DEFAULT_INIT),
'#description' => t('If you specify only from date, the block will be visible on that date onwards.'),
);

Thanks

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.

Automated test of your git repo shows error please fix them.
you can found them at URL
http://pareview.sh/pareview/httpgitdrupalorgsandboxrameshbabug2401745git

please change you git clone command to

git clone --branch 7.x-1.x http://git.drupal.org/sandbox/rameshbabug/2401745.git

Manual Review

Individual user account

Yes: Follows

No duplication

Yes: Does not cause

Master Branch

Yes: Follows

Licensing

Yes: Follows

3rd party assets/code

Yes: Follows

README.txt/README.md

No: Does not follow You should improve your project page, you can do it following Project page template. Also update your README.txt file using Readme template

Code long/complex enough for review

Yes: Follows

Secure code

Yes: Meets the security requirements.

Coding style & Drupal API usage

1.(*) Until user provide full date year month hour minute in

date_select

data does not save, can we have an error message at least.
2.(*) If $from_date is greater than $to_date then it is not working. look for ($form_state['values']['module'] == 'block')

The starred items (*) are fairly big issues and warrant going back to Needs Work.

This review uses the Project Application Review Template.

The issue should be in the category: Task

It can be a good module particularly where the timelines are there like Job posting with end date to submit application, advertisements with specific timeline displays.

Hi Ramesh - I would suggest you to check with existing module using which same functionality can be done like bean and hook_block_list_alter. I hope it helps.

Even https://www.drupal.org/project/context_date along with context module also provides way to display block based on date criteria. But this module is easy to use for content editors as they can see/change this in the visibility options, rather than complicating it.

Duplication

This sounds like a feature that should live in the existing context_date, bean project. Module duplication and fragmentation is a huge problem on drupal.org and we prefer collaboration over competition. Please open an issue in the context_date, bean issue queue to discuss what you need. You should also get in contact with the maintainer(s) to offer your help to move the project forward. If you cannot reach the maintainer(s) please follow the abandoned project process.

If that fails for whatever reason please get back to us and set this back to "needs review".

I would say just because this is simpler to configure its generally not a good idea to introduce a new module.
With time, when adding additional features the module will get more and more complicated and after a time you end with a duplicate of the other module.

My opinion would be to try to help out these modules in the issue queue and try to make there UI better.

I have found issue on http://pareview.sh/pareview/httpgitdrupalorgsandboxrameshbabug2401745git
Use $form_state['values'] instead of $form_state['input']

Hi Ramesh - Drupal modules list is getting more duplicate and confusing. Still I can see the duplicate modules are getting approved. We can add one more as you have valid points here :)

Module work fine and you can optimize control statement here - http://cgit.drupalcode.org/sandbox-rameshbabug-2401745/tree/block_by_dat... and follow Drupal coding standard to restrict in 80 characters.

@rameshbabu.g I think you should use date_default_timezone function instead of date_default_timezone variable. This way if user choose to select different timezone than site default, that case will also be considered.
Implementation tip - Better to do all comparison / storing dates in timestamp format so you don't have to worry about time zones.

Test this scenario after implementation -
Drupal site default timezone is timezone0.
Set visibility option by user1 with timezone1.
Check the visibility option by logging in with user2 with timezone2. It should not show the same date, also the date difference should match the difference between timezone1 and timezone2.

@rameshbabu.g I think this is a good module and should be released as full project. If you don't have time, please provide me access on this project. I will take it to completion.

Hi @rameshbabu.g

This is very useful module, thanks for you contribution. Please find my comments as:

Blocker:
If we enable the Page & Block caching then block will not disappear for anonymous user.

Some minor improvements are:

1. Remove one line space between 'core' & 'package' from block_by_date.info file.
2. Use t() in "'#title' => 'Date & Time'," line no#56 in .module file.
3. Remove extra space from "'#group' => ' visibility'," line#59 in .module file.

Suggestions:
I think you should consider the authenticated user timezone rather than site timezone. If user does not set their timezone then site timezone should be in consideration.

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.

Module works great, it is easy to use, coding standards are well met, and project page is ok.

Suggested improvements are:

Grammar and Semantics:
1) typo in readme - 'USE date popup' instead of 'used'
2) module file line #70 grammar: 'FROM that date onwards' instead of 'on'
3) module file line #134 typo: lessa

Bugs:
1) End date can be chosen before from date on configure block form; it works well on add block form.

In block_by_date_alter_validate_settings, why do you need check if ($form_state['values']['module'] == 'block') ???

Second, if I understood correctly, you want to set error if checkbox is checked but no end date is entered, right?

So, instead of checking this:

$to_date == variable_get('block_refresh_default_automatic_to_date', BLOCK_BY_DATE_TO_DATE_DEFAULT_INIT)

you can just check this:

!empty($to_date)

Because, if I set variable 'block_refresh_default_automatic_to_date' to value 2015-06-24 15:30 and then choose that exact time for end date, it will throw an exception for no reason....

2) It is possible to save only end date, but it does not work. In hook_block_list_alter() this option is not covered. It would be nice if I could only set end date, so my block would be shown until that day...

Suggested solution in hook_block_list_alter():

elseif (empty($default_value_from) && !empty($default_value_to)) {
  if (strtotime($current_date) > strtotime($default_value_to)) {
    unset($blocks[$key]);
  }
}

You can probably make it even simpler, by using only these conditions (but I haven't really tested that):

if (!empty($default_value_from))  {
  if (strtotime($current_date) < strtotime($default_value_from) {
    // Disabling block.
    unset($blocks[$key]);
  }
}
// If $default_value_from is not set, nothing will happen.
if (!empty($default_value_to)) {
  if (strtotime($current_date) > strtotime($default_value_to)) {
    // Disabling block.
    if (isset($blocks[$key]) {
      unset($blocks[$key]);
    }
  }
}
// If $default_value_to is not set, nothing will happen; 
// If the first condition is met, and the second is not, block will be hidden.

Anyway, you do not need cases that do nothing.... It will look better if you just change the conditions.

3) javascript is used to set text on fieldset, but not it does not work so great...
If I check on checkbox, but leave dates empty, it will set value 'Restricted by certain from and to dates'.

So, I would not use this check at all: todate_checkbox != undefined.
Instead, I would check only from_date and to_date values...
And, if you decide to allow end date only, there can be 4 messages:
-Restricted by From Date.
-Restricted by End Date.
-Restricted by certain from and to dates.
-Not restricted.

Suggestions:
1) module file line #163: Check if to_date field is not empty, instead of is checkbox checked....
2) Why are you setting this value if date fields are emtpy??

$dates['to_date'] = variable_get('block_refresh_default_automatic_to_date', BLOCK_BY_DATE_TO_DATE_DEFAULT_INIT);

Shouldn't this ALWAYS be NULL?
What if someone changes the value of the variables?
Maybe BLOCK_BY_DATE_TO_DATE_DEFAULT_INIT and BLOCK_BY_DATE_FROM_DATE_DEFAULT_INIT should be used here just as constants, not as varialbe values....

Normally, we frown on RTBC'ing your own project application. However, I took a final look at this, and I think we're okay to move forward.

Thanks for your contribution!

I updated your account so you can 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 stay 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.

I am giving credits to the users who participated in this issue.