IBX-6494: Fixed copying of non-translatable fields to later versions #394

barw4 · 2023-11-14T17:41:30Z

Question	Answer
JIRA issue	IBX-6494
Type	bug
Target Ibexa version	`v3.3`
BC breaks	no

The issue was present before the copyNonTranslatableFieldsFromPublishedVersion was introduced. However, this method would allow us to handle also the case given in the public ticket.

Maintainer update

QA

This fix makes non-translatable fields to be copied regardless of if they're coming from higher or lower version number, so we need some extensive testing both for scenario in the ticket and some other ideas of how Editor could manipulate content and possibly break it. The version number simple math comparison (>) was there probably since eZ Publish. However I think this was never a good determinant of if something should be overridden.

Side note: would be nice to have in such case a version comparison response which would result in an UI dialog stating that the field changed in some other version, prompting user to choose proper value. But we're nowhere near that yet.

Checklist:

Provided PR description.
Tested the solution manually.
Provided automated test coverage.
Checked that target branch is set correctly (master for features, the oldest supported for bugs).
Ran PHP CS Fixer for new PHP code (use $ composer fix-cs).
Asked for a review (ping @ezsystems/engineering-team).

sonarcloud · 2023-11-14T17:44:35Z

Kudos, SonarCloud Quality Gate passed!

0 Bugs
0 Vulnerabilities
0 Security Hotspots
0 Code Smells

No Coverage information
0.0% Duplication

ViniTou · 2023-11-28T09:27:13Z

eZ/Publish/Core/Repository/ContentService.php

