migrate: find+replace flag prop with flags across all mock flow data
#611
Conversation
| @@ -13,58 +12,63 @@ import { | |||
| resultOverrides, | |||
| } from "../types"; | |||
|
|
|||
| // Functions in this file need to stay in sync with planx-new/editor.planx.uk/src/pages/FlowEditor/lib/store/preview.ts !! | |||
| // TODO eventually replace/combine ?? | |||
There was a problem hiding this comment.
This is important and annoying ! I'd missed this when updating result store functions in planx-new around the time of automation changes ~2 months ago - which means the CSV & overview HTML have only been including result data based on the legacy "flag" format of old questions/checklist options until now.
This change brings the functions back in sync, but ultimately we still need to revisit this so that we don't need to maintain these in x2 places to begin with. (Can we store result on session data & eliminate need to "calculate" it in planx-core at all?)
There was a problem hiding this comment.
Agree that this would be handly, took me a bit to work out the need for the two.
Are they both doing the same thing but using the return in different places. We recalculate the result for payload generation in planx-core while we use the resultData in the Result component in planx-new? ?
There was a problem hiding this comment.
Correct - result data is a computation based on breadcrumbs & flow data like the passport, but unlike the passport which is saved on lowcal_sessions.data, result is computed on the fly in planx-new and therefore needs to be re-computed when referenced in planx-core rather than simply fetched from the session data like we can with a passport
| expect(path).toHaveLength(58); | ||
| expect(path[57].id).toBe("_root"); | ||
| expect(path).toHaveLength(62); | ||
| expect(path[61].id).toBe("_root"); // _root is always last |
There was a problem hiding this comment.
The mock flows changed here so the length of node paths has legitimately changed too
| export type NodeData = Record<string, Value> & NodeTags; | ||
| export type NodeFlags = { flags?: string[] }; | ||
|
|
||
| export type NodeData = Record<string, Value> & NodeTags & NodeFlags; |
There was a problem hiding this comment.
Duped again by Value type ! Explicitly defining NodeFlags is a "workaround" that lets us do stuff like node.data.flags.forEach without throwing a Value TypeError
There was a problem hiding this comment.
Ran through the changes to check for any typos or overlooked changes but couldn't find any. Skimming the mocks seemed unwieldy so I kept that to a ctrl+f for possible typos or missed ones, but I imagine these to be pretty spot on.
These changes are scheduled before switching the flag values in the flatFlags array. Will we have to change the mocks again at this point to incorporate the new values?
I think it's the right approach to go step by step with the changes, I just want to check my understanding of upcoming changes on this.
All looks good to me though
Per data migration plan here: https://docs.google.com/spreadsheets/d/1Vtxp5BLweDPDooQoNhgOCYjCIBPRYIcOuyArGJRqOkI/edit?gid=0#gid=0
Corresponds to planx-new PR here: theopensystemslab/planx-new#4098
Changes:
flagtoflags(plus making value an array) across these, I've simply copied new versions from the entire flows from staging post-database migration scripts this morning. This means they now have the correct structure, but general content changes as wellgetResultDatamethods are updated and synced to planx-new preview store (see comment below)