PT-453 | Add cultural route support to enrolment form#495
Conversation
Fix "yarn typecheck":
```
$ yarn typecheck
yarn run v1.22.22
$ tsc --project ./tsconfig.json --noEmit
src/domain/headless-cms/components/__tests__/CmsPage.test.tsx:113:11 -
error TS2322: Type '"primary"' is not assignable to type
'ButtonVariant.Primary | ButtonVariant.Secondary | ButtonVariant.Success
| ButtonVariant.Danger | ButtonVariant.Clear | undefined'.
113 loadMoreButtonVariant: 'primary',
~~~~~~~~~~~~~~~~~~~~~
src/domain/headless-cms/components/__tests__/CmsPage.test.tsx:114:11 -
error TS2322: Type '"coat"' is not assignable to type 'ButtonPresetTheme
| ButtonTheme | undefined'.
114 loadMoreButtonTheme: 'coat',
~~~~~~~~~~~~~~~~~~~
src/playwright/tests/search.spec.ts:55:41 - error TS2345: Argument of
type '"Detaljerad sökning"' is not assignable to parameter of type
'SearchButton'.
55 await frontPageSv.clickSearchButton('Detaljerad sökning');
~~~~~~~~~~~~~~~~~~~~
src/playwright/tests/search.spec.ts:71:41 - error TS2345: Argument of
type '"Advanced search"' is not assignable to parameter of type
'SearchButton'.
71 await frontPageEn.clickSearchButton('Advanced search');
~~~~~~~~~~~~~~~~~
```
Refs: PT-453
karisal-anders
changed the title
|
KULTUS-UI branch is deployed to platta: https://kultus-ui-pr495.dev.hel.ninja 🚀🚀🚀 |
![sentry[bot]](https://avatars.githubusercontent.com/in/12637?s=60&v=4){kind=small}
also: - with the request of the product owner don't restore any previous answer to "Is part of cultural route" question from local storage persisted Formik form's data, but instead always reset the question to its initial value i.e. no data given at all - update generated GraphQL types using backend running City-of-Helsinki/palvelutarjotin#509 i.e. the backend support for the `is_part_of_cultural_route` field in enrolments and queue enrolments - add tests - update playwright tests Refs: PT-453
karisal-anders
force-pushed
the
PT-453-add-culture-route-support
branch
from
f08c5ab to
f37539f
Compare
|
|
KULTUS-UI branch is deployed to platta: https://kultus-ui-pr495.dev.hel.ninja 🚀🚀🚀 |
nikomakela
left a comment
nikomakela
left a comment
There was a problem hiding this comment.
The new isPartOfCulturalRoute field in EnrolmentForm is now a bit hard to understand, which makes it also hard to maintain. Because of good unit tests, I think it's. still approvable.
| <FormGroup> | ||
| <IsPartOfCulturalRouteField formikFieldName="isPartOfCulturalRoute" /> | ||
| </FormGroup> |
There was a problem hiding this comment.
I don't think it changes the result or analysis any how, but now I could understand this is "No / I don't know what cultural route is / I don't want to give an answer".
I would have personally used just a checkbox. If I know what a cultural route is, I would check it. I'm enrolling and I need to check that all the information that I have given with form is valid. If we would really trust in users and in the given input, this would be easy boolean analysis and way easier to implement and even cleaner UI and UX. Now we are doing much work only to guide the report analysis, because the input would still be the same -- This implementation still does not answer to report analysis question whether or not the user has seen this question.
There was a problem hiding this comment.
I talked with the product owner about their needs and they wanted this so that people always choose a value, that they need to make a choice. So, making the user always choose a value for this is intentional and as per product owner's wishes.
| "hasEmailNotification": true, | ||
| "hasSmsNotification": true, | ||
| "isMandatoryAdditionalInformationRequired": false, | ||
| "isPartOfCulturalRoute": "YES", |
There was a problem hiding this comment.
so can it now be "", " ", true, false, "YES" and "NO_OR_UNKNOWN"? But it's still converted to true / false when sent to graphql endpoint?
Then in backend, at least in reports interface, it's again converted back to string from boolean as in City-of-Helsinki/palvelutarjotin#509?
There was a problem hiding this comment.
In the enrolment form the values are "" (empty string) or either of the IsPartOfCulturalRoute enum's values i.e. IsPartOfCulturalRoute.NO_OR_UNKNOWN (i.e. "NO_OR_UNKNOWN" as a string value) or IsPartOfCulturalRoute.YES (i.e. "YES" as a string value).
The tests for validIsPartOfCulturalRouteToBoolean test that the enum values above pass and are converted to boolean values, and others like e.g. any of ['', ' ', 'yes', 'NO', 'unanswered', null, undefined, true, 0, 1] raise an exception.
In the backend the value has slightly different meaning than in the frontend here, as in the backend there's also the possibility of "Unanswered" for old enrolments and queue enrolments made before this functionality has been deployed to production. On user input "Unanswered" is not allowed, but in the backend when converting EnrolmentBase.is_part_of_cultural_route boolean values the False value has "Unanswered", "No" and "I don't know" all in it, and conveying the reality of the information is IMHO good to have in reports so people interpreting the reports don't make false assumptions based on incorrectly presented data (If the "Unanswered" and "No / I don't know" cases saved as False would need to be separated it could still be done and that information conveyed by splitting the False values according to EnrolmentBase.enrolment_time to before deployment of this functionality and after).
3 participants
Description ✨
Needs backend support from City-of-Helsinki/palvelutarjotin#509
NOTE:
Add cultural route support to enrolment form
Also:
answer to "Is part of cultural route" question from local storage
persisted Formik form's data, but instead always reset the question
to its initial value i.e. no data given at all
PT-453 | Add backend support for is_part_of_cultural_route boolean field to enrolments, queue enrolments and their reports palvelutarjotin#509 i.e. the backend support for the
is_part_of_cultural_routefield in enrolments and queue enrolmentsRelated 🤝
Testing ⚗️
Automated tests ⚙️️
Manual testing 👷♂️
Screenshots 📸
See the screenshots from City-of-Helsinki/palvelutarjotin#509 as this PR was used with the backend support in that PR.
Additional notes 🗒️