Hey all,

First of, fantastic module, have been looking low and high for something like this for a long time, and it’s been here under my nose all the time.

The only thing that makes this not work perfectly for my scenario is that I would like to check “Publishing date/time is required” to remind users to assign a publishing date/time, but if they go and edit the node after it has been published, and the “Publish on” field has been nulled, they get an error message when they try to save their changes.

Ideally, the “Publishing date/time is required” should count if and only if the node is not published. If it is published, it should not be required.

Is there some way to get this effect right now that I am missing or is it a new feature that needs to be added?

- Egil

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

egil’s picture

Title: Editing a published node with "Publishing date/time is required" forces users to set a new date/time » Editing a published node with "Publishing date/time is required" forces users to set a new "Publish on" date/time
jonathan1055’s picture

Hi Egil,
Yes I have just tried this and you are right. I'm not sure that there is anything you can do to avoid the error.

Eric, would the correct work-flow be that the 'required' testing is only done when adding a node and not when editing an existing one? Or would it be, as Egil suggests, only check if the node is not already published? Currently the behaviour is that a publishing date is checked when you create a node, even if you tick the published checkbox, which implies that the setting for 'publish on required' actually means 'you have to schedule it, you cannot publish immediately'. Is that what you intended when you added the option?

Jonathan

greta_drupal’s picture

But, you must take into consideration that a user might want to publish (or unpublish) a scheduled node immediately.

I don't mind it forcing the user to check the pub dates upon editing a published or unpublished node, but you must be able to set a time just a few minutes ahead -- to in affect cause an immediate change in published status.

I am trying to test a content type with required publishing schedule, and I continue to receive the "must publish in future" error. I tried to set the publish-on date only a few minutes ahead. And, my local time, Drupal time, and server time are all in sync. I had to change the hour to get it to finally accept.

Using Drupal 6.20 / Scheduler 6.x-1.8

shinz83’s picture

I had this issue too. The way to make it work is to remove the "required" option of scheduler on the content type setup.

In my case, I had a scheduler set up for a blog entry. When I needed to make a small punctuation change, it was forcing me to reschedule it.

On D7, the option to uncheck is called "Publishing date/time is required" and it located under the "Publishing Settings."

greta_drupal’s picture

Indeed, that works for testing, but not in the wild. Content types, such as events, you definitely want to require and unpublish date.

jonathan1055’s picture

Thanks for your comments in #3-#5 but lets not get distracted from the original issue. The problem is that when editing a node which has already been published, the code re-tests for a scheduling date, and this does not make sense and causes trouble. We really need to decide what the actual aim of the 'publishing date/time required' option is - see my question in #2. There are plenty of ways to fix this, and I'm happy to look into the code and see what can be done. However, I after thinking about it a bit, I am struggling to understand exactly what this option is for. If we have an answer then I'll fix the code. If there is no answer for why the option is there, and no case for its use, then the solution may be to remove it.

Note, I am only talking about the 'publishing date required' option. I can perfectly see the reason for the 'unpublishing date required' option.

Jonathan

greta_drupal’s picture

I see the publish-on option as a good one. I use it regularly for events and announcements. Gives the content creator the opportunity to go ahead and create content that wouldn't be ripe to publish at the time of creation. Perhaps it rarely needs to be a required field, which is already at the control of content type admins. But, I can see scenarios where it would.

Let's say that you have staff that enters events/announcements/news-releases for days/weeks/months in advance -- sales events, price increases, concert dates, etc. It is possible that the announcement timing for some/all of the events it important. If the publish-on date was not a requirement, a careless staffer could inadvertantly leak an event by not setting a publish-on date -- thereby publishing to the default 'upon creation'. The alternative for the content creator would be to set the default for all events/announcements as unpublished, which takes away the automation of Scheduler.

But, I agree that as a required field, it needs a solution for editing existing published content. As I have suggested, even simply allowing the user to set the publish-on time by the minute, so that it can resume display same day.

Eric-Alexander Schaefer’s picture

IIRC the option was requested for forcing the author to supply a scheduling date.

This problem only occurs if a existing node is edited. Would it be sufficient to check if the node was already published or are there any other scenarios?

jonathan1055’s picture

Hi all,
I can think of two scenarios where the admin may have turned on the 'publishing date required' option:

  1. To remind the author to set a date otherwise the content will not get published - because the default status is 0 and they may not have access to change the actual status directly
  2. To force the author to think about the date for publishing and not leak something by accidentally publishing the content immediately - maybe the default status is 1

