Recently upgraded to Scheduler-6.x-1.8 and now our main content guy is complaining that he used to be able to schedule a node by simply putting in a date. Now Scheduler apparently requires a time in addition to the date. Couldn't the time just default to 00:00 local time when it isn't specified?

For publish on, I put:
2011-02-23

The error I get is:
The 'publish on' value does not match the expected format of 2011-02-22 11:09:31

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

bret.miller’s picture

Title: Publish on date no longer when when time is omitted » Publish on date no longer works when when time is omitted
Eric-Alexander Schaefer’s picture

What version scheduler did you update from?

If you set the date format omitting the time values (e.g. "Y-m-d" instead of "Y-m-d H:i:s") you do not need to enter the time. However, if you do that, you cannot enter times at all.
You can set the date format at admin/settings/scheduler.

bret.miller’s picture

Looks like the last backup copy was 6.x-1.7, so not a big version jump. So there's no way to make the time optional? I can discuss it with the content guys. If they never use the time, there's no reason to require it.

Eric-Alexander Schaefer’s picture

No, right now there is no way to make the time optional.

I do not know why this should have worked in 1.7. No idea.

bret.miller’s picture

No idea here either. Anyway, the content creators are content with just a date, so that's what they have now. Thanks.

Eric-Alexander Schaefer’s picture

Status: Active » Closed (works as designed)
jonathan1055’s picture

Title: Default time with user override » Publish on date no longer works when when time is omitted
Version: 7.x-1.x-dev » 6.x-1.8
Category: feature » bug
Status: Postponed » Closed (works as designed)

Eric, I'd noticed this too in 1.8 and also obviously the D7 version.
I think it might be worth discussing/considering how we could have a admin option to set a default but allow users to override. My main user nearly always sets publishing for 6.00am but just occassionally wants other times. I would like the ability to set a default time, particularly when using the date pop-up because tabbing to the time entry field does not work for all browsers and you have to do it with the mouse which slows you down entry. So a default time, pre-populated into the text entry field would be very useful.

I'd be happy to work on this (when other things have settled down)

Eric-Alexander Schaefer’s picture

Status: Closed (works as designed) » Postponed

OK. Fine with me.

Eric-Alexander Schaefer’s picture

Title: Publish on date no longer works when when time is omitted » Publish on date no longer works when time is omitted
Eric-Alexander Schaefer’s picture

Title: Publish on date no longer works when time is omitted » Default time with user override
Version: 6.x-1.8 » 7.x-1.x-dev
Category: bug » feature
Jo_’s picture

Subscribing: If you use settings d-m-Y in scheduler options to avoid entering the time, you can't use date popup as it requires the time.
So the choice seems to be use date-popup and enter the time manually, or use text entry and force the user to enter a date manually, which I think is worse.

bignall’s picture

Title: Publish on date no longer works when when time is omitted » Default time with user override
Version: 6.x-1.8 » 7.x-1.x-dev
Category: bug » feature
Status: Closed (works as designed) » Postponed

I'd really like to see either an option for no time (which would assume 0:0:0) or a default time. On one site I work on we don't really care about the time of day, it's only the date that really matters.

dotist’s picture

default time is a must for me. i've spent the better part of the past two days trying to set form defaults for the time input w/o any luck. is this possible with form_alter or is there a function of the module that one can override?
thanks

rv0’s picture

Status: Postponed » Active

The latest version of Date module (since last month or so?) includes
" All Day "
" Show End Date "

The "All day" functionality does exactly that, it removes the "time".

The "Show End Date" functionality can be compared to the "unpublish on" feature.

Maybe Scheduler can use or mimic these features?

m4nux’s picture

subscribe

jox’s picture

Here's a fix that sets a default value when the time is left blank.

To fix it for any date popup field within your Drupal installation, you can put this into a custom module:

function yourmodule_date_popup_pre_validate_alter($element, $form_state, &$input) {
  if ($input['date'] != '' && $input['time'] == '') {
    $input['time'] = '00:00:00';
  }
}

It could also be limited to (and incorporated into) the scheduler module:

function scheduler_date_popup_pre_validate_alter($element, $form_state, &$input) {
  if ($element['#array_parents'][0] == 'scheduler_settings' && $input['date'] != '' && $input['time'] == '') {
    $input['time'] = '00:00:00';
  }
}
Juc1’s picture

@ jox

Sorry for being clueless but can that just be pasted at the bottom of the 'scheduler.module' file in Scheduler-6.x-1.8?

This doesn't seem to work =

http://imgur.com/OPUvx

Thanks

seworthi’s picture

