Add option to display confirmation form in a modal to the formatter

This looks great! Thanks! I wish you had submitted your patch a day earlier so this would have made it into the release.

Updated the patch to account for the fix introduced in #3220111: Url route parameters should be merged when redirecting to the confirmation form.

Also... would be great to have a functional javascript test that tests the modal, or we could simply assert that "links" are rendered instead.

Same patch, with a functional javascript test that ensures the links are present, and that the form is opened. I tried also applying the transition by clicking the buttons but this didn't work right away and don't think it's worth investigating this since we're already testing the exact same confirmation form in a regular functional test.

Committed! Thank you!

I have noticed two issues with this patch, which perhaps can still be fixed?

When we introduced the state transition form, it was built so that there is no hard requirement on the state-transition-form link template (using the 'old' behavior when an entity does not define it). With this patch there is a hard requirement introduced and viewing an entity without a link template results in an error.
I think we should wrap this in a conditional?

Also when the 'require confirmation' checkbox on the view display is turned off (after first turning the modal option on), the summary displays:

Do not require confirmation before applying the state transition
Display confirmation in a modal dialog

I think it would be better to strip the whole dialog text form the summary as well.

Finally, I am having some issues with the confirmation form not showing when the modal is off. But I am not yet sure if that is coming from this issue.

Confirming that the form works when using the modal, but with the modal turned off I get redirect to /admin/commerce/orders/carts as soon as I try to place an order from the commerce backend. No form, no state change.

Upping to critical due loss of function.

@ Neograph734: Could you have a fresh look? Also now ensure that the settings for requiring the confirmation and using the modal aren't displayed when the target entity type doesn't support the confirmation form.

Made several other changes that should fix all the issues reported.

Thanks for testing!

@jsacksick, looking good! I ran into one little issue.

When you have the confirmation form enabled (modal or not) and you perform a transition you get redirected back to the entity view. When disable confirmation you get redirected to a collection page?

Reproduce by

1. switching confirmation off
2. putting an item in a cart
3. Visit /admin/commerce/orders/carts
4. open the order and do the transition
5. You are then redirected to /admin/commerce/orders/carts instead of still looking at the order.

It could be a minor annoyance for some people. Specifically because the cart is now an order and does no longer show in the list.

Also now ensure that the settings for requiring the confirmation and using the modal aren't displayed when the target entity type doesn't support the confirmation form.

Great and sensible last minute improvement! Perhaps it might be even better to display 'This entity type does not support transitions'? So people do not go looking for a way to enable them? But I leave that up to you.

Hm... regarding:

"'This entity type does not support transitions'? "

