Overview

Discovered #3509509: When adding single hero component Auto-save shows a change when there is not one

Steps to reproduce:

  1. Create a node
  2. Edit in XB
  3. Change the node title (probably any field in Page Data?)
  4. Open the review changes dialog
  5. Publish all changes
  6. Close the dialog
  7. Wait 10s
  8. See it report there is 1 new change even though you did nothing

Or

  1. Create a node
  2. Edit in XB
  3. Change the node title (probably any field in Page Data?)
  4. Wait 12 secs(to be cautious).
  5. Should see review 1 change
  6. Change the node title back to original
  7. Wait 12 secs(to be cautious).
  8. Should still see review 1 change, even though change has been reverted
  9. reload page
  10. Should still see review 1 change, even though change has been reverted

The problem seem to be that entity_form_fields changes in between the call to \Drupal\experience_builder\AutoSave\AutoSaveManager::recordInitialClientSideRepresentation and the call to \Drupal\experience_builder\AutoSave\AutoSaveManager::save both in ApiLayoutController even though there have been no changes in the UI

Proposed resolution

User interface changes

Issue fork experience_builder-3512017

Command icon Show commands

Start within a Git clone of the project using the version control instructions.

Or, if you do not have SSH keys set up on git.drupalcode.org:

About issue forks

Comments

tedbow created an issue. See original summary.

tedbow’s picture

tedbow
he/him
Primary language English
Location Ithaca, NY, USA
commented
Issue summary: View changes
tedbow’s picture

tedbow
he/him
Primary language English
Location Ithaca, NY, USA
commented

This seems like a similar problem space to #3506467: Make Media Library widget work from "page data" (content entity form) because that is changing on the way entity_form_fields is set

tedbow’s picture

tedbow
he/him
Primary language English
Location Ithaca, NY, USA
commented
Related issues: +#3506467: Make Media Library widget work from "page data" (content entity form)

tedbow opened merge request !784

tedbow’s picture

tedbow
he/him
Primary language English
Location Ithaca, NY, USA
commented
Issue summary: View changes

So far all I have done in the merge request is bring over my attempts in #3509509: When adding single hero component Auto-save shows a change when there is not one to try to make entity_form_fields not show a difference after entity is published and there no additional changes. That was before I split that problem in to this issue

The current changes do reduced number of changes in entity_form_fields but didn't eliminate them

tedbow’s picture

tedbow
he/him
Primary language English
Location Ithaca, NY, USA
commented
Status: Active » Needs work

I did some further manual testing.

it seems that with the current MR and the standard profile the parts of entity_form_fields that still change are field_tags and field_image, both entity reference fields.

So I

  1. Installed the standard profile and enabled xb_dev_standard
  2. deleted both the tags and images field from Article
  3. created an article
  4. opened it in XB
  5. changed the title
  6. published the change
  7. waited for 10 seconds to see if "review 1 change" showed

In the MR there doesn't result in "review 1 change" but 0.x does

In 0.x the difference between the call from AutoSaveManager::recordInitialClientSideRepresentation and AutoSaveManager::save is in save() there are 2 extra items under entity_form_fields ,advanced__active_tab and revision

larowlan’s picture

larowlan
Location 🇦🇺🏝.au GMT+10
commented
Related issues: +#3513317: ImageItem::defaultStorageSettings() should override display_default

#3513317: ImageItem::defaultStorageSettings() should override display_default is the core bug

wim leers’s picture

wim leers
Location Ghent 🇧🇪🇪🇺
commented
Issue tags: +Needs upstream bugfix

Reviewed & RTBC'd the core issue.

tedbow’s picture

tedbow
he/him
Primary language English
Location Ithaca, NY, USA
commented
Issue tags: +stable blocker, +Needs tests

Obviously we need tests because nothing is failing now because of this. Also should be stable blocker