Both of these situations only really apply when first creating the content. If the node already exists and is being editted then it would be wrong to force a new 'publish on' date. Obviously if the node is currently pubished we should not force the check, but even if the node is not currently published the check should not be done (eg the node may have been unpublished for a reason and is being prepared for possible re-publication but no date is applicable yet).

So I would say we only want to do the the test for 'publishing on date required' when the node is being created, and never when it is subsequently editted.

Jonathan

greta_drupal’s picture

The drawback to having the check only for creating a node would be where a user with publishing-options editing privileges accidentally deletes/changes the publish date, and then are _not_ prompted to complete that emptied field. Probably a small-case scenario. But, the app simply needs to confirm that the publish field is _not_ NULL, if "publish on date required" has been specified. (Doesn't even need to verify the date itself.) So, when editing the node, the original date as-is passes the test fine.

[corrections]

jonathan1055’s picture

Hi Greta,

accidentally deletes/changes the unpublish date ...

I was only talking about the 'publish on' check being limited to the node creation. This was the original problem. I know they can seem linked but we are not doing anything with the 'unpublish on required' checks in this thread.

Jonathan

greta_drupal’s picture

Actually meant "publish" rather than "unpublish". Either way, same issue/drawback.

VortexCentrum’s picture

Yes, there is a problem when an already published node is edited. In a production environment, it is not feasible to have to go to the settings, change the "publish on required" flag for a content type, re-save the amended page and then go back to settings and reset the flag. In any case, most content writers do not have permissions enabling access to the settings which means that an article cannot be saved without the intervention of an editor or administrator.

Also, the module needs to allow immediate publication. A check box requiring the writer to select publish now or schedule for publication is essential in a production environment.

As it stands, we are going to have to remove the module.

Which is a shame because scheduling auto-releases would make our lives a whole lot easier!

akosipax’s picture

Version: 7.x-1.0 » 6.x-1.8

How about check status -

if published, no need to require
if unpublished, (which also includes creating new nodes) require the field.

Eric-Alexander Schaefer’s picture

I think the best option is to require publish_on only if the node is currently not published.
What about unpublish_on? Same thing?

Eric-Alexander Schaefer’s picture

Status: Active » Needs review
FileSize
673 bytes

simple patch for publish_on

Eric-Alexander Schaefer’s picture

Version: 6.x-1.8 » 7.x-1.x-dev

Patch was for D7 btw. We will backport this to D6 when the issue is resolved.

idflood’s picture

Status: Needs review » Reviewed & tested by the community

Patch in #16 works great and looks good. Thanks Eric!

rickmanelius’s picture

Status: Reviewed & tested by the community » Needs work

I think we need to handle both cases (required publish and required unpublish). Therefore I'll set to needs work. The patch in #16 will need another small adjustment for that case and we can set this to RBTC and commit.

rickmanelius’s picture

Status: Needs work » Fixed

This was already RBTC'd in #18. And the adjustment for the other case was trivial, so I committed it here.
http://drupalcode.org/project/scheduler.git/commit/2e2f3b3

We can re-open if the additional code doesn't work as expected.

jonathan1055’s picture

Status: Fixed » Active

Hi Rick,

I'm not sure that the change you made for unpublishing_required is exactly what we want. See the discussion above, particularly #2, 8, 9, 11.
We need to think about what we are trying to achieve with these options. With your new setting of unpublishing_required an author who is creating new content will never be forced to set the unpublish date, and I guess that is the scenario where it is most commonly needed. The checking will be bypassed until the node is published. If the node is never revisited then the unpublish date will never be forced to be set, even though the 'required' option is turned on by scheduler admins.

Jonathan
[edit: apology for setting this to active. Hope that is the right thing to do in this case]

jonathan1055’s picture

rickmanelius’s picture

Hi Jonathan,

I see your point. Let's try to break this down...

In the case of an unpublished node, the typical use case is scheduling a date to publish and then (optionally) unpublish the node. Let's call this (A)
In the case of a published node, one only really wants to schedule a future date to unpublish the node. However there may be a situation where one wants to move a node to an unpublished and then a published state (unlikely, but a possible use case). Let's call this (B)

In (A) it's ok to set an unpublish date because there is a built in assumption that one would set it after a publish date. Requiring a user to do this is not a big deal, particularly for pieces of content that have a shelf life. In (B), requiring a publish date on an already published node really doesn't make sense ever because it's not typical to unpublish and re-publish the node on a scheduled date/time.

Therefore, I will revert the additional component of commit #20 and stick to just #16. Sound good?

-Rick

jonathan1055’s picture

Hi Rick,
Yes, definitely revert the additional change you did in #20. But I am not convinced by the change made in #16 either.
If you look at my comment in #9 where I was working out the scenarios where the option 'publishing date required' would be used, they are both during the creation of the node. The change in #16 forces the user to re-enter a schedule date if the node is edited and unpublished. This could be a problem for example where a node was published but has since been unpublished (because the content was not ready for public viewing or the author had second thoughts about it and wanted to save it to review later). Forcing a new date to be entered each time the node is edited I think will just cause more hassle for the users - see the comment in #13

I suggest we find a simple way in scheduler_form_alter() to check if the node being edited is new (being created) or old and only do the 'required' test during creation. Checking the existence of $form['#node']->nid might be a way to detect this?

Jonathan

pfrenssen’s picture

#1472190: Valid date required for 'publish on' has been marked as a duplicate of this issue.

pfrenssen’s picture

In the case of an unpublished node, the typical use case is scheduling a date to publish and then (optionally) unpublish the node. Let's call this (A)
In the case of a published node, one only really wants to schedule a future date to unpublish the node. However there may be a situation where one wants to move a node to an unpublished and then a published state (unlikely, but a possible use case). Let's call this (B)

I would disregard option B in the scope of this issue. Repeated publishing / unpublishing is a use case, but to solve this cleanly you would need to implement a multivalue field to store all future publishing and unpublishing dates. That sounds like something for a different issue :)

