IBX-3265: Disabled User account form during translation #61

barw4 · 2022-07-20T15:44:07Z

Question	Answer
Tickets	IBX-3265
Bug fix?	yes
New feature?	no
BC breaks?	no
Tests pass?	yes
Doc needed?	no
License	GPL-2.0

As the title states.

Checklist:

Coding standards ($ composer fix-cs)
Ready for Code Review

konradoboza · 2022-07-21T07:13:07Z

src/lib/FieldType/Mapper/UserAccountFieldValueFormMapper.php

+        $isTranslation = $formData instanceof ContentUpdateData;
+        if ($isTranslation) {
+            return;
+        }


Why are you now verifying instance of ContentUpdateData instead? If it's needed, at least $isTranslation should be properly renamed. Besides, since this variable is used only once, maybe:

Suggested change

$isTranslation = $formData instanceof ContentUpdateData;

if ($isTranslation) {

return;

}

if ($formData instanceof ContentUpdateData) {

return;

}

Because ContentUpdateData is used when translating

Isnt that the root of the bug? That ContentUpdateData is used for User instead of ContentTranslationData?

@ViniTou @konradoboza
translating action goes through proxyTranslateAction and then, its response is always content/edit/draft, then hits editVersionDraftAction and then we are in ContentEditViewFilter when we are using ContentUpdateMapper -> ContentUpdateData. The same happens for every other content which seems logical because we are in fact editing drafts when translating.

Besides even in TranslationFormProcessor ContentUpdateData is built.

The source of the bug is the fact that at this point in the form we don't have UserCreateData and UserUpdateData. There is no such thing as UserTranslateData at all. Every translation ends up with ContentUpdateData.

Thanks for the clarification. Not sure if we want to block translating users from the business point of view though. Besides, obtaining content type via $rootForm->getData()->contentDraft->contentType seems to work.

Okay, but ContentTranslationData is not used when translating any content, not just user one.

Yes, and it just means that the issue is bigger.

I believe similar logic is (disabling when translating) is done with CustomerGroup FT in product catalog. Do we have similar issue there as well?? @adamwojs @Steveb-p

~~Customer Group is rather a separate object not a Field Type so the approach is quite different there.~~ brainfart 😓

I'll put this topic on hold until we decide how it should really work and by that I mean:

usage of ContentTranslationData

UI behavior (should we hide the form? should we show it as disabled? - JS validation has to be fixed in this case)

external storage behavior - should the user_account data be copied? set to null? don't insert anything?

As I said, it is the same issue here then:
https://github.com/ibexa/product-catalog/blob/main/src/bundle/FieldType/CustomerGroup/FieldValueFormMapper.php#L23

I would say, we should use ContentTranslationData/UserTranslationData if needed.
And show form as disabled, as we did till now.

Even in the PR title you stated disabling and not hiding/removing ;)
But I agree, we can wait for some proper decisions.

sonarcloud · 2022-07-22T08:38:54Z

Kudos, SonarCloud Quality Gate passed!

0 Bugs
0 Vulnerabilities
0 Security Hotspots
0 Code Smells

No Coverage information
0.0% Duplication

adamwojs · 2022-08-22T11:44:00Z

src/lib/FieldType/Mapper/UserAccountFieldValueFormMapper.php

                ],
            ]);

