Use modals for creating, updating, and deleting workflows, with a new DialogFormTrait

Cross posting from https://www.drupal.org/node/2779647#comment-11821941

Here is a new prototype for workflow:
- keeps the same functionality and technical implementation
- user doesn't leave page before workflow and moderation configuration is done as it all happens on same page instead of 4 different pages - reduces the amount of clicks and pages needed
- simplifies the UX and reduces "up to 25%" of time needed for creating and administering workflows and moderation
- improves the Workflows listing by showing important information such as workflow type, which entity types it affects and states available

Below is a video comparing creating workflow, 1 new state and 1 new transition and configuring moderation for 1 content type in the current and proposed versions.
Video: https://youtu.be/xB-AbupfDxY

Interactive prototype: https://marvelapp.com/1124911

In my opinion (long term) it would be great if the UI would mirror how the comments work, by which I mean:
1. configure 'Workflow types' under '/admin/structure'
2. add field of type 'Workflow' to the bundle under 'Manage fields' and pick one from step 1
Then allow only one of them to have control over submit button (configurable under 'Edit field settings' on 'Manage fields' tab)

Not sure it's in the scope of this issue, looks more like it's meant to be a minor clean-up of what went in. Opened #2835545: Provide a Workflow FieldType that references a workflow entity and tracks the current state of an entity for this direction.

Opened #2843083: Select entity type / bundle in workflow settings to work on some of this.

@alexpott - Are these concerns from a UX or performance point of view? It seems unanimous from the UX site that the selection of entity types and bundles a workflow can be used on needs to move out of the bundle settings. In the latest UX call (https://youtu.be/xWLXFRbs-EM?t=245) Kevin O'Leary expressed concerns about how deep the setting was.

Without a 'new' form element I am unsure how to resolve @alex his issue. The only suitable pattern we have would be the way we present fields for selection, in the Views UI.

So my comment is not about the entity types bundles thing but more about the general wall of information effect that this UI is presenting. I think it would be great if we could get to a simpler UI that folds together a lot of the tasks that have been pulled apart here into their own discrete feildsets, modals etc.

The one that's been bugging me the most is the transitions UI because I believe it may be the most difficult for people to wrap their heads around. I made this codepen in an effort to work out a UI that would do this without adding too much front-end code. Here's what it looks like:

And here's the link to the codepen prototype: http://codepen.io/tkoleary/pen/EWEXWQ?editors=1100

Setting aside the entity types, this is really all the UI you need to for 80% of tasks. The tabs across the top I envision just loading the next transition into the same space via ajax. The "add transition" button-link could behave the same way, and the "add state" button-link could also load in place or via a modal. The important thing is that it will bring the user right back to this visualization.

The important thing is that I can quickly flip back and forth between the transitions and follow the selected states and the leader lines to see which states are going to where.

I also get a description that tells me what's going to appear in the button, because otherwise I have to—in my head—go "Ok the button always says 'Save and' so My label needs to be 'save and something' but without the 'save and'"

I really like the idea in #10, but do wonder if it'll be unfamiliar to Drupal site builders. The current workflows edit form is very similar to many other admin pages within Drupal. Just my 2 cents.

I actually came here to close this issue in favour of #2843083: Select entity type / bundle in workflow settings which is implementing @jojototh's designs from #2 and nearing completion. Forgot about #10.

This is currently a "must-have" for Workflows to become beta (#2843494: WI: Workflows module roadmap) so looking to action on that.

@timmillwood

I really like the idea in #10, but do wonder if it'll be unfamiliar to Drupal site builders. The current workflows edit form is very similar to many other admin pages within Drupal. Just my 2 cents.

That is true, but the first question we should be asking is whether it's a simpler and more understandable pattern for *any* user, brand new or experienced with Drupal. We shouldn't willy-nilly introduce new patterns into core, but there are instances when core patterns are insufficient. Given the complexity of the mental model her I think this is one of those instances.

Having said that, this pattern is so new that it does warrant some usability testing.

Can we centralise the issue that we discuss that UI in? I've seen it in another issue too.

I am very keen on introducing new UI's, we try to fit everything into tables - especially with sequential things like this, its just the best fit. I am very much in agreement with Kevin on this one, a new pattern in this case may be a better fit.

ok, but does this still need to be a "must have" in #2843494: WI: Workflows module roadmap?

I think that Kevin's proposal is actually trying to achieve the same thing as I originally proposed for Workflows, but it wasn't yet implemented. See the proposal at: https://marvelapp.com/1124911/screen/17578780

Initial patch on proposal in #15

Patch contains:

1. Implemented only for transition edit and add link
2. Updated "to" to selectbox

Yet to do:
1. Apply to other elements. If the implementation is OK.
2. On validation error, going back to page
3. Probably fix alignment issue on loading icon (ref: screenshot)

Great stuff @vijaycs85!
Took the liberty to continue where you left off =)

• We now submit the modal form and close the modal
• We now update the values on the page for the transitions (I had to simplify how we render these and add some consistency in order to target them on our ajaxresponse).
• We now display a title like "Edit the Create New Draft transition for the Editorial workflow" for the form.

Still to do (at least):

• Fix the throbber bug
• Display the form errors within on the TRANSITIONS details element (on the ajaxCallback())

Here's some details on how to get the form errors to display, https://api.drupal.org/api/drupal/core%21core.api.php/group/ajax/8.2.x#s...

More progress on this:

• Form errors now display within the modal
• Saving successfully now displays the messages just on top of the details for transitions

Still to do:

• Fix throbber bug
• On the edit form there is a link to delete the transition - I think we should either change it for a cancel button, or perhaps at least keep the delete confirmation form on the modal, not sure, but as it is it feels weird going to another page.

The delete link it inherited from EntityForm, I'd be happy to remove it so the only place to delete it a drop button on the workflows page.

Some more progress...

• Removed the Delete link from the edit form, placed there instead a Cancel button which closes the dialogue.
• Made the Delete link display the confirmation form on a modal, and ajaxified it so that the deleted transition is removed from the table, etc.

Still to do:

• Fix the throbber bug
• See what to do with the Adding of a new transition. With this patch the form shows on a modal, but it does not submit via ajax. Doing so would mean we'd have to add a new row to the table which is not trivial.

Couldn't help myself, so I did the same for the workflow states:

In my opinion we should remove the operations links on the transitions table from WorkflowStateEditForm::form(), since it wont work well on a modal context, and we can already edit them below without leaving the same page... thoughts?

Fixing the failing test (we removed the delete link from the edit form), and some coding standards violations.

My take on fixing the throbber on these kinds of buttons, not sure if we should do this as a separate issue, if so the interdiff could easily serve as the starting patch.

Looks great.

1. One concern not related to this issue is amount of duplicate code on all 3 entities' add/edit forms. We might need to open a separate ticket to reduce duplicate.

2. +++ b/core/modules/workflows/src/Form/WorkflowStateDeleteForm.php
   @@ -96,4 +111,46 @@ public function submitForm(array &$form, FormStateInterface $form_state) {
   +   * Ajax callback to close the modal and remove the deleted state from the
   +   * table.
   

   minor: CS issue: Short description should be < 80 character

3. +++ b/core/modules/workflows/src/Form/WorkflowStateDeleteForm.php
   @@ -96,4 +111,46 @@ public function submitForm(array &$form, FormStateInterface $form_state) {
   +   * @return \Drupal\Core\Ajax\AjaxResponse
   

   minor: CS issue: empty line between @param and @return missing

#27.1 - maybe we need a WorkflowFomBase or similar to inherit shared methods.

Thanks for the reviews - fixing CS issues mentioned in #27, reviewed the rest of the patch with that in mind, and adding proper titles to:

• entity.workflow.add_state_form
• entity.workflow.edit_state_form
• entity.workflow.add_transition_form
• entity.workflow.edit_transition_form

As far as adding a base class for all these forms, do you think it's time to start optimizing this?
Should we have a base form for edit, a base form for adding, etc?

For now I can only see an obvious method that could be reused cancelAjaxCallback(), and perhaps strip out the error handling part of submitAjaxCallback() somehow...

Rerolled, there was a tiny conflict on core/themes/seven/css/components/dropbutton.component.css

OK I did a bit of refactoring, introducing WorkflowFormBase. Both WorkflowTransitionEditForm and WorkflowStateEditForm extend it.

Was able to pull out actions(), cancelAjaxCallback() and the error handling part of submitAjaxCallback().

Unfortunately cancelAjaxCallback is stil duplicated on the delete forms, but since those extend ConfirmFormBase, they can't use WorkflowFormBase, which extends EntityForm. So perhaps there is a better approach.

To be honest, I think WorkflowFormBase could be used for editing any entity in a modal in theory...

Maybe a trait would make more sense.

I think we should go down the route of adding a trait, and add it to /core/lib/Drupal/Core/Form.

Thanks @timmillwood for the help on this one, introducing Drupal\Core\Form\ModalFormTrait, instead of Drupal\workflows\Form\WorkflowFormBase.

interdiff.txt is against #31 which introduced the base form, so probably not terribly insightful to look at.
interdiff-30-34.txt is against #30 which is perhaps better to see the effect of introducing the trait itself.

Introducing functional javascript tests:

• core/modules/content_moderation/tests/src/FunctionalJavascript/WorkflowTypeEditFormTest.php - tests for what we introduced on #2843083: Select entity type / bundle in workflow settings.
• core/modules/workflows/tests/src/FunctionalJavascript/WorkflowEditFormTest.php tests what we're trying to do on this issue. Not finished yet.

These led me to discover that we weren't attaching the 'core/drupal.dialog.ajax' library to WorkflowEditForm so doing this as well.

I am blocked on WorkflowEditFormTest::testWorkflowTransitionsForm(), which fails saying that RuntimeException: Unable to complete AJAX request. when trying to save the transition edit form. It has driven me slightly insane: it is identical to the states edit form test, except updating a checkbox field as well as the label. However i have tried the test without updating the transition at all and it still fails. Any pointers would be very welcome!

Self review:

+++ b/core/modules/workflows/src/Form/WorkflowTransitionDeleteForm.php
@@ -129,7 +129,7 @@ public function submitAjaxCallback() {
-    $response->addCommand(new HtmlCommand('[data-drupal-selector="edit-transitions-container"] [data-drupal-selector="edit-modal-messages"]', \Drupal::service('renderer')->renderRoot($status_messages)));
+    $response->addCommand(new HtmlCommand('[data-drupal-selector="edit-states-modal-messages"]', \Drupal::service('renderer')->renderRoot($status_messages)));

This should be edit-transitions-modal-messages, but im not testing this yet so my blockade remains.

Fixing #36 while I'm at it.

The test results at least show why the ajax request doesn't finish, however, that looks to be an unrelated bug to what I'm trying to do?
Error: Syntax error, unrecognized expression: [data-drupal-selector="edit-transitions-publish-to]

OK I'm such a retard sometimes haha
Yes its totally related to what I'm doing, and yes, it is totally my fault!
Tests should come back green now, I'll work on expanding the coverage now.

Adding tests for deleting a transition, and cleaning up a bit the rest of assertions. With this I think I'm finished with the JavascriptTests =)

Failure seems unrelated, fixing coding standards in the mean time...

Oops, wrong patch.

This is looking pretty close and so tempted to RTBC, but one thing is bugging me.

When editing a workflow state (in a modal), clicking the edit or delete link for a transition reloads the page. I think this should close the state modal and open the transition modal.

Re #46: If you take the route of editing the transition in a modal from there, then when you save the transition, the user would expect to be back where you started (the state edit modal), and that is going to get very complex if at all possible currently.

This would be doable perhaps if we had nested modals working, which we unfortunately don't (see #2741877: Nested modals don't work: opening a modal from a modal closes the original)

I agree that this part needs some kind of decision on what to do. I proposed this on #23:

In my opinion we should remove the operations links on the transitions table from WorkflowStateEditForm::form(), since it wont work well on a modal context, and we can already edit them below without leaving the same page... thoughts?

Since the issue clearly went quite forward from *reviewing* the UI, let's update the issue summary/title with clear goals, so we can tell when this is done. As it is now, it is entirely open ended. As a must-have issue for #2843494: WI: Workflows module roadmap, we should have clear goals. If certain things are defined while others need more definition that should be documented too.

Updating the title and issue summary to reflect that we are now at the stage of implementing everything suggested in #1.

@Manuel Garcia - ok, I think removing the edit and delete links for transitions within the state modals sounds good. The mock ups in #1 doesn't have them.

Removing the transitions table from the states edit form to reflect what we have on #1.
Also noticed we were adding the attributes twice to the Add a new state link, so removing that from the patch.

Nice, will RTBC once we have screenshots.

I'll do the screenshots (or animated gif) when I get chance, but anyone is welcome to generate them.

While doing the screenshots i realized that if you generate a form error on the transition / state add forms they take you to their add page (for example admin/config/workflow/workflows/manage/editorial/add_transition)
This totally breaks the flow... so we need to do the submit on the add forms via ajax and display any errors that way.

Working on that

OK now the form submits via ajax, displays the errors if any on the modal, and if there are none, it saves and redirects to the current workflow edit form.

I avoided closing the modal because otherwise when the page reloads it looks funky. Like this you just see the loading icon and then the page reloads.

Thanks for all the work, this looks really interesting.

From my understanding of this thread at #15, we diverged from the conversation and went with a solution proposed by jojototh using tables as a way to form the states, transitions and applied entities. This is in line with much of core.

I would like to challenge the thinking that this is sufficient for RTBC. The concepts in this screen are complex and the user needs to be guided how to effectively set things up, the fact that very different concepts look homogeneous implies linear sequence and equal importance. While in reality there is an interplay between states and transitions (at least) and this can easily become quite complex.

Can we explore having at least a representation of the states and transitions, like proposed in #10 by tkoleary? I understand making it interactive is a challenge, but the representation will greatly improve the usability.

This would be the review for now, will happily provide more context. From the usability perspective, this is not close to RTBC.

Initial high level code review of #54. Reviewed the code before seeing the suggestions made in #55, but I think that comment and this one is enough for NW.

1. +++ b/core/lib/Drupal/Core/Form/ModalFormTrait.php
   @@ -0,0 +1,83 @@
   +  /**
   +   * Generates a form's submit and action button elemetns with the #ajax
   +   * functionality set up.
   

   First line should be 80 chars.

2. +++ b/core/lib/Drupal/Core/Form/ModalFormTrait.php
   @@ -0,0 +1,83 @@
   +        'callback' => [$this, 'modalformsubmithandleerrors'],
   ...
   +        'callback' => [$this, 'modalformclosedialog'],
   

   Why are these methods not camel case?

3. +++ b/core/modules/content_moderation/tests/src/FunctionalJavascript/WorkflowTypeEditFormTest.php
   @@ -0,0 +1,58 @@
   +    $assert_session = $this->assertSession();
   +    $page = $this->getSession()->getPage();
   

   Is it necasary to cache these? 40 occourances of "$this->assertSession();" in core vs 330 "$this->assertSession()->"

4. +++ b/core/modules/workflows/src/Form/WorkflowEditForm.php
   @@ -85,7 +89,14 @@ public function form(array $form, FormStateInterface $form_state) {
   +              'width' => 700,
   

   This appears a few times in core. I wonder if some constants for standard modal widths would be useful.

5. +++ b/core/modules/workflows/src/Form/WorkflowEditForm.php
   @@ -148,16 +181,33 @@ public function form(array $form, FormStateInterface $form_state) {
   +          'aria-label' => $this->t('Edit \'@transition\' transition', ['@transition' => $transition->label()]),
   

   Why not use double quotes to enclose the single quotes.

6. +++ b/core/modules/workflows/src/Form/WorkflowStateAddForm.php
   @@ -111,12 +115,39 @@ public function save(array $form, FormStateInterface $form_state) {
   +    $response = $this->modalFormSubmitHandleErrors($form, $form_state);
   +    if ($commands = $response->getCommands()) {
   +      return $response;
   +    }
   

   Seems cleaner to check form_state for errors here.

7. +++ b/core/modules/workflows/src/Form/WorkflowStateDeleteForm.php
   @@ -96,4 +110,24 @@ public function submitForm(array &$form, FormStateInterface $form_state) {
   +    $response = $this->modalFormCloseDialog();
   +
   +    $data_selector = 'edit-states-' . strtolower(str_replace('_', '-', $this->stateId));
   +
   +    $response->addCommand(new RemoveCommand('[data-drupal-selector="' . $data_selector . '"]'));
   +
   +    $status_messages = ['#type' => 'status_messages'];
   +    $response->addCommand(new HtmlCommand('[data-drupal-selector="edit-states-modal-messages"]', \Drupal::service('renderer')->renderRoot($status_messages)));
   +
   +    return $response;
   

   Can all of this be avoided by sending a RedirectCommand which reloads the page?

8. +++ b/core/modules/workflows/src/Form/WorkflowStateEditForm.php
   @@ -146,29 +120,41 @@ public function save(array $form, FormStateInterface $form_state) {
   +    $data_selector = 'edit-states-' . strtolower(str_replace('_', '-', $this->stateId));
   

   Maybe something like \Drupal\Component\Utility\Html::cleanCssIdentifier could help us here.

9. +++ b/core/modules/workflows/src/Form/WorkflowStateEditForm.php
   @@ -146,29 +120,41 @@ public function save(array $form, FormStateInterface $form_state) {
   +    $title = $this->t('Edit the @state state for the @workflow', ['@state' => $workflow->getState($workflow_state)->label(), '@workflow' => $workflow->label()]);
   

   Does this string need work. "Edit the Draft state for the Editorial"?

10. +++ b/core/modules/workflows/src/Form/WorkflowTransitionAddForm.php
    @@ -147,12 +151,39 @@ public function save(array $form, FormStateInterface $form_state) {
    +    $title = $this->t('Add a new transition to the @workflow', ['@workflow' => $workflow->label()]);
    

    Same here.

11. +++ b/core/modules/workflows/src/Form/WorkflowTransitionEditForm.php
    @@ -149,30 +161,43 @@ public function save(array $form, FormStateInterface $form_state) {
    +    $title = $this->t('Edit the @transition transition for the @workflow', ['@transition' => $workflow->getTransition($workflow_transition)->label(), '@workflow' => $workflow->label()]);
    

    Same here.

@bojhan thanks for the feedback! I gave it a few thoughts and found some concerns with what tkoleary proposed in #10 that need to be figured out:

1. you need to select one of multiple options on the “To state” side so it wouldn’t be as simple as in the codepen example
2. when you go to “add new transition” modal no option would be selected and if there weren’t any checkboxes users might not know what to do, where to click
3. It also doesn’t communicate that you can select multiple on the “From state” side but only one on the “To state” side

Here is an option, that could benefit from tkoleary's proposal and answer the concerns above

1. It is not evident that "To" field on "Edit transition" form can't be edited (it's a bit strange, because user can accidentaly save wrong end state and in this case, he should delete the transition and create another). Better to show it like a disabled field.
   
2. Bug. Are shown errors "The transition from ... to ... already exists." after click on Cancel button (and refresh the page). It is expected to not show the error in this case. No matters what user selected, if he didn't save it. video

Bug. Are shown errors "The transition from ... to ... already exists." after click on Cancel button (and refresh the page). It is expected to not show the error in this case. No matters what user selected, if he didn't save it. video

Thanks for testing!
Those were probably errors from previous form submissions, but with the latest patch that should not happen since we are already displaying them within the modals. Did you get this with the latest patch (#54) applied?

Yes, Manuel, it was the latest patch (#54).
These errors, as you said, are shown within the modals, but only when user clicks "Save".
But,
when user clicks "Cancel" and refresh the page he will see these errors (anyway, no need to show errors in this case, no matters on the page or modal).
You are welcome if you have the questions).

Thank you @timisoreana for kindly explaining, I will check why this happens when we have a consensus around the UI and I get back to this patch.

In #2850549: Allow changing of transition 'to' on edit we talk about changing the "to" state of a transition. Therefore here we need to assume the "to" state cannot be changed, then handle in the other issue if and how we allow it to be changed.

We discussed this issue in the UX call today.

Gabor suggested we move forward with the direction Jozef originally proposed here. Then open a new issue regarding the proposal in #58. Thus we get the initial improvements in, and they don't end up getting derailed and delayed with discussions about the new transitions interface.

For the record (and anyone can watch the actual recording literally at https://www.youtube.com/watch?v=7afbnHveAJA :D) I said if we agree that using these modals is the best way forward then getting the modals committed as a first step would be best so we can work on refining the *content* of the modals later on. I think introducing the modals and refining the content of the modals in the same issue may result in unfair coupling, hard to review patches, etc. and the modals may not end up in core if we don't end up being able to solve the rest of the technology or argue endlessly about it.

If we don't agree the modals help and there is a more holistic improvement needed not "just" refining the contents of individual pages after they are made modals, then it does not help to commit modals first.

OK then, worked on stabilizing what we have so far:

Re #57:
1. That line is 72 chars, if I added the next word to it it would go over 80. That said I have reworded that so it makes a bit more sense as to what it does.
2. Fixed
3. Done
4. I chose that width because of that, for example its what's used on the block configuration page. Also works in this case fine.
5. Removed the single quotes, we don't need them and we aren't using them on the sates forms either.
6. Yep you're right, fixed.
7. Yeah I agree, updating all the ui in ajax calbacks is getting very messy. I've reworked all submit ajax callbacks so they first display any errors in the modal if there are any and then reloads the page using a RedirectCommand.
8. No longer necessary since we are reloading the page instead of updating the ui via ajax.
9, 10 & 11. The Editorial workflow is actually called "Editorial workflow". If I add ' workflow' to the string, its going to end up displaying: Edit the Published state for the Editorial workflow workflow. This is an ongoing problem in content_moderation, and we should fix this as a different issue in my opinion, because it probably has an effect on a lot of other different strings.

Re #59.2 - Fixed

So a lot of changes, patch should come back complaining about WorkflowEditFormTest, but I wanted to get this up and see if anything else breaks. Nice that this way the patch is smaller though :)

Refactored the tests to actualy just test the js functionality (modals and in-modal form errors), however im getting strange errors locally:
Zumba\GastonJS\Exception\BrowserError: There was an error inside the PhantomJS portion of GastonJS.
Uploading to see if its just me.

I also fixed a bug introduced in #66 which I found by the new tests =)

OK so its not just me... anyone seen this before?

Perhaps #2773791: Clicking elements with children in Javascript tests throws a GastonJS exception ?

Edit:
Failing at
$this->click('#edit-states-container tr:contains(Published) button .dropbutton-arrow');

Thanks @tacituseu

OK that selector makes no sense because Published does not have a dropdown button. Duh.
Switching it now to using the Archived state, which does. However the test still fails :S

If I run (with the new selector) jQuery('#edit-states-container tr:contains(Archived) button .dropbutton-arrow').click(); on the js console, and the dropdown shows the delete button properly, so the selector apparently should not the problem now?

I also tried applying the patch on that issue as well, but the tests still fail :S

Other selectors I've tried (both work on the js console):

• '#edit-states-container tr:contains(Archived) .dropbutton-toggle button'
• '[data-drupal-selector="edit-states-archived-operations"] .dropbutton-toggle button'

Might be the reason Drupal\Tests\views_ui\FunctionalJavascript\FilterCriteriaTest::testFilterCriteriaDialog() does it like this:

$page = $this->getSession()->getPage();
$dropbutton = $page->find('css', '.views-ui-display-tab-bucket.filter .dropbutton-toggle button');
$dropbutton->click(); 

Thanks @tacituseu again for helping me out debug this.

It ends up that the selectors weren't the problem. Instead it had these two problems:

1. There is no Archived state in this test (lol). So I'm using the Draft state instead.
2. We needed to close the previous modal window in order to continue with the test, taking screenshots is what helped me find this out.

So, in this patch:

• Fixed the failing tests.
• Changed the selectors so they are simpler.
• Added coverage for the cancel buttons closing the modals.

Argh, ignore the patch on #75 its just the interdiff with a different name, sorry bot! So the interdiff on #75 applies to this one.

Self review: we should probably merge Drupal\Tests\workflows\FunctionalJavascript|WorkflowEditFormTest into Drupal\Tests\content_moderation\FunctionalJavascript\WorkflowTypeEditFormTest.

Re #78: actually as @timmillwood pointed out, one is for workflows and the other is for content_moderation so it's probably fine as it is...

Finalized the test that handles the 'This workflow applies to:' section.

It now also checks the UI updates that happen when you change the selection of entities that the workflow applies to, both adding and removing selections. With this I think we are well covered when it comes to JS tests for what's going on on this page.

Click on Cancel button doesn't close the modal.
Steps:

1. Open "Edit transition" modal
2. Check all initial states (there should be at least 1 already existing transition)
3. Click Cancel

ER Modal should be closed
AR: Modal is not closed video
Was applied patch #80

Thanks for testing and the report @timisoreana!

Apparently setting '#limit_validation_errors' => [], on the cancel buttons is causing some issues... its preventing validation which prevents the form errors, but apparently the form is still being submitted, and triggers this as it thinks the user is trying to save the form:

InvalidArgumentException: The 'create_new_draft' transition already allows 'draft' to 'draft' transitions in workflow 'editorial' in Drupal\workflows\Entity\Workflow->setTransitionFromStates() (line 440 of /var/www/drupal8/core/modules/workflows/src/Entity/Workflow.php).

setTransitionFromStates() is only being called from Drupal\workflows\Form\WorkflowTransitionEditForm::copyFormValuesToEntity(), which I'm not sure why its being triggered. The cancel button is just a button, not a submit button... any pointers welcome!

The entity gets built no matter if the submit is happening or not, so what we need to go is ignore the exception, and depend on form validation, we could do this with a try / catch similar to the patch in #2886567: Adding a workflow state or transition with an integer ID results in unrecoverable fatal errors.

This should do the trick:

diff --git a/core/modules/workflows/src/Form/WorkflowTransitionEditForm.php b/core/modules/workflows/src/Form/WorkflowTransitionEditForm.php
index ecc4a8196b..d0b27d4cb1 100644
--- a/core/modules/workflows/src/Form/WorkflowTransitionEditForm.php
+++ b/core/modules/workflows/src/Form/WorkflowTransitionEditForm.php
@@ -136,15 +136,21 @@ protected function copyFormValuesToEntity(EntityInterface $entity, array $form,
       // Only do something once form validation is complete.
       return;
     }
-    /** @var \Drupal\workflows\WorkflowInterface $entity */
-    $values = $form_state->getValues();
-    $form_state->set('created_transition', FALSE);
-    $entity->setTransitionLabel($values['id'], $values['label']);
-    $entity->setTransitionFromStates($values['id'], array_filter($values['from']));
-    if (isset($values['type_settings'])) {
-      $configuration = $entity->getTypePlugin()->getConfiguration();
-      $configuration['transitions'][$values['id']] = $values['type_settings'][$entity->getTypePlugin()->getPluginId()];
-      $entity->set('type_settings', $configuration);
+    try {
+      /** @var \Drupal\workflows\WorkflowInterface $entity */
+      $values = $form_state->getValues();
+      $form_state->set('created_transition', FALSE);
+      $entity->setTransitionLabel($values['id'], $values['label']);
+      $entity->setTransitionFromStates($values['id'], array_filter($values['from']));
+      if (isset($values['type_settings'])) {
+        $configuration = $entity->getTypePlugin()->getConfiguration();
+        $configuration['transitions'][$values['id']] = $values['type_settings'][$entity->getTypePlugin()
+          ->getPluginId()];
+        $entity->set('type_settings', $configuration);
+      }
+    }
+    catch (\Exception $e) {
+      // Do nothing, if there is an exception we want validation to handle it.
     }
   }

It looks like the follow up to implement the design from #58 is #2758623: [meta] Redesign workflow configuration page to better visually describe the flow.

Thanks @timmillwood makes sense, doing so in this patch, which fixes the bug.

Adding test coverage for what was fixed on #85.

Attached a test-only patch which should fail (doesn't include the interdiff from #85), and a patch with both the fix and the test.

Awesome work on this @Manuel Garcia!

Specifying the type of exception we're catching on WorkflowTransitionEditForm::copyFormValuesToEntity(), and applying the same fix on the transition add form, where we had the same bug.

Switching back the transition add form to field to its original radios type (out of scope change)

Adding explanatory comments to the try/catch on copyFormValuesToEntity, and cleaning up an old unnecessary @todo.

Looks better.

I quite like the try/catch approach to bypassing the exceptions to get the form api validation. We could equally duplicate the validation, but not sure we want to duplicate things.

Hmm... I'm not excited by the try-catch, as I've mentioned elsewhere in particular because we already decided on the other approach in the other issue. What bothers me even more, though, is that with the patch we have an inconsistency in the validation between e.g. WorkflowStateAddForm::copyFormValuesToEntity() and WorkflowTransitionAddForm::copyFormValuesToEntity(). I really don't think we can commit it like this. I think we should either copy the validation for now and then @timmillwood you can open a follow-up to discuss to change that into a try-catch (for both forms) or we use a try-catch for both foms directly, but then we would need e.g. @Berdir to RTBC as a "tie-breaker" between @timmillwood and me. Kicking back to "needs review" for now.

The other issue where we're implementing a try/catch is #2886567: Adding a workflow state or transition with an integer ID results in unrecoverable fatal errors. I do like it because I am a firm believer in keeping the code D.R.Y, but we could have some validation method, or just a method that returns the regex to help this, try/catch just seemed like a simple solution.

That being said, I think we do need consistency between the State and Transition forms.

@berdir taking a look sounds like a good idea. ;)

One minor thing:

+++ b/core/modules/workflows/src/Form/WorkflowEditForm.php
@@ -85,7 +86,14 @@ public function form(array $form, FormStateInterface $form_state) {
+            'class' => ['use-ajax'],
+            'data-dialog-type' => 'modal',
+            'data-dialog-options' => Json::encode([
+              'width' => 700,
+            ]),

@@ -95,12 +103,22 @@ public function form(array $form, FormStateInterface $form_state) {
+              'width' => 700,

@@ -116,7 +134,16 @@ public function form(array $form, FormStateInterface $form_state) {
+          'width' => 700,

@@ -148,16 +175,33 @@ public function form(array $form, FormStateInterface $form_state) {
+            'width' => 700,
...
+            'width' => 700,

@@ -179,7 +225,16 @@ public function form(array $form, FormStateInterface $form_state) {
+          'width' => 700,

Can we store the default dialog attributes in a variable to ensure we have to change them just once, if needed, and no inconsistencies are introduced?

Addressing #94

I just realized that since we are reloading the page when submitting the modal forms instead of updating the UI on the ajax callbacks, we no longer need to change those parts of the form in order to have markup to target.

Removing those changes in this patch which should make it slightly smaller...

Patch looks good to me, but we still need to argue about the try/catch Vs. replicating the validation.

Right, let's try to summon @Berdir then.

Opened up #2894499: Rename 'Editorial workflow' to 'Editorial' in order to be able to address #57.9, 10, and 11 properly.
(See my reply to it on #66).

Sorry for the delay.

I'm not sure. Neither duplicating the validation nor the try/catch will really help us long term if we want to handle config entity validation like content entities, that is, validate using the validation API/config schema. Which we will need if we want to support REST for them.

But we also have problems there with content entities, for example #2820364: Entity + Field + Property validation constraints are processed in the incorrect order.

I don't really have a preference for try/catch or duplicating the validation. Having a @todo + follow-up seems like a good idea, and if I understand it correctly, if we'd do #2886567: Adding a workflow state or transition with an integer ID results in unrecoverable fatal errors then both forms would use try/catch and would be "consistent"? So I think I'd be ok with the try/catch in both issues and a follow-up to figure out a better solution.

As an idea, we could handle things like this by changing the entity validation in entity forms in a way that would only build and validate the entity if there are no form-level validation errors. Then we could have #element_validate and similar things for elements that could break an entity just by setting them. And rest could possibly introduce a pre validate method or something that would allow entities to inspect the raw values before they are set? Or we'd define a special exception interface that rest would be able to handle in a meaningful way.

- Created follow up #2895310: Building entities in ContentEntityForm::validateForm() can throw exceptions to fix entity validation in regards to forms as @berdir suggested in #101.
- Added @todo to the patch pointing to the follow up.

Ok, leaving RTBC to @tstoeckler.

The title of this issue terrifies me every time I look at the roadmap, so trying to give it a more descriptive title based on my understanding. :) The summary could use an update with some more detail and screenshots, also.

Ideally, the ModalFormTrait would be adde in a separate issue, with its own tests etc., since it is a new API.

OK, don't want to hold this up any longer. And yes, it's not ideal either way. So let's do this!

Oh wait, did #104 want to set this to needs work to update the issue summary? Either way, this is fine by me.

Someone pointed me to this issue an hour ago. It adds a new public API to the Form subsystem.
I haven't had a chance to review it yet.

Just to note, we could do this without the new api or trait, but it seemed to make sense.

Per @tim.plunkett and @tedbow, this is adding a trait that's very similar to (but not quite the same as) one added in #2785047: In Outside In mode, form validation messages should appear in the off-canvas tray, not the main page, so I do think we should split that trait out into a new dedicated issue so it can be reviewed and used for both. Also since it should have test coverage. (Since the proposed trait is an addition for stable core, we want to get it in sooner than the improvements for the two experimental modules.) Thanks!

I am working on seeing I get 1 trait to working for both issues

Ok is here is try at using most of the trait here #2785047: In Outside In mode, form validation messages should appear in the off-canvas tray, not the main page to work on this issue.

Here is what I did

1. Copied in the trait from #2785047 but renamed in DialogFormTrait. Modals and the Off-canvas tray are just dialog types.
2. Change the redirect logic to use destination from the query string. So WorkflowEditForm was changed to pass along the destination.
3. submitAjaxCallback() is not longer need for each form class
4. actions() is not longer needed either.

I am just about to update #2785047: In Outside In mode, form validation messages should appear in the off-canvas tray, not the main page with the exact same trait.

Realized I didn't override buildForm in WorkflowTransitionAddForm

It seems like this issue has broken the form actions, as we now have a "delete" showing up on the state edit form when it shouldn't, and it also seems to be linking to the wrong place.

@timmillwood, the patch in general, or @tedbow's work on the trait? Something that needs tests either way I think.

I still really think we should put the trait in a dedicated issue since it's a separate concept in a different (stable) subsystem. But we can validate here first that the modal is working correctly for this usecase before splitting it off.

@xjm It's @tedbow's patch in #112, previously we were overriding the EntityForm actions method, the #112 patch reverts this.

Ok this updates the trait to this #2785047-57: In Outside In mode, form validation messages should appear in the off-canvas tray, not the main page

It allows passing the CSS selector of the dialog, defaults to modal selector which this module uses.

The modal forms are redirecting to config/workflow/workflows/manage/editorial (its missing the admin/ chunk).

The edit forms are now showing the delete links (we were removing them):

The add forms are also showing delete links (they shouldn't):

This patch introduces WorkflowEntityForm which has common methods used by 4 of the workflow forms, such as actions, fixing the delete link issue.
The redirect issue is fixed by an update to \Drupal\Core\Form\DialogFormTrait::submitFormDialog which @tedbow seems to be having issues with when running Drupal in a sub directory. The code is similar to that used in \Drupal\Core\Form\ConfirmFormHelper::buildCancelLink, so should work for all unless we have an issue there too.

Wanted to mention here that @effulgentsia was reviewing #2785047: In Outside In mode, form validation messages should appear in the off-canvas tray, not the main page which using this trait and suggested that we might not want to use a trait all. See comment and my reply https://www.drupal.org/node/2785047#comment-12175310

RE #120

I am still getting a page not found when the I use the State edit form in the modal.

I get redirected to http://www.d8.dev/d8_3_rest/d8_3_rest/admin/config/workflow/workflows/ma...

Notice my sub directory d8_3_rest is in their twice.

I know the test bot runs in a sub directory so I check I and I don't there is JS test that actually saves this form in the modal.

WorkflowEditFormTest just checks the form and that validation errors show.

I think if the test coverage was added this would show the failure.

Will try updating the trait here #2785047: In Outside In mode, form validation messages should appear in the off-canvas tray, not the main page because there is test for the redirect on save. See if it indeed fails

+++ b/core/lib/Drupal/Core/Form/DialogFormTrait.php
@@ -0,0 +1,201 @@
+   *   If TRUE the create submit button will be created.q

Weird "q" at the end of line. I probably did this.

Manual reviewing the UI, this is looking really cool. One interesting observation (which is probably entirely follow-up or wont-fix territory) is that hitting "Cancel" gets a "close modal" command from the server and then does it's thing, but the actual "X" button at the top right of the modal has a way to dismiss it instantly. I wonder if we can use the same mechanism on both buttons.

While the trait is being discussed, I've reviewed all the non-trait code. There are a few standards violations that the patch introduces, which we can fix in a round of nits once the patch is ready.

1. +++ b/core/modules/workflows/src/Form/WorkflowEditForm.php
   index 0000000000..dc4e14be13
   --- /dev/null
   
   --- /dev/null
   +++ b/core/modules/workflows/src/Form/WorkflowEntityForm.php
   

   If we aren't intending on use this form as it is and it's designed to be extended, should possibly be called "WorkflowEntityFormBase" or be made abstract. If its primary function is making the other forms that extend it modal-friendly, possibly should indicate that in its name too?

2. +++ b/core/modules/workflows/src/Form/WorkflowEntityForm.php
   @@ -0,0 +1,34 @@
   +/**
   + * Class WorkflowEntityForm.
   + */
   

   Some better docs here maybe?

3. +++ b/core/modules/workflows/src/Form/WorkflowEntityForm.php
   @@ -0,0 +1,34 @@
   +class WorkflowEntityForm extends EntityForm {
   +
   +  use DialogFormTrait;
   
   +++ b/core/modules/workflows/src/Form/WorkflowStateAddForm.php
   @@ -2,14 +2,16 @@
   +class WorkflowStateAddForm extends WorkflowEntityForm {
   +
   +  use DialogFormTrait;
   
   +++ b/core/modules/workflows/src/Form/WorkflowTransitionAddForm.php
   @@ -2,15 +2,17 @@
   +class WorkflowTransitionAddForm extends WorkflowEntityForm {
   +
   +  use DialogFormTrait;
   

   If the base uses the trait, do we have to use it again on the classes that extend it?

4. +++ b/core/modules/workflows/src/Form/WorkflowStateAddForm.php
   @@ -108,15 +110,11 @@ public function save(array $form, FormStateInterface $form_state) {
   +  public function getTitle($workflow = NULL) {
   +    $title = $this->t('Add a new state to the @workflow', ['@workflow' => $workflow->label()]);
   +    return $title;
   

   Once #2894499: Rename 'Editorial workflow' to 'Editorial' gets in, this will read "Add a new state of the editorial". Just something to be aware of.

Expanding WorkflowEditFormTest for covering submitting the edit and delete transition&state forms, and verifying that we are still on the same path and the right message is shown.

In #2785047: In Outside In mode, form validation messages should appear in the off-canvas tray, not the main page it was decided not to use a common trait, so here's a patch moving the trait back to Workflows, but with many of the advances we've seen since the last workflow specific trait.

+++ b/core/modules/workflows/src/Form/DialogFormTrait.php
@@ -120,7 +119,9 @@ public function submitFormDialog(array &$form, FormStateInterface $form_state) {
+      $command = new RedirectCommand($workflow->toUrl('edit-form')->toString());

Per RedirectCommand's constructor, this should have a ->toAbsolute() call before the ->toString()

Removing the tag I added when this was touching \Drupal\Core\Form

Thanks @tim.plunkett!

#2894499: Rename 'Editorial workflow' to 'Editorial' has landed, so we should now do #57 9, 10, and 11

First a reroll, manually fixed conflicts on:

• core/modules/workflows/src/Form/WorkflowTransitionEditForm.php
• core/modules/workflows/src/Form/WorkflowTransitionAddForm.php
• core/modules/workflows/src/Form/WorkflowStateDeleteForm.php

And here the changes adjusting the workflow strings due to #2894499: Rename 'Editorial workflow' to 'Editorial'. Fixes #57 9, 10, and 11 - I also searched for "@workflow" in order to find other possible places to fix.

Modified:

• core/modules/content_moderation/src/Form/WorkflowTypeEditForm.php
• core/modules/workflows/src/Form/WorkflowStateAddForm.php
• core/modules/workflows/src/Form/WorkflowStateEditForm.php
• core/modules/workflows/src/Form/WorkflowTransitionAddForm.php
• core/modules/workflows/src/Form/WorkflowTransitionEditForm.php

1. +++ b/core/modules/workflows/src/Form/DialogFormTrait.php
   @@ -0,0 +1,154 @@
   +   * @param string $dialog_selector
   +   *   The CSS selector for the associated dialog.
   

   Is this necessary anymore? I think I added this change when the trait was in #2896535: Create a helper trait for Forms in ajax dialogs but since this trait is only used inside the workflow module now this will never be used.

   If it is removed you won't have the need for test coverage for different dialog types.

2. +++ b/core/modules/workflows/src/Form/DialogFormTrait.php
   @@ -0,0 +1,154 @@
   +    if (!empty($form['actions']['submit'])) {
   ...
   +    if (!empty($form['actions']['cancel'])) {
   

   Are there case in the workflow module where there will not be a "submit" button? a "cancel" button?

   If not these if statements and $ajax_callback_added could be removed.

3. +++ b/core/modules/workflows/src/Form/DialogFormTrait.php
   @@ -0,0 +1,154 @@
   +          'event' => 'click',
   

   Does the event need to be specified here? Will it work without it?

4. +++ b/core/modules/workflows/src/Form/DialogFormTrait.php
   @@ -0,0 +1,154 @@
   +      $form['#attributes']['id'] = 'dialog-form';
   ...
   +    ]);
   ...
   +      $command = new ReplaceCommand('#dialog-form', $form);
   

   It maybe better to use data-drupal-selector rather id for replaement as we did in #2785047: In Outside In mode, form validation messages should appear in the off-canvas tray, not the main page but you want to look at that patch to see how we got around #2897377: $form['#attributes']['data-drupal-selector'] is set to a random value so can't be used for its intended purpose

5. +++ b/core/modules/workflows/src/Form/DialogFormTrait.php
   @@ -0,0 +1,154 @@
   +    return in_array($this->getRequestWrapperFormat(), [
   +      'drupal_ajax',
   +      'drupal_dialog',
   +      'drupal_modal',
   +      'drupal_dialog.off_canvas',
   

   This should only need to check for "drupal_modal" now that is trait only used within Workflow and only used in modals and not other types of dialogs.

   Also I think having "drupal_ajax" could be a problem because this would pick up a ajax callback even if the form was not in dialog.

   We solved by sending the dialogType back on submit. See the interdiff here #2785047-70: In Outside In mode, form validation messages should appear in the off-canvas tray, not the main page.

+++ b/core/modules/workflows/src/Form/DialogFormTrait.php
@@ -0,0 +1,154 @@
+      $command = new RedirectCommand($workflow->toUrl('edit-form')->setAbsolute()->toString());

+++ b/core/modules/workflows/src/Form/WorkflowEditForm.php
@@ -82,22 +83,36 @@ public function form(array $form, FormStateInterface $form_state) {
+    $options = ['query' => ['destination' => $this->getEntity()->toUrl('edit-form')->toString()]];

Looks like the WorkflowEditForm still uses the query string destination logic I added in previously but the trait itself is now not using it for a redirect. I think it can be removed from the form.

Thanks @tedbow for the review!

First a reroll, manually fixed conflicts on:

• core/modules/workflows/tests/src/Functional/WorkflowUiTest.php
• core/modules/workflows/src/Form/WorkflowTransitionEditForm.php
• core/modules/workflows/src/Form/WorkflowTransitionDeleteForm.php
• core/modules/workflows/src/Form/WorkflowTransitionAddForm.php

I'm nearly done fixing the issues raised by @tedbow, patch coming soon!

Fixing all items mentioned by @tedbow in #134.

Updating the patch due to #2849827: Move workflow "settings" setters and getters to WorkflowTypeInterface, and fixing coding style violations found by the bot.
I think however that displaying the form errors on the modal forms no longer works so this should still fail.

Fixing the error messages in the same way as #2785047: In Outside In mode, form validation messages should appear in the off-canvas tray, not the main page.

#141 fixes the failes from #140, but the #141 fails looks to be unrelated.

ok I think only #134.3 was the only remain item from my review.
So fixed in this patch. Let me know if we need "click" event specified.

The other changes to this patch are test related.

#141 was NOT failing on DrupalCI but was failing for me locally.
Ajax dialog testing is tricky and we have seen a lot random failures on DrupalCI. but problem is usually works locally but doesn't on DrupalCI which is the opposite of what I am seeing now.
@see #2870146: Even more random fails in \Drupal\Tests\outside_in\FunctionalJavascript\OutsideInBlockFormTest
#2830485: \Drupal\Tests\outside_in\FunctionalJavascript\OutsideInBlockFormTest fails randomly
and #2848177: More \Drupal\Tests\outside_in\FunctionalJavascript\OutsideInBlockFormTest random fails

Most of these revolve around waiting for events to happen

I think this cause of my local fails

$this->assertSession()->assertWaitOnAjaxRequest();
this->assertSession()->addressEquals($path);
$this->assertSession()->pageTextContains('Saved Drafty state.');

assertWaitOnAjaxRequest is tricky with ajax redirect. I think what is happening locally is that that assert passes without the redirect being done.
Then the path 1 passes and the last one fails.

I have changed this to not worry about ajax all and just wait for the "saved" method to appear on the page and then check the path.

Thanks @timmillwood & @tedbow!

+++ b/core/modules/workflows/tests/src/FunctionalJavascript/WorkflowEditFormTest.php
@@ -187,9 +182,23 @@ public function testWorkflowTransitionsForm() {
+  protected function assertElementVisibleAfterWait($selector, $locator, $timeout = 10000) {

While I see how this method makes the tests easier to follow, can't help but think that this would also be useful on other situations... let's find a better place for it... perhaps JavascriptTestBase?

@Manuel Garcia should we make a follow up to add assertElementVisibleAfterWait \Drupal\FunctionalJavascriptTests\JSWebAssert where waitForElementVisible lives.

@tedbow follow up to move it to JSWebAssert makes sense to me

Thanks for the patch update @tedbow, looks RTBC ready to me!

created #2898777: Add assertElementVisibleAfterWait to JSWebAssert

@timmillwood yes looks very close.

Added to @todo to remove assertElementVisibleAfterWait after #2898777: Add assertElementVisibleAfterWait to JSWebAssert

I think this is looking good now, thanks @tedbow.

I'm going to RTBC, but think this needs a +1 from @tedbow and / or Manuel as I wrote a lot of the patch.

+++ b/core/modules/workflows/src/Form/DialogFormTrait.php
@@ -0,0 +1,129 @@
+  public function closeDialog(array &$form, FormStateInterface $form_state) {

We no longer need either 1 of these parameters to this function

Otherwise than that +1 on RTBC