Problem/Motivation

There's a design flaw in the logic of this module, currently. It assumes that "state is published" and "transitioning to this state should make it the latest revision" are the same thing. That precludes an Archived state, in which you want an entity to have a new revision set as the default revision BUT that revision should be NOT PUBLISHED.

Proposed resolution

Add a second checkbox/property to the State definition for "make default". Both "Published" and "Archived" would check that. Technically others could too, but that's out of scope. :-)

Question: Should checking "published" also imply "make default"? I'm thinking yes.

Remaining tasks

Do it.

Members fund testing for the Drupal project. Drupal Association Learn more

Comments

Crell created an issue. See original summary.

larowlan’s picture

+1

Crell’s picture

Project: Moderation State » Workbench Moderation
Version: » 8.x-1.x-dev

  • becw committed 619449a on archived-states-2637278
    refs #2637278: Add a configurable 'Live revision' property to moderation...
becw’s picture

Assigned: Unassigned » becw

I've begun work on this issue in branch archived-states-2637278.

  • becw committed e5b3a71 on archived-states-2637278
    refs #2637278: separate updating the live revision from publishing the...
becw’s picture

I have working code for this in the archived-states-2637278, so if you'd like to review my approach, that would be cool. Patch and screenshot are attached.

However, this needs tests, and the existing d8 functionality for transitions that publish does not have tests, which means this should probably get a little extra testing attention.

  • becw committed 15d7d7a on archived-states-2637278
    refs #2637278: add default archived state.
    

  • becw committed 0a9eaa0 on archived-states-2637278
    refs #2637278: Begin moderation state entity tests.
    

  • becw committed 70ef451 on archived-states-2637278
    refs #2637278: all published states should also trigger updating the...
  • becw committed 933e744 on archived-states-2637278
    refs #2637278: Additional moderation state published/live_revision test...

  • becw committed b1a7e63 on archived-states-2637278
    refs #2637278: fix phpunit annotation syntax.
    

  • becw committed 6433b94 on archived-states-2637278
    refs #2637278: are ModerationState::save() and ModerationState::load()...
  • becw committed 703e297 on archived-states-2637278
    refs #2637278: test node transition from a published to unpublished+live...
becw’s picture

Here's an updated patch with tests. I have a bunch of questions about the testing, which I've included as @todo comments, and will of course remove before merging.

Status: Needs review » Needs work

The last submitted patch, 13: workbench_moderation-archived_states-2637278-13.patch, failed testing.

The last submitted patch, 13: workbench_moderation-archived_states-2637278-13.patch, failed testing.

The last submitted patch, 13: workbench_moderation-archived_states-2637278-13.patch, failed testing.

  • becw committed 033fa76 on archived-states-2637278
    refs #2637278: Begin moderation state entity tests.
    
  • becw committed 1f05243 on archived-states-2637278
    refs #2637278: add default archived state.
    
  • becw committed 2f2f3cd on archived-states-2637278
    refs #2637278: test node transition from a published to unpublished+live...
  • becw committed 5b8cebe on archived-states-2637278
    refs #2637278: all published states should also trigger updating the...
  • becw committed 5e4bccf on archived-states-2637278
    refs #2637278: Add a configurable 'Live revision' property to moderation...
  • becw committed 6dd22eb on archived-states-2637278
    refs #2637278: are ModerationState::save() and ModerationState::load()...
  • becw committed 8c443b2 on archived-states-2637278
    refs #2637278: fix phpunit annotation syntax.
    
  • becw committed dcc2f9d on archived-states-2637278
    refs #2637278: separate updating the live revision from publishing the...
  • becw committed e63736a on archived-states-2637278
    refs #2637278: Additional moderation state published/live_revision test...

  • becw committed 5f00d7d on archived-states-2637278
    refs #2637278: tweak simpletest to click the second edit link on the...
becw’s picture

The test was failing because I added a new default state, "Archived", that appears above "Draft" in the list of moderation states. Moderation states should probably have a weight, and not rely on the alpha order (which just happens to work for the draft/new revision/published states).

Updated patch attached.

