-
Notifications
You must be signed in to change notification settings - Fork 258
fix: update cht-form to not fail when loading a form using extension-… #9840
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
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great work! i think I have one question inline.
// @ts-expect-error it does exist | ||
component.formModel = undefined; | ||
// @ts-expect-error it does exist | ||
component.contactSummary = undefined; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have a PR that adds an additional field to formContext. This link between form-context AND the cht-form sems very loose. I wouldn't know I had to reset this new field if I had not read this code.
Is it possible to somehow iterate over the possible properties of the formContext interface, to get their names, and reset their corresponding value in this component instead of hardcoding them here as well?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have a PR that adds an additional field to formContext. This link between form-context AND the cht-form sems very loose.
To be clear, these fields that we need to reset here are the @input
fields defined above for this component instance. They are not directly related to the EnketoFormContext
from the enketo.service. Particularly if the fields are optional, you should not need to make any changes here in cht-form unless you explicitly wanted to be able to inject that data (and in that case you would need to add a new @input
above and then clear it out down here).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ah, I see.
This new field would be required to be testable, as it's going to contain the contact summary of the logged in user.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is there any way we could connect the cht-form with the EnketoFormContext interface?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In theory I think we could make EnketoFormContext
an interface and then include an implementation here in cht-form. Then, when the interface changes, the cht-form build should break if its data model is not updated with the new interface changes. I do kind of like this idea! 🤔
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Okay, I have updated EnektoFormContext
so that webapp and cht-form have separate implementations. This seems to work out well since tsc
will require them to be in-sync. Additionally, each implementation can be extended with properties/methods needed that are specific to webapp/cht-form. Let me know what you think!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If something forces the two types to be synced, I'm happy!
webapp/web-components/cht-form/tests/karma/stubs/cht-datasource.service.spec.ts
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Approving to unblock, since all questions have been addressed!
Thanks @jkuester !
Co-authored-by: Diana Barsan <[email protected]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sweeeeeet!
@@ -351,3 +352,40 @@ export class FormService { | |||
} | |||
} | |||
|
|||
export class WebappEnketoFormContext implements EnketoFormContext { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice moving this to forms service instead of enketo service! I was very tempted to do the same thing in a PR of mine, but it was too much an unrelated change. Thanks for making this switch!
// @ts-expect-error it does exist | ||
component.formModel = undefined; | ||
// @ts-expect-error it does exist | ||
component.contactSummary = undefined; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If something forces the two types to be synced, I'm happy!
Description
Closes #9844
Code review checklist
can_view_old_navigation
permission to see the old design. Test it has appropriate design for RTL languages.Compose URLs
If Build CI hasn't passed, these may 404:
License
The software is provided under AGPL-3.0. Contributions to this project are accepted under the same license.