EZP-29290: As an Editor, I want to have preconfigured "Author" section while creating the content #2392

konradoboza · 2018-07-11T13:58:42Z

Question	Answer
JIRA issue	EZP-29290
Bug/Improvement	no
New feature	yes
Target version	`master`
BC breaks	no
Tests pass	yes
Doc needed	yes

JIRA excerpt:

Currently, in v2 there is no preconfigured author section (fields filled in with the name and e-mail of the currently logged user). It would be more convenient for the editors to have this fields set automatically while creating a new content. Also, prefilling section with the default data should be configurable at the Author FieldType level.

This is the kernel implementation of this feature. It is followed by the changes in repository-forms and admin-ui-bundle which will enable a user to choose whether Author FieldType should be prefilled by default from the ContentType creation view.

Is required by:

TODO:

Implement feature / fix a bug.
Implement tests.
Fix new code according to Coding Standards ($ composer fix-cs).
Ask for Code Review.

gggeek · 2018-07-11T14:24:32Z

I'd say default for the fieldType should be Empty, not current-user.

In fact in more than 10 years using eZ, I have never seen anyone using an author field to store the current user. That info is embedded into content metadata anyway. Whenever an author field is used, it is because the person logged in to input the data is not the author that has to be shown as content creator

konradoboza · 2018-07-12T07:04:01Z

@gggeek thank you for the input. Default option could be easily changed, that's not a problem at all. Prefilling author FieldType with current user's data was present in eZ Publish and eZ Platform v1 so as a matter of consistency we would like to have it also in v2. We also aim to give the Editor possibility to keep current behavior as well.

…n while creating the content

…dtype' into EZP-29290-prefilling_author_fieldtype # Conflicts: # eZ/Publish/Core/Persistence/Legacy/Content/FieldValue/Converter/AuthorConverter.php # eZ/Publish/Core/Persistence/Legacy/Tests/Content/FieldValue/Converter/AuthorTest.php # eZ/Publish/SPI/Tests/FieldType/AuthorIntegrationTest.php

konradoboza · 2018-07-12T09:31:57Z

Few things I'm not sure about:

is injecting Repository into AuthorConverter the best way to solve it? Tests got more complicated, maybe I'm missing something.
test is most likely failing because of this https://github.com/ezsystems/ezpublish-kernel/blob/master/eZ/Publish/API/Repository/Tests/FieldType/BaseIntegrationTest.php#L1038. when we decide to have author data prefilled by default this array won't be empty. How should I fix it?
should logic responsible for creating mocks which is duplicated in SPI/Tests/FieldType/AuthorIntegrationTest.php and Core/Persistence/Legacy/Tests/Content/FieldValue/Converter/AuthorTest.php be moved to e.g. Trait?

Any advice would be much appreciated :) @andrerom @alongosz

andrerom · 2018-07-16T14:29:00Z

is injecting Repository into AuthorConverter the best way to solve it? Tests got more complicated, maybe I'm missing something.

That is probably not the way to do it, as Converter is performed down in Storage Engine and should now know about repository, besides circular references.

As for how and the other question maybe @alongosz and @bdunogier can give you some input.

konradoboza · 2018-08-02T11:17:44Z

I changed approach towards implementing this feature in repository-forms. Followup PR's need to wait for ezsystems/repository-forms#249 as it requires further changes.
In the meantime, please review @andrerom @alongosz.

andrerom · 2018-08-02T12:08:22Z

eZ/Publish/Core/FieldType/Author/Type.php

+    /**
+     * Default value empty.
+     */
+    const DEFAULT_EMPTY = -1;


Any reason why you picked -1 over for instance 0? Other cases you found like that?
Asking as I can't right now remember us using negative ints much for int fields.

I was having hard times to differentiate this value from this which I got from the database for existing ContentTypes (e.g. Article), as we wanted to have prefilling by default there.

Ok, so you're not using 0 as todays default (Null) for data_int1. is casted to int in Storage engine so it becomes 0, which is causing your trouble to decide on value. Right?

Is 0 mapped to DEFAULT_CURRENT_USER somewhere for us btw, so editing existing data works as intended?

@alongosz SIDE: you see any issues with using negative int's for data_int1 column? Or should we rather keep the values in the positive range?