Crell’s picture

  1. +++ b/config/schema/workbench_moderation.schema.yml
    @@ -11,6 +11,9 @@ workbench_moderation.moderation_state.*:
    +    live_revision:
    +      type: boolean
    +      label: 'Is live revision'
    

    The Drupal core language here is "default" revision, not live. (I prefer live too, but we should follow core's language.)

    (Unfortunately that change is going to hit just about every line in the patch. :-( )

  2. +++ b/src/Form/ModerationStateForm.php
    @@ -49,6 +49,18 @@ class ModerationStateForm extends EntityForm {
    +      // @todo When these are added, the checkbox default value does not apply properly.
    

    Hm. That's... weird. We don't have that problem with the other states usage. Anyone else know what's going on here?

  3. +++ b/src/Form/ModerationStateForm.php
    @@ -49,6 +49,18 @@ class ModerationStateForm extends EntityForm {
    +      // '#states' => array(
    +      //   'checked' => array(':input[name="published"]' => array('checked' => TRUE)),
    +      //   'disabled' => array(':input[name="published"]' => array('checked' => TRUE)),
    

    Please use short-array syntax globally.

  4. +++ b/tests/src/Kernel/EntityOperationsTest.php
    @@ -37,6 +38,9 @@ class EntityOperationsTest extends KernelTestBase {
    +    // @todo is this conflating testing the default config with testing the
    +    // state behaviors? Should we be creating states that are specific to the
    +    // test?
         $this->installConfig('workbench_moderation');
    

    Possibly. At the moment this does conflate the two, but I wasn't sure when writing it (vis, when dawhener was walking me through it) how to go about building test-only config. If it's straightforward enough to do, we should probably switch to test-only config snapshots.

  5. +++ b/tests/src/Kernel/EntityOperationsTest.php
    @@ -72,6 +76,11 @@ class EntityOperationsTest extends KernelTestBase {
         $page = Node::load($id);
         $this->assertEquals('A', $page->getTitle());
    +    // @todo Isn't the point of Node::load() that it loads the default
    +    // revision? If we're going to use Node::isDefaultRevision() in these
    +    // tests, shouldn't we specifically load our new revisions and verify that
    +    // they are not the default revision (if not the first draft) or are the
    +    // default revision (first draft, any published revision)?
         $this->assertTrue($page->isDefaultRevision());
         $this->assertFalse($page->isPublished());
    

    We do that later in the test. This block is verifying that Node::load() is returning the default revision, ie, that the presence of forward revisions isn't confusing the default load mechanism. (That is, that you won't inadvertently get a forward revision, which could be seen as a security hole.)

  6. +++ b/tests/src/Kernel/EntityOperationsTest.php
    @@ -147,7 +156,58 @@ class EntityOperationsTest extends KernelTestBase {
    +   * @todo This method is creating ModerationState entities that are specific to
    +   * the test. It is NOT creating transitions, which don't actually seem to be
    +   * validated when changing the moderation state internally (that might be ok,
    +   * is probably as designed, but I just wanted to check).
    

    Validating legal transitions on API changes is... yeah we should probably do that. The problem is that valid transitions are user-dependent, and in the future will, I hope, be Context API-dependent. That makes the potential API much more "interesting" as it needs at least a user object, and potentially an arbitrary set of context objects. I don't know how to do that. If you have any ideas about how we could build that in I am all ears.

    (This same concern comes up when dealing with scheduling, as discussed with Ted.)

  7. +++ b/tests/src/Kernel/EntityOperationsTest.php
    @@ -147,7 +156,58 @@ class EntityOperationsTest extends KernelTestBase {
    +  public function testLiveRevision() {
    

    This is a valid test, although it is testing a specific use case (archiving). It should probably be renamed to testArchive() or similar.

    a testDefaultRevision() method should fully exercise the various combinations of published/default_revision that can exist and make sure that they all work as expected. Which... could be a good dataProvider use case. :-)

  8. +++ b/tests/src/Kernel/EntityOperationsTest.php
    @@ -147,7 +156,58 @@ class EntityOperationsTest extends KernelTestBase {
    +      'published' => true,
    +      'live_revision' => true,
    

    Drupal coding standards are all-caps TRUE. (The rest of the PHP world uses all-lowercase. It's annoying and trips me up, too.)

  9. +++ b/tests/src/Kernel/ModerationStateEntityTest.php
    @@ -0,0 +1,90 @@
    +  public function testModerationStateCrud() {
    

    We can probably break this up using a dataProvider. It's just a stack of "create state, save, load, check values", with different params put in, so that's a perfect case for a dataProvider.

  10. +++ b/tests/src/Kernel/ModerationStateEntityTest.php
    @@ -0,0 +1,90 @@
    +    // @todo is it necessary to do ModerationState::load() every time? if this test is focused on the methods, rather than the properties themselves, maybe ::save and ::load are unnecessary?
    

    I'd say that in a Unit test, we wouldn't save/load at all and just test that the methods themselves work in isolation. However, those methods are trivial enough that there's nothing really that can go wrong there other than a typo.

    A Kernel test is for integration tests, which reasonably include integrating with the load/save routines of Entity API/Config API. That is, what's being tested here isn't so much set('published', TRUE) as that the entity is presented to Drupal correctly, which includes set('published' TRUE).

    So I'd say the save/load here is entirely appropriate.

  11. +++ b/tests/src/Kernel/ModerationStateEntityTest.php
    @@ -0,0 +1,90 @@
    +    // When we delete a moderation state, it should go away.
    +    // @todo this probably isn't necessary -- this case should be covered by
    +    //       regular entity tests, right? however, this test class name does
    +    //       say "crud"...
    +    $moderation_state->delete();
    +    $this->assertNull(ModerationState::load($moderation_state_id));
    

    I don't think it hurts anything to keep it.

  • becw committed b1c899e on archived-states-2637278
    refs #2637278: use 'default revision' instead of 'live revision'.
    
becw’s picture

RE note 7 in comment #20 above:

7.

+++ b/tests/src/Kernel/EntityOperationsTest.php
@@ -147,7 +156,58 @@ class EntityOperationsTest extends KernelTestBase {
+  public function testLiveRevision() {

This is a valid test, although it is testing a specific use case (archiving). It should probably be renamed to testArchive() or similar.

a testDefaultRevision() method should fully exercise the various combinations of published/default_revision that can exist and make sure that they all work as expected. Which... could be a good dataProvider use case. :-)

I was wondering if I should combine this with what is happening in EntityOperationsTest::testForwardRevisions, because the play of published/unpublished/default revision is common to them both.

  • becw committed 2b09ae5 on archived-states-2637278
    refs #2637278: remove todo questions.
    
  • becw committed 2c8e068 on archived-states-2637278
    refs #2637278: use a data provider to test the ModerationState::...
becw’s picture

I've updated the code style issues and the ModerationStateEntityTest tests, so I'm gonna let testbot review it tonight.

We should check in on the EntityOperationsTest, since I'd like to see if we can combine/consolidate the published/unpublished/default revision/forward revision testing, because all of those things interact with eachother.

Crell’s picture

+++ b/tests/src/Kernel/ModerationStateEntityTest.php
@@ -0,0 +1,104 @@
+  public function testModerationStateProperties($published, $default_revision, $is_published, $is_default) {
+    $moderation_state_id = $this->randomMachineName();
+    $moderation_state = ModerationState::create([
+      'id' => $moderation_state_id,
+      'label' => $this->randomString(),
+      'published' => $published,
+      'default_revision' => $default_revision,
+    ]);
+
+    $this->assertEquals($is_published, $moderation_state->isPublishedState());
+    $this->assertEquals($is_default, $moderation_state->isDefaultRevisionState());
+  }

Since there's no save happening here, we're only testing the create() method itself. This should have a save/load cycle in it.

Which would likely render the testModerationStateCrud() method redundant?

We should check in on the EntityOperationsTest, since I'd like to see if we can combine/consolidate the published/unpublished/default revision/forward revision testing, because all of those things interact with eachother.

Is that safe to consolidate? The tests here are verifying the manipulation of the state entity itself. EntityOperationsTest is verifying that the behavior applies to the entity being moderated. If anything, we likely need to add tests for block_content, and/or generalize the tests to work on arbitrary entity types (if that's even a thing).

becw’s picture

I figured that the ModerationStateEntityTest::testModerationStateProperties was testing the isPublishedState() and isDefaultRevisionState() methods. But I think I'll simplify the stuff in the ModerationStateEntityTest class after all.

And, re: EntityOperationsTest, I was referring to consolidating EntityOperationsTest::testForwardRevisions and EntityOperationsTest::testArchive -- which both test the node properties/state after applying a moderation state.

  • becw committed 1c9a790 on archived-states-2637278
    refs #2637278: remove redundant tests.
    
becw’s picture

Ok, per chat conversations, it sounds like I should make a followup branch for the consolidating EntityOperationsTest::testForwardRevisions and EntityOperationsTest::testArchive if I want to do that.

Given that, I consider this patch ready to go.

  • becw committed 1d61dbd on archived-states-2637278
    refs #2637278: add default archived state.
    
  • becw committed 227831c on archived-states-2637278
    refs #2637278: use a data provider to test the ModerationState::...
  • becw committed 656ae37 on archived-states-2637278
    refs #2637278: remove todo questions.
    
  • becw committed 77768e4 on archived-states-2637278
    refs #2637278: all published states should also trigger updating the...
  • becw committed 784c975 on archived-states-2637278
    refs #2637278: Begin moderation state entity tests.
    
  • becw committed 7de89c7 on archived-states-2637278
    refs #2637278: Additional moderation state published/live_revision test...
  • becw committed 8f8feb0 on archived-states-2637278
    refs #2637278: Add a configurable 'Live revision' property to moderation...
  • becw committed ab288d8 on archived-states-2637278
    refs #2637278: are ModerationState::save() and ModerationState::load()...
  • becw committed ad05c93 on archived-states-2637278
    refs #2637278: tweak simpletest to click the second edit link on the...
  • becw committed b24413f on archived-states-2637278
    refs #2637278: fix phpunit annotation syntax.
    
  • becw committed b2e6ee2 on archived-states-2637278
    refs #2637278: use 'default revision' instead of 'live revision'.
    
  • becw committed b5f25b2 on archived-states-2637278
    refs #2637278: remove redundant tests.
    
  • becw committed d306268 on archived-states-2637278
    refs #2637278: test node transition from a published to unpublished+live...
  • becw committed dc683d2 on archived-states-2637278
    refs #2637278: add more description text since we can't support this...
  • becw committed e808e79 on archived-states-2637278
    refs #2637278: separate updating the live revision from publishing the...
becw’s picture

becw’s picture

  • becw committed 1d61dbd on 8.x-1.x
    refs #2637278: add default archived state.
    
  • becw committed 227831c on 8.x-1.x
    refs #2637278: use a data provider to test the ModerationState::...
  • Crell committed 40e68b8 on 8.x-1.x authored by becw
    Issue #2637278 by becw: Add "Archived" state support
    
  • becw committed 656ae37 on 8.x-1.x
    refs #2637278: remove todo questions.
    
  • becw committed 77768e4 on 8.x-1.x
    refs #2637278: all published states should also trigger updating the...
  • becw committed 784c975 on 8.x-1.x
    refs #2637278: Begin moderation state entity tests.
    
  • becw committed 7de89c7 on 8.x-1.x
    refs #2637278: Additional moderation state published/live_revision test...
  • becw committed 8f8feb0 on 8.x-1.x
    refs #2637278: Add a configurable 'Live revision' property to moderation...
  • becw committed ab288d8 on 8.x-1.x
    refs #2637278: are ModerationState::save() and ModerationState::load()...
  • becw committed ad05c93 on 8.x-1.x
    refs #2637278: tweak simpletest to click the second edit link on the...
  • becw committed b24413f on 8.x-1.x
    refs #2637278: fix phpunit annotation syntax.
    
  • becw committed b2e6ee2 on 8.x-1.x
    refs #2637278: use 'default revision' instead of 'live revision'.
    
  • becw committed b5f25b2 on 8.x-1.x
    refs #2637278: remove redundant tests.
    
  • becw committed d306268 on 8.x-1.x
    refs #2637278: test node transition from a published to unpublished+live...
  • becw committed dc683d2 on 8.x-1.x
    refs #2637278: add more description text since we can't support this...
  • becw committed e808e79 on 8.x-1.x
    refs #2637278: separate updating the live revision from publishing the...
Crell’s picture

Assigned: Crell » becw
Status: Needs review » Fixed

I opened a follow-up for the form #state issue, which we're pretty sure is a core bug, and left a @see in the code: #2645614: Add form #state to force "make default" on when "published" is on for a state

And with that, merge it! Thanks Bec!

Status: Fixed » Closed (fixed)

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