Problem/Motivation

Currently quickedit kicks on pages even when there is a forward revision. When content is updated on the page, the values from the default revision override the forward revision. The mental model that the content displayed on the page is always the content which should be changed is incorrect in this instance.

Screencast: https://www.youtube.com/watch?v=gvnXaT7HCeU

Proposed resolution

Only add quickedit when the content being rendered is the latest revision.

Remaining tasks

User interface changes

API changes

Data model changes

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

Sam152 created an issue. See original summary.

Sam152’s picture

Status: Active » Needs review
FileSize
3.57 KB

Kicking this off with an approach for fixing this.

It doesn't deny any access to the routes which do the updating, so if someone was determined the content could still be changed, but they'd be modifying something they have access to anyway.

Status: Needs review » Needs work

The last submitted patch, 2: 2903524-2.patch, failed testing. View results

samuel.mortenson’s picture

Status: Needs work » Needs review
samuel.mortenson’s picture

+++ b/core/modules/quickedit/quickedit.module
@@ -157,9 +158,40 @@ function quickedit_preprocess_field(&$variables) {
+  return $entity->getLoadedRevisionId() == array_keys($revision_ids)[0];

Will $revision_ids always have a length of at least one? If not, we should be checking if it's empty.

Test coverage looks great! Normally I would include a positive test case in the method but Quick Edit has fairly extensive testing already.

amateescu’s picture

Status: Needs review » Needs work
  1. +++ b/core/modules/quickedit/quickedit.module
    @@ -157,9 +158,40 @@ function quickedit_preprocess_field(&$variables) {
    + *   A boolean for if the loaded entity is the latest revision.
    

    We usually write boolean return descriptions like this:

    TRUE if the entity is the latest revision, FALSE otherwise.

  2. +++ b/core/modules/quickedit/quickedit.module
    @@ -157,9 +158,40 @@ function quickedit_preprocess_field(&$variables) {
    + * @todo Remove this method once better support for forward revisions is added
    + * to core https://www.drupal.org/node/2784201.
    
    +++ b/core/modules/quickedit/src/Tests/QuickEditLoadingTest.php
    @@ -318,6 +325,21 @@ public function testUserWithPermission() {
    +   * Test quickedit does not appear for entities with forward revisions.
    

    We call them pending revisions now :)

    And the doxygen sentence should start with: Tests that ...

  3. +++ b/core/modules/quickedit/quickedit.module
    @@ -157,9 +158,40 @@ function quickedit_preprocess_field(&$variables) {
    + * @internal
    + */
    +function quickedit_entity_is_latest_revision(ContentEntityInterface $entity) {
    

    Procedural functions that are @internal should start their name with an underscore.

  4. +++ b/core/modules/quickedit/src/Tests/QuickEditLoadingTest.php
    @@ -318,6 +325,21 @@ public function testUserWithPermission() {
    +    $this->drupalGet('node/1');
    ...
    +    $this->assertNoRaw('data-quickedit-entity-id="node/1"');
    

    Can we use $this->testNode->id() instead of hardcoding '1'?

    Also, the assertion is a negative one so it doesn't really mean anything if it's not paired with a positive one.

    So we need to somehow display two nodes, using the default revision for the first node and the latest revision for the second one and then we can have both a positive and a negative assertion for 'data-quickedit-entity-id'.

    Edit: Now I see this was also mentioned in #5, so feel free to leave it like that if you think it's not that important to have a positive assertion as well :)

timmillwood’s picture

Status: Needs work » Needs review
Issue tags: +Workflow Initiative
FileSize
2.83 KB
3.79 KB

This addresses everything in #6, including a simple solution for #6.5.

amateescu’s picture

Status: Needs review » Reviewed & tested by the community

That looks much better!

yoroy’s picture

Issue summary: View changes

Added link to the screencast that explains and shows the problem.

yoroy’s picture

This does "fix" things by making it impossible to do things. Preventing errors or unexpected results is good. Not letting people do things that in other places (content not under moderation) they can do is taking away control. This is not good. But I think this solution is the right trade-off to make. I'm also weighing here that from what we know from usability testing that quick edit is not very discoverable itself.

  • catch committed bf31e8c on 8.5.x
    Issue #2903524 by timmillwood, Sam152, amateescu, yoroy: Don't add...

  • catch committed c15b27a on 8.4.x
    Issue #2903524 by timmillwood, Sam152, amateescu, yoroy: Don't add...

catch credited catch.

catch’s picture

Version: 8.5.x-dev » 8.4.x-dev
Status: Reviewed & tested by the community » Fixed
Related issues: +#2815221: Add ability to use Quick Edit to the latest_revision route

This doesn't link back to #2815221: Add ability to use Quick Edit to the latest_revision route, adding it as a related issue.

I can definitely see quickedit working with workspaces (when you're in a workspace we let you edit any content with quickedit and it will be added to the set of content in that workspace, also it will show the draft revision linked to the workspace when viewing content so no mis-match). Locking things down for now though is good until we resolve those issues.

Committed/pushed to 8.5.x and cherry-picked to 8.4.x. Thanks!

Wim Leers’s picture

Issue tags: +Usability
+++ b/core/modules/quickedit/quickedit.module
@@ -157,9 +158,40 @@ function quickedit_preprocess_field(&$variables) {
-  if (!\Drupal::currentUser()->hasPermission('access in-place editing')) {
+  if (!\Drupal::currentUser()->hasPermission('access in-place editing') || !_quickedit_entity_is_latest_revision($entity)) {
...
+ * @todo Remove this method once better support for pending revisions is added
+ * to core https://www.drupal.org/node/2784201.
...
+ * @internal

+++ b/core/modules/quickedit/src/Tests/QuickEditLoadingTest.php
@@ -318,6 +325,24 @@ public function testUserWithPermission() {
+  public function testWithPendingRevision() {

A change in behavior using an @internal helper method with explicit integration test coverage and a @todo to improve it in the future.

As the module maintainer: thank you, this is perfect :)

Status: Fixed » Closed (fixed)

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

samuel.mortenson’s picture

FileSize
992 bytes

Can we commit this patch as a quick follow-up? The committed patch does not add the data-quickedit-entity-id attribute to non-latest entities, but always adds the data-quickedit-field-id attribute. This means that JS errors ("Quick Edit could not associate the rendered entity field markup...") will be thrown whenever viewing a default, non-latest entity. I'm posting this here and not in a new issue as it's basically the same code, just added to quickedit_preprocess_field as well.

Sam152’s picture

Can we assert data-quickedit-field-id is gone in the test case?

samuel.mortenson’s picture

@Sam152 Yes - we should do that. I'll open a new issue today so that the test bot can run.

samuel.mortenson’s picture