The 'date only' option must be turned on for a format with no time element

It is not obvious to me that this should be an error. Before we had the default time functionality a format such as d-m-Y was acceptable. The user entered the date, and there was an implicit default time of 00:00 on that day. It still works ok like that now.

If the admin wants to specify a time other than 00:00 then they can, but they should not be forced to tick the option. I suppose using the date-only option does give the benefit that the description displays the default time, but that is the only difference as far as user experience is concerned.

Was there a particular test scenario which caused you to want this, or was it just a separate idea?
Jonathan

I'd be ok if you wanted to implement this, you didn't need to close it yet ;-) The change does have the advantage that the users then see the time that will be used.

I did not actually look at your patch, but I have done now and at first glance you've done quite a few useful changes. Marking as 'needs review' and I will look at in detail tomorrow.

Jonathan

Having considered the coding in the patch, I think that this is worth doing. The benefit is that the user is told the default time in the text beneath the entry box. Also you have re-factored scheduler_admin_submit() to use internal functions, and removed the call to the date_popup function. The messages are better formed too.

The patch does not apply to the current git .module or .test, due to the recent patches committed, but I tested the code manually and it works as expected. Just one point - coder review does not like the call to drupal_set_message(implode(' ', $messages))

Hopefully you can find a neat way to avoid that, as it is really helpful now that coder review gives a clean pass of .module.

Jonathan

Making the title into a positive statement

How about the simple, but clear (and coder-review clean):

  if (empty($time_only_format)) {
    drupal_set_message(t('The date part of the Scheduler format is %date_part. There is no time part', array('%date_part' => $date_only_format)));
  }
  else {
    drupal_set_message(t('The date part of the Scheduler format is %date_part and the time part is %time_part.', array('%date_part' => $date_only_format, '%time_part' => $time_only_format)));
  }

The patch needed re-rolling as a lot of code has been changed since Nov 2013.

In .module I kept it the same except (a) $format was no longer used and the two lines had been merged into one when setting $time_format, so I replicated that, and (b) I incorporated my drupal_set_message suggestion from #9 to keep it coder-compliant.

For .test the change to $this->drupalCreateUser is not needed as we have already added the 'administer scheduler' permission since the original patch.

So, essentially the same patch. If you are OK with this it can be committed.

Jonathan

I have re-worked the existing tests into the new loop, first for textfield, then for date_popup. This is better (and hence safer) than directly setting the config variables, as we have found out before. Patch is against 1.2+11-dev

Patch needed re-rolling, now against 7.x-1.2+15