Doc(adr): move formFields from CMS into app #1339
Conversation
|
|
||
| Loader->>CMS: GET(slug) | ||
| CMS-->>Loader: content, fieldnames, formLabels | ||
| Loader->>Frontend: content, fieldnames, formLabels |
There was a problem hiding this comment.
Is there a better word than "Frontend"? It's very generic. Is this the ValidatedForm maybe?
There was a problem hiding this comment.
I thought staying generic might be less confusing than specifying the exact consumer of each (between the nested page/form/field components)...
| - `stepSchema`: All schemas required for a single page, e.g.: | ||
| ```typescript | ||
| { | ||
| hasAnwalt: z.enum(["yes", "no"]) }, | ||
| hasContacted: z.enum(["yes", "no"]) }, | ||
| } | ||
| ``` |
There was a problem hiding this comment.
I don't think that concept currently exists? The schemaStore merely returns a list of validations?
There was a problem hiding this comment.
The word does not exist but we do build a validation for exactly the formData object (not a list)
| CMS-->>Loader: content, fieldnames, formLabels | ||
| Loader->>Frontend: content, fieldnames, formLabels | ||
| Frontend->>SchemaStore: fieldnames | ||
| SchemaStore->>Frontend: stepSchema |
There was a problem hiding this comment.
Maybe to make it more clear a self-loop with "validation" would help? To be clear what the schemas are even used for in the frontend?
|
|
||
| - Its hard to understand where which fields are filled out on what page (requires manually checking the CMS) | ||
| - The app **cannot** know why fields where seen when traversing a flow (pruner needs to request the CMS) | ||
| - The app **cannot** work without the CMS |
There was a problem hiding this comment.
Is this really the only point that keeps us from showing pages without content? If we do the change, this problem doesn't exist anymore?
There was a problem hiding this comment.
I.. think so? can't think of anything else 🤔
| ### Pros: | ||
|
|
||
| - Full decoupling of domains (flow configuration vs content) | ||
| - Follows mental model of domain driven architecture |
There was a problem hiding this comment.
rephrased, does it make more sense?
|
|
||
| - `fieldname`: the name of a field in the form, e.g. `hasAnwalt` | ||
| - `fieldSchema`: Combination of fieldname and its schema, e.g. `hasAnwalt: z.enum(["yes", "no"]) }` | ||
| - `stepSchema`: All schemas required for a single page, e.g.: |
There was a problem hiding this comment.
I'm still wondering if we could go one step further and not only represent pages this way but pages within whole subflows. Such as:
anwaltlicheVertretung: {
anwaltEntscheidung: {
hasAnwalt: z.enum(["yes", "no"]) },
hasContacted: z.enum(["yes", "no"]) },
}
}That would mean that we could validate whole sub-areas of the flow by checking only for anwaltlicheVertretung and gathering all fields in there.
I don't know, probably a thought for the future as this is not that well thought-out, still wanted to leave it here.
There was a problem hiding this comment.
I'm not sure I follow - but I have to admit, nested schemas (and arrays) are still not prototyped using this approach.
Right now I think we could either encode them the way you showed (which matches our current userInputSchemas) or do it like in the CMS (eg anwaltEntscheidung.hasAnwalt: z.enum(["yes", "no"]) }).
There was a problem hiding this comment.
I think that's essentially the same thing? With the first option just giving us more variety (because you can define things on the object-level)?
But yes, this is probably just one step further and not part of this ADR necessarily
There was a problem hiding this comment.
I'm bit confusing about the two parts:
- How the associate the fieldNames from the app with the CMS
- How these changes will improve the developer experience. The changes sounds huge and it's not clear if it worth to do it. I would need some examples before to approve it.
Another thing we need to think, it's about how to move all the contexts to stepSchema (please think about a better name), because it exists a lot of fields in the current flows, specially for beratungshilfe and flugggastrechte. How we will need to do all the change without break the application.
| - `field`: A user-facing input that appears on a page inside a flow | ||
| - `fieldName`: the name of a field in the form, e.g. `hasAnwalt` | ||
| - `fieldSchema`: Combination of field name and its schema, e.g. `hasAnwalt: z.enum(["yes", "no"]) }` | ||
| - `fieldLabel`: The user-facing string associated |
There was a problem hiding this comment.
| - `fieldLabel`: The user-facing string associated | |
| - `fieldLabel`: Description value render on the UI. Value can be optional for a few components. |
|
|
||
| Loader->>PageConfig: pathname | ||
| PageConfig->>Loader: fieldNames | ||
| Loader->>CMS: GET(pathname, fieldNames) |
There was a problem hiding this comment.
For me still not clear how the fieldNames will be connected with CMS. Can you better explain this integration? And where the validation should be set? In the CMS or source code?
There was a problem hiding this comment.
I agree this is confusing because we probably wouldn't send the fieldnames right now (as they already exist on each flow page). The schemas continue to live in the app just like they do now
| const pages = { | ||
| anwalt: { | ||
| stepId: "/anwalt", | ||
| stepSchema: { hasAnwalt: z.enum(["yes", "no"]) }, |
There was a problem hiding this comment.
How are going to associate the values and translation from Strapi? Or it's going to be as it's?
There was a problem hiding this comment.
Generally I think our data model in strapi also needs to be refactored, but I didn't want to include this in this ADR (and also not create another ADR, we already have a lot...).
So for now I propose to leave as is because each flow page in Strapi already contains the correct labels etc.
But I could be convinced to park this ADR in favor of restructuring our CMS first (I think @rbrtrfl prefers that too)
There was a problem hiding this comment.
so should we first restructuring our CMS first before to make the change proposal in this ADR?
There was a problem hiding this comment.
I dont have a strong opinion on the order, however I have the feeling restructuring the CMS is probably going to be a bigger task and I think the benefits of this are useful already.
But I would be fine with either order
|
|
||
| ## Consequences | ||
|
|
||
| ### Pros: |
There was a problem hiding this comment.
How this changes would improve the development process?
There was a problem hiding this comment.
I think having an understanding of which page fills out which field is going to be useful when reasoning about flow logic. And creating new flows should be much faster.
There was a problem hiding this comment.
Do you mean about the flow logic, only if I use the .ts instead of .json for the configuration of the xstate?
There was a problem hiding this comment.
No, this would apply to any flow. I think this will become more of a problem as we add more flows because its will get harder and harder to remember with field comes from which page
| const pages = { | ||
| anwalt: { | ||
| stepId: "/anwalt", | ||
| stepSchema: { hasAnwalt: z.enum(["yes", "no"]) }, |
There was a problem hiding this comment.
Why is called stepSchema? Those are not fieldSchema? Not sure why we have decided for the name step. It bring a bit of confusion when I need to explain it for other people, specially for non engineers.
There was a problem hiding this comment.
Its the collection of all fieldSchemas for specific step. For Example:
const airportStepSchema = {
startAirport: z.string(),
endAirport: z.string(),
}startAirport and endAirport would each be a fieldSchema and airportsSchema would be a stepSchema. Maybe pageSchema is a better name?
Note: This construct already exists as the return value of validatorFromFieldnames but is generated on the fly and therefore quite hidden
|
|
||
| - Domain driven architecture: Full decoupling of flow configuration from content | ||
| - Reduced mental load when reasoning about form fields and their position inside the flow | ||
| - App works without CMS (pruner, prototyping, ...) |
There was a problem hiding this comment.
I am unsure if "App works without CMS" is a good thing. If the app can function without the CMS, does that mean it might not alert us to issues with missing content? Since content is essential to delivering value to users, wouldn’t it be better for the app to indicate when the CMS isn’t working or content is missing? This way, we’d catch potential issues during testing or production instead of relying on users to report them. or maybe I understood it differently?
There was a problem hiding this comment.
Of course altering of missing content should still happen. but we shouldn't need to crash the whole application, especially when prototyping. Also the pruner shouldn't need to rely on the CMS
chohner
changed the title
I recommend reading the ADR with rendered markdown to display diagrams:
https://github.com/digitalservicebund/a2j-rechtsantragstelle/blob/adr_pageConfig_inside_app/doc/adr/0013-pageConfig-inside-application.md