Refactor the step-by-step storage setup #17738
Conversation
| #++ | ||
|
|
||
| module Storages | ||
| class Wizard |
There was a problem hiding this comment.
This class was written pretty generically, not with only storages in mind. I could for example imagine to also use it for the OIDC provider setup, which from the UX point of view looks pretty similar.
Any opinions on moving this class to the core and calling it Wizard instead of Storages::Wizard?
There was a problem hiding this comment.
I can't wait to see the Fighter, Rogue and Cleric joining the party 🤣
There was a problem hiding this comment.
This class was written pretty generically, not with only storages in mind. I could for example imagine to also use it for the OIDC provider setup, which from the UX point of view looks pretty similar.
Any opinions on moving this class to the core and calling it
Wizardinstead ofStorages::Wizard?
Really nice one Jan! 👏🏾 I would imagine this as a (view) component concern app/components/concerns ? The bare namespace Wizard might be too generic IMO- I suggest MultiStepWizard
There was a problem hiding this comment.
Two challenges to that.
Strongest one first: I don't think that this belongs into concerns anywhere, because as far as I can tell this directory is intended for modules that enhance/change the behaviour of classes including them, taking the name from ActiveSupport::Concern. Since the wizard is not an includable module, but an inheritable base class, it doesn't belong into concerns.
I'd also go as far as not wanting to put it into components, because it doesn't implement a component itself. But I see that after the latest iteration it at least influences components strongly, so I might be convinced on that. Which other places would there be? services is my "catch-all" for classes that do things. I think we will not have enough to warrant the creation of a wizards directory.
Second challenge: In naming things, I see the desire to clarify by adding words, though this leads to long class names. Consistently MultiStepWizard would lead to NextcloudStorageMultiStepWizard etc. But then, the term wizard already implies multiple steps, I didn't make that term up, I just think it describes what we are doing here :)
| preparation: ->(storage) { storage.build_oauth_client } | ||
|
|
||
| step :redirect_uri, | ||
| completed_if: ->(storage) { storage.oauth_client && storage.oauth_client.created_at < 3.seconds.ago } # HACK HACK HACK |
There was a problem hiding this comment.
We are running into a limitation with the approach of expecting all state on the model.
The purpose of this last step in the UI is as follows: Previously you configured the oauth_client and now we want to show you the corresponding redirect URL, because you need to copy it into the OAuth 2.0 AS.
However, there is no change happening on the storage that we could observe to confirm that this step is done.
The current (hacky!!!) solution is to check whether the client was recently created, if yes you need to see the URL, else not (if your browser takes too long for the redirect, bad luck).
Alternatively we could store something in the options of the storage to confirm that this piece of UX was shown, but then we'd have something UI-specific saved on the storage. Doing that also feels wrong.
Good ideas are welcome.
There was a problem hiding this comment.
Can´t we pass down some flag around to represent a newly created OAuth Client?
There was a problem hiding this comment.
So the whole idea is that our code can safely come from a redirect to (e.g.) .../new?continue_wizard=:id. We explicitly wanted to achieve independence between steps, so redirecting to something like .../new?continue_wizard=:id&did_i_just_create_a_client=yes would defeat the purpose, because then the client creating step would need to know, that the next step is also interested in the client (which it only is for OneDrive, but not for Nextcloud).
Also, depending on information that's not stored on the model would mean, that the wizard state is not reproducible. E.g. after completing the wizard, I might see pending_steps again. This would practically work for our current use case, but looks like a design flaw to me. It would definitely prevent use cases such as sourcing the "complete"/"incomplete" labels from a wizard.
There was a problem hiding this comment.
What do you think about persisting the user's acknowledgement of copying the redirect URL into the database?
No-no? Or acceptable?
NobodysNightmare
force-pushed
the
wizard-configuration
branch
4 times, most recently
from
034cd45 to
4397711
Compare
|
One more thought: I was considering to replace the However, I decided against it (for now), because also checks like |
| if wizard_step == "oauth_client" | ||
| render(Storages::Admin::Forms::OAuthClientFormComponent.new(oauth_client: storage.oauth_client, storage:, in_wizard: true)) | ||
| else | ||
| render(Storages::Admin::OAuthClientInfoComponent.new(oauth_client: storage.oauth_client, storage:)) | ||
| end |
There was a problem hiding this comment.
You probably tried, but let me ask anyway: have you tried to move all these conditionals to the component itself (the Object)?
There was a problem hiding this comment.
I added another commit that drives the refactoring further. While that keeps the conditional inside the StorageViewComponent, it reduces the number of conditionals for that purpose to one instead of many.
NobodysNightmare
force-pushed
the
wizard-configuration
branch
8 times, most recently
from
5e5956a to
141b73d
Compare
The idea is that now there is a central class (a Wizard) managing the steps necessary to guide the user through creation of a new storage. This avoids problems such as the update action of step A being required to prepare some parts of step B already, so that the view/component for step B can be rendered correctly. Since the same components are used inside and outside of the step-by-step wizard, we are explicitly transferring the state on whether we are currently in a wizard via parameters. This results in some degree of boilerplate, but is hopefully more clear than guessing based on context, whether or not we should show the next step after submission or go back to regular edit mode.
The storage view component now has no own "opinion" on the display order and grouping of steps anymore, but will merely reflect the wizard configuration. Changes in the wizard are immediately reflected in the UI as well. Selection of the right components was also extracted into the storage registries. Renamings of existing translations were in most cases avoided, by renaming wizard steps according to existing translations. This made it necessary to unify the instantiation of all components rendered inside the storage view, which was relatively straight-forward for most of them. The OAuthClientFormComponent was challenging, as there was one use case where it needed to be initialized with a client that was not yet assigned to the storage.
NobodysNightmare
force-pushed
the
wizard-configuration
branch
from
141b73d to
7a0f1db
Compare
The idea is that now there is a central class (a Wizard) managing the steps necessary to guide the user through creation of a new storage.
This avoids problems such as the update action of step A being required to prepare some parts of step B already, so that the view/component for step B can be rendered correctly.
Since the same components are used inside and outside of the step-by-step wizard, we are explicitly transferring the state on whether we are currently in a wizard via parameters. This results in some degree of boilerplate, but is hopefully more clear than guessing based on context, whether or not we should show the next step after submission or go back to regular edit mode.
Ticket
https://community.openproject.org/projects/cross-application-user-integration-stream/work_packages/60930
What are you trying to accomplish?
Reducing the coupling between steps in the storage setup, so that the exchange of steps is easy, without influencing other steps in the form.
What approach did you choose and why?
The idea is that there is a wizard that is responsible for managing the necessary steps, while the state is fully stored on the model. This means the wizard should at any point be able to look at the model and decide which steps still need to be done and which steps have been completed already.
This means a controller does not need to know the next step in the process to prepare that already. However, a controller will still need to distinguish between the normal "edit" case (redirecting back to edit) and the wizard/step-by-step case, where it will redirect back to a wizard url (the URL itself does not encode state, except for the model id).
Merge checklist