Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Doc(adr): move formFields from CMS into app #1339

Draft
wants to merge 3 commits into
base: main
Choose a base branch
from
Draft
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
137 changes: 137 additions & 0 deletions doc/adr/0013-pageConfig-inside-application.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,137 @@
# 13. pageConfig inside application
chohner marked this conversation as resolved.
Show resolved Hide resolved

Date: 2024-10-22

## Status

Proposal

## Context

Currently, the information about what field appears on what page is **solely** encoded in the CMS. In order to render a page, the app has to fetch the content for a given URL from the CMS, which may contain a list of fields and their labels (next to the page content).

#### Terms

- `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
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
- `fieldLabel`: The user-facing string associated
- `fieldLabel`: Description value render on the UI. Value can be optional for a few components.

- `stepSchema`: All schemas required for a single page, e.g.:
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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"]) }).

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

```typescript
{
hasAnwalt: z.enum(["yes", "no"]),
hasContacted: z.enum(["yes", "no"]),
}
```
- `SchemaStore`: A service that can return a `stepSchema` based on a list of `fieldNames`. Our current function is called `validatorForFieldNames()`.

#### Diagram

Both the front- and back-end need the `stepSchema` to validate user input. The following is a rough sequence diagram:

```mermaid
sequenceDiagram

Loader->>CMS: GET(pathname)
CMS-->>Loader: content, fieldNames, fieldLabels
Loader->>Frontend: content, fieldNames, fieldLabels
Frontend->>SchemaStore: fieldNames
SchemaStore->>Frontend: stepSchema
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Frontend->>Action: POST(formData)
Action->>SchemaStore: fieldNames
SchemaStore->>Action: stepSchema
```

Note that the `fieldNames` are passed from the CMS via the loader + front/back-end into the `SchemaStore`.

#### Downsides

- Resilience: The app **cannot** work without the CMS
- Slow prototyping of new flows, as it requires constant addition of pages to the CMS
- Mental load: Its hard to understand what fields are filled out on what page (requires manually checking the CMS)
- Additional traffic: When the app needs to know about fields on pages, it needs to make an additional request to the CMS (for example in the pruner)
- Because unchecked checkboxes and radio groups are not sent in html forms, we inject a hidden input to "inform" the app that the input _should_ be there (currently affects our `Checkbox`, `RadioGroup` and `TileGroup` components). See this [StackOverflow article](https://stackoverflow.com/a/1992745) for more context.
- In the CMS, field names (`hasAnwalt`, ...) and field types (`radio`, `dropdown`, ...) are hard-coded on each flow page. They need to exactly match the name & schema in the app (but are free inputs with a label `Do not change`). Any mismatch breaks the flow (eg "validation for field "abc" could not be found").

## Proposal

Instead, the app should be the single-source-of-truth for flow configuration, which includes the `stepSchema`. The CMS should serve content _only_ (which of course would include the form labels).

A new `PageConfig` abstraction should be added, that links the `stepId` and `stepSchema` of each page and would replace our current `SchemaStore` (aka `context`/`userInputSchema`):

```typescript
type StepSchema = Record<string, z.ZodType>;
type PageConfig = Record<string, { stepId: string; stepSchema: StepSchema }>;

const pages = {
anwalt: {
stepId: "/anwalt",
stepSchema: { hasAnwalt: z.enum(["yes", "no"]) },
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How are going to associate the values and translation from Strapi? Or it's going to be as it's?

Copy link
Member Author

@chohner chohner Oct 28, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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)

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

so should we first restructuring our CMS first before to make the change proposal in this ADR?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member Author

@chohner chohner Oct 28, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

},
} satisfies PageConfig;
```

#### Diagram

Now, any component can receive a `stepSchema` directly from the `PageConfig` by passing the `pathname`:

```mermaid
sequenceDiagram

Loader->>PageConfig: pathname
PageConfig->>Loader: fieldNames
Loader->>CMS: GET(pathname, fieldNames)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

CMS-->>Loader: content, fieldLabels
Loader->>Frontend: content, fieldLabels
Frontend->>PageConfig: pathname
PageConfig->>Frontend: stepSchema
Frontend->>Action: POST(formData)
Action->>PageConfig: pathname
PageConfig->>Action: stepSchema
```

#### Note

While not necessary, this could be integrated in our existing xStateConfig:

```typescript
const xStateConfig = {
states: {
[pages.anwalt.stepId]: {
meta: { stepSchema: pages.anwalt.stepSchema },
},
},
};
```

## Consequences

### Pros:
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How this changes would improve the development process?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do you mean about the flow logic, only if I use the .ts instead of .json for the configuration of the xstate?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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


- Domain driven architecture: Full decoupling of flow configuration from content
- Reduced mental load when reasoning about form fields and their position inside the flow
aaschlote marked this conversation as resolved.
Show resolved Hide resolved
- App works without CMS (pruner, prototyping, ...)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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


### Cons:

- Field names and definition in the CMS still need to match the ones in the app. However, on a mismatch we would "just" have missing labels, rather than full app breakage
- Cleanly moving a field from between existing pages might become more complicated (See below)
aaschlote marked this conversation as resolved.
Show resolved Hide resolved

### Future consideration

#### Moving form field to another page

While this is rarely done, it would be a bit more complicated due to the separate app & content release:

1. Add form field to new page
2. Trigger content release (the new page wouldn't change as the app is in charge of whats shown)
3. Field is moved to its new location in the `PageConfig`
4. Trigger app release (now the form field will appear on the new page)
5. Remove form field from the old page in CMS

#### Synchronizing form fields

The downside of doubling field names & types could be mitigated by synchronizing the CMS with the app. This could be either done by:

- Publishing the fieldNames via an endpoint that the CMS consumes
- Pushing changes to the CMS on deploy (probably via a simple POST request)