I wonder if we want a more complex e2e test where we start with no media item select a media item and then remove it again. This should return to the start state and not show a change but I wonder if it actually would. Though I think that would be follow-up because getting that done with reliable(won't fail randomly) e2e test might tricky.

larowlan’s picture

larowlan
Location 🇦🇺🏝.au GMT+10
commented

Without the core fix or a workaround tests fail in the media library branch - exactly as we'd expect them to. So this may end up being solved by #3506467: Make Media Library widget work from "page data" (content entity form)

larowlan’s picture

larowlan
Location 🇦🇺🏝.au GMT+10
commented
Title: Changing a value in Page form then publishing, Auto-save shows a change when there is not one » [PP-1] Changing a value in Page form then publishing, Auto-save shows a change when there is not one
Status: Needs work » Postponed

Postponing on that issue

wim leers’s picture

wim leers
Location Ghent 🇧🇪🇪🇺
commented
Title: [PP-1] Changing a value in Page form then publishing, Auto-save shows a change when there is not one » Changing a value in Page form then publishing, Auto-save shows a change when there is not one
Status: Postponed » Needs work

#3506467: Make Media Library widget work from "page data" (content entity form) landed.

wim leers’s picture

wim leers
Location Ghent 🇧🇪🇪🇺
commented

FYI: thanks to @longwave, #3513317: ImageItem::defaultStorageSettings() should override display_default already landed in core, but it hasn't shipped in a release yet.

tedbow’s picture

tedbow
he/him
Primary language English
Location Ithaca, NY, USA
commented
Title: Changing a value in Page form then publishing, Auto-save shows a change when there is not one » [pp-1] Changing a value in Page form then publishing, Auto-save shows a change when there is not one
Status: Needs work » Postponed
Related issues: +#3515870: Node author changes to anonymous on publish

I think #3506467: Make Media Library widget work from "page data" (content entity form) caused #3515870: Node author changes to anonymous on publish. Think we need to postpone on that because it changes the entity fields and will always show a change after publish

wim leers’s picture

wim leers
Location Ghent 🇧🇪🇪🇺
commented
Title: [pp-1] Changing a value in Page form then publishing, Auto-save shows a change when there is not one » Changing a value in Page form then publishing, Auto-save shows a change when there is not one
Assigned: Unassigned » mayur-sose
Status: Postponed » Active

Is this still relevant?

wim leers’s picture

wim leers
Location Ghent 🇧🇪🇪🇺
commented
Issue tags: -stable blocker +beta blocker
Parent issue: » #3521002: [META] Maintainable client-side data model + internal HTTP API

If it is, this sounds like a beta blocker because it'd drive any user insane.

mayur-sose’s picture

mayur-sose commented

There are two scenarios mentioned in the issue description, but only one is currently reproducible. Below is the scenario that reproduces the issue:

Steps to reproduce:

  1. Create a node.
  2. Edit the node in Experience Builder (XB).
  3. Change the node title (or possibly any field in Page Data).
  4. Wait for approximately 12 seconds to ensure the auto-save is triggered.
  5. Observe that the "Review 1 change" prompt appears.
  6. Revert the node title back to its original value.
  7. Wait another 12 seconds.
  8. The "Review 1 change" prompt still appears, even though the change has been reverted.
  9. Reload the page.
  10. The "Review 1 change" prompt still persists despite no actual change being present.

This suggests that reverted changes are not being properly cleared from the change tracking logic.

mayur-sose’s picture

mayur-sose commented
Assigned: mayur-sose » Unassigned
penyaskito’s picture

penyaskito
he/him
Primary language Spanish
Location 🧑🏽‍🌾 Seville 💃 Andalusia, UTC+2 🇪🇺
commented

In that edge-case, I'd assume there is a change: the updated timestamp.
Not sure how we would like to handle it, but IMHO works as expected.

lauriii’s picture

lauriii
he/him
Primary language Finnish
Location Finland
commented
Issue tags: -beta blocker

I don't think this is a beta or stable blocker but would be a good to prioritize this with other non-blocking bugs.

wim leers’s picture

wim leers
Location Ghent 🇧🇪🇪🇺
commented
Status: Active » Postponed (maintainer needs more info)
Related issues: +#3529622: Make auto-save manager only support passing and returning entity objects, remove support for arbitrary data, use deterministic hashing to ensure entries are actually updates

Is this not a duplicate of #3529622: Make auto-save manager only support passing and returning entity objects, remove support for arbitrary data, use deterministic hashing to ensure entries are actually updates?

larowlan closed merge request !784

larowlan’s picture

larowlan
Location 🇦🇺🏝.au GMT+10
commented
Status: Postponed (maintainer needs more info) » Closed (duplicate)

Agreed