Ok, so you're not using 0 as todays default (Null) for data_int1. is casted to int in Storage engine so it becomes 0, which is causing your trouble to decide on value. Right?

Yes, that was my point.

Is 0 mapped to DEFAULT_CURRENT_USER somewhere for us btw, so editing existing data works as intended?

So you are suggesting switching values? (0, both for prefilling chosen by editor and also for no value from db, 1 for no-prefilling option). It could be confusing, but probably would work.

So you are suggesting switching values?

not necessarily, before even suggesting that I was asking how the system behaves when db has null value. Both when it comes to content type view/edit and for content edit.

context: People upgrading would expect default value to just be current user somehow, so if code here handles that or if we are missing upgrade script where those upgrading can choose between setting these to 1 for user or -1/2 for todays null value.

@andrerom I tested adding/editing content/content type with/without prefilling, everything seems to be working fine.

@alongosz SIDE: you see any issues with using negative int's for data_int1 column? Or should we rather keep the values in the positive range?

No, this is fine. data_int1 is NOT NULL anyway, so it has to be something that is non-null and non-zero.

@konradoboza what I don't get is how these constants apply to this solution? What is their purpose?

The naming is very generic and the PhpDoc description says the same thing, which is the perfect example of how not to document code

I can guess based on the other parts of the PR, but if I need to guess it, then it implies it needs an improvement :-)
I prefer you find some better naming so code is either self-commenting or requires only brief explanation. If you can't find better name, then at least provide more descriptive doc block.

... or maybe, since this is a default value, it should belong to the Value class, not type? Even despite the fact that you're using it in the Type class.

context: People upgrading would expect default value to just be current user somehow,

@andrerom Actually... No. Why? This is BC break.

I know PMs really want this by default, so AdminUI can set it differently.

From my POV both cases would be BC break, but we should at least make API BC.

cc @SylvainGuittard

andrerom · 2018-08-03T08:25:59Z

@konradoboza I'm wondering if this phase 2 on kernel and repo forms should rather go to master as it's a new feature and not requested by customer so we don't need to rush it. Would that work for you?

konradoboza · 2018-08-03T08:29:22Z

@andrerom sure, it is fine with me. So this one should be rebased to master, how about the other two?

andrerom · 2018-08-03T08:34:53Z

@konradoboza so which one depends on which? Which PR's are "phase 1", aka minimal scope, and which once are followups to add config in field type?

It's the followups I'm suggesting we take to master basically, leaving phase 1 for the shortly upcoming 2.2.2 release.

konradoboza · 2018-08-03T08:40:00Z

@andrerom all three PR's should be treated as one feature, so this is "phase 2", I updated descriptions for ezsystems/repository-forms#251 and ezsystems/ezplatform-admin-ui#586 as they depend on kernel.

"Phase 1" was already merged and was about having prefilled Author FieldType, without having a config. Ref: ezsystems/repository-forms#249

andrerom · 2018-08-03T08:48:06Z

Ok, clear. Then go for master on the remaining PR's, should not have any conflicts so hopefully simple rebase ;)

andrerom · 2018-08-03T08:48:41Z

~~Actually, we can try to just change target on PR's, if it does not conflict we are good!~~ Done

konradoboza · 2018-08-03T13:23:09Z

Fixed checking if any value came from the database in order to have a default selected radio in the Content Type edit view for existing ones. 9a91b9c#diff-564956cdbfd9b8b86cfb41dc3ddbf243R79

andrerom · 2018-08-06T08:46:51Z

eZ/Publish/Core/REST/Common/Tests/FieldTypeProcessor/AuthorProcessorTest.php