jonathan1055’s picture

Version: 7.x-1.x-dev » 7.x-1.1
Status: Active » Needs review
FileSize
1.33 KB

The changes above do not cater for all the situations properly. There are two basic scenarios to consider - creating a new node and editing an existing node. Other attributes will also affect the desired setting of the 'required' flags.

Publishing Required

The current definition is:

$publishing_required = variable_get('scheduler_publish_required_' . $form['type']['#value'], 0) == 1 && $node->status == 0;

Publishing Required - creating a new node

Test Operation Default node status Publish-on date
should be required?
Current behaviour
1 Create a new node unpublished Yes Required. Correct
2 Create a new node published Yes Not required - this is wrong

When creating a new node and 'publishing required' is turned on for the content type, the user should have enter a date regardless of the default status of the content type and regardless of whether they have access to the 'publishing options' tab during node edit. There is currently a problem when the content type default status is 1 for published because the 'required' attribute does not get set (test 2).

Publishing Required - editing an existing node

Test Operation Node status Node is scheduled
for publishing
Publish-on date
should be required?
Current behavior
3 Edit existing node published - No Not required. Correct
4 Edit existing node unpublished Yes Yes Required. Correct
5 Edit existing node unpublished No No Required - this is wrong

Currently if the existing node is unpublished the user has to enter (or re-enter) a publishing date in all cases. This is wrong. The date should only be required if the node is still due to be published by scheduler. When the node has been published and subsequently unpublished we should not set the 'required' flag and force the node to be re-published (test 5).

Using empty($node->nid) to tell us whether the node is being newly created or not, the above conditions can be fixed by changing the definition to:

$publishing_required = variable_get('scheduler_publish_required_' . $form['type']['#value'], 0) == 1
  && (empty($node->nid) || ($node->status == 0 && !empty($node->publish_on)));


Unpublishing Required

The current definition is:

$unpublishing_required = variable_get('scheduler_unpublish_required_' . $form['type']['#value'], 0) == 1 && $node->status == 1;

Unpublishing Required - creating a new node

Test Operation Default node status Unpublish-on date
should be required?
Current behaviour
6 Create a new node unpublished Yes Not Required - this is wrong
7 Create a new node published Yes Required. Correct

The definition of $unpublishing_required was changed by adding 'and status=1'. This did partly solve the problem, but for new nodes in the process of being created $node->status may or may not be 1 depending on the content type's default setting. If the default is 0 (for unpublished) then $unpublishing_required does not get set when it should be (test 6).

Unpublishing Required - editing an existing node

Test Operation Node status Node is scheduled
for publishing
Unpublish-on date
should be required?
Current behavior
8 Edit existing node published - Yes Required. Correct
9 Edit existing node unpublished Yes Yes Not required - this is wrong
10 Edit existing node unpublished No No Not required. Correct

If the node does have a scheduled publishing date (ie the scheduled publishing time has not been reached yet) then we should still require the unpublishing date and the user should not be able to erase it (test 9).

