Previewing translatable nodes can lead to data loss

Created on 6 September 2017, almost 8 years ago
Updated 5 September 2024, 10 months ago

Problem/Motivation

When a node is translatable, it's possible you have two edit screens open from the same node, but in a different language.
The tempstore has no clue about the language, so hitting preview might overwrite the node stored in tempstore.

Steps to reproduce

- Open two languages for the same node in two tabs
- http://localhost/en/node/1/edit
- http://localhost/fr/node/1/edit
- press preview on the french one, then preview in the english one
- Press back to editing in the french one, you are now editing in English

Borderline critical as there is data loss involved here.

Proposed resolution

Remaining tasks

User interface changes

Introduced terminology

API changes

Data model changes

Release notes snippet

๐Ÿ› Bug report
Status

Needs work

Version

11.0 ๐Ÿ”ฅ

Component
Node systemย  โ†’

Last updated about 6 hours ago

No maintainer
Created by

๐Ÿ‡ง๐Ÿ‡ชBelgium swentel

Live updates comments and jobs are added and updated live.
  • Needs tests

    The change is currently missing an automated test that fails when run with the original code, and succeeds when the bug has been fixed.

Sign in to follow issues

Merge Requests

Comments & Activities

Not all content is available!

It's likely this issue predates Contrib.social: some issue and comment data are missing.

  • ๐Ÿ‡ณ๐Ÿ‡ฟNew Zealand quietone

    Tested on Drupal 11.x, umami install. I was able to reproduce the problem.

  • ๐Ÿ‡ฎ๐Ÿ‡ณIndia Sahana _N

    sahana _n โ†’ made their first commit to this issueโ€™s fork.

  • Status changed to Needs review 10 months ago
  • ๐Ÿ‡ฎ๐Ÿ‡ณIndia Sahana _N

    Hi,

    I am to reproduce the issue in 11.x I created an MR please review it.

    Here, the โ€œBack to content editโ€ URL redirects to the default language edit URL. I updated that

    I attached the screenshot for reference.

    I am happy to accept more suggestions for improvement.

    Thank you!!

  • Pipeline finished with Success
    10 months ago
    Total: 423s
    #274603
  • Status changed to Needs work 10 months ago
  • ๐Ÿ‡บ๐Ÿ‡ธUnited States smustgrave

    Was previously tagged for tests which are still needed

    Issue summary is also incomplete.

  • Status changed to Needs review 10 months ago
  • ๐Ÿ‡ฎ๐Ÿ‡ณIndia Sahana _N

    Updated the MR with tests please review it.
    Thanks!!

  • Pipeline finished with Failed
    10 months ago
    Total: 154s
    #278138
  • Status changed to Needs work 10 months ago
  • ๐Ÿ‡บ๐Ÿ‡ธUnited States smustgrave

    Please see all tags before moving to review, only delays things more.

  • First commit to issue fork.
  • ๐Ÿ‡ฆ๐Ÿ‡บAustralia acbramley

    I've started a new branch here because the existing MR did not fix the issue for me and was quite out of date.

    The issue is that the temp store uses the node UUID as the key, and is loaded verbatim regardless of language.

    Changing the language on the back link won't fix that, in fact the back link already contains the correct langcode in my testing.

  • ๐Ÿ‡ฆ๐Ÿ‡บAustralia acbramley

    acbramley โ†’ changed the visibility of the branch 11.x to hidden.

  • ๐Ÿ‡ฆ๐Ÿ‡บAustralia acbramley
  • Pipeline finished with Failed
    16 days ago
    Total: 639s
    #524954
  • ๐Ÿ‡ฆ๐Ÿ‡บAustralia acbramley

    acbramley โ†’ changed the visibility of the branch 2907091-previewing-translatable-nodes to hidden.

  • Pipeline finished with Failed
    15 days ago
    #525782
  • Pipeline finished with Success
    15 days ago
    Total: 496s
    #525802
  • Pipeline finished with Running
    15 days ago
    #525806
  • ๐Ÿ‡ฆ๐Ÿ‡บAustralia larowlan ๐Ÿ‡ฆ๐Ÿ‡บ๐Ÿ.au GMT+10
  • ๐Ÿ‡ฆ๐Ÿ‡บAustralia acbramley

    Crediting @larowlan for helping debug the language prefix issue with toUrl().

    This is ready for a review, I'll create a CR once we agree this is a decent solution.

  • ๐Ÿ‡ฌ๐Ÿ‡งUnited Kingdom oily Greater London

    Nitty rewording of one code comment. Run test-only test.

  • Pipeline finished with Success
    15 days ago
    Total: 379s
    #525981
  • ๐Ÿ‡ฌ๐Ÿ‡งUnited Kingdom oily Greater London

    Added further suggestion to review. My review is based on my understanding that the sequence in the test is as follows:

    1. Edit same node in two tabs, one is english translation, the other spanish
    2. Change the title in one of the tabs eg english translation
    3. Preview the spanish version of the node in other tab
    4. Check whether the spanish version title has been changed to the new title in the english version
  • ๐Ÿ‡ฌ๐Ÿ‡งUnited Kingdom oily Greater London

    Test-only test fails as it should. Tests are green.

  • ๐Ÿ‡ฌ๐Ÿ‡งUnited Kingdom oily Greater London

    Rebase.

  • ๐Ÿ‡ฌ๐Ÿ‡งUnited Kingdom oily Greater London

    Edit test comment for greater detail/ clarity.

  • Pipeline finished with Failed
    13 days ago
    Total: 616s
    #528130
  • ๐Ÿ‡ฆ๐Ÿ‡บAustralia acbramley

    Hi @oily

    Thank you for trying to improve the documentation but it now exceeds the 80 character line length and I don't think the extra words actually clarify it any better. "a node" implies it is a single node, and we are not opening separate browser tabs as that's not possible. We are simulating a similar scenario to what happens in a browser by previewing 2 translations in a row and then going back to the edit form of the first translation. IMO this is all explained clearly in the current doc block and inline comments.

  • ๐Ÿ‡ฌ๐Ÿ‡งUnited Kingdom oily Greater London

    @acbramley Okay 'opening separate browser tabs' might be I was thinking of a nightwatch test. I'll have another review of the latest. My suggestions were nitty.

  • Pipeline finished with Success
    11 days ago
    Total: 2800s
    #528722
Production build 0.71.5 2024