I improved upon #13 by adding the ability to set the default time in the admin interface. Sorry, no patch, just code against 7.x-1.0:
Line 4:

// Define new variable for default time format
define('SCHEDULER_TIME_DEFAULT', '00:00:00');

Line 148 (scheduler_admin()):

  // Textfield for setting default time
  $form['scheduler_time_default'] = array(
    '#type' => 'textfield',
    '#title' => t('Default time'),
    '#default_value' => variable_get('scheduler_time_default', SCHEDULER_TIME_DEFAULT),
    '#size' => 20,
    '#maxlength' => 20,
    '#description' => t('This is the default time to use on all scheduler inputs if no time is entered.  This must be in a H:i:s format.'),
  );

end of scheduler_admin_validate() function:

  // Check if valid time entered
  if(!preg_match('/^[0-2][0-9]:[0-5][0-9]:[0-5][0-9]$/', $form_state['values']['scheduler_time_default'])){
    form_set_error('scheduler_time_default', t('A valid time if the format of 00:00:00 is required'));
  }

scheduler_form_alter() function in $form['scheduler_settings'] (add description):

'#description' => t('Leave time blank to use default time of !time', array('!time' => variable_get('scheduler_time_default', SCHEDULER_TIME_DEFAULT))),

End of file

/**
 * Function to set default time to if no time entered.
 */
function scheduler_date_popup_pre_validate_alter($element, $form_state, &$input) {
  if ($element['#array_parents'][0] == 'scheduler_settings' && $input['date'] != '' && $input['time'] == '') {
    $input['time'] = variable_get('scheduler_time_default', SCHEDULER_TIME_DEFAULT);
  }
}
cjamesrun’s picture

#18 works for me! Excellent. That would be great to add to the next release.

jonathan1055’s picture

This is good.

Is there a way to set the default time value into the text entry box in the form, so that it just appears there when the form is first shown? That would be even easier for users to see and understand. If the value was subsequently erased then we could still populate with the default value via scheduler_date_popup_pre_validate_alter()

Jonathan

tripper54’s picture

Status: Active » Needs review
FileSize
2.52 KB

I have rolled the changes in #18 into a patch against 7.x branch. Seems to work well for me.

Thanks for your work on this, y'all!

lolandese’s picture

Status: Needs review » Reviewed & tested by the community

Patch works as expected but only when using Date Popup. Obviously, if committed, this should be mentioned in the README.txt.

Off topic:
The default branch in the repository should not be the master branch (see http://drupal.org/node/1127732).

Thanks.

jonathan1055’s picture

Status: Reviewed & tested by the community » Needs work

Hi,

works as expected but only when using Date Popup

If Date Popup is not being used, can we set the default time as a pre-filled entry in the time input text field? (as mentioned in my comments in #3 and #20). Then it makes the default applicable to all situations and that would be very useful. If a site is using calendar popup then has to revert to plain entry (for whatever reason) you would not want the functionality to be altered.

Jonathan

pfrenssen’s picture

Review of the patch from #21:

The patch does not apply to the latest 7.x-1.x branch anymore, it needs to be rerolled.

+// Define new variable for default time format
+define('SCHEDULER_TIME_DEFAULT', '00:00:00');
  • Comment should end with a period.
  • The documentation is wrong: this is not describing a variable, but a named constant. And 00:00:00 is not a time format (such as eg. "H:i:s").
  • It would be better to use SCHEDULER_DEFAULT_TIME instead of SCHEDULER_TIME_DEFAULT. Core also does it like this, for example: MENU_DEFAULT_LOCAL_TASK, RDF_DEFAULT_BUNDLE, ...
+  
+  // Textfield for setting default time
+  $form['scheduler_time_default'] = array(
+    '#type' => 'textfield',
+    '#title' => t('Default time'),
+    '#default_value' => variable_get('scheduler_time_default', SCHEDULER_TIME_DEFAULT),
+    '#size' => 20,
+    '#maxlength' => 20,
+    '#description' => t('This is the default time to use on all scheduler inputs if no time is entered.  This must be in a H:i:s format.'),
+  );
  • Trailing spaces.
  • Comment should end in a period.
  • The variable "scheduler_time_default" would be better renamed to "scheduler_default_time" for the same reasons as outlined above.
  • The "H:i:s" date format is really only helpful for PHP developers. Many site administrators might not understand this. Replace it with "HH:MM:SS"
     if (!in_array($time_format, $acceptable)) {
       form_set_error('scheduler_date_format', t('The Date Popup module only accepts the following formats: !formats', array('!formats' => implode($acceptable, ', '))));
     }
+    
+  //Check if a valid time was entered.
+  if(!preg_match('/^[0-2][0-9]:[0-5][0-9]:[0-5][0-9]$/', $form_state['values']['scheduler_time_default'])){
+    form_set_error('scheduler_time_default', t('A valid time if the format of 00:00:00 is required'));
+  }
   }
 }
  • Trailing spaces.
  • Should be indented by 4 spaces.
  • Missing space between the slashes and the comment.
  • There should be a space between the if and the opening parenthesis.
  • The regex does not validate the input correctly. It would accept 29:00:00 as a valid time.
  • The error message is not correct. It says "if" instead of "of", and mentions that the time is required, but this is not true, this field is optional.
+        '#description' => t('Leave time blank to use default time of !time', array('!time' => variable_get('scheduler_time_default', SCHEDULER_TIME_DEFAULT))),
  • This should use @time instead of !time.
+
+/**
+ * Function to set default time to if no time entered.
+ */
+function scheduler_date_popup_pre_validate_alter($element, $form_state, &$input) {
+  if ($element['#array_parents'][0] == 'scheduler_settings' && $input['date'] != '' && $input['time'] == '') {
+    $input['time'] = variable_get('scheduler_time_default', SCHEDULER_TIME_DEFAULT);
+  }
+}
pfrenssen’s picture

Rerolled patch and addressed a number of the remarks.

pfrenssen’s picture

Assigned: Unassigned » pfrenssen

Going to work on this. Will address remarks from #23 and #24.

pfrenssen’s picture

Assigned: pfrenssen » Unassigned
Status: Needs work » Needs review
FileSize
13.48 KB

I have addressed the remarks and tried to make this work properly with and without date popups, and added support for custom date formats:

  • The default time functionality can now be switched on and off. Some sites might need to require users to enter a time for every node.
  • To be able to support custom date formats when a standard text field is used for input, I have added a "Date only" date format configuration. This defaults to 'Y-m-d'.
  • When date popups are used this "Date only" configuration option is hidden, as this is handled by the Date module.

I've also added a test. There are two exceptions occurring during the test, but these are addressed in #1319410: Fix "warning: trim() expects parameter 1 to be string" when entering date with no time.

Warning: trim() expects parameter 1 to be string, array given in _scheduler_strtotime() (line 557 of scheduler/scheduler.module).

jonathan1055’s picture

This looks interesting, thanks for your work on it. I'm not sure I will have time to test right now, and will be away during the weekend, but I will definitely test it next week, and hopefully get to RTBC soon.

Jonathan

pfrenssen’s picture

@jonathan1055, don't worry, just take your time, you've already been very much on the ball on all my recent changes and requests. It is incredible to get feedback this fast, thanks a lot!!

nadeeke’s picture

Title: Default time with user override » Default date and time
Category: feature » support
Priority: Normal » Major

I want to publish all articles when we done in the day on next morning at 4.00

So we want to set a default date and time

Where can I change to it in the code or any solution please.

jonathan1055’s picture

Title: Default date and time » Default time with user override
Category: support » feature
Priority: Major » Normal

Hi nadeeke,

To set bulk scheduling time for many nodes, I am currently working on integrating Scheduler with the Rules and Views Bulk Operation modules. That will give exactly what you want. Watch the issue queue, I will post it when it is ready.

For future reference, please do not change the title and category of an existing issue unless it is actually making the title more relevant to the issue. Do not change it just to match your issue - for that you should start a new issue. Also there is no point in marking it 'major' when you are asking for help. That will not get a response any quicker. You will notice that I respond to issues fairly promptly anyway.

Jonathan

jonathan1055’s picture

Status: Needs review » Needs work

