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.

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

EZP-29757: As an administrator, I want to be able to translate a content type (AdminUI frontend) #263

Merged
merged 8 commits into from Dec 14, 2018

Conversation

7 participants
@ViniTou
Copy link
Contributor

ViniTou commented Dec 7, 2018

Question Answer
Tickets https://jira.ez.no/browse/EZP-29757
Bug fix? no
New feature? yes
BC breaks? no
Tests pass? yes
Doc needed? yes
License GPL-2.0

Checklist:

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

@ViniTou ViniTou requested review from webhdx, Nattfarinn and adamwojs Dec 7, 2018

@ezrobot

This comment was marked as resolved.

Copy link

ezrobot commented Dec 7, 2018

Tool version : PHP CS Fixer 2.7.1 Sandy Pool by Fabien Potencier and Dariusz Ruminski
Command executed php-cs-fixer --dry-run -v fix
This Pull Request does not respect PSR-2 Coding Standards, please, see the suggested diff below:

diff --git a/lib/Data/ContentTypeData.php b/lib/Data/ContentTypeData.php
index 291d71a..0d521f7 100644
--- a/lib/Data/ContentTypeData.php
+++ b/lib/Data/ContentTypeData.php
@@ -34,7 +34,7 @@ class ContentTypeData extends ContentTypeUpdateStruct implements NewnessCheckabl
     protected $fieldDefinitionsData = [];
 
     /**
-     * Language Code of currently edited contentTypeDraft
+     * Language Code of currently edited contentTypeDraft.
      *
      * @var string|null
      */
@@ -56,7 +56,7 @@ class ContentTypeData extends ContentTypeUpdateStruct implements NewnessCheckabl
             $this->fieldDefinitionsData,
             function (FieldDefinitionData $fieldDefinitionData) use ($fieldDefinitionIdentifier) {
                 return $fieldDefinitionIdentifier === $fieldDefinitionData->identifier;
-        });
+            });
 
         $keyToReplace = array_keys($currentFieldDefinition);
 
diff --git a/lib/Data/Mapper/ContentTypeDraftMapper.php b/lib/Data/Mapper/ContentTypeDraftMapper.php
index 0bbfc55..d8a83f9 100644
--- a/lib/Data/Mapper/ContentTypeDraftMapper.php
+++ b/lib/Data/Mapper/ContentTypeDraftMapper.php
@@ -60,7 +60,6 @@ class ContentTypeDraftMapper implements FormDataMapperInterface
         }
 
         foreach ($contentTypeDraft->fieldDefinitions as $fieldDef) {
-
             $names = $fieldDef->getNames();
             $descriptions = $fieldDef->getDescriptions();
             if ($baseLanguage && $language) {

@ViniTou ViniTou force-pushed the ViniTou:EZP-29757-content-type-translate branch from 4b5274a to bea16be Dec 7, 2018

@@ -24,6 +26,16 @@ class ContentTypeDraftMapper implements FormDataMapperInterface
*/
public function mapToFormData(ValueObject $contentTypeDraft, array $params = [])
{
$optionsResolver = new OptionsResolver();
$this->configureOptions($optionsResolver);
$params = $optionsResolver->resolve($params);

This comment has been minimized.

Copy link
@adamwojs

adamwojs Dec 7, 2018

Member

I think you can extract the following lines:

$optionsResolver = new OptionsResolver();
$this->configureOptions($optionsResolver);
$params = $optionsResolver->resolve($params);

into private method:

private function resolveOptions(array $params): array
{
    $optionsResolver = new OptionsResolver();
    $optionsResolver
        ->setDefined(['language'])
        ->setDefined(['baseLanguage'])
        ->setAllowedTypes('baseLanguage', ['null', Language::class])
        ->setAllowedTypes('language', Language::class);

    return $optionsResolver->resolve($params);
}
@@ -58,15 +58,16 @@ public function mapFieldValueForm(FormInterface $fieldForm, FieldData $data)
$fieldSettings = $fieldDefinition->getFieldSettings();
$formConfig = $fieldForm->getConfig();
$names = $fieldDefinition->getNames();
$label = $fieldDefinition->getName($formConfig->getOption('mainLanguageCode')) ?: reset($names);
$label = $fieldDefinition->getName($formConfig->getOption('languageCode'))

This comment has been minimized.

Copy link
@adamwojs

adamwojs Dec 7, 2018

Member

Can we avoid duplication of this logic across FieldValueMappers implementations ?

This comment has been minimized.

Copy link
@ViniTou

ViniTou Dec 7, 2018

Author Contributor

Was thinking about it, but either injecting service just for that or using trait seems like worse solutions than this. Unless you think different / have better option.

@@ -179,13 +179,15 @@ private function loadLanguage(string $languageCode): Language
*
* @param int $contentTypeId
*

This comment has been minimized.

Copy link
@mikadamczyk

mikadamczyk Dec 10, 2018

Contributor

nitpick: not needed empty line

Show resolved Hide resolved lib/Data/ContentTypeData.php Outdated
if ($data->mainLanguageCode === $data->usedLanguageCode) {
return;
}
$this->disableNotTranslatableFields($form);

This comment has been minimized.

Copy link
@mikadamczyk

mikadamczyk Dec 10, 2018

Contributor

like in method below

Suggested change
$this->disableNotTranslatableFields($form);
$this->disableNotTranslatableFields($event->getForm());
@webhdx

webhdx approved these changes Dec 10, 2018

*
* @var string|null
*/
public $usedLanguageCode = null;

This comment has been minimized.

Copy link
@webhdx

webhdx Dec 10, 2018

Contributor

Why not just languageCode?

This comment has been minimized.

Copy link
@ViniTou

ViniTou Dec 10, 2018

Author Contributor

No particular reasons, thought that would give more emphasis to it. Going to change it.

@ezrobot

This comment was marked as resolved.

Copy link

ezrobot commented Dec 10, 2018

Tool version : PHP CS Fixer 2.7.1 Sandy Pool by Fabien Potencier and Dariusz Ruminski
Command executed php-cs-fixer --dry-run -v fix
This Pull Request does not respect PSR-2 Coding Standards, please, see the suggested diff below:

diff --git a/tests/RepositoryForms/Form/Processor/ContentTypeFormProcessorTest.php b/tests/RepositoryForms/Form/Processor/ContentTypeFormProcessorTest.php
index ef00e98..7cd55b6 100644
--- a/tests/RepositoryForms/Form/Processor/ContentTypeFormProcessorTest.php
+++ b/tests/RepositoryForms/Form/Processor/ContentTypeFormProcessorTest.php
@@ -114,7 +114,7 @@ class ContentTypeFormProcessorTest extends TestCase
         $contentTypeDraft = new ContentTypeDraft([
             'innerContentType' => new ContentType([
                 'fieldDefinitions' => $existingFieldDefinitions,
-                'mainLanguageCode' => $languageCode
+                'mainLanguageCode' => $languageCode,
             ]),
         ]);
         $expectedNewFieldDefIdentifier = sprintf(
->with('languageCode')
->willReturn('eng-GB');
$this->config->expects($this->at(1))

This comment has been minimized.

Copy link
@webhdx

webhdx Dec 10, 2018

Contributor

->method()->willReturnMap(...) is more readable

@@ -44,6 +54,11 @@ public function testMapFieldValueFormWithLanguageCode()
->with('fieldDefinition')
->willReturn($fieldDefinition);
$this->config->expects($this->once())

This comment has been minimized.

Copy link
@webhdx

webhdx Dec 10, 2018

Contributor

Is it meant to be called exactly once? It is usually not true for getters. It will make test fail after minor and not destructive change in the code i.e. by calling getOption twice.

@@ -133,6 +135,18 @@ public function testAddFieldDefinition()
->with('fieldTypeSelection')
->willReturn($fieldTypeSelectionForm);
$formConfig = $this->createMock(FormConfigInterface::class);
$formConfig
->expects($this->once())

This comment has been minimized.

Copy link
@webhdx

webhdx Dec 10, 2018

Contributor

See above. I see a pattern here.

Update lib/Data/ContentTypeData.php
Co-Authored-By: ViniTou <vinniczek@gmail.com>
@ezrobot

This comment was marked as resolved.

Copy link

ezrobot commented Dec 10, 2018

Tool version : PHP CS Fixer 2.7.1 Sandy Pool by Fabien Potencier and Dariusz Ruminski
Command executed php-cs-fixer --dry-run -v fix
This Pull Request does not respect PSR-2 Coding Standards, please, see the suggested diff below:

diff --git a/lib/Data/ContentTypeData.php b/lib/Data/ContentTypeData.php
index b75f109..7a1e6a1 100644
--- a/lib/Data/ContentTypeData.php
+++ b/lib/Data/ContentTypeData.php
@@ -61,7 +61,7 @@ class ContentTypeData extends ContentTypeUpdateStruct implements NewnessCheckabl
 
         $keyToReplace = array_keys($currentFieldDefinition);
 
-        $this->fieldDefinitionsData[key($currentFieldDefinition)] = $fieldDefinitionData; 
+        $this->fieldDefinitionsData[key($currentFieldDefinition)] = $fieldDefinitionData;
     }
 
     /**
diff --git a/tests/RepositoryForms/Form/Processor/ContentTypeFormProcessorTest.php b/tests/RepositoryForms/Form/Processor/ContentTypeFormProcessorTest.php
index ef00e98..7cd55b6 100644
--- a/tests/RepositoryForms/Form/Processor/ContentTypeFormProcessorTest.php
+++ b/tests/RepositoryForms/Form/Processor/ContentTypeFormProcessorTest.php
@@ -114,7 +114,7 @@ class ContentTypeFormProcessorTest extends TestCase
         $contentTypeDraft = new ContentTypeDraft([
             'innerContentType' => new ContentType([
                 'fieldDefinitions' => $existingFieldDefinitions,
-                'mainLanguageCode' => $languageCode
+                'mainLanguageCode' => $languageCode,
             ]),
         ]);
         $expectedNewFieldDefIdentifier = sprintf(
@ezrobot

This comment was marked as resolved.

Copy link

ezrobot commented Dec 10, 2018

Tool version : PHP CS Fixer 2.7.1 Sandy Pool by Fabien Potencier and Dariusz Ruminski
Command executed php-cs-fixer --dry-run -v fix
This Pull Request does not respect PSR-2 Coding Standards, please, see the suggested diff below:

diff --git a/tests/RepositoryForms/FieldType/Mapper/UserAccountFieldValueFormMapperTest.php b/tests/RepositoryForms/FieldType/Mapper/UserAccountFieldValueFormMapperTest.php
index d9e3cdb..aecfe79 100644
--- a/tests/RepositoryForms/FieldType/Mapper/UserAccountFieldValueFormMapperTest.php
+++ b/tests/RepositoryForms/FieldType/Mapper/UserAccountFieldValueFormMapperTest.php
@@ -52,7 +52,6 @@ class UserAccountFieldValueFormMapperTest extends BaseMapperTest
                 ['mainLanguageCode', null, 'eng-GB'],
             ]);
 
-
         $mapper->mapFieldValueForm($this->fieldForm, $this->data);
     }
 

@ViniTou ViniTou force-pushed the ViniTou:EZP-29757-content-type-translate branch from 25ee4e8 to b3f6e40 Dec 10, 2018

@ezrobot

This comment has been minimized.

Copy link

ezrobot commented Dec 14, 2018

Tool version : PHP CS Fixer 2.7.1 Sandy Pool by Fabien Potencier and Dariusz Ruminski
Command executed php-cs-fixer --dry-run -v fix
This Pull Request does not respect PSR-2 Coding Standards, please, see the suggested diff below:

diff --git a/lib/Form/Type/ContentType/ContentTypeUpdateType.php b/lib/Form/Type/ContentType/ContentTypeUpdateType.php
index 5db511d..9622de2 100644
--- a/lib/Form/Type/ContentType/ContentTypeUpdateType.php
+++ b/lib/Form/Type/ContentType/ContentTypeUpdateType.php
@@ -8,7 +8,6 @@
  */
 namespace EzSystems\RepositoryForms\Form\Type\ContentType;
 
-use EzSystems\RepositoryForms\Data\ContentTypeData;
 use EzSystems\RepositoryForms\Form\DataTransformer\TranslatablePropertyTransformer;
 use EzSystems\RepositoryForms\Form\Type\FieldDefinition\FieldDefinitionType;
 use Symfony\Component\Form\AbstractType;
@@ -17,9 +16,6 @@ use Symfony\Component\Form\Extension\Core\Type\CollectionType;
 use Symfony\Component\Form\Extension\Core\Type\SubmitType;
 use Symfony\Component\Form\Extension\Core\Type\TextType;
 use Symfony\Component\Form\FormBuilderInterface;
-use Symfony\Component\Form\FormEvent;
-use Symfony\Component\Form\FormEvents;
-use Symfony\Component\Form\FormInterface;
 use Symfony\Component\OptionsResolver\OptionsResolver;
 
 /**
@@ -43,7 +39,7 @@ class ContentTypeUpdateType extends AbstractType
             ->setDefaults([
                 'data_class' => 'EzSystems\RepositoryForms\Data\ContentTypeData',
                 'translation_domain' => 'ezrepoforms_content_type',
-                'mainLanguageCode' => null
+                'mainLanguageCode' => null,
             ])
             ->setDefined(['mainLanguageCode'])
             ->setAllowedTypes('mainLanguageCode', ['null', 'string'])
diff --git a/lib/Form/Type/FieldDefinition/FieldDefinitionType.php b/lib/Form/Type/FieldDefinition/FieldDefinitionType.php
index 376d86f..0111e7f 100644
--- a/lib/Form/Type/FieldDefinition/FieldDefinitionType.php
+++ b/lib/Form/Type/FieldDefinition/FieldDefinitionType.php
@@ -20,7 +20,6 @@ use Symfony\Component\Form\Extension\Core\Type\TextType;
 use Symfony\Component\Form\FormBuilderInterface;
 use Symfony\Component\Form\FormEvent;
 use Symfony\Component\Form\FormEvents;
-use Symfony\Component\Form\FormInterface;
 use Symfony\Component\OptionsResolver\OptionsResolver;
 
 /**
@@ -60,7 +59,7 @@ class FieldDefinitionType extends AbstractType
             ->setDefaults([
                 'data_class' => 'EzSystems\RepositoryForms\Data\FieldDefinitionData',
                 'translation_domain' => 'ezrepoforms_content_type',
-                'mainLanguageCode' => null
+                'mainLanguageCode' => null,
             ])
             ->setDefined(['mainLanguageCode'])
             ->setAllowedTypes('mainLanguageCode', ['null', 'string'])
@webhdx

webhdx approved these changes Dec 14, 2018

@webhdx

This comment has been minimized.

Copy link
Contributor

webhdx commented Dec 14, 2018

Looks like you need to run php-cs-fixer here.

@mnocon

mnocon approved these changes Dec 14, 2018

@lserwatka lserwatka merged commit b9d291e into ezsystems:master Dec 14, 2018

2 checks passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
ezrobot/phpcsfixer Code review by ezrobot
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.