EZP-28841: Content On the Fly v2 #366

dew326 · 2018-03-02T08:15:13Z

Question	Answer
Tickets	https://jira.ez.no/browse/EZP-28841
Requires	ezsystems/ezplatform-admin-ui-modules#55
Bug fix?	no
New feature?	yes
BC breaks?	no
Tests pass?	yes
Doc needed?	no
License	GPL-2.0

Checklist:

Coding standards ($ composer fix-cs)
Ready for Code Review
restrict non-container in trash restoring (https://jira.ez.no/browse/EZP-28027)
expose languageCode in meta
check permission endpoint
route names in ezplatform-admin-ui-modules: EZP-28841: Content On the Fly v2 #366 (comment)

ViniTou

just some phpdocs nitpicks on my part

ViniTou · 2018-03-07T07:56:26Z

src/bundle/Controller/ContentController.php

+    /** @var LocationService */
+    private $locationService;
+
+    private $contentActionDispatcher;


missing phpdoc here and in constructor

ViniTou · 2018-03-07T08:04:34Z

src/bundle/Controller/ContentController.php

+     * @param ContentType $contentType
+     * @param Location $parentLocation
+     *
+     * @return ContentCreateView


ContentCreateView | Response
?

ViniTou · 2018-03-07T08:12:06Z

src/lib/RepositoryForms/Form/Processor/Content/ContentOnTheFlyProcessor.php

+{
+    private $contentService;
+
+    private $twig;


ViniTou · 2018-03-07T08:12:25Z

src/lib/RepositoryForms/View/ViewTemplatesListener.php

+
+    /**
+     * @var string
+     */


/** @var string */

ViniTou · 2018-03-07T08:12:57Z

src/lib/UI/Config/Provider/Module/UniversalDiscoveryWidget.php

+    }
+
+    /**
+     * @return int


int|null according to typehint

mnocon · 2018-03-07T11:13:12Z

@Nattfarinn have you seen the PHPUnit failure?

Nattfarinn · 2018-03-07T16:37:05Z

@ViniTou Fixed.
@mnocon Fixed.

mnocon · 2018-03-08T10:38:07Z

Three things so far:

we do not check if the user has content/publish permission, so it's possible to receive a 403 inside UDW
it looks like there are infinite permission checking requests in the Network tab
In RichText's embed image the language is restricted to the current one, but in ezimage field type it does not happen (all languages are available). Ezimage field type should also have restricted languages available.

bdunogier · 2018-03-08T12:14:01Z

Is there anything as for developer documentation ? I'm mostly thinking about how you integrate COTF in your custom code and how you may customize the locations/content types that are liste.

From the code's perspective, I think that a ContentOnTheFlyController would help keep ContentController thinner.

Nattfarinn · 2018-03-08T13:23:34Z

@mnocon

we do not check if the user has content/publish permission, so it's possible to receive a 403 inside UDW

Fixed.

Nattfarinn · 2018-03-08T13:41:42Z

@bdunogier

From the code's perspective, I think that a ContentOnTheFlyController would help keep ContentController thinner.

True. Now it has its own controller.

@dew326 Route names changed. I guess you have to update ezplatform-admin-ui-modules repository. It's WIP till fixed there.

-ezplatform.content.create.on_the_fly:
+ezplatform.content_on_the_fly.create:

-ezplatform.content.create.on_the_fly.has_access:
+ezplatform.content_on_the_fly.has_access:

lserwatka · 2018-03-08T14:07:41Z

@dew326 could you address this change so we can unblock QA here?

dew326 · 2018-03-08T14:13:36Z

@mnocon done, please update branch in ezplatform-admin-ui-modules.

mnocon · 2018-03-08T14:40:37Z

I cannot create anything now, the "Create content" button is disabled all the time. In Network tab, the request: http://v2eedemo.prod/admin/content/create/onthefly/article/eng-GB/43/hasaccess ends with 500 Internal Server Error 😞 :

Class "EzPlatformAdminUiBundle" does not exist. [in vendor/symfony/symfony/src/Symfony/Component/HttpKernel/Controller/ControllerResolver.php:180]

Nattfarinn · 2018-03-08T14:55:51Z

@mnocon Fixed

lserwatka · 2018-03-08T22:08:42Z

@Nattfarinn we need a rebase here too.

webhdx · 2018-03-12T08:47:36Z

src/bundle/Controller/ContentOnTheFlyController.php

+     * @return ContentCreateOnTheFlyView|Response
+     *
+     * @throws ApiException\NotFoundException
+     * @throws \eZ\Publish\Core\Base\Exceptions\InvalidArgumentType


We are using short class names instead of FQCN in doc block.

webhdx · 2018-03-12T08:47:52Z

src/bundle/Controller/ContentOnTheFlyController.php

+     * @throws ApiException\NotFoundException
+     * @throws \eZ\Publish\Core\Base\Exceptions\InvalidArgumentType
+     */
+    public function createContentAction(Request $request, string $languageCode, ContentType $contentType, Location $parentLocation)


Split into multiple lines.

webhdx · 2018-03-12T08:51:22Z

src/bundle/Resources/config/routing.yml

+        expose: true
+
+ezplatform.content_on_the_fly.has_access:
+    path: /content/create/onthefly/{contentTypeIdentifier}/{languageCode}/{locationId}/hasaccess


Idon'tlikeseeingstufflikethis 😅Can we use path: /content/create/onthefly/{contentTypeIdentifier}/{languageCode}/{locationId}/has-access ?

So... onthefly is fine, but hasaccess is not? :P

webhdx · 2018-03-12T08:53:48Z

src/lib/RepositoryForms/Form/Processor/Content/ContentOnTheFlyProcessor.php

+     * @throws \Twig_Error_Runtime
+     * @throws \Twig_Error_Syntax
+     * @throws \eZ\Publish\API\Repository\Exceptions\BadStateException
+     * @throws \eZ\Publish\API\Repository\Exceptions\ContentFieldValidationException


Short name instead of FQCN

We agreed to keep FQCN

webhdx · 2018-03-12T08:53:55Z

src/lib/RepositoryForms/Form/Processor/Content/ContentOnTheFlyProcessor.php

+     */
+    public function processPublish(FormActionEvent $event)
+    {
+        /** @var \EzSystems\RepositoryForms\Data\Content\ContentCreateData|\EzSystems\RepositoryForms\Data\Content\ContentUpdateData $data */


webhdx · 2018-03-12T08:57:15Z

src/lib/RepositoryForms/View/ViewTemplatesListener.php

+use eZ\Publish\Core\MVC\Symfony\MVCEvents;
+use Symfony\Component\EventDispatcher\EventSubscriberInterface;
+
+class ViewTemplatesListener implements EventSubscriberInterface


As we discussed you can set default templates on View object creation. That will make this listener no longer needed.

bdunogier · 2018-03-12T11:50:36Z

I've been through the code, and I'd like to know how flexible we are on the starting location / content type / language options. This includes existing inclusions of COTF, like Relation or RichText. For those, I'd like to keep the system flexible enough so that:

an integrator can hook into the business logic and adapt the choices to the customer
we can develop configurable mechanisms, such as "Create images in the Media/Images directory when creating content for the image field of the article type".

dew326 · 2018-03-12T12:34:57Z

@bdunogier Starting location and content type are not developed. The only configuration option is forced language, this is an option per usage in button config.

bdunogier · 2018-03-12T12:38:39Z

What about this ?

startingLocationId: window.eZ.adminUiConfig.universalDiscoveryWidget.startingLocationId

As far as I could see, the value comes from the Config Resolver.

My concern is that we limit our options in terms of BC. Without thinking it very far, ideally, there would be some kind of event, when COTF is loaded, that can be hooked into and lets you decide of those options. Our own subscriber would set it to the globally configured value for the first iteration. The two options I have described above would still be open for implementation / integration.

lserwatka · 2018-03-12T12:55:29Z

@Nattfarinn rebase is needed

Nattfarinn · 2018-03-12T13:02:46Z

@bdunogier I agree, but as far as I know, it was more like "Nice to have." instead of requirement. The scope of this was discussed on preplanning and planning, and turned out that we won't be able to deliver everything within one sprint. We can make improvements for next version I guess and I don't think refining a way how the values for a new UDW options/flags is picked will be a BC break.

bdunogier · 2018-03-12T13:08:40Z

To be honest, I'd like it to become a requirement. Do you think that a dedicated story, with detailed expectations, makes sense for this ?

As long as it can be done, I don't require it for that particular pull-request. On the other hand, having such an event from the start would also help us keep the doors open for any change we would have in mind.

The way I see it, the event would contain:

The "source" of the COTF request (content_edit, dashboard_block...)
Extra data related to each event:
- The content type being edited
- The content id for the current draft
- The location it is edited for
- ... (maybe this should be open)

That way, we can go as much in detail as we want, without requiring anything complex. Where would you see this happening ? I was thinking that one REST call, that would resolve the options, when clicking the COTF button, could work.

dew326 mentioned this pull request Mar 2, 2018

EZP-28841: Content On the Fly v2 ezsystems/ezplatform-admin-ui-modules#55

Merged

3 tasks

dew326 assigned dew326 and Nattfarinn Mar 2, 2018

dew326 added the Improvement label Mar 2, 2018

dew326 changed the title ~~[WIP] EZP-28841: Content On the Fly v2~~ EZP-28841: Content On the Fly v2 Mar 7, 2018

dew326 requested review from mikadamczyk, sunpietro, webhdx and ViniTou March 7, 2018 07:51

ezsystems deleted a comment from ezrobot Mar 7, 2018

Nattfarinn force-pushed the coft branch from de3be2e to 01d4347 Compare March 7, 2018 08:03

ViniTou reviewed Mar 7, 2018

View reviewed changes

lserwatka added the Ready for QA label Mar 7, 2018

Nattfarinn force-pushed the coft branch 2 times, most recently from 47070f8 to 80c792e Compare March 7, 2018 16:36

Nattfarinn force-pushed the coft branch from e7d7b60 to a0938f3 Compare March 8, 2018 13:46

ezsystems deleted a comment from ezrobot Mar 8, 2018

Nattfarinn changed the title ~~EZP-28841: Content On the Fly v2~~ (WIP) EZP-28841: Content On the Fly v2 Mar 8, 2018

Nattfarinn force-pushed the coft branch from a0938f3 to 1406559 Compare March 8, 2018 13:54

ezsystems deleted a comment from ezrobot Mar 8, 2018

lserwatka changed the title ~~(WIP) EZP-28841: Content On the Fly v2~~ EZP-28841: Content On the Fly v2 Mar 8, 2018

Nattfarinn force-pushed the coft branch from 1406559 to 2b4ab62 Compare March 8, 2018 14:55

mnocon removed the Ready for QA label Mar 9, 2018

webhdx suggested changes Mar 12, 2018

View reviewed changes

Nattfarinn force-pushed the coft branch 2 times, most recently from 40863eb to 5d16387 Compare March 12, 2018 11:17

webhdx approved these changes Mar 12, 2018

View reviewed changes

Nattfarinn force-pushed the coft branch from 5d16387 to e9fbb7e Compare March 12, 2018 13:06

EZP-28841: Content On the Fly v2

76ead8c

Nattfarinn force-pushed the coft branch from e9fbb7e to 76ead8c Compare March 12, 2018 13:43

lserwatka approved these changes Mar 12, 2018

View reviewed changes

lserwatka merged commit cb56e99 into ezsystems:master Mar 12, 2018

mateuszdebinski pushed a commit that referenced this pull request Oct 25, 2022

IBX-1593: view switcher - simple dropdown (#366)

b3bb49e

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

EZP-28841: Content On the Fly v2 #366

EZP-28841: Content On the Fly v2 #366

dew326 commented Mar 2, 2018 •

edited by lserwatka

Loading

ViniTou left a comment

ViniTou Mar 7, 2018

ViniTou Mar 7, 2018

ViniTou Mar 7, 2018

ViniTou Mar 7, 2018

ViniTou Mar 7, 2018

mnocon commented Mar 7, 2018

Nattfarinn commented Mar 7, 2018

mnocon commented Mar 8, 2018 •

edited

Loading

bdunogier commented Mar 8, 2018

Nattfarinn commented Mar 8, 2018

Nattfarinn commented Mar 8, 2018 •

edited

Loading

lserwatka commented Mar 8, 2018

dew326 commented Mar 8, 2018

mnocon commented Mar 8, 2018

Nattfarinn commented Mar 8, 2018

lserwatka commented Mar 8, 2018

webhdx Mar 12, 2018

webhdx Mar 12, 2018

webhdx Mar 12, 2018

Nattfarinn Mar 12, 2018

webhdx Mar 12, 2018

Nattfarinn Mar 12, 2018

webhdx Mar 12, 2018

webhdx Mar 12, 2018

bdunogier commented Mar 12, 2018

dew326 commented Mar 12, 2018

bdunogier commented Mar 12, 2018 •

edited

Loading

lserwatka commented Mar 12, 2018

Nattfarinn commented Mar 12, 2018

bdunogier commented Mar 12, 2018 •

edited

Loading

EZP-28841: Content On the Fly v2 #366

EZP-28841: Content On the Fly v2 #366

Conversation

dew326 commented Mar 2, 2018 • edited by lserwatka Loading

Checklist:

ViniTou left a 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

mnocon commented Mar 7, 2018

Nattfarinn commented Mar 7, 2018

mnocon commented Mar 8, 2018 • edited Loading

bdunogier commented Mar 8, 2018

Nattfarinn commented Mar 8, 2018

Nattfarinn commented Mar 8, 2018 • edited Loading

lserwatka commented Mar 8, 2018

dew326 commented Mar 8, 2018

mnocon commented Mar 8, 2018

Nattfarinn commented Mar 8, 2018

lserwatka commented Mar 8, 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

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

bdunogier commented Mar 12, 2018

dew326 commented Mar 12, 2018

bdunogier commented Mar 12, 2018 • edited Loading

lserwatka commented Mar 12, 2018

Nattfarinn commented Mar 12, 2018

bdunogier commented Mar 12, 2018 • edited Loading

dew326 commented Mar 2, 2018 •

edited by lserwatka

Loading

mnocon commented Mar 8, 2018 •

edited

Loading

Nattfarinn commented Mar 8, 2018 •

edited

Loading

bdunogier commented Mar 12, 2018 •

edited

Loading

bdunogier commented Mar 12, 2018 •

edited

Loading