Hi Pieter (and the others who contributed to this code),
I have tested the patch in #27 and in general terms it works reasonably well. It is a great start to the solution, but it does need some work in places. Here is what I have found so far:

  1. Using ordinary text entry, with 'Y-m-d h:i:sA' as the scheduler format, if an invalid time is entered it is ignored and the default time is used. For example, the user misses the second : when entering a real time (they do not want the default time). In _scheduler_strtotime() we have
    $str = 2013-08-21 10:0000PM  
    $date_format = Y-m-d h:i:sA
    $time = not set
    $date_only_format = Y-m-d
    $time from date_only = 2013-08-21 12:00:00AM
    $time with default = 2013-08-21 05:07:02PM
    

    So the default time is used, when it would have been better to give an error saying 'does not match the expected format'. This is partly a quirk of how scheduler alreday checks the values entered, but the result is worse now that we have the 'second chance' of getting a valid value from the 'date only' format.

  2. Using date popup, with 'Y-m-d h:i:sA' as the scheduler format, when no time is entered the plain text value of the default is set in scheduler_date_popup_pre_validate_alter().
    -- in scheduler_date_popup_pre_validate_alter --
    $input: Array
    (
        [date] => 2013-08-20
        [time] => 
    )
    $input after setting default: Array
    (
        [date] => 2013-08-20
        [time] => 17:07:02
    )
    

    This results in the errors "The value input for field Unpublish on is invalid" and "The value 2013-08-20 17:07:02 does not match the expected format." The default value needs to be added in the format expected, which in this case should be 05:07:02PM. We either need to validate the default when it is defined in the admin settings, to match the time part of the full format, or re-format it in scheduler_date_popup_pre_validate_alter() to the expected text.

  3. It would be very nice to derive the 'date only' format in the admin settings from the value entered for the full format, instead of having a separate entry field with the comment "To avoid confusion, make sure this matches the date format above." We should get the software to derive the format required, not rely on the adin to type two things which are consistent.

The date module has a very nice function date_limit_format() in date/date_api/date_api.module which extracts the parts of a full format which relate to any given granularity. However, we currently do not insist that scheduler requires date module. If we could use this function (or duplicate it in scheduler) this would allow us to solve all of the problems listed above, because we could extract and store internally the separate date part and time part of the full format. Or there are probably many other ways to solve this.

Some text strings and commenting need a little tidying up to improve the clarity and the English, and also to add the default time within the descriptions, but those can wait until the technical issues are resolved.

Looking forward to having a good discussion on possible solutions.

Jonathan

jonathan1055’s picture

I looked at the date_api function again, and it was more involved than we needed, plus it also called other date_api functions. So I thought it should be fairly easy to pick out the date_only and time_only parts of the format ourselves. Here is what I came up with, in scheduler_admin_validate()

  // Extract the date part and time part of the full format, for use with the
  // default time functionality. Assume the date and time time parts begin
  // and end with a letter, but any punctuation between these will be retained.
  $format = $form_state['values']['scheduler_date_format'];

  $time_letters = 'gGhHisaA';
  $time_start = strcspn ($format,  $time_letters);
  $time_length = strlen($format) - strcspn (strrev($format),  $time_letters) - $time_start;
  $time_only_format = substr($format, $time_start, $time_length);
  variable_set('scheduler_time_only_format', $time_only_format);

  $date_letters = 'djFmMnyY';
  $date_start = strcspn ($format,  $date_letters);
  $date_length = strlen($format) - strcspn (strrev($format),  $date_letters) - $date_start;
  $date_only_format = substr($format, $date_start, $date_length);
  variable_set('scheduler_date_only_format', $date_only_format);

This means that point (2) above can be solved by formating the numerical default time value using sheduler_time_only_format. It also means that for (3) we can remove the extra textfield for date_only_format in the admin form, as this variable will now always be kept automatically in sync with the full format.

pfrenssen’s picture

Cool, it looks like this can be automated, so we would not need to provide a "date only" configuration option. Unfortunately I'm very busy for the coming weeks and have very little time to look into this. I will try to find some time but can't make promises :) I'll also propose to the team if one of my collegues wants to follow up on this.

jonathan1055’s picture

Yes, we can remove the 'date only' format in the config form and create that variable automatically.

I could re-roll your patch, with the changes above, then maybe one of your team could test it? Would that help?

kristofdg’s picture

Sounds like a good idea Jonathan. Could you submit the patch please?

jonathan1055’s picture

Status: Needs work » Needs review
FileSize
26.47 KB

Here is an updated patch and I have fixed points 1 -3 in #32 above. I have also improved the description handling beneath the fields, displaying the default time, set the main format to be required, but not the default time (this is now only validated if the date-only option is ticked). The form is neater because we do not have the secondary 'date only' input format as this is derived internally from the main format. Hence it no longer needs to be a fieldset, just a single entry field for the default time which is displayed when the checkbox is ticked.

I have also revamped the entire scheduler.test file, making one common base class but dividing the basic functionality tests and the new default time tests into separate groups. In future we can add more groups for other aspects of testing and it means you can run just the set of tests you want, not all the scheduler tests. I changed a function name from assertDefaultTime to checkDefaultTime. This is better because if the name starts with 'assert' then all the actual test results have the row number of that single assert function, which makes tracking the actual failing test much harder. Now the different row numbers of the actual ->assert executable rows are shown in the results, which is much more useful.

