-
Notifications
You must be signed in to change notification settings - Fork 2
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
chore: bump planx-core (multiple schemas support) #4206
base: main
Are you sure you want to change the base?
Conversation
…ss/bump-planx-core-3b39b02
…ss/bump-planx-core-3b39b02
} else { | ||
nodeIds?.map(([nodeId, _nodeData]) => nodeId); | ||
return nodeIds?.map(([nodeId, _nodeData]) => nodeId)?.length; |
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.
These changes are unrelated to the most recent planx-core change, but came across them when investigating why the validate.test.ts
tests were still passing without a mock in this module - spotted a data field issue & additionally added test coverage ! (Data field issue does not impact current live usage of these helper functions, but likely would have cropped up next time we try to use elsewhere!)
.fn() | ||
.mockImplementation(() => ["ldc", "ldc.p", "ldc.e", "pp", "pa"]), | ||
}; | ||
}); |
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.
These mocks resolve questions raised earlier here: https://opensystemslab.slack.com/archives/C01E3AC0C03/p1737996349338569
When testing the route via supertest
, then getValidSchemaValues
and planx-core will be called "for real" and not require a mock (eg see modules/flows/validate/validate.test.ts
>> describe("ODP Schema file type validation on diff"
), but when simply unit testing like here we need to mock & include partial, realistic mockImplementation
return values.
Boolean(props.type !== TextInputType.Short && characterCountLimit); | ||
const displayCharacterCount = Boolean( | ||
props.type !== TextInputType.Short && characterCountLimit, | ||
); |
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.
These are linting changes only picked up below here after a rebase to main
- sorry for extra noise !
…ss/bump-planx-core-3b39b02
Relies on theopensystemslab/planx-core#629
Testing:
I've pre-loaded some submitted session data from staging onto this pizza so that we can easily review sample payloads using the following admin endpoints:
preApp
https://api.4206.planx.pizza/admin/session/6ece5b06-d83d-4258-a184-d77b7350c99f/digital-planning-applicationmetadata.schema
points topreApplication.json
, anddata.application
has preApp-unique fields like "harmful" & "sensitive"application
payload - now it's specifiically apreApp
payload which is the key change here!ldc.p
https://api.4206.planx.pizza/admin/session/3fcc7f52-9409-45f7-8ea2-b6b0c1bf2ffd/digital-planning-applicationmetadata.schema
points toapplication.json
discretionary (breach)
https://api.4206.planx.pizza/admin/session/9bb50c1f-1945-4e17-9867-ccadec9c6a91/digital-planning-application?skipValidation=trueskipValidation=true
because not statutory, confirm thatmetadata.schema
points toapplication.json
Other schema-dependent editor features to review on the pizza: