IBX-4486: Disabled Autosave feature for content creation #2084
Conversation
|
Kudos, SonarCloud Quality Gate passed!
|
| try { | ||
| $this->innerContentFormProcessor->processSaveDraft($event); | ||
| $statusCode = Response::HTTP_OK; | ||
| if ($event->getData() instanceof ContentUpdateData) { |
There was a problem hiding this comment.
Is this a reliable way to detect /nodraft requests? Can we have some test coverage to be sure that we didn't disabled autosave for cases that shouldn't be affected by this change?
There was a problem hiding this comment.
@konradoboza I can create some tests, but what other cases do you have in mind that shouldn't be affected by this change? In processSaveDraft we explicitly mention only two cases - ContentCreateData and ContentUpdateData (https://github.com/ezsystems/ezplatform-content-forms/blob/1.3/src/lib/Form/Processor/ContentFormProcessor.php#L83). It's even more reasonable to block other unexpected Datas here and leave only the one fitting.
There was a problem hiding this comment.
I assume that with this change we don't block autosaving while being on content creation form, right? The naming itself is a bit hard to wrap a head around without context. That's why I asked about some test coverage. Nevertheless, if it isn't the case, I am fine with proposed changes.
There was a problem hiding this comment.
If by "content creation form" you mean creation of content that is in fact updating its first draft then it is not blocking it. That's how Back Office works usually. This PR only affects the "nodraft" content creation when a draft for a given content doesn't exist yet and therefore there is no need to update/autosave it.
There was a problem hiding this comment.
If by content creation form you mean creation of content that is in fact updating its first draft
That was indeed my assumption, but wasn't sure.
There was a problem hiding this comment.
This form has drafts_enabled option, wouldnt that be a better way to check it? It just seems it is set wrong for /nodraft action and it is only used in UDW.
There was a problem hiding this comment.
I guess best would be to make this: https://github.com/ezsystems/ezplatform-content-forms/blob/1.3/src/lib/Content/View/Filter/ContentCreateViewFilter.php#L132 optional parameter and set it to false here: https://github.com/ezsystems/ezplatform-content-forms/blob/1.3/src/lib/Content/View/Filter/ContentCreateViewFilter.php#L87
This should give us reliable information whether drafts are enabled or not.
There was a problem hiding this comment.
@ViniTou @Nattfarinn setting it to false will break content create forms - https://github.com/ezsystems/ezplatform-content-forms/blob/1.3/src/lib/Form/Type/Content/ContentEditType.php#L46-L65.
In the create.html.twig (https://github.com/ezsystems/ezplatform-admin-ui/blob/2.3/src/bundle/Resources/views/themes/admin/content/create/create.html.twig#L29-L30) we are using both cancel and save draft buttons. So perhaps we could introduce another option that is autosave_enabled?
There was a problem hiding this comment.
yup, for me form option sounds better then instance of check.
On the other hand, if it only breaks displaying of buttons, then maybe it is ok. If people wants to have Save draft button, when using /nodraft path, that contradicts a little to each other but I guess thats another discussion.
There was a problem hiding this comment.
@ViniTou @Nattfarinn Closing this in favor of ezsystems/ezplatform-content-forms#66
| try { | ||
| $this->innerContentFormProcessor->processSaveDraft($event); | ||
| $statusCode = Response::HTTP_OK; | ||
| if ($event->getData() instanceof ContentUpdateData) { |
There was a problem hiding this comment.
I guess best would be to make this: https://github.com/ezsystems/ezplatform-content-forms/blob/1.3/src/lib/Content/View/Filter/ContentCreateViewFilter.php#L132 optional parameter and set it to false here: https://github.com/ezsystems/ezplatform-content-forms/blob/1.3/src/lib/Content/View/Filter/ContentCreateViewFilter.php#L87
This should give us reliable information whether drafts are enabled or not.
Autosave shouldn't be enabled for
nodraftrequests.Checklist:
$ composer fix-cs)