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

minor: SummaryViewModel outputs #1264

Merged
merged 19 commits into from
Jun 14, 2024
Merged

Conversation

jenbutongit
Copy link
Contributor

@jenbutongit jenbutongit commented Jun 6, 2024

Description

This is a refactor of SummaryViewModel and the outputs it creates.

Previously, SummaryViewModel was created to reduce the size of SummaryPageController and make the code more readable. Much of SummaryPageController's constructor code was just moved to SummaryViewModel, including code which created outputs.

It was easiest to generate outputs (specifically the webhook output) from the SummaryViewModel, since it was a "flattened" form of the state data, which included the "human readable" titles of fields. However, some additional iteration/nested loops still happened between SummaryViewModel.details (i.e. what is needed to render the summary page) and pages to "fill in the gaps".

This has caused tight coupling between SummaryViewModel.details and WebhookModel. The tight coupling makes #1263 (#798) too big of a PR and introduce more loops to fill the gaps/try to determine iterations (as exhibited by the current repeated code!).

A further refactor of SummaryViewModel is necessary, so that SummaryViewModel is only responsible for parsing data for the purposes of rendering the page.

  • Create a new Outputs class, which encapsulates Output creation
    • This is still instantiated within SummaryViewModel, but can be easily abstracted out
  • Move getRelevantPages to FormModel
  • WebhookModel now only relies on FormModel and state

Type of change

Please delete options that are not relevant.

  • Refactor

How Has This Been Tested?

Please describe the tests that you ran to verify your changes. Provide instructions so we can reproduce
the testing if necessary.

  • Unit tests
    • During the refactor, tests were written for the new and old WebhookModels to ensure that the output was the same

Checklist:

  • I have performed a self-review of my own code
  • I have commented my code, particularly in hard-to-understand areas
  • I have made corresponding changes to the documentation and versioning
  • I have updated the architecture diagrams as per Contribute.md OR added an architectural decision record entry

@jenbutongit jenbutongit changed the title add tests for webhook model refactor: SummaryViewModel outputs Jun 10, 2024
@@ -38,7 +28,7 @@ export function NotifyModel(
model: FormModel,
outputConfiguration: NotifyOutputConfiguration,
state: FormSubmissionState
): NotifyModel {
): TNotifyModel {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I was getting clashes with NotifyModel the function and NotifyModel the data type, so I've renamed it to TNotifyModel

item.isRepeatable ? (index = i) : 0;
const fields = [detailItemToField(item)];
function createToFieldsMap(state: FormSubmissionState) {
return function (component: FormComponent | SelectionControlField): Field {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Should this be FormComponent | SelectionControlField | ComponentCollection? Just thinking that below you're looking for a children collection, so wasn't sure whether that was related to ComponentCollection or whether it's somewhere in the SelectionControlField that I couldn't see

Copy link
Contributor Author

@jenbutongit jenbutongit Jun 12, 2024

Choose a reason for hiding this comment

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

I'm specifically looking for what would be conditionally revealed components, which would live on SelectionControlFields. This test should fail every time because SelectionControlField.items.childrenCollection?.formItems doesn't exist anymore

Just double checked that this doesn't clash with component collection implementation though. I think this would only be an issue for date parts fields, which is first passed through as a FormComponent (subclass). We're just retrieving from state directly here, not via the component.

lmk if you think I've missed something though 🫡

@jenbutongit jenbutongit changed the title refactor: SummaryViewModel outputs minor: SummaryViewModel outputs Jun 14, 2024
@jenbutongit jenbutongit merged commit ced4842 into main Jun 14, 2024
13 checks passed
@jenbutongit jenbutongit deleted the refactor/decouple-output-models branch June 14, 2024 11:42
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants