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 #730

Merged
merged 26 commits into from Dec 14, 2018

Conversation

@ViniTou
Copy link
Contributor

commented Nov 30, 2018

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

Checklist:

  • Implement translation remove
  • Update translations/twig views(?)
  • Add proper styling
  • Coding standards ($ composer fix-cs)
  • Ready for Code Review

@ViniTou ViniTou force-pushed the ViniTou:EZP-29757-content-type-translate branch from e3ee3ad to 4ae81a2 Nov 30, 2018

@ezsystems ezsystems deleted a comment from ezrobot Nov 30, 2018

@ezrobot

This comment was marked as resolved.

Copy link

commented Nov 30, 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/src/bundle/Controller/ContentTypeController.php b/src/bundle/Controller/ContentTypeController.php
index 947ada0..b902ad7 100644
--- a/src/bundle/Controller/ContentTypeController.php
+++ b/src/bundle/Controller/ContentTypeController.php
@@ -645,6 +645,7 @@ class ContentTypeController extends Controller
      * @param \eZ\Publish\API\Repository\Values\ContentType\ContentTypeDraft $contentTypeDraft
      *
      * @return \eZ\Publish\API\Repository\Values\Content\Language
+     *
      * @throws \eZ\Publish\API\Repository\Exceptions\NotFoundException
      */
     private function getDefaultLanguage(ContentTypeDraft $contentTypeDraft
diff --git a/src/lib/UI/Dataset/TranslationsDataset.php b/src/lib/UI/Dataset/TranslationsDataset.php
index 023bd71..5bc9e42 100644
--- a/src/lib/UI/Dataset/TranslationsDataset.php
+++ b/src/lib/UI/Dataset/TranslationsDataset.php
@@ -62,6 +62,7 @@ class TranslationsDataset
      * @param \eZ\Publish\API\Repository\Values\ContentType\ContentType $contentType
      *
      * @return \EzSystems\EzPlatformAdminUi\UI\Dataset\TranslationsDataset
+     *
      * @throws \eZ\Publish\API\Repository\Exceptions\NotFoundException
      */
     public function loadFromContentType(ContentType $contentType): self
diff --git a/src/lib/UI/Value/ValueFactory.php b/src/lib/UI/Value/ValueFactory.php
index a3afd48..5b0efb3 100644
--- a/src/lib/UI/Value/ValueFactory.php
+++ b/src/lib/UI/Value/ValueFactory.php
@@ -263,6 +263,7 @@ class ValueFactory
      * @param \eZ\Publish\API\Repository\Values\ContentType\ContentType $contentType
      *
      * @return \EzSystems\EzPlatformAdminUi\UI\Value\Content\Language
+     *
      * @throws \eZ\Publish\API\Repository\Exceptions\BadStateException
      * @throws \eZ\Publish\API\Repository\Exceptions\InvalidArgumentException
      */

@ViniTou ViniTou force-pushed the ViniTou:EZP-29757-content-type-translate branch from 4ae81a2 to bdfc0b9 Nov 30, 2018

@lserwatka

This comment has been minimized.

Copy link
Member

commented Dec 5, 2018

@ViniTou rebase is needed

@ViniTou ViniTou force-pushed the ViniTou:EZP-29757-content-type-translate branch from bdfc0b9 to bcd0819 Dec 6, 2018

@ViniTou ViniTou requested a review from adamwojs Dec 7, 2018

@adamwojs
Copy link
Member

left a comment

Please pay attention to the missing docblocks and FQCN (skipped in this review)

private function getLanguage(string $languageCode): Language
{
$language = $this->languageService->loadLanguage($languageCode);
if (!$language) {

This comment has been minimized.

Copy link
@adamwojs

adamwojs Dec 7, 2018

Member

This implementation is incorrect. \eZ\Publish\API\Repository\LanguageService::loadLanguage throws NotFoundException witch should be caught here and wrapped into NotFoundHttpException.

class ContentTypeEditData
{
/** @var \eZ\Publish\API\Repository\Values\ContentType\ContentType|null */
protected $contentType;

This comment has been minimized.

Copy link
@adamwojs

adamwojs Dec 7, 2018

Member

Nitpick: this property (and others) can be private.

* @param \Twig\Environment $twig
* @param \Symfony\Component\Translation\TranslatorInterface $translator
*/
public function __construct(

This comment has been minimized.

Copy link
@adamwojs

adamwojs Dec 7, 2018

Member

Nitpick: this constructor is redundant

use eZ\Publish\API\Repository\UserService;
/**
* KnpMenuBundle Menu Builder service implementation for AdminUI Location View contextual sidebar menu.

This comment has been minimized.

Copy link
@adamwojs

adamwojs Dec 7, 2018

Member
Suggested change
* KnpMenuBundle Menu Builder service implementation for AdminUI Location View contextual sidebar menu.
* KnpMenuBundle Menu Builder service implementation for AdminUI Content Type contextual sidebar menu.
@dew326
Copy link
Member

left a comment

Please rethink the Twig architecture.

@@ -0,0 +1,14 @@
(function (global, doc) {

This comment has been minimized.

Copy link
@dew326

dew326 Dec 7, 2018

Member

Incorrect file name: content.type.edit.js


editButton.addEventListener('click', () => {
languageRadioOption.checked = true;
const form = doc.querySelector('.ez-extra-actions--edit form');

This comment has been minimized.

Copy link
@dew326

dew326 Dec 7, 2018

Member

const at the top of a function, and after const we add empty line.

@@ -59,7 +59,9 @@
title="{{ 'content_type.view.edit.content_field_definition.delete'|trans|desc('Delete Content field definition') }}"
class="btn btn-danger"
data-toggle="modal"
data-target="#delete-content-type-modal">
data-target="#delete-content-type-modal"
{% if form.addFieldDefinition.vars.disabled %}disabled = disabled{% endif %}

This comment has been minimized.

Copy link
@dew326

dew326 Dec 7, 2018

Member

disabled is enough


<div class="modal fade ez-modal ez-translation" id="add-translation-modal" tabindex="-1" role="dialog">
<div class="modal-dialog" role="document">
{{ form_start(form, {'action': path('ezplatform.content_type.add_translation')}) }}

This comment has been minimized.

Copy link
@dew326

dew326 Dec 7, 2018

Member

This twig is the same as in the content translation. The only one change is the acton.
This should share the same twig and be passed one option action

{% form_theme form_translation_remove '@ezdesign/parts/form/flat_widgets.html.twig' %}

{% import _self as tab %}
<section>

This comment has been minimized.

Copy link
@dew326

dew326 Dec 7, 2018

Member

Also looks copied and added one if. It can be done easier to maintain.

@ezrobot

This comment was marked as resolved.

Copy link

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/src/bundle/ParamConverter/LanguageCodeParamConverter.php b/src/bundle/ParamConverter/LanguageCodeParamConverter.php
index fb93aaa..f7030f8 100644
--- a/src/bundle/ParamConverter/LanguageCodeParamConverter.php
+++ b/src/bundle/ParamConverter/LanguageCodeParamConverter.php
@@ -67,7 +67,7 @@ class LanguageCodeParamConverter implements ParamConverterInterface
         } catch (NotFoundException $e) {
             throw new NotFoundHttpException("Language $languageCode not found!");
         }
-        
+
         return $language;
     }
 }
@ezrobot

This comment was marked as resolved.

Copy link

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/src/bundle/Controller/ContentTypeController.php b/src/bundle/Controller/ContentTypeController.php
index 2806821..21ef1fe 100644
--- a/src/bundle/Controller/ContentTypeController.php
+++ b/src/bundle/Controller/ContentTypeController.php
@@ -400,6 +400,7 @@ class ContentTypeController extends Controller
      * @param \eZ\Publish\API\Repository\Values\Content\Language|null $baseLanguage
      *
      * @return \Symfony\Component\HttpFoundation\Response
+     *
      * @throws \eZ\Publish\API\Repository\Exceptions\NotFoundException
      */
     public function updateAction(

@ViniTou ViniTou force-pushed the ViniTou:EZP-29757-content-type-translate branch from 204977f to c22a95e Dec 7, 2018

@ViniTou ViniTou changed the title [WIP] EZP-29757: As an administrator, I want to be able to translate a content type (AdminUI frontend) EZP-29757: As an administrator, I want to be able to translate a content type (AdminUI frontend) Dec 10, 2018

class LanguageCodeParamConverter implements ParamConverterInterface
{
const PARAMETER_LANGUAGE_CODE_TO = 'toLanguageCode';

This comment has been minimized.

Copy link
@mikadamczyk

mikadamczyk Dec 10, 2018

Contributor
Suggested change
const PARAMETER_LANGUAGE_CODE_TO = 'toLanguageCode';
const PARAMETER_TO_LANGUAGE_CODE = 'toLanguageCode';
class LanguageCodeParamConverter implements ParamConverterInterface
{
const PARAMETER_LANGUAGE_CODE_TO = 'toLanguageCode';
const PARAMETER_LANGUAGE_CODE_FROM = 'fromLanguageCode';

This comment has been minimized.

Copy link
@mikadamczyk

mikadamczyk Dec 10, 2018

Contributor
Suggested change
const PARAMETER_LANGUAGE_CODE_FROM = 'fromLanguageCode';
const PARAMETER_FROM_LANGUAGE_CODE = 'fromLanguageCode';

This comment has been minimized.

Copy link
@webhdx

webhdx Dec 10, 2018

Contributor

Actually Dawid's name is better because it's easier to pick up when it's type hinted in IDE

This comment has been minimized.

Copy link
@mikadamczyk

mikadamczyk Dec 10, 2018

Contributor

I prefer better readability, but I am ok with this

@@ -59,7 +59,9 @@
title="{{ 'content_type.view.edit.content_field_definition.delete'|trans|desc('Delete Content field definition') }}"
class="btn btn-danger"
data-toggle="modal"
data-target="#delete-content-type-modal">
data-target="#delete-content-type-modal"
{% if form.addFieldDefinition.vars.disabled %}disabled{% endif %}

This comment has been minimized.

Copy link
@mikadamczyk

mikadamczyk Dec 10, 2018

Contributor
Suggested change
{% if form.addFieldDefinition.vars.disabled %}disabled{% endif %}
{% if form.addFieldDefinition.vars.disabled %}disabled{% endif %}
@@ -70,14 +72,15 @@
{% include '@ezdesign/admin/content_type/delete_confirmation_modal.html.twig' with {'form': form} %}

<div class="card-body">
{% set language_code = content_type.mainLanguageCode %}
{% set language_code = language_code is defined ? language_code : content_type.mainLanguageCode %}

This comment has been minimized.

Copy link
@mikadamczyk

mikadamczyk Dec 10, 2018

Contributor
Suggested change
{% set language_code = language_code is defined ? language_code : content_type.mainLanguageCode %}
{% set language_code = language_code ?? content_type.mainLanguageCode %}
{% for field_definition in form.fieldDefinitionsData %}
{% set value = field_definition.vars.value %}

<div class="card ez-card ez-card--fieldtype-container mb-3 ez-card--collapsed">
<div id="field-definition-{{ value.identifier }}" class="ez-card__header ez-card__header--secondary">
{% set name = value.names[language_code] is defined ? value.names[language_code] : value.names[content_type.mainLanguageCode] %}

This comment has been minimized.

Copy link
@mikadamczyk

mikadamczyk Dec 10, 2018

Contributor
Suggested change
{% set name = value.names[language_code] is defined ? value.names[language_code] : value.names[content_type.mainLanguageCode] %}
{% set name = value.names[language_code] ?? value.names[content_type.mainLanguageCode] %}
<form class="form-inline justify-content-end mb-4">
<select class="form-control ez-location-language-change">
{% for language in languages %}
<option value="{{ path('ezplatform.content_type.view', {

This comment has been minimized.

Copy link
@mikadamczyk

mikadamczyk Dec 10, 2018

Contributor

maybe extracting path to variable will improve readability

@@ -4,7 +4,8 @@

<div class="modal fade ez-modal ez-translation" id="add-translation-modal" tabindex="-1" role="dialog">
<div class="modal-dialog" role="document">
{{ form_start(form, {'action': path('ezplatform.translation.add')}) }}
{% set action = action is defined ? action : path('ezplatform.translation.add') %}

This comment has been minimized.

Copy link
@mikadamczyk

mikadamczyk Dec 10, 2018

Contributor
Suggested change
{% set action = action is defined ? action : path('ezplatform.translation.add') %}
{% set action = action ?? path('ezplatform.translation.add') %}
@@ -3,12 +3,18 @@
{% form_theme form_translation_remove '@ezdesign/parts/form/flat_widgets.html.twig' %}

{% import _self as tab %}
{% set can_translate = can_translate is defined ? can_translate : true %}

This comment has been minimized.

Copy link
@mikadamczyk

mikadamczyk Dec 10, 2018

Contributor
Suggested change
{% set can_translate = can_translate is defined ? can_translate : true %}
{% set can_translate = can_translate ?? true %}
use Symfony\Component\HttpFoundation\Request;
use Symfony\Component\HttpKernel\Exception\NotFoundHttpException;
class LanguageCodeParamConverter implements ParamConverterInterface

This comment has been minimized.

Copy link
@webhdx

webhdx Dec 10, 2018

Contributor

Class name is very confusing. Look, you have both LanguageCodeParamConverter as well as LanguageParamConverter. Also, LanguageCodeParamConverter is misleading as I'd expect it to use languageCode route parameter to convert from. TranslationLanguageParamConverter is better bet but personally I'd avoid using them at all since they sometimes lead to unwanted results.

services:
EzSystems\EzPlatformAdminUi\Tab\ContentType\:
resource: "../../../lib/Tab/ContentType/*"
parent: EzSystems\EzPlatformAdminUi\Tab\AbstractTab

This comment has been minimized.

Copy link
@webhdx

webhdx Dec 10, 2018

Contributor

Please use AbstractEventDispatchingTab as a parent instead. It has been recently merged #728

@@ -70,14 +72,15 @@
{% include '@ezdesign/admin/content_type/delete_confirmation_modal.html.twig' with {'form': form} %}

<div class="card-body">
{% set language_code = content_type.mainLanguageCode %}
{% set language_code = language_code is defined ? language_code : content_type.mainLanguageCode %}

This comment has been minimized.

Copy link
@webhdx

webhdx Dec 10, 2018

Contributor

You might prefer shorter notation:

Suggested change
{% set language_code = language_code is defined ? language_code : content_type.mainLanguageCode %}
{% set language_code = language_code|default(content_type.mainLanguageCode) %}
{% for field_definition in form.fieldDefinitionsData %}
{% set value = field_definition.vars.value %}

<div class="card ez-card ez-card--fieldtype-container mb-3 ez-card--collapsed">
<div id="field-definition-{{ value.identifier }}" class="ez-card__header ez-card__header--secondary">
{% set name = value.names[language_code] is defined ? value.names[language_code] : value.names[content_type.mainLanguageCode] %}

This comment has been minimized.

Copy link
@webhdx

webhdx Dec 10, 2018

Contributor
Suggested change
{% set name = value.names[language_code] is defined ? value.names[language_code] : value.names[content_type.mainLanguageCode] %}
{% set name = value.names[language_code] ?? value.names[content_type.mainLanguageCode] %}
{{ form_widget(field_definition.selected, {
'label': '%s (%s)' | format(value.names[language_code], value.fieldTypeIdentifier),
'label': '%s (%s)' | format(name, value.fieldTypeIdentifier),

This comment has been minimized.

Copy link
@webhdx

webhdx Dec 10, 2018

Contributor

No spaces before and after pipe character |.

'expanded' => true,
'choice_loader' => new CallbackChoiceLoader(function () use ($contentLanguages) {
return $this->loadLanguages(
function (Language $language) use ($contentLanguages) {

This comment has been minimized.

Copy link
@webhdx

webhdx Dec 10, 2018

Contributor

Same as above.

*/
public function createStructure(array $options): ItemInterface
{
/** @var ContentType $contentType */

This comment has been minimized.

Copy link
@webhdx

webhdx Dec 10, 2018

Contributor

FQCN

/** @var ContentType $contentType */
$contentType = $options['content_type'];
/** @var ItemInterface|ItemInterface[] $menu */

This comment has been minimized.

Copy link
@webhdx

webhdx Dec 10, 2018

Contributor

FQCN

use Symfony\Component\Translation\TranslatorInterface;
use Twig\Environment;
class TranslationsTab extends AbstractTab implements OrderedTabInterface

This comment has been minimized.

Copy link
@webhdx

webhdx Dec 10, 2018

Contributor

Please extend AbstractEventDispatchigTab added in #728

use EzSystems\EzPlatformAdminUi\Tab\AbstractTab;
use EzSystems\EzPlatformAdminUi\Tab\OrderedTabInterface;
class ViewTab extends AbstractTab implements OrderedTabInterface

This comment has been minimized.

Copy link
@webhdx

webhdx Dec 10, 2018

Contributor

Same as above

@ViniTou ViniTou force-pushed the ViniTou:EZP-29757-content-type-translate branch 2 times, most recently from 55361b4 to 06013fe Dec 10, 2018

@ezsystems ezsystems deleted a comment from ezrobot Dec 10, 2018

@webhdx

webhdx approved these changes Dec 10, 2018

@dew326

dew326 approved these changes Dec 10, 2018

@@ -50,7 +50,8 @@
}
}

.nav-tabs-systeminfo {
.nav-tabs-systeminfo,
.nav-tabs-content-type {

This comment has been minimized.

Copy link
@dew326

dew326 Dec 10, 2018

Member

This class should be named: ez-nav-tabs--content-type

Show resolved Hide resolved src/bundle/Resources/views/admin/content_type/edit.html.twig Outdated
Show resolved Hide resolved src/bundle/Resources/views/admin/content_type/tab/view.html.twig Outdated
Show resolved Hide resolved src/lib/Form/Factory/ContentTypeFormFactory.php Outdated
Show resolved Hide resolved src/lib/Form/Factory/ContentTypeFormFactory.php Outdated
* @throws \eZ\Publish\API\Repository\Exceptions\NotFoundException
*/
private function getDefaultLanguage(ContentTypeDraft $contentTypeDraft
): Language {

This comment has been minimized.

Copy link
@adamwojs

adamwojs Dec 10, 2018

Member

NItpick: CS

Show resolved Hide resolved src/bundle/Controller/ContentTypeController.php Outdated
@alongosz
Copy link
Member

left a comment

cc @ViniTou
POV ping @webhdx

@@ -0,0 +1,3 @@
{% include '@ezdesign/content/modal_add_translation.html.twig' with {

This comment has been minimized.

Copy link
@alongosz

alongosz Dec 10, 2018

Member

Seems we have name collision here. ezplatform-ee-demo enables EzPlatformAutomatedTranslationBundle which overrides this template. Maybe this is one of the cases where we should go back to traditional Symfony/Twig way of including templates, so @EzPlatformAdminUi/content/modal_add_translation.html.twig.
This won't disable advantages of eZ Design. Developer will just need to explicitly override @ezdesign/admin/content_type/modal_add_translation.html.twig, which makes perfect sense TBH.

@alongosz alongosz changed the title EZP-29757: As an administrator, I want to be able to translate a content type (AdminUI frontend) EZP-29757: As an administrator, I want to be able to translate a content type Dec 10, 2018

@ViniTou ViniTou force-pushed the ViniTou:EZP-29757-content-type-translate branch from 4e58662 to 5655610 Dec 14, 2018

@ezsystems ezsystems deleted a comment from ezrobot Dec 14, 2018


{% if language_code != content_type.mainLanguageCode %}
{% include '@admin/admin/content_type/parts/nontranslatable_fields_disabled.html.twig' %}

This comment has been minimized.

Copy link
@webhdx

webhdx Dec 14, 2018

Contributor

Was it meant to be @ezdesign?

This comment has been minimized.

Copy link
@ViniTou

ViniTou Dec 14, 2018

Author Contributor

yup, it should be safe this time ;)

@ViniTou

This comment has been minimized.

Copy link
Contributor Author

commented Dec 14, 2018

1,2,3 - fixed
4 - content type translate, similar to content type edit requires content type create policy as contentType draft is created along the way. Now you need to have two policies to have buttons available.
5 - fixed
6 - fixed in PR to page builder

maybe there is a followup story for that already

there is non, but there will be as I had discussed that with @alongosz

private $searchService;
/** @var \EzSystems\EzPlatformAdminUiBundle\Templating\Twig\UniversalDiscoveryExtension */
private $udwExtension;

This comment has been minimized.

Copy link
@webhdx

webhdx Dec 14, 2018

Contributor

This is never used. Looks like more properties are never used.

@ViniTou

This comment has been minimized.

Copy link
Contributor Author

commented Dec 14, 2018

@webhdx
both fixed

@mnocon

mnocon approved these changes Dec 14, 2018

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

1 of 2 checks passed

continuous-integration/travis-ci/pr The Travis CI build failed
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.