EZP-26751: Value of fields not flagged as Translatable can be updated #788

yannickroger · 2017-01-27T08:29:34Z

Link: https://jira.ez.no/browse/EZP-26751

Description

Field that can not be translated are not flagged as they should on other languages. They also can be updated. This PR fixes that.

Tests

Manual test and unit test update

TODO

Squash commits

dpobel · 2017-01-27T08:39:51Z

Resources/public/js/views/ez-contenteditformview.js

@@ -85,6 +88,7 @@ YUI.add('ez-contenteditformview', function (Y) {
                            field: field,
                            config: config,
                            languageCode: languageCode,
+                            isNotTranslatable: isNotTranslatable,


not super convinced by the name but I don't have a better idea at the moment

dpobel · 2017-01-27T08:40:05Z

Resources/public/templates/fields/edit/checkbox.hbt

@@ -1,4 +1,4 @@
-<div class="pure-g ez-editfield-row">
+<div class="pure-g ez-editfield-row {{#if isNotTranslatable}}ez-editfield-not-translatable{{/if}}">


I would add this class on the view container instead. Also, given it represents a state, it should be name is-not-translatable (or something like that depending on the name of this state)

dpobel · 2017-01-27T08:42:39Z

Resources/public/css/theme/views/contenteditform.css

@@ -24,3 +24,22 @@
 .ez-view-contenteditformview .is-collapsed .fieldgroup-name:after {
    content: "\E604";
 }
+
+.ez-view-contenteditformview .ez-editfield-not-translatable-msg {


this should not be in contenteditform.css but in fieldedit.css as you actually style a field edit view

dpobel · 2017-01-27T08:48:29Z

Resources/public/js/views/ez-contenteditformview.js


                if (field) {
                    try {
+                        isNotTranslatable = !(def.isTranslatable || languageCode === content.get('mainLanguageCode'));


did you check that when creating a Content item ?

dpobel · 2017-01-27T14:29:21Z

Resources/public/js/views/ez-fieldeditview.js

+
+            this.set('isNotTranslatable', this.get('translating') && !this.get('fieldDefinition').isTranslatable);
+
+            this.after('activeChange', Y.bind(function() {


you should subscribe to isNotTranslatableChange event instead

dpobel · 2017-01-27T14:31:19Z

Resources/public/js/views/ez-fieldeditview.js

@@ -276,6 +277,14 @@ YUI.add('ez-fieldeditview', function (Y) {
            this.after('errorStatusChange', this._errorUI);

            this._addDOMEventHandlers(_events);
+
+            this.set('isNotTranslatable', this.get('translating') && !this.get('fieldDefinition').isTranslatable);


isNotTranslatable should be marked as read only since it is a computed value so here you should use _set and the attribute should have the read only flag

dpobel · 2017-02-09T15:31:03Z

Resources/public/js/views/fields/ez-richtext-editview.js

@@ -351,7 +357,11 @@ YUI.add('ez-richtext-editview', function (Y) {
         * @return {Object}
         */
        _getFieldValue: function () {
-            var value = this._getXHTML5EditValue();
+            var value = "";


most likely you should init it with the field value

yannickroger · 2017-02-21T08:37:19Z

Feedback taken into account @dpobel and PR is complete fo review.

andrerom · 2017-02-21T13:58:59Z

As this is support issue this should be against 1.7 branch right?

yannickroger · 2017-02-21T14:21:35Z

@andrerom I was considering it at first. But the diff is quite big and it kind of changes the behavior so it is more than a bugfix.
@dpobel often referred this as an improvement as it is a border line bug (everything works without this patch, the interface is just misleading).

andrerom · 2017-02-21T16:18:08Z

It is indeed border line, but the size of a change is not what we choose to call it a bug or not on, with test coverage, own manual testing, QA story testing, patch release certification, you should be able to be confident in what you are proposing and decide depending on if it can provide greater customer value then the risk it introduces.

bdunogier · 2017-02-21T18:43:37Z

The fact remains that this patch is much more of an improvement, or even a new feature, than a bugfix. The feature fixes a bug for a user, but it is still a feature, and we do not backport features as per semver.

Could we think of a limited, less intrusive version of the feature for 1.7, that would unblock the user ?

yannickroger · 2017-02-21T18:56:44Z

We can provide a patch that applies on 1.7 for users wanting it, but they would have to reapply it after each update.
Or split the fix so it could be applied only on non translatable fields that they use to have a smaller codeprint.

andrerom · 2017-02-22T07:27:02Z

providing patch but not applying it to 1.7 won't give much value here, my comment was about being clear and decide on bug / improvement, I have updated the comment to more clearly reflect that.

dpobel

some small changes to make but overall it's good.
For the target branch, given the patch, I think we can exceptionally apply it to 1.7 even if it does not strictly comply with semver and that it actually provides a new feature. But that means the QA will have to be very complete to avoid regressions.

dpobel · 2017-02-22T08:32:44Z

Resources/public/js/views/ez-fieldeditview.js

+             *
+             * @attribute translating
+             * @type {Boolean}
+             * @default null


you should remove the default value and mark the attribute as required

dpobel · 2017-02-22T08:33:40Z

Resources/public/js/views/ez-fieldeditview.js

+             * True if the field is not translatable
+             *
+             * @attribute isNotTranslatable
+             * @readonly


@readOnly (unsure if yuidoc understands the @readonly)

dpobel · 2017-02-22T08:35:38Z

Resources/public/js/views/ez-fieldeditview.js

+             * @default null
+             */
+            isNotTranslatable: {
+                value: null,


I would put false as the default value

dpobel · 2017-02-22T08:48:10Z

Resources/public/css/views/fieldedit.css

+}
+
+.is-not-translatable .ez-editfield-not-translatable:before {
+    content: "\e61b";


this should be in the theme css

dpobel · 2017-02-22T08:49:59Z

Resources/public/css/views/fields/edit/author.css

@@ -32,3 +32,7 @@
    display: block;
    margin-top: 3em;
 }
+
+.ez-view-authoreditview.is-not-translatable .ez-field-sublabel {
+    opacity: 0.3;


all the opacity changes (in this CSS file and the others) should be moved in the theme CSS

yannickroger · 2017-02-22T13:42:24Z

Feedback by @dpobel taken into account. Base branch is now 1.7.

andrerom

LGTM (side: ping @sunpietro, maybe check if there is something on studio side that needs to be adapted for this, I doubt it but I ping anyway ;))

sunpietro · 2017-03-09T08:34:34Z

Looks good 👍 it should not break anything in Studio

dpobel suggested changes Jan 27, 2017

View reviewed changes

dpobel reviewed Jan 27, 2017

View reviewed changes

dpobel suggested changes Jan 27, 2017

View reviewed changes

yannickroger force-pushed the ezp-26751-translatable_fields branch from 7e374e7 to e806c53 Compare January 27, 2017 15:14

yannickroger force-pushed the ezp-26751-translatable_fields branch 2 times, most recently from 916319c to 5127239 Compare February 9, 2017 10:45

dpobel reviewed Feb 9, 2017

View reviewed changes

yannickroger force-pushed the ezp-26751-translatable_fields branch from 5a48e0a to 85c5609 Compare February 10, 2017 10:59

yannickroger force-pushed the ezp-26751-translatable_fields branch 3 times, most recently from ddf5c65 to 8833c25 Compare February 21, 2017 08:35

yannickroger requested a review from StephaneDiot February 21, 2017 08:36

dpobel suggested changes Feb 22, 2017

View reviewed changes

yannickroger force-pushed the ezp-26751-translatable_fields branch from 8833c25 to 4a643b9 Compare February 22, 2017 13:36

yannickroger changed the base branch from master to 1.7 February 22, 2017 13:37

yannickroger force-pushed the ezp-26751-translatable_fields branch from 4a643b9 to fa52c1c Compare February 22, 2017 13:41

StephaneDiot approved these changes Feb 27, 2017

View reviewed changes

andrerom approved these changes Mar 8, 2017

View reviewed changes

EZP-26751: Value of fields not flagged as Translatable can be updated

fbfa46f

yannickroger force-pushed the ezp-26751-translatable_fields branch from e6476d9 to fbfa46f Compare March 14, 2017 08:46

dpobel approved these changes Mar 14, 2017

View reviewed changes

yannickroger merged commit 4d1ee39 into 1.7 Mar 14, 2017

yannickroger deleted the ezp-26751-translatable_fields branch March 14, 2017 09:49

yannickroger mentioned this pull request Mar 29, 2017

Fix EZP-27147: Not translatable binary fields can be updated using drag&drop #828

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

EZP-26751: Value of fields not flagged as Translatable can be updated #788

EZP-26751: Value of fields not flagged as Translatable can be updated #788

yannickroger commented Jan 27, 2017 •

edited

Loading

dpobel Jan 27, 2017

dpobel Jan 27, 2017

dpobel Jan 27, 2017

dpobel Jan 27, 2017

dpobel Jan 27, 2017

dpobel Jan 27, 2017

dpobel Feb 9, 2017

yannickroger commented Feb 21, 2017

andrerom commented Feb 21, 2017

yannickroger commented Feb 21, 2017

andrerom commented Feb 21, 2017 •

edited

Loading

bdunogier commented Feb 21, 2017 •

edited

Loading

yannickroger commented Feb 21, 2017

andrerom commented Feb 22, 2017 •

edited

Loading

dpobel left a comment •

edited

Loading

dpobel Feb 22, 2017

dpobel Feb 22, 2017

dpobel Feb 22, 2017

dpobel Feb 22, 2017

dpobel Feb 22, 2017

yannickroger commented Feb 22, 2017 •

edited

Loading

andrerom left a comment

sunpietro commented Mar 9, 2017

		@@ -1,4 +1,4 @@
		<div class="pure-g ez-editfield-row">
		<div class="pure-g ez-editfield-row {{#if isNotTranslatable}}ez-editfield-not-translatable{{/if}}">


		this.set('isNotTranslatable', this.get('translating') && !this.get('fieldDefinition').isTranslatable);

		this.after('activeChange', Y.bind(function() {

EZP-26751: Value of fields not flagged as Translatable can be updated #788

EZP-26751: Value of fields not flagged as Translatable can be updated #788

Conversation

yannickroger commented Jan 27, 2017 • edited Loading

Description

Tests

TODO

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

yannickroger commented Feb 21, 2017

andrerom commented Feb 21, 2017

yannickroger commented Feb 21, 2017

andrerom commented Feb 21, 2017 • edited Loading

bdunogier commented Feb 21, 2017 • edited Loading

yannickroger commented Feb 21, 2017

andrerom commented Feb 22, 2017 • edited Loading

dpobel left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

yannickroger commented Feb 22, 2017 • edited Loading

andrerom left a comment

Choose a reason for hiding this comment

sunpietro commented Mar 9, 2017

yannickroger commented Jan 27, 2017 •

edited

Loading

andrerom commented Feb 21, 2017 •

edited

Loading

bdunogier commented Feb 21, 2017 •

edited

Loading

andrerom commented Feb 22, 2017 •

edited

Loading

dpobel left a comment •

edited

Loading

yannickroger commented Feb 22, 2017 •

edited

Loading