Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Closed
wants to merge 5 commits into from
Closed

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

Changes from 3 commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
b7d3c6f
IBX-3265: Disabled User account form during translation
barw4 Jul 20, 2022
309228a
IBX-3265: CS
barw4 Jul 20, 2022
fcf7635
IBX-3265: CS
barw4 Jul 20, 2022
dc9ed1c
IBX-3265: CS
barw4 Jul 21, 2022
06ac50c
IBX-3265: Load User Content with proper language
barw4 Jul 22, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
17 changes: 9 additions & 8 deletions src/lib/FieldType/Mapper/UserAccountFieldValueFormMapper.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,8 +10,8 @@

use eZ\Publish\API\Repository\Values\ContentType\FieldDefinition;
use eZ\Publish\Core\FieldType\User\Value as ApiUserValue;
use EzSystems\EzPlatformContentForms\Data\Content\ContentUpdateData;
use EzSystems\EzPlatformContentForms\Data\Content\FieldData;
use EzSystems\EzPlatformContentForms\Data\ContentTranslationData;
use EzSystems\EzPlatformContentForms\Data\User\UserAccountFieldData;
use EzSystems\EzPlatformContentForms\FieldType\FieldValueFormMapperInterface;
use EzSystems\EzPlatformContentForms\Form\Type\FieldType\UserAccountFieldType;
Expand Down Expand Up @@ -46,22 +46,23 @@ public function mapFieldValueForm(FormInterface $fieldForm, FieldData $data)
$formConfig = $fieldForm->getConfig();
$rootForm = $fieldForm->getRoot()->getRoot();
$formIntent = $rootForm->getConfig()->getOption('intent');
$isTranslation = $rootForm->getData() instanceof ContentTranslationData;
$formData = $rootForm->getData();
$isTranslation = $formData instanceof ContentUpdateData;
if ($isTranslation) {
return;
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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;
}

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Because ContentUpdateData is used when translating

Copy link
Contributor

@ViniTou ViniTou Jul 21, 2022 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member Author

@barw4 barw4 Jul 21, 2022 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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.

Copy link
Member

@konradoboza konradoboza Jul 22, 2022 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member

@konradoboza konradoboza Jul 22, 2022 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member Author

@barw4 barw4 Jul 22, 2022 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Contributor

@ViniTou ViniTou Jul 22, 2022 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.


$formBuilder = $formConfig->getFormFactory()->createBuilder()
->create('value', UserAccountFieldType::class, [
'required' => true,
'label' => $fieldDefinition->getName(),
'intent' => $formIntent,
'constraints' => [
new UserAccountPassword(['contentType' => $rootForm->getData()->contentType]),
new UserAccountPassword(['contentType' => $formData->contentType]),
],
]);

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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?

} else {
$formBuilder->addModelTransformer($this->getModelTransformer());
}
$formBuilder->addModelTransformer($this->getModelTransformer());

$formBuilder->setAutoInitialize(false);

Expand Down