The above conditions can be fixed by changing the definition to:

      $unpublishing_required = variable_get('scheduler_unpublish_required_' . $form['type']['#value'], 0) == 1
        && (empty($node->nid) || $node->status == 1 || !empty($node->publish_on));

Attached is a patch against 1.1+10 dev of 27th July.

Jonathan

jonathan1055’s picture

The patch in #27 still applies ok (with an offset) to 7.x-1.1+15 dev of 25th Sept.

jonathan1055’s picture

Version: 7.x-1.1 » 7.x-1.x-dev
FileSize
1.39 KB

Here is a patch in a/ b/ format, so that automated testing will run. This patch is against 1.1+19 and does not conflict with the .module patch in #1566024: Coding Standards and Coder Review so they can be applied and tested simultaneously.

pfrenssen’s picture

Very interesting to see the manual test results in #27. I will convert these scenarios in automated tests.

Assigning to me for writing the tests. I'm leaving the issue to "Needs review" for the moment, the patch in #29 still needs a review.

goldlilys’s picture

Tried #29 and it works well for me. Thank you so much.

pfrenssen’s picture

I created a test that covers all the test scenarios from #27. I found that the current dev release also fails when creating a new unpublished node with required publishing enabled (test case 1 from comment#27). So this seems to have been broken in the meanwhile.

The patch from #29 solves all issues. I have also reviewed it and it looks good. If this test comes back successful this is RTBC for me. I would welcome someone to review my test.

I have created a branch for this issue in the git repo that contains the patch from #29 and my test.

jonathan1055’s picture

Thanks very much. I'll review your new tests.

jonathan1055’s picture

I was also surprised that your automated tests showed test case #1 to be failing, and I rechecked the behavior. It definitely works as expected when you do those operations manually. I then looked at the rest of the tests and matched them up with the test case numbers and found another difference: Test case #6 I said gives the wrong behaviour but in the automated testing it was passing. Something was not quite right.

I matched up the coded tests with the case numbers 1-10 used in #27 and added 'id' into the $test_case array to hold this I also created a string for use in the title which displays all the options being used. This was very helpful when viewing the generated html test output.

My manual testing was giving fails for cases 2, 5, 6 and 9 but the automated testing was giving fails for 1, 2, 5 and 9. This allowed me to see that the tests with discrepancies to the current behaviour of dev (without the patch) are where the default node status is 'published'. I realised that when creating a node you cannot just specify the status in $edit, it also needs to be set in the node_page_options variable. I made this addition:

      // Set the default node status, used when creating a new node.
      $node_options_page = !empty($test_case['status']) ? array('status') : array();
      variable_set('node_options_page', $node_options_page);

and this gives exactly the same behaviour as the manual testing, ie cases 2, 5, 6 and 9 fail.


Incidentally test cases 1 & 2 were swapped round in position in the file but I decided to leave them in the same place to make the interdiff easier to interpret. Likewise cases 6 & 7 are in the wrong sequence. We can move these to the right order next time. Moving those lines now just makes it harder to see the actual differences. Nothing in the test_case values has changed apart from adding the case number.

If you agree with my changes to the tests, I will re-roll the full patch.

Status: Needs review » Needs work

The last submitted patch, 1198788_34.required_flags_test_only.patch, failed testing.

pfrenssen’s picture

Ah that explains why the results were different!

In the end it should not matter how the actual publication status of an individual node is set (either by checking the checkbox, or by manipulating the default node status) as long as the end result is correct. I assume that if you run your version with the patch applied the test is also green?

If you don't mind I'd prefer to keep my version, setting the node status by clicking the checkbox in the node form is a bit cleaner than manipulating the default node status variable. It also better matches the typical way an administrator works with nodes.

I like your assert messages but it is not really necessary to generate pretty assert messages in tests, especially if a simpler message could save us from using regexes :) In the end what matters if a test fails is that you have a test name, a line number and an assert message to know what went wrong.

jonathan1055’s picture

Hi Pieter, Thanks for your feedback.

In the end it should not matter how the actual publication status of an individual node is set (either by checking the checkbox, or by manipulating the default node status) as long as the end result is correct.

Maybe you have not realised the significance of the default node status as used in these tests. As you know, the #required attribute is set during the form build process, before the user gets to make any changes to the checkboxes, and it is only the #required values which we need to check in these tests. The change I made setting the default status was not so as to affect the final publishing status of the saved node, it was to emulate the environmental variables which will exist when the node form is generated. It is this default status which is available in hook_form_alter as $node->status. This is precisely the reason why the current .module code has the unwanted behaviour that the patch is fixing. The previous changes ignored the cases of creating a new node and simply used $node->status without realising how it would affect the #required setting.

