Avoid submitting duplicate Connected Apps #12481
Conversation
Simple Rails forms prevent double-clicking on submit already. Converting the StimulusReflex interaction to a simple form submit to a controller solves the race condition. The UX is slightly worse because the whole page is reloaded instead rendering only the connected app panel. But we can solve that when we add more apps and want to activate them independently. By then, we may have good patterns for working with Turbo. Technically, the new buttons are a form within a form which is invalid HTML, but it works.
mkllnk
added
the
technical changes only
| @@ -6,16 +6,14 @@ | |||
| %p= t ".tagline" | |||
| %div | |||
| - if enterprise.connected_apps.empty? | |||
| %button{ data: {reflex: "click->Admin::ConnectedApp#create", enterprise_id: enterprise.id} } | |||
| = t ".enable" | |||
| = button_to t(".enable"), admin_enterprise_connected_apps_path(enterprise.id), method: :post | |||
There was a problem hiding this comment.
Technically, the new buttons are a form within a form which is invalid HTML, but it works.
That doesn't seem ideal to me, and might have unexpected drawbacks. But I can't think of a better, so if it works, it works!
I'm guessing if you have other unsaved changes on the Enterprise (from another tab), it will display a confirmation as usual? That could be a little confusing for users, but I think that would be rare and fine for this case.
I'm not sure if the enterprise fields would also be submitted by this button, but even if they are, I expect they'd be silently ignored.
There was a problem hiding this comment.
This may be the cause for the latest bug:
My tests show that it was this commit.
|
Hey @mkllnk , I was able to reproduce the multiple click scenario on staging-FR, but indeed only as superadmin. The connect button was not immediately disabled, which led to multiple connect attempts, and eventually, an error 500 (not seen in the console, but on Bugsnag only): The button remains on an endless spinner state:
After this, I've staged the PR, and could verify that:
This becomes an issue, as the button becomes broken and unusable (even after hard refresh and/or login/out). Again, not introduced by this PR, but I can't spot an improvement. How do you think we should proceed? Is there any significant code improvement you see should be included in master, as is? If so, please feel free to merge. But if not, I guess we need to move it back to In Dev. |
filipefurtad0
added
feedback-needed
and removed
pr-staged-fr
|
Was Connected Apps built with StimulusReflex? There is also a report on this in #12425 |
Yes, I think so @sigmundpetersen, as on the #12472 PR, there is a specific test case to check for this connection. So I guess I've stumbled on #12425 indeed. Thanks for pointing it out ✨ |
Oh, that's another bug. You shouldn't be able to see that button as super admin unless you are a manager of the enterprise. If you are not a manager of the enterprise, your API key isn't authorised to fetch data from that enterprise and the connection doesn't work. I'll merge this PR but I will look into the super-admin problem. |
What? Why?
Simple Rails forms prevent double-clicking on submit already. Converting the StimulusReflex interaction to a simple form submit to a controller solves the race condition.
The UX is slightly worse because the whole page is reloaded instead rendering only the connected app panel. But we can solve that when we add more apps and want to activate them independently. By then, we may have good patterns for working with Turbo.
Technically, the new buttons are a form within a form which is invalid HTML, but it works.
What should we test?
Release notes
Changelog Category (reviewers may add a label for the release notes):
The title of the pull request will be included in the release notes.
Dependencies
Documentation updates