+        return array_map(
+            function ($constantName) {
+                return array(
+                    array('defaultAuthor' => $constantName),


[]

Aka use [] in new code and lines that are changed, and leave array in places you don't change to unnecessary branch conflicts.

andrerom · 2018-08-06T08:47:11Z

eZ/Publish/SPI/Tests/FieldType/AuthorIntegrationTest.php

@@ -98,17 +109,17 @@ public function getFieldDefinitionData()
    public function getInitialValue()
    {
        return new Content\FieldValue(
-            array(
-                'data' => array(
-                    array(


can skip these changes

andrerom · 2018-08-06T08:47:42Z

eZ/Publish/Core/Persistence/Legacy/Content/FieldValue/Converter/AuthorConverter.php

@@ -123,20 +131,20 @@ private function generateXmlString(array $authorValue)
     *
     * @param string $xmlString XML String stored in storage engine
     *
-     * @return \eZ\Publish\Core\FieldType\Author\Value
+     * @return array


\eZ\Publish\Core\FieldType\Author\Value[]

andrerom · 2018-08-06T08:47:50Z

eZ/Publish/Core/Persistence/Legacy/Content/FieldValue/Converter/AuthorConverter.php

     */
    private function restoreValueFromXmlString($xmlString)
    {
        $dom = new DOMDocument('1.0', 'utf-8');
-        $authors = array();
+        $authors = [];


can skip these changes

…n while creating the content

konradoboza · 2018-08-06T09:36:53Z

I reverted all the unnecessary changes to array short notation. When there were [] and array() in one file I changed these to array() to be consistent with the previous versions and with the other tests.

alongosz · 2018-08-08T08:59:28Z

eZ/Publish/API/Repository/Tests/FieldType/AuthorIntegrationTest.php

@@ -192,7 +198,7 @@ public function provideInvalidCreationFieldData()
    /**
     * Get update field externals data.
     *
-     * @return array
+     * @return AuthorValue


Please use FQCN

alongosz · 2018-08-08T09:13:41Z

eZ/Publish/Core/FieldType/Author/Type.php

+    /**
+     * Validates the fieldSettings of a FieldDefinitionCreateStruct or FieldDefinitionUpdateStruct.
+     *
+     * @param mixed $fieldSettings


this is always array, right? Although we can't change interface prototype, I think here we can safely say what we expect.

Should it be changed for every Type class then? As I see mixed is used in all FieldTypes which have settings.

No, let's not get out of scope here. It is indeed inconsistent but let's not duplicate past mistakes.

alongosz · 2018-08-08T09:20:20Z

eZ/Publish/Core/FieldType/Author/Type.php

+            }
+
+            switch ($name) {
+                case 'defaultAuthor':


This looks really weird. Probably taken from other implementations, right?

It would make sense if there were more settings. Here what I'd do is early continue: if ($name !== 'defaultAuthor') { continue; } and get rid of unnecessary indent of the remainder of the code. We should refactor this to switch only when we get to a point where we need more settings, because effort is not gonna be big (it really is a gut feeling when to prepare for possible future and when not to).

If you still prefer the switch, then I'd see logic for the case moved to a separate method.

Just for readability.

The question is whether we want to be consistent with the other implementations or improve things when it comes to FieldTypes, https://github.com/ezsystems/ezpublish-kernel/blob/master/eZ/Publish/Core/FieldType/Date/Type.php#L224.

Each Field Type can be treated a separate, so implementations can differ. What we need to avoid is switching code style approaches between valid alternatives. For example pushing towards short array syntax was my mistake, it should be done for entire codebase at once.

In this case I find Field Type implementations quite confusing, so I would rather improve.

That said, maybe just extract contents of this case block to a separate method? It keeps somewhat consistency and improves readability.

alongosz · 2018-08-08T09:25:13Z

eZ/Publish/Core/FieldType/Author/Type.php

+                    ];
+                    if (!in_array($value, $definedTypes, true)) {
+                        $validationErrors[] = new ValidationError(
+                            "Setting '%setting%' is of unknown type",


I think this kinda relates to previous comment about naming - this setting is not a "type" to me. It's a default value, right?

Yes, it is actually my bad, $definedValues sounds much better.

alongosz · 2018-08-08T09:29:33Z

eZ/Publish/Core/Persistence/Legacy/Content/FieldValue/Converter/AuthorConverter.php

@@ -61,7 +63,7 @@ public function toFieldValue(StorageFieldValue $value, FieldValue $fieldValue)
     */
    public function toStorageFieldDefinition(FieldDefinition $fieldDef, StorageFieldDefinition $storageDef)
    {
-        // Nothing to store
+        $storageDef->dataInt1 = $fieldDef->fieldTypeConstraints->fieldSettings['defaultAuthor'];


Please cast it to (int) or PostgreSQL will die should this data come from an actual web form. Unless it is already casted elsewhere?

alongosz · 2018-08-08T09:30:56Z

eZ/Publish/Core/Persistence/Legacy/Tests/Content/FieldValue/Converter/AuthorTest.php

@@ -22,6 +27,7 @@
 */
 class AuthorTest extends TestCase
 {
+    const ADMINISTRATOR_USER_ID = 14;


It doesn't seem it is used.

Great catch! I must have forgotten to remove it alongside with the mocks from the previous implementation.

alongosz · 2018-08-08T09:31:54Z

eZ/Publish/Core/Persistence/Legacy/Tests/Content/FieldValue/Converter/AuthorTest.php

 use eZ\Publish\Core\Persistence\Legacy\Content\FieldValue\Converter\AuthorConverter;
+use eZ\Publish\SPI\Persistence\Content\Type\FieldDefinition as PersistenceFieldDefinition;


The naming convention is rather SPIFieldDefinition.

alongosz · 2018-08-08T09:39:59Z

eZ/Publish/Core/FieldType/Author/Type.php

+    /**
+     * Default value empty.
+     */
+    const DEFAULT_EMPTY = -1;


@alongosz SIDE: you see any issues with using negative int's for data_int1 column? Or should we rather keep the values in the positive range?

No, this is fine. data_int1 is NOT NULL anyway, so it has to be something that is non-null and non-zero.

@konradoboza what I don't get is how these constants apply to this solution? What is their purpose?

The naming is very generic and the PhpDoc description says the same thing, which is the perfect example of how not to document code

I can guess based on the other parts of the PR, but if I need to guess it, then it implies it needs an improvement :-)
I prefer you find some better naming so code is either self-commenting or requires only brief explanation. If you can't find better name, then at least provide more descriptive doc block.

... or maybe, since this is a default value, it should belong to the Value class, not type? Even despite the fact that you're using it in the Type class.

context: People upgrading would expect default value to just be current user somehow,

@andrerom Actually... No. Why? This is BC break.

I know PMs really want this by default, so AdminUI can set it differently.

From my POV both cases would be BC break, but we should at least make API BC.

cc @SylvainGuittard

alongosz · 2018-08-08T09:47:43Z

eZ/Publish/Core/REST/Common/FieldTypeProcessor/AuthorProcessor.php

+                    $incomingSettingsHash['defaultAuthor'] = Type::DEFAULT_EMPTY;
+                    break;
+                default:
+                    $incomingSettingsHash['defaultAuthor'] = Type::DEFAULT_CURRENT_USER;


as I've said above - it's a BC break.

alongosz · 2018-08-08T09:59:36Z

eZ/Bundle/EzPublishCoreBundle/Resources/views/fielddefinition_settings.html.twig

+    {% endif %}
+    {{ block( 'settings_defaultvalue' ) }}
+</ul>
+{% endblock %}


Front-end review ping @dew326 Just kidding.

@webhdx, could you take a look? Looks good to me, but with Twigs I don't trust myself :P

It looks okay.

konradoboza · 2018-08-08T14:19:13Z

Suggestions from @alongosz applied. Regarding BC: in v1 and Legacy we had prefilled Author data by default. Within the scope of this (and related) PR we give editors an option in CT creation view. Existing CT's (e.g. article) will be prefilled by default. /cc @lserwatka

…n while creating the content

lserwatka · 2018-08-09T07:57:08Z

@barbaragr let's proceed with QA here.

alongosz · 2018-08-09T08:25:56Z

Code looks now good.

If we want to merge a BC break, at least please add a note in doc/bc stating that since new version, Author Field Type will be prefilled with current user by default.

I'm against making BC breaks in places we have a choice (e.g. not forced by bugs), but since this is what PMs want, BC note is the least we could do.

konradoboza · 2018-08-09T09:39:05Z

BC break note added in: #2411

WIP prefilling author field type

2748513

konradoboza added the Work in progress label Jul 11, 2018

konradoboza added 2 commits July 12, 2018 10:22

EZP-29290: As an Editor, I want to have preconfigured "Author" sectio…

3af4346

…n while creating the content

ezsystems deleted a comment from ezrobot Jul 12, 2018

konradoboza requested review from alongosz, andrerom and kmadejski July 12, 2018 09:32

konradoboza mentioned this pull request Jul 30, 2018

EZP-29138: Author FieldType isn't prefilled with logged user data ezsystems/repository-forms#249

Merged

3 tasks

konradoboza changed the title ~~[WiP] EZP-29290: As an Editor, I want to have preconfigured "Author" section while creating the content~~ EZP-29290: As an Editor, I want to have preconfigured "Author" section while creating the content Aug 2, 2018

konradoboza added Ready for review and removed Work in progress labels Aug 2, 2018

andrerom reviewed Aug 2, 2018

View reviewed changes

andrerom changed the base branch from 7.2 to master August 3, 2018 08:48

konradoboza force-pushed the EZP-29290-prefilling_author_fieldtype branch 2 times, most recently from db992b2 to 9a91b9c Compare August 3, 2018 13:21

andrerom approved these changes Aug 6, 2018

View reviewed changes

konradoboza force-pushed the EZP-29290-prefilling_author_fieldtype branch from 9a91b9c to 87481f2 Compare August 6, 2018 09:30

EZP-29290: As an Editor, I want to have preconfigured "Author" sectio…

836060c

…n while creating the content

konradoboza force-pushed the EZP-29290-prefilling_author_fieldtype branch from 87481f2 to 836060c Compare August 6, 2018 09:33

alongosz requested changes Aug 8, 2018

View reviewed changes

konradoboza force-pushed the EZP-29290-prefilling_author_fieldtype branch from a0cd246 to d6728bd Compare August 8, 2018 14:36

ezsystems deleted a comment from ezrobot Aug 8, 2018

EZP-29290: As an Editor, I want to have preconfigured "Author" sectio…

7f4bc34

…n while creating the content

konradoboza force-pushed the EZP-29290-prefilling_author_fieldtype branch from d6728bd to 7f4bc34 Compare August 9, 2018 07:54

lserwatka added the Ready for QA label Aug 9, 2018

kmadejski approved these changes Aug 9, 2018

View reviewed changes

alongosz mentioned this pull request Aug 9, 2018

Added Author Field Type default prefilling BC break note #2411

Closed

alongosz approved these changes Aug 9, 2018

View reviewed changes

lserwatka merged commit 766c4a3 into ezsystems:master Aug 9, 2018

konradoboza mentioned this pull request Aug 13, 2018

EZP-29290: Author Field Type prefilled with empty value by default #2417

Merged

4 tasks

		use eZ\Publish\Core\Persistence\Legacy\Content\FieldValue\Converter\AuthorConverter;
		use eZ\Publish\SPI\Persistence\Content\Type\FieldDefinition as PersistenceFieldDefinition;

EZP-29290: As an Editor, I want to have preconfigured "Author" section while creating the content #2392

EZP-29290: As an Editor, I want to have preconfigured "Author" section while creating the content #2392

Conversation

konradoboza commented Jul 11, 2018 • edited

gggeek commented Jul 11, 2018 • edited

konradoboza commented Jul 12, 2018

konradoboza commented Jul 12, 2018 • edited

andrerom commented Jul 16, 2018

konradoboza commented Aug 2, 2018 • edited

Choose a reason for hiding this comment

konradoboza Aug 2, 2018 • edited

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

andrerom commented Aug 3, 2018 • edited

konradoboza commented Aug 3, 2018 • edited

andrerom commented Aug 3, 2018 • edited

konradoboza commented Aug 3, 2018

andrerom commented Aug 3, 2018

andrerom commented Aug 3, 2018 • edited

konradoboza commented Aug 3, 2018

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

konradoboza commented Aug 6, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

konradoboza Aug 8, 2018 • edited

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

konradoboza Aug 8, 2018 • edited

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

konradoboza commented Aug 8, 2018

lserwatka commented Aug 9, 2018

alongosz commented Aug 9, 2018

konradoboza commented Aug 9, 2018

konradoboza commented Jul 11, 2018 •

edited

gggeek commented Jul 11, 2018 •

edited

konradoboza commented Jul 12, 2018 •

edited

konradoboza commented Aug 2, 2018 •

edited

konradoboza Aug 2, 2018 •

edited

andrerom commented Aug 3, 2018 •

edited

konradoboza commented Aug 3, 2018 •

edited

andrerom commented Aug 3, 2018 •

edited

andrerom commented Aug 3, 2018 •

edited

konradoboza Aug 8, 2018 •

edited

konradoboza Aug 8, 2018 •

edited