If you leave the tests like they are, then sure they are all green now, but we are leaving loopholes open which could give false-positive results in the future, if the derivations had to be changed again. Indeed I could write a derivation for #required which passes your current test case #6 but which gives the unwanted behaviour we are currently trying to fix.

In fact I realised that in the actual test operations we do not need to have the 'status' field at all. We are only interested in what happens when the dates are left blank. This simplifies the coding because the same $edit array can be used regardless of whether we are creating a new node or editing an existing one. So

      // Only fill in the title and status when creating a new node. When we are
      // editing an existing node these fields are already filled in.
      $edit = $test_case['operation'] == 'add' ? array(
        'title' => $title,
        'status' => $test_case['status'],
      ) : array();

      // Make sure the publication date fields are empty so we can check if they
      // throw form validation errors when they are required.
      $edit += array(
        'publish_on' => '',
        'unpublish_on' => '',
      );

can become

      // Make sure the publication date fields are empty so we can check if they
      // throw form validation errors when they are required.
      $edit = array(
        'title' => $title,
        'publish_on' => '',
        'unpublish_on' => '',
      );
I like your assert messages but it is not really necessary to generate pretty assert messages in tests, especially if a simpler message could save us from using regexes :) In the end what matters if a test fails is that you have a test name, a line number and an assert message to know what went wrong.

OK I take your point about regex. But it is the complexity of ten test cases having quite similar test messages which was making my checking and debugging more tiresome. There are only two different assert line numbers in use here, each used five times because of the loop, and trying to work out which test was which is tedious. Having the id number at the start makes it so much easier. In the automated testing on Drupal you do not even get the context of the results, only the tests which fail, so you cannot even count down to see which test you are looking at. See the test results from #34 (shown below) for how easy it was to prove which tests they are, when the id is shown.

I've re-rolled the patch without the print_r and regexes, it's easy to achieve the same output using plain easy-to-read php. Given that this entire issue has taken so long to resolve, due to the complexity of the scenarios, anything which assists us to understand what is happening should be used. It took a while to find out why the tests did not match the manual behavior, but now I feel we have really made progress and sorted this out.

I hope you will accept this revised patch.

Jonathan

jonathan1055’s picture

Issue summary: View changes

[this comment was auto-generated during Drupal.org D7 upgrade]

jonathan1055’s picture

Patch file did not apply after dev 1.1+38 so re-rolled. The content is identical to the patch in #37. Hopefully this can be committed soon, as it does cause a hindrance when the required markers do not operate as they should.

pfrenssen’s picture

Priority: Normal » Major

Raising priority.

jonathan1055’s picture

I'm happy to mark this RTBC if you are ok too?

pfrenssen’s picture

I finally managed to review this, sorry for keeping you waiting!

I have only two remarks, but these are both minor things and are not blockers for me.

     // To assist viewing and analysing the generated test result pages create
      // a text string showing all the test case parameters.
      $title_data = array();
      foreach ($test_case as $key => $value) {
        if ($key != 'message') {
          $title_data[] = $key . ' = ' . $value;
        }
      }
      $title = implode(', ', $title_data);

I feel that this is unnecessary. The tests screenshots are not really intended to be viewed, so this is a bit pointless. The assert messages give enough detail to immediately know which test failed. I prefer a simple $title = $this->randomString().

        'id' => 1,
        'required' => 'publish',
        'operation' => 'add',
        'status' => 0,
        'expected' => 'required',
        'message' => 'When scheduled publishing is required and a new unpublished node is created, entering a date in the publish on field is required.',
      ),

Likewise for the ids. There is full documentation in the test itself, so there is little need to refer to this issue.

If you think these things are valuable and would prefer to keep them, then that's OK for me.

jonathan1055’s picture

I think given that they are in the patch I would like them to stay in. It took a lot of working out to discover which of the tests were behaving differently from the manual test results, and the titles and ids were invaluable. If we ever have to change the definitions of $required in the future then they will be useful again.

But hopefully we have the definitions right now, and we will not have to revisit this topic. It will be good to see this issue fixed and green :-)

pfrenssen’s picture

Status: Needs review » Fixed

Alright! I've committed it to 7.x-1.x: 4d0207d. Happy to see this fixed. Thanks all!

jonathan1055’s picture

Excellent!

Status: Fixed » Closed (fixed)

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