If the "unpublishing" option is disabled (scheduler_unpublish_enable_CONTENT_TYPE variable), it should not check if it's required (scheduler_unpublish_required_CONTENT_TYPE variable) when saving a node, as this will prevent saving altogether.

This is a bug in both 8.x and 7.x Scheduler

Steps to reproduce

  1. For a content type, tick both 'Enable scheduled unpublishing' and 'Require scheduled unpublishing'
  2. Save the content type
  3. Re-edit the content type and untick 'Enable scheduled unpublishing' and save
  4. Create a new content for this type, and try to save

This produces the error "The Unpublish on date is required". However, there is no Unpublish input field because the main option is not enabled.

The same problem happens with Publish On.

CommentFileSizeAuthor
#18 3084867-18.interdif-16-18.txt1.72 KBjonathan1055
#18 3084867-18.non-enabled-7x.patch9.08 KBjonathan1055
#16 3084867-16.interdif-14-16.txt1.03 KBjonathan1055
#16 3084867-16.non-enabled-7x-new-test-WILL-FAIL.patch7.36 KBjonathan1055
#14 3084867-14.non-enabled-7x-new-test-WILL-FAIL.patch7.26 KBjonathan1055
#12 3084867-12.interdif-10-12.txt1010 bytesjonathan1055
#12 3084867-12.non-enabled-8x.patch4.79 KBjonathan1055
#10 3084867-10.non-enabled-8x.patch4.79 KBjonathan1055
#10 3084867-10.interdiff-8-10.txt2.77 KBjonathan1055
#8 3084867-8.non-enabled-8x-test-WILL-FAIL.patch1.42 KBjonathan1055
#2 3084867-2-do_not_check_required_if_disabled_unpublished_on.patch822 bytesfjgarlin
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

fjgarlin created an issue. See original summary.

fjgarlin’s picture

fjgarlin CreditAttribution: fjgarlin as a volunteer and at Amazee Labs commented
Status: Active » Needs review
FileSize
3084867-2-do_not_check_required_if_disabled_unpublished_on.patch822 bytes

Attached patch to fix the issue.

jonathan1055’s picture

jonathan1055 CreditAttribution: jonathan1055 as a volunteer commented
Title: Do not check required unpublished on date if scheduling is disabled on node save » Do not check required unpublish_on date if scheduled unpublishing is disabled

Hi fjgarlin,

Thanks for reporting this and for the patch. I'm surprised we have not discovered this before. Maybe you only get into this situation if the content type had been enabled for unpublishing, then the required option was set, then the content type is disabled for unpublishing (but the required variable is still left on). An alternative solution would be to reset the required variable when disabling unpublishing.

If we change this, it would be good to have test coverage. Are you a developer who can write tests? If so we need to expand the current tests to prove the bug, then we can decide how best to fix it. Let me know if writing tests is not something you want to do, and I'll take that on.

Jonathan

fjgarlin’s picture

fjgarlin CreditAttribution: fjgarlin as a volunteer and at Amazee Labs commented

Hi Jonathan,

Yes that was the case. It was on before (both checkboxes), and then they only click on the enable checkbox, without unchecking the required.

Test wise, I don't have that much experience writing tests but it's mostly time what my issue is as I'm full time working on a few projects, so I don't want to commit to it and then not deliver. I'd be happier if you take it and I can help RTBC if needed.

Best,
Fran.

jonathan1055’s picture

jonathan1055 CreditAttribution: jonathan1055 as a volunteer commented
Version: 7.x-1.x-dev » 8.x-1.x-dev
Issue summary: View changes
Status: Needs review » Needs work

I have just checked and this same problem exists in Scheduler for D8 so we need to fix it there first.

jonathan1055’s picture

jonathan1055 CreditAttribution: jonathan1055 as a volunteer commented

This first commit re-writes the existing 8.x NonEnabledType test to use @dataProvider. This was a task to do anyway, and will make the additional test change easier.

  • jonathan1055 committed 56b776f on 8.x-1.x 
    Issue #3084867 by jonathan1055: Re-write testNonEnabledType() to use @...
jonathan1055’s picture

jonathan1055 CreditAttribution: jonathan1055 as a volunteer commented
Status: Needs work » Needs review
FileSize
3084867-8.non-enabled-8x-test-WILL-FAIL.patch1.42 KB

Here's an updated SchedulerNonEnabledTypeTest for 8.x which should fail, showing the fault in the existing code.

Status: Needs review » Needs work

The last submitted patch, 8: 3084867-8.non-enabled-8x-test-WILL-FAIL.patch, failed testing. View results

jonathan1055’s picture

jonathan1055 CreditAttribution: jonathan1055 as a volunteer commented
Title: Do not check required unpublish_on date if scheduled unpublishing is disabled » Do not check required (un)publish_on date if scheduled (un)publishing is disabled
Issue summary: View changes
Status: Needs work » Needs review
FileSize
3084867-10.interdiff-8-10.txt2.77 KB
3084867-10.non-enabled-8x.patch4.79 KB

As expected, the updated test failed for four out of the five test data scenarios. Here's a patch which contains the updated test plus corrrections to SchedulerUnpublishOnConstraintValidator which fix test cases 0 & 1 and scheduler.module which fix test cases 2 & 3.

Status: Needs review » Needs work

The last submitted patch, 10: 3084867-10.non-enabled-8x.patch, failed testing. View results

jonathan1055’s picture

jonathan1055 CreditAttribution: jonathan1055 as a volunteer commented
Issue summary: View changes
Status: Needs work » Needs review
FileSize
3084867-12.non-enabled-8x.patch4.79 KB
3084867-12.interdif-10-12.txt1010 bytes

Typo in the correction for SchedulerUnpublishOnConstraintValidator

  • jonathan1055 committed 1ed4d83 on 8.x-1.x 
    Issue #3084867 by jonathan1055, fjgarlin: Do not check required (un)...
jonathan1055’s picture

jonathan1055 CreditAttribution: jonathan1055 as a volunteer commented
Version: 8.x-1.x-dev » 7.x-1.x-dev
FileSize
3084867-14.non-enabled-7x-new-test-WILL-FAIL.patch7.26 KB

New test added for 7.x. This will fail.

Status: Needs review » Needs work

The last submitted patch, 14: 3084867-14.non-enabled-7x-new-test-WILL-FAIL.patch, failed testing. View results

jonathan1055’s picture

jonathan1055 CreditAttribution: jonathan1055 as a volunteer commented
Status: Needs work » Needs review
FileSize
3084867-16.non-enabled-7x-new-test-WILL-FAIL.patch7.36 KB
3084867-16.interdif-14-16.txt1.03 KB

The test output only showed exceptions, however in the xml there was better info about the test failures, as expected. This patch will fail but without the exceptions.

Status: Needs review » Needs work

The last submitted patch, 16: 3084867-16.non-enabled-7x-new-test-WILL-FAIL.patch, failed testing. View results

jonathan1055’s picture

jonathan1055 CreditAttribution: jonathan1055 as a volunteer commented
Status: Needs work » Needs review
FileSize
3084867-18.non-enabled-7x.patch9.08 KB
3084867-18.interdif-16-18.txt1.72 KB

Now with the code fixes. This also showed up another slight bug in that if a scheduled date was still set on a node after scheduling had been disabled, the node would still get processed during cron. Even though rare, this is wrong, so I have fixed that in .cron.inc, in addition to the fix provided by fjgarlin in patch #2

  • jonathan1055 committed 859947d on 7.x-1.x 
    Issue #3084867 by jonathan1055, fjgarlin: Do not check required (un)...
jonathan1055’s picture

jonathan1055 CreditAttribution: jonathan1055 as a volunteer commented
Status: Needs review » Fixed

Fixed and committed at 8.x and 7.x
Thank you @fjgarlin for noticing the fault in the first place. My focus has been on 8.x recently, but finally got round to fixing this.

Status: Fixed » Closed (fixed)

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