This isn't true, the entity type doesn't support showing a confirmation form (there's a difference)... But this is too technical and perhaps we shouldn't display anything in this case (Attaching a patch for that)... So users don't look for a way to enable this behavior.

I don't see where we're redirecting to the collection url, oh actually, we aren't, this is probably due to the destination parameter from the url.

Perhaps the form should enforce redirecting to the canonical url, or we could keep this as is... (Since this behavior has always been in place).

This isn't true, the entity type doesn't support showing a confirmation form

Ah yes, that is what I meant. Not showing anything at least removes the confusion, so that should be good.

Perhaps the form should enforce redirecting to the canonical url, or we could keep this as is... (Since this behavior has always been in place).

I came to same conclusion, we did not touch it... Canonical makes slightly more sense to me because then you can see the state change happening. In the end it does not really matter to me, I just noticed it when switching options on and off during testing. I suppose it is not something to block a release on (specifically because the current release has the issue from #9?)

We can definitely redirect to the canonical url too though someone relying on the destination parameter would suddenly complain about the change.

This can be fixed by ensuring the destination isn't present in the url.

Ah, now I see. That same destination parameter is the reason I dit not see a form or a state change in #9. I got (and am now again) redirected back to the cart overview page. So the issue from 9 is not yet solved. I am not sure why it did not show when testing in #12 though...

I do agree that the destination parameter should be respected. But then we should pass it along for one more form. Something like this?

Current situation:

1. Require confirmation is enabled, no modal.
2. View cart order on /admin/commerce/orders/14?destination=/admin/commerce/orders/carts from cart overview page.
3. Click 'Place order'.
4. Get redirected back to cart overview page without transition (because the destination parameter takes precedence)

Desired situation:

1. Require confirmation is enabled, no modal.
2. View cart order on /admin/commerce/orders/14?destination=/admin/commerce/orders/carts from cart overview page.
3. Click 'Place order'.
4. See confirmation form and confirm.
5. Get redirected back to cart overview page from the destination parameter on the confirmation form. (Canonical otherwise)

Ok the attached patch is now outputting links to the confirmation form (instead of a redirect), regardless or whether the modal is used.

This should fix #17.

Also, we probably should have opened a different issue for clarity as this is not a bug introduced by the work done here...

Confirming that the expected behavior is restored and the forms are working as expected.

But this solution now contradicts with #15? I think it would be possible to read the destination from the state transition form and pass it along to the confirmation form. But that is less urgent. (We could follow that up in a separate issue.)

@Neograph734: Thanks a lot for your thorough testing, that truly helped!

Regarding the redirect, I believe the behavior should be the same both when using the confirmation form and when not using it.

The destination parameter should be respected either way. We can probably now tag a release as is...

By the way, you can update the view to not append the destination parameter to the order operations links, thus fixing the redirection and you should remain on the canonical url.

I guess the only change introduced by the latest patch in that regard is that the destination parameter is not passed along the confirmation form... correct?

That's probably the only inconsistency (i.e not pass along the destination parameter), but my preference is also to redirect to the canonical url.

I'm not sure we can easily ensure the destination parameter gets ignored.

Yes, when you transition via the confirmation form you end up at the canonical display, whereas if you have no confirmation you follow the destination redirect.

I'm not sure we can easily ensure the destination parameter gets ignored.

Well, actually I think you already did that. If I am at /admin/commerce/orders/17?destination=/admin/commerce/orders/carts and I click the 'place order' button, the redirect is removed and I go to /admin/commerce/orders/17/state/place, which then goes to /admin/commerce/orders/17.

To make everything consistent I would recommend that the 'place order' button goes to /admin/commerce/orders/17/state/place?destination=/admin/commerce/orders/carts instead. So reading it from the request in the state transition form and passing it along to the confirmation form builder. I suppose (did not test) that I would again end up at the cart overview page.

But I agree that this should be good enough to tag a release. Everything is working as it should.

Arguably it would indeed be better to remove the redirect from the views buttons, which I might just do.

Well, actually I think you already did that. If I am at /admin/commerce/orders/17?destination=/admin/commerce/orders/carts and I click the 'place order' button, the redirect is removed and I go to /admin/commerce/orders/17/state/place, which then goes to /admin/commerce/orders/17.

I did that for the confirmation form (where it's not passed along).
I was referring to the other case (when not using the confirmation form) where it's actually complicated to not respect the destination).

But when not using the confirmation form, the destination parameter would still be respected.

Not really in favor of passing it along, because in the case you're describing, redirecting to the order collection feels more like a bug than a feature :p.

Not really in favor of passing it along, because in the case you're describing, redirecting to the order collection feels more like a bug than a feature :p.

I agree with you that it makes more sense to end up at the canonical view :) But, that might also be a commerce issue which should not be passing the destination parameter. That would allow state machine to respect it when it is presented. IMO that woud be the best experience. But I shall not force you into last minute changes to commerce ;)

It is currently good enough for me and the past few days here have led to major improvements to state machine!

I agree with you that it makes more sense to end up at the canonical view :) But, that might also be a commerce issue which should not be passing the destination parameter. That would allow state machine to respect it when it is presented. IMO that woud be the best experience. But I shall not force you into last minute changes to commerce ;)

Well, we can update the default view to not append the destination (on new installs), however there are other cases where it might be useful (i.e when deleting an order).

Without it, I believe you stay on the same delete page which becomes a not found... So while it may work in this particular case, it's a potential problem for other operations.

Then just tag it ;)