@@ -1517,10 +1517,7 @@ protected function copyNonTranslatableFieldsFromPublishedVersion(APIContent $cur
        $publishedContent = $this->internalLoadContentById($versionInfo->getContentInfo()->getId());
        $publishedVersionInfo = $publishedContent->getVersionInfo();

-        if (
-            !$publishedVersionInfo->isPublished()


Isnt that going to break some cases when things are not copied for translations (\Ibexa\Core\Repository\ContentService::copyTranslationsFromPublishedVersion) and then some other fields will be copied here?

Or isnt it going to break publishing older version, with overriding all fields with current ones?

@ViniTou

Isnt that going to break some cases when things are not copied for translations (\Ibexa\Core\Repository\ContentService::copyTranslationsFromPublishedVersion) and then some other fields will be copied here?

Hmm, I cannot see why removing this condition would break that. The updated method only deals with nontranslatable fields and those are not handled in copyTranslationsFromPublishedVersion.

Or isnt it going to break publishing older version, with overriding all fields with current ones?

I've just tested this and it seems that version 2 (3 is published) properly overridden version 3 so values from version 3 weren't copied into earlier version 2 during publishing if that's what you had in mind.

The updated method only deals with nontranslatable fields and those are not handled in copyTranslationsFromPublishedVersion.

Yes, my point was, that right now some fields could be copied/not copied there, and be copied/not copied here as the logic now differs.

But I guess, maybe QA will stumble on specific case as I cant come up with proper testing scenario for it.

ViniTou · 2023-12-08T08:58:10Z

eZ/Publish/Core/Repository/ContentService.php

@@ -1517,10 +1517,7 @@ protected function copyNonTranslatableFieldsFromPublishedVersion(APIContent $cur
        $publishedContent = $this->internalLoadContentById($versionInfo->getContentInfo()->getId());
        $publishedVersionInfo = $publishedContent->getVersionInfo();

-        if (
-            !$publishedVersionInfo->isPublished()


The updated method only deals with nontranslatable fields and those are not handled in copyTranslationsFromPublishedVersion.

Yes, my point was, that right now some fields could be copied/not copied there, and be copied/not copied here as the logic now differs.

But I guess, maybe QA will stumble on specific case as I cant come up with proper testing scenario for it.

alongosz

@barw4 could you rebase this PR so we can see the current state of CI.

eZ/Publish/Core/Repository/ContentService.php

barw4 · 2024-03-26T13:37:08Z

@barw4 could you rebase this PR so we can see the current state of CI.

Done, will send to QA after it's green

tomaszszopinski · 2024-04-09T06:25:33Z

Note from QA: Ive observed a behaviour that might be a bug;
Steps to reproduce:

Create foo content item (from JIRA)
Add translation to this content item (.ie ger-DE)
Edit eng-GB translation (change image)
Publish
Image in ger-DE translation is not changing (it changes only after editing & publishing)

Without this PR this behaviour is not happening.

kernel-394.mov

barw4 · 2024-04-15T21:09:28Z

@alongosz @ViniTou @konradoboza @Nattfarinn calling for a second review.

As @tomaszszopinski noticed the first iteration of this PR fixed the issue from the ticket but broke the original idea for this method (older versions with different languages were not updated properly with non-translatable fields). It's really hard to explain but the newest change will allow changing a value of a non-translatable field with an older published field's value only if a current version is older than a published one. Test was added that validates this update.

sonarcloud · 2024-04-15T21:22:46Z

Quality Gate passed

Issues
1 New issue
0 Accepted issues

Measures
0 Security Hotspots
No data about Coverage
0.0% Duplication on New Code

See analysis details on SonarCloud

tomaszszopinski

QA approved on IbexaDXP 3.3 commerce.

barw4 added Bug Something isn't working Ready for review labels Nov 14, 2023

barw4 self-assigned this Nov 14, 2023

github-actions bot assigned kisztof, alongosz and mikadamczyk Nov 14, 2023

barw4 marked this pull request as ready for review November 15, 2023 09:14

barw4 requested a review from a team November 15, 2023 09:14

ViniTou reviewed Nov 28, 2023

View reviewed changes

barw4 requested review from ViniTou and a team November 30, 2023 15:28

ViniTou approved these changes Dec 8, 2023

View reviewed changes

barw4 requested a review from a team December 8, 2023 09:36

kisztof approved these changes Dec 19, 2023

View reviewed changes

alongosz approved these changes Mar 26, 2024

View reviewed changes

eZ/Publish/Core/Repository/ContentService.php Show resolved Hide resolved

alongosz changed the title ~~IBX-6494: Copy nontranslatable fields to later versions~~ IBX-6494: Fixed copying of non-translatable fields to later versions Mar 26, 2024

IBX-6494: Copy nontranslatable fields to later versions

38d2838

barw4 force-pushed the ibx-6494-copy-nontranslatable-to-later-versions branch from d785b44 to 38d2838 Compare March 26, 2024 13:36

barw4 added Ready for QA and removed Ready for review labels Mar 26, 2024

alongosz requested a review from a team March 26, 2024 13:40

konradoboza approved these changes Mar 26, 2024

View reviewed changes

Nattfarinn approved these changes Mar 27, 2024

View reviewed changes

tomaszszopinski self-assigned this Apr 4, 2024

tomaszszopinski mentioned this pull request Apr 5, 2024

Run regression for IBX-6494 ibexa/commerce#791

Closed

IBX-6494: Refactored solution

a257944

barw4 removed the Ready for QA label Apr 15, 2024

barw4 added the Ready for review label Apr 15, 2024

github-actions bot assigned tischsoic Apr 15, 2024

barw4 requested review from Nattfarinn, alongosz, ViniTou and konradoboza April 15, 2024 21:09

IBX-6494: CS

ceb8c7a

alongosz approved these changes May 6, 2024

View reviewed changes

alongosz requested a review from a team May 6, 2024 09:28

konradoboza approved these changes May 6, 2024

View reviewed changes

konradoboza requested a review from a team May 6, 2024 11:42

barw4 added Ready for QA and removed Ready for review labels May 6, 2024

tomaszszopinski added QA approved and removed Ready for QA labels May 7, 2024

tomaszszopinski approved these changes May 7, 2024

View reviewed changes

alongosz merged commit b5fe9ad into 1.3 May 7, 2024
23 checks passed

alongosz deleted the ibx-6494-copy-nontranslatable-to-later-versions branch May 7, 2024 12:42

alongosz mentioned this pull request May 7, 2024

Merge branch '1.3' of ezsystems/ezplatform-kernel into 4.6 ibexa/core#365

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

IBX-6494: Fixed copying of non-translatable fields to later versions #394

IBX-6494: Fixed copying of non-translatable fields to later versions #394

barw4 commented Nov 14, 2023 •

edited by alongosz

Loading

sonarcloud bot commented Nov 14, 2023

ViniTou Nov 28, 2023

barw4 Nov 30, 2023 •

edited

Loading

ViniTou Dec 8, 2023

ViniTou Dec 8, 2023

alongosz left a comment

barw4 commented Mar 26, 2024

tomaszszopinski commented Apr 9, 2024 •

edited

Loading

barw4 commented Apr 15, 2024 •

edited

Loading

sonarcloud bot commented Apr 15, 2024

tomaszszopinski left a comment

IBX-6494: Fixed copying of non-translatable fields to later versions #394

IBX-6494: Fixed copying of non-translatable fields to later versions #394

Conversation

barw4 commented Nov 14, 2023 • edited by alongosz Loading

QA

Checklist:

sonarcloud bot commented Nov 14, 2023

ViniTou Nov 28, 2023

Choose a reason for hiding this comment

barw4 Nov 30, 2023 • edited Loading

Choose a reason for hiding this comment

ViniTou Dec 8, 2023

Choose a reason for hiding this comment

ViniTou Dec 8, 2023

Choose a reason for hiding this comment

alongosz left a comment

Choose a reason for hiding this comment

barw4 commented Mar 26, 2024

tomaszszopinski commented Apr 9, 2024 • edited Loading

barw4 commented Apr 15, 2024 • edited Loading

sonarcloud bot commented Apr 15, 2024

Quality Gate passed

tomaszszopinski left a comment

Choose a reason for hiding this comment

barw4 commented Nov 14, 2023 •

edited by alongosz

Loading

barw4 Nov 30, 2023 •

edited

Loading

tomaszszopinski commented Apr 9, 2024 •

edited

Loading

barw4 commented Apr 15, 2024 •

edited

Loading