-        if ($isTranslation) {
-            $formBuilder->addModelTransformer($this->getModelTransformerForTranslation($fieldDefinition));


getModelTransformerForTranslation method should be deprecated as it's not used anymore.

@adamwojs could you, please, take a look at #61 (comment)? This PR has to be put on hold until we decide what to do with content translating, as discussed:
Creating - userCreateData (instead of contentCreateData)
Updating - userUpdateData (instead of contentUpdateData)
Translating - contentUpdateData (standard across all contents)

Dawid says we should use contentTranslateData system-wise but we have not been using it at all anywhere. Could you please share your point of view on this?

adamwojs · 2022-10-28T08:42:18Z

Looking into 4.x code I see that translation mode check is based on ContentTranslationData as @ViniTou suggests: https://github.com/ibexa/content-forms/blob/main/src/lib/FieldType/Mapper/UserAccountFieldValueFormMapper.php#L44

ibexa/content-forms@92ce5c0 backport might solve the issue for 3.3 branch as well.

adamwojs · 2022-11-09T11:49:44Z

@barw4 are you willing to continue work on this issue ?

barw4 · 2022-11-09T11:52:52Z

@adamwojs yes, I'll take care of it probably next week

barw4 · 2022-11-17T17:31:03Z

@adamwojs after going back to the topic, here are some conclusions:

https://github.com/ibexa/content-forms/blob/main/src/lib/FieldType/Mapper/UserAccountFieldValueFormMapper.php#L44 will never be true because "translating" is in fact updating a draft of a given language. Besides, linked implementation already exists in v3.3.
I cannot see why would we use ContentTranslationData as @ViniTou suggested, because when would we then use ContentUpdateData? Basically, translating = updating, so every time we are updating a draft in any language this is in fact "translating". This is the exact same thing. How do we recognize between the two? I don't think we can.
ContentTranslationData is only used when creating a draft in a new language and it should stay as so according to URL.
I think I still stand by my idea in this PR. We cannot rely on ContentTranslationData as if we would, ContentUpdateData wouldn't be used in any case as translating from the user's point of view = updating from the app's point of view, please notice the URL when looking at content's form in new language: content/edit/draft/129/6/fre-FR/109 -> this is updating, not translating.

webhdx · 2022-11-29T21:47:20Z

The issue was much more extensive and I start to believe it may never worked correctly or became broken a long time ago. Even if it worked in the past, introducing autosave feature definitely messed it up as we changed translation interface to generic content update form.

I cleared it up in #63 and now it uses translation form as intended + disables user field and only lets update translatable fields (name, last name etc.).

I'm closing this PR in favor of #63.

IBX-3265: Disabled User account form during translation

b7d3c6f

barw4 added Bug Something isn't working Ready for review labels Jul 20, 2022

barw4 requested a review from a team July 20, 2022 15:44

barw4 self-assigned this Jul 20, 2022

IBX-3265: CS

309228a

github-actions bot assigned Nattfarinn, ViniTou and webhdx Jul 20, 2022

IBX-3265: CS

fcf7635

konradoboza reviewed Jul 21, 2022

View reviewed changes

konradoboza requested review from a team and ViniTou July 21, 2022 07:13

IBX-3265: CS

dc9ed1c

barw4 requested a review from konradoboza July 21, 2022 09:43

IBX-3265: Load User Content with proper language

06ac50c

adamwojs reviewed Aug 22, 2022

View reviewed changes

barw4 requested a review from adamwojs November 17, 2022 17:33

webhdx closed this Nov 29, 2022

webhdx deleted the ibx-3265-user-translation branch November 29, 2022 21:47

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

IBX-3265: Disabled User account form during translation #61

IBX-3265: Disabled User account form during translation #61

barw4 commented Jul 20, 2022

konradoboza Jul 21, 2022

barw4 Jul 21, 2022

ViniTou Jul 21, 2022 •

edited

Loading

barw4 Jul 21, 2022 •

edited

Loading

konradoboza Jul 22, 2022 •

edited

Loading

barw4 Jul 22, 2022

ViniTou Jul 22, 2022

konradoboza Jul 22, 2022 •

edited

Loading

barw4 Jul 22, 2022 •

edited

Loading

ViniTou Jul 22, 2022 •

edited

Loading

sonarcloud bot commented Jul 22, 2022

adamwojs Aug 22, 2022

barw4 Aug 24, 2022

adamwojs commented Oct 28, 2022

adamwojs commented Nov 9, 2022

barw4 commented Nov 9, 2022

barw4 commented Nov 17, 2022 •

edited

Loading

webhdx commented Nov 29, 2022

IBX-3265: Disabled User account form during translation #61

IBX-3265: Disabled User account form during translation #61

Conversation

barw4 commented Jul 20, 2022

Checklist:

konradoboza Jul 21, 2022

Choose a reason for hiding this comment

barw4 Jul 21, 2022

Choose a reason for hiding this comment

ViniTou Jul 21, 2022 • edited Loading

Choose a reason for hiding this comment

barw4 Jul 21, 2022 • edited Loading

Choose a reason for hiding this comment

konradoboza Jul 22, 2022 • edited Loading

Choose a reason for hiding this comment

barw4 Jul 22, 2022

Choose a reason for hiding this comment

ViniTou Jul 22, 2022

Choose a reason for hiding this comment

konradoboza Jul 22, 2022 • edited Loading

Choose a reason for hiding this comment

barw4 Jul 22, 2022 • edited Loading

Choose a reason for hiding this comment

ViniTou Jul 22, 2022 • edited Loading

Choose a reason for hiding this comment

sonarcloud bot commented Jul 22, 2022

adamwojs Aug 22, 2022

Choose a reason for hiding this comment

barw4 Aug 24, 2022

Choose a reason for hiding this comment

adamwojs commented Oct 28, 2022

adamwojs commented Nov 9, 2022

barw4 commented Nov 9, 2022

barw4 commented Nov 17, 2022 • edited Loading

webhdx commented Nov 29, 2022

ViniTou Jul 21, 2022 •

edited

Loading

barw4 Jul 21, 2022 •

edited

Loading

konradoboza Jul 22, 2022 •

edited

Loading

konradoboza Jul 22, 2022 •

edited

Loading

barw4 Jul 22, 2022 •

edited

Loading

ViniTou Jul 22, 2022 •

edited

Loading

barw4 commented Nov 17, 2022 •

edited

Loading