In the test setUp() I have enabled the devel module, but not set this as a test requirement in .info - I am not sure of the implications of requiring it. I'd like your view on this. Do you think most of us use Devel in our test environments?

Patch against 7.x-1.1+10 of 27th July. Hope you like it.

Jonathan

pfrenssen’s picture

  1. --- scheduler.info	2013-07-28 02:47:46.000000000 +0100
    +++ scheduler.info	2013-09-02 18:50:44.000000000 +0100
    

    The patch was not saved in the standard patch format. It should be applicable with "patch -p1". If the test bot was enabled it would fail on this.

  2. +++ scheduler.module	2013-09-05 17:21:08.000000000 +0100
    @@ -156,6 +166,29 @@ function scheduler_admin() {
    +    '#prefix' => '<label>' . t('Date Only') . '</label>',
    

    It is not standard practice to provide markup like this to 'fake' a second title for checkboxes. By default the #title property of a checkbox is displayed next to it, and nothing is shown above it.

    Also, when a title has multiple words only the first one should be capitalized.

  3. +++ scheduler.module	2013-09-05 17:21:08.000000000 +0100
    @@ -171,16 +204,52 @@ function scheduler_admin() {
    +  variable_set('scheduler_time_only_format', $time_only_format);
    +
    

    Don't store settings in the validate handler. This should be done in the submit handler. The validate handler should only validate the given input.

  4. +++ scheduler.module	2013-09-05 17:21:08.000000000 +0100
    @@ -171,16 +204,52 @@ function scheduler_admin() {
    +  variable_set('scheduler_date_only_format', $date_only_format);
    +  drupal_set_message(t('The date part of the Scheduler format is %date_part and the time part is %time_part', array(
    

    Same as above, don't store settings in a validate handler.

  5. +++ scheduler.module	2013-09-05 17:21:08.000000000 +0100
    @@ -336,11 +408,23 @@ function scheduler_form_alter(&$form, $f
    +      // Show the default time so users know what they will get if they omit the time.
    +      if ($date_only_allowed) {
    

    Comment exceeds 80 characters.

  6. +++ scheduler.test	2013-09-05 17:27:15.000000000 +0100
    @@ -5,42 +5,132 @@
    +  // This class is a common base class for all Scheduler tests. It does not
    +  // contain any tests or the getInfo() function. The actual scheudler test
    +  // groups are called SchedulerNN (NN = 01, 02 etc) and within these classes
    +  // are the actual tests (all starting with the word 'test').
    +
    +  /**
    +   * standard setUp.
    +   */
       function setUp() {
    

    If this is a base class, then do not provide a setUp() method.

    Do not name tests with numbers, but provide a meaningful name for each test class, so that they can be recognized when running with run-tests.php.

    Overall I do not think that it is in scope of this issue to completely overhaul the testing infrastructure. Looking at the code now I have no idea how all of these changes will impact the other tests.

  7. +++ scheduler.test	2013-09-05 17:27:15.000000000 +0100
    @@ -5,42 +5,132 @@
    +    parent::setUp('scheduler', 'date', 'date_popup', 'devel');
    +
    

    Why do you enable the devel module? I don't see its functions used anywhere?

    This also forces anyone that is using continuous integration to install the devel module in their testing environments, and often they want to run tests on an environment that is as close as possible to the production environment.

    Overall, it is best to only enable modules that are strictly necessary for the tests to be run.

  8. +++ scheduler.test	2013-09-05 17:27:15.000000000 +0100
    @@ -5,42 +5,132 @@
    +/**
    + * Scheduler01 contains the tests for basic Scheduler functionality.
    + */
    +class Scheduler01 extends SchedulerTestCase {
    

    I am a fan of having each class in a separate file.

  9. +++ scheduler.test	2013-09-05 17:27:15.000000000 +0100
    @@ -62,5 +158,136 @@ class SchedulerTestCase extends DrupalWe
    +      debug('Change format to ' . $fmt);
    +      variable_set('scheduler_date_format', $fmt);
    

    Don't use debug() inside tests. This will show up in the output of the tests. I know I am guilty of this too, I always use this during development but I forgot to remove these from my patch :)

  10. +++ scheduler.test	2013-09-05 17:27:15.000000000 +0100
    @@ -62,5 +158,136 @@ class SchedulerTestCase extends DrupalWe
    +  /**
    +   * Checks that the default time works as expected.
    +   */
    +  protected function checkDefaultTime() {
    

    Usually the methods that do checks are prefixed with 'assert' rather than 'check'.

  11. +++ scheduler.test	2013-09-05 17:27:15.000000000 +0100
    @@ -62,5 +158,136 @@ class SchedulerTestCase extends DrupalWe
    +    $this->assertNoRaw(
    +      "The 'publish on' value does not match the expected format of",
    +      'If the default time option is enabled the user can skip the time when scheduling content for publication.'
    +    );
    

    I wouldn't split asserts over multiple rows. It's not such a big deal, but (at least in core) these are always put on one line to improve readability of the overall code. Nobody cares about the assert message anyway :)

I only did a very superficial code review, and did not actually tried the patch.

Overall I'm afraid that the sweeping changes that have been made to the tests will prevent this from being accepted by the maintainers. This might best be handled in a separate issue.

jonathan1055’s picture

Status: Needs work » Needs review
FileSize
19.86 KB

OK, the revamping of the automated tests should be handled in a separate issue. I have reverted to the .test file as modified by your patch in #27 and not changed it any further here. That covers 6 - 11 in your list. The new issue is #2109945: Reorganising and expanding the automated tests

So responding to your points 1-5 above

  1. I use diff as I do not have git diff. I can't get diff to produce the required format. Do you know how? Maybe I should try to find some other software to produce patches.
  2. I have changed the two 'Date only' fields to be in a fieldset, as this nicely groups the two linked fields and shows the title.
  3. Setting the variable is now moved to new submit handler scheduler_admin_submit()
  4. Same as 3.
  5. Fixed

To summarise the changes in this new patch compared to yours in #27

  • a. New constant SCHEDULER_TIME_ONLY_FORMAT, for use as a default for the stored variable
  • b. In the admin form scheduler_date_format is now set to be required. Everything fails if this is empty
  • c. Minor wording changes to form descriptions
  • d. Removed the text entry field for date_only_format and derive this from scheduler_date_format, thus ensuring they match
  • e. Cater for using default time when no time entry is allowed, ie the user can only enter a date.
  • f. Better handling of descriptions beneath the date and time entry fields, as these need to vary depending on the options in force, and the current method of doing them was clumsy and difficult to modify
  • g. In scheduler_date_popup_pre_validate_alter() set the ['time'] part using the necessary format given in scheduler_time_only_format instead of just using the plain text default time string.

  • Mainly this is very similar to what you produced, hope you can review this, as it would be very good to get this functionality in.

    Jonathan

    [edit: added link to new issue on re-organising the tests]

pfrenssen’s picture

Thanks, I will have a look at this soon, after I get back from Drupalcon :)

jonathan1055’s picture

Cheers. Have a great time!

inversed’s picture

Creation time not updating when publishing content immediately.

I noticed something with the patch from #39. It works well for me so far with minimal testing, however, if a content type is set to "Change content creation time to match the scheduled publish time" and the Advanced Options are set to "Publish the content immediately after saving", the node's created date will not be updated to reflect the publish date and time.

If the Advanced Options are set to "Schedule the content for publication on the next cron run", then the created date will be updated after a cron run.

jonathan1055’s picture

Hi inversed,

Thanks for testing. I think you must have another patch applied, because the options you are talking about relate to #1819074: Allow publish_on dates in the past not the patch in #39 above. If this is right, you can copy your comments above to that thread instead. Better to keep the feedback in the right issue ;-)

Thanks

Jonathan

jonathan1055’s picture

A few hunks did not apply in the previous patch after updating to 7.x-1.1+15 from 25th Sept, so here is a re-rolled patch. Also you'll like it because it is in the git diff a/ b/ format

angel.h’s picture

I did a review on the last patch and it looks good overall - just some coding standard and readability issues - i supplied interdiff so you can see only the changes applied by me and patch including my changes over the patch in #44.
For the new test - I would keep the general test in current class and put the new each big functionality in a new class - thus I would split this test into a new class (but it's not a biggie).

Status: Needs review » Needs work

The last submitted patch, 1069668-45.scheduler.default_time.patch, failed testing.

jonathan1055’s picture

Hi Angel,
Thanks for the review, that is very helpful. Yes, the layout of t() with the array() items on separate lines does make it more readable. Thanks for spotting the double spaces too. Coder Riview does not report them. The change to break

if (!$default_time = strptime($form_state['values']['scheduler_default_time'], '%H:%M:%S'))

into two lines, I am fine with (I did not code that original line, it was Pieter's)

I tried to apply your patch, and .module, .install and .test all patch ok. The one change to .info failed, and it looks like you have made the patch against the .info file which does not have the additional data lines added by the packaging script:

; Information added by drupal.org packaging script on 2013-09-25
version = "7.x-1.1+15-dev"
core = "7.x"
project = "scheduler"
datestamp = "1380103653" 

Maybe you used the code direct from the git repository? It's my understanding that we should be patching against the latest released dev, so that any users can test the patch more easily. Is this right?

Jonathan

angel.h’s picture

Status: Needs work » Needs review

Status: Needs review » Needs work

The last submitted patch, 1069668-45.scheduler.default_time.patch, failed testing.

angel.h’s picture

Status: Needs work » Needs review

Hello Jonathan,

Yes, I used the git repository to do the patch and the interdiff but I think this is a common approach. The

; Information added by drupal.org packaging script on 2013-09-25
version = "7.x-1.1+15-dev"
core = "7.x"
project = "scheduler"
datestamp = "1380103653" 

is added by Drupal automatically so it shouldn't be in my patch.

I requeued the patch for testing because I think there was a cache on the test dependancy.

Angel

angel.h’s picture

Test was failing because the patch did not apply - I saw there was a new commit just 40 minutes ago :). Here's the patch merged with the latest 7.x.

Status: Needs review » Needs work

The last submitted patch, 1069668-51.scheduler.default_time.patch, failed testing.

pfrenssen’s picture

Status: Needs review » Needs work

This is failing because the test bots have not picked up the new dependency on the date module for running the tests. I have had this situation before with another module. I believe it has to be merged into a release before it gets picked up.

jonathan1055’s picture

Yes, in that case can you commit just the patch to .info separately first? We know it is going to be needed. Then when that change is in the git repository the patch can be re-rolled without the .info change and it should run ok?

pfrenssen’s picture

Good plan, I'm going to try that.

pfrenssen’s picture

I have found the issue in which I had the same problem with adding a test dependency. I also tried to add the test dependency in a seperate commit, but this didn't seem to help. In the end I just took the risk of merging the branch into the master, and there are no comments following that so it seems to have been successful.

Ref. https://drupal.org/node/1342996#comment-7125402

jonathan1055’s picture

I read the comments in the linked issue. Are you saying that the test dependencies are not read from the .info file in the latest -dev git repository, but only from the most recent real release? That sounds odd. How about trying to go with the plan anyway. Patch the .info file, then wait until we are sure we have a new dev release before re-queing for testing. It is worth a try - you never know, that problem may have been fixed since the other issue back in March. There is nothing to lose and everything to gain and we may learn more about how the test bots operate.

pfrenssen’s picture

I was assuming that the dependencies were updated on dev releases but from my comment #16 in that issue it seems that I was mistaken. I don't know how it works. I was indeed planning on just committing this when it reaches RTBC, and if any problems arise I will look deeper into it. The code that is responsible for resolving the test dependencies is probably either in the Project Dependency or PIFR modules which are used by the test bots.

jonathan1055’s picture

Status: Needs work » Needs review

Your commit to add the test_dependency was in the dev release 1.1+22 on 11th Oct and there have been a couple more commits since then. I will re-queue the patch in #51 and see what happens.

jonathan1055’s picture

Status: Needs review » Needs work

The last submitted patch, 1069668-51.scheduler.default_time.patch, failed testing.

jonathan1055’s picture

Of course, the patch needs re-rolling because the .info change is already committed. I'll get on to that.

jonathan1055’s picture

Status: Needs work » Needs review
FileSize
20.11 KB

Re-rolled patch against 1.1+23 of 14th Oct
The change to .info adding test dependency is no longer in this patch. Also the patch to .test is slightly different (catering for the rename of $this->web_user to $this->admin_user)
No changes to .install and .module compared to patch in #51

jonathan1055’s picture

That's better. The test dependencies are taken from the latest dev release. Good. Maybe waiting a few days helped, or waiting until a further commit on top of the one we needed? I don't know what caching there is with the test bots, or how often they refresh - maybe in the previous problem in https://drupal.org/node/1342996#comment-7125402 the dev release was not ready. Anyway it works now, as we hoped.

This is essentially the same code as Angel reviewed in #45 so we could mark it RTBC? The tests as they stand do not actually cover all cases and error prevention which I had to code for in the changes in #32, #33 and #37. How comprehensive are we going to be with writing tests? Should we have that discussion over on #2109945: Reorganising and expanding the automated tests and let this issue get committed? I'd be happy with that.

Jonathan

jonathan1055’s picture

Status: Needs review » Reviewed & tested by the community
FileSize
20.2 KB

The patch needed re-rolling after 1.1+34. Marking this RTBC as the functionality has been tested by Angel and the code reviewed thoroughly by Pieter. Hope that's ok with you?

Jonathan

pfrenssen’s picture

Status: Reviewed & tested by the community » Fixed

Excellent, this has finally been committed in 249ece4, thanks everybody!!

I made a followup issue to split the test up in a separate file: #2122013: Separate tests that depend on the Date module, that is a minor thing and can be picked up when we are through the current "sprint" and start refactoring the tests.

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.

jonathan1055’s picture

Sorry to re-open, but just found a bug when using default time in conjunction with date-popup. If you do not want any time entry, and set the Scheduler date format to anything other than Y-m-d then you get the familiar message "value does not match the expected format of ...". For example, if I want d-m-Y not Y-m-d, this is what I get:

As you can see, the text value returned does match the requested format. I'll look at the code and see what is going on.

jonathan1055’s picture

When there is no time part to the Scheduler format _scheduler_strtotime() was using the value in variable 'scheduler_date_only_format' to extract the date. But date_popup returns the value in its own fixed format defined by constant DATE_FORMAT_DATETIME.

Luckily this is easy to fix, as I have already done the majority of the work in the patch on #2123103: Allow more popup formats. It will just take a couple of lines chaning in that patch. When done, we can commit that fix and close this issue again.

Jonathan

TXChetG’s picture

Ran into a small error after patching Scheduler.

Whenever I activate the Date Only section and leave the default time as SCHEDULER_DEFAULT_TIME, I am informed that my entry must be limited to 20 characters, and that my current entry is 22.

This is probably easily avoidable by just setting a default time following the format, but it felt off that the included variable didn't work.

jonathan1055’s picture

That's interesting. I do not see this behaviour, even after deleting the stored variable from the database and going to the page fresh. The default value is '00:00:00' which is only 8 characters. Can you just clarify some things:

  1. Is this warning in the Admin configuration page, or when editing a node?
  2. What is the exact text of the message?
  3. If you could give a screen-shot that would be very helpful
  4. What version of Scheduler are you running? A full release or a -dev version?
  5. What patches have you applied? Just the one in this issue, or others too?

Thanks for reporting this. I'm sure we can get it fixed.

Jonathan

jonathan1055’s picture

@TXChetG I've just read your post in #70 again, and are you saying that the text SCHEDULER_DEFAULT_TIME is shown in the entry field for the default time? That string is 22 characters long. This is an odd thing to happen. You say you had this error "after patching Scheduler" but the code changes for default time are now committed into the -dev release (since 28th Oct), so you should not need to patch it anymore. Tell us what you did and I'm sure we can fix it for you.

Jonathan

TXChetG’s picture

@jonathan1055 Thanks for taking a look. I had wanted to add more to my comment yesterday, but kept getting a 502 Error from Drupal when I would attempt to edit. So here's my update!

My issue may be caused by some patching failures. I'm still getting my feet wet here in Drupal. After applying the patch I received notice of several failed chunks. I wanted to include the contents of my .rej files, but Drupal seems to be unhappy with a comment that large. I could just try installing the dev version as you had mentioned.

TXChetG’s picture

@jonathan1055 Thanks for taking a look. I had wanted to add more to my comment yesterday, but kept getting a 502 Error from Drupal when I would attempt to edit. So here's my update!

My issue may be caused by some patching failures. I'm still getting my feet wet here in Drupal. After applying the patch I received notice of several failed chunks. I wanted to include the contents of my .rej files, but Drupal seems to be unhappy with a comment that large. I could just try installing the dev version as you had mentioned.

jonathan1055’s picture

The simplest solution is for you to install the latest -dev release, which has this functionality, with no patching required. Also look out for when #2123103: Allow more popup formats gets committed, as that contains a fix which improves the date_popup + default time functionality.

Just for future reference, we really should be having this discussion in a separate issue. It's better to keep different problems distinct from existing issue threads. But thank you for your interest in our module.

Jonathan

pfrenssen’s picture

Quick recap of the current status on this issue:

There is still a bug in this functionality. A form validation error occurs if the time part of the date format is omitted, Date popup is enabled and a default time is provided. If the time part is provided then the bug does not occur. For more information please see comment #68 of this issue.

The patch in #2123103: Allow more popup formats fixes this problem. This issue will remain open until that issue lands, which is expected to happen within a few days.

pfrenssen’s picture

Found a small bug in this: it is possible to provide a date format without a time part and omitting to set the default time. I created a new issue for this: #2143061: The 'date only' option must be turned on for a format with no time element.

jonathan1055’s picture

Now that #2123103: Allow more popup formats is committed, this issue can be returned to Fixed.

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.