refactor: store analytics_logs allow_list_answers from arrays of objects to object #2945
Conversation
- Align the data structure of the `allow_list_answers` to the `submission_services_summary` - Filter out null answer values from getAnswers - Return null when no allow_list_answers found
- Convert from an array of objects to a single object - Currently we have no records in prod where the [allow_list_answers is longer than 1](https://metabase.editor.planx.uk/question/354-number-of-analytics-logs-with-allow-list-answers-greater-than-1) so we can use this to convert - Update the `analytics_summary` db view to handle the change to the data structure
Mike-Heneghan
force-pushed
the
mh/align-allow-list-answers-data-structure
branch
from
5d2cec6 to
48cb555
Compare
- Assumption that only one passport variable was set at a time and that it was defined by the flow was proven wrong - Therefore node_fn was not accurate and the tracking of it unnecessary as the associated passport key is stored in the allow_list_answers object
|
🤖 Hasura Change Summary compared a subset of table metadata including permissions: Updated Tables (1)
|
|
Removed vultr server and associated DNS entries |
| if (!nodeFn || !ALLOW_LIST.includes(nodeFn)) return []; | ||
| const nodeFn: string | undefined = data?.fn || data?.val; | ||
|
|
||
| if (!nodeFn || !ALLOW_LIST.includes(nodeFn as AllowListKey)) return; |
There was a problem hiding this comment.
| if (!nodeFn || !ALLOW_LIST.includes(nodeFn as AllowListKey)) return; | |
| const isAllowListKey = (nodeFn: string): nodeFn is AllowListKey => { | |
| return (ALLOW_LIST as readonly string[]).includes(nodeFn); | |
| }; | |
| if (!nodeFn || !isAllowListKey(nodeFn)) return; |
If we add a type guard here instead of just .includes() we'll get type narrowing and can drop the casting to AllowListKey on line 526.
There was a problem hiding this comment.
This is a much neater solution 🙌
I wasn't happy with the type casting but I couldn't figure out an improvement, I'll keep this pattern in mind going forward 😊
Should be sorted here: 9bccce1
| const filteredEntries = Object.entries(dataSetByNode) | ||
| .filter( | ||
| ([key, value]) => | ||
| ALLOW_LIST.includes(key as AllowListKey) && Boolean(value), |
There was a problem hiding this comment.
See previous comment on using a type guard (is function) - we can probably use one here to avoid casting twice.
There was a problem hiding this comment.
Should also be sorted by: 9bccce1
- As per: #2945 (comment) - As per: #2945 (comment) Co-authored-by: Dafydd Llŷr Pearson <[email protected]>
What
allow_list_answersas an objectgetAnswersto filter outnullanswerValuesto avoid storingnullvalues: https://metabase.editor.planx.uk/question/353-null-allow-list-valuesallow_list_answers> 1 none of our ALLOW_LIST actually gets set by the same node: https://metabase.editor.planx.uk/question/354-number-of-analytics-logs-with-allow-list-answers-greater-than-1analytics_summaryto handle the change in data structure ofallow_list_answersnode_fnfrom trackingWhy
allow_list_answersas an array is harder to work withlowcal_sessionsnullvalues from some edge case nodesFollow Up
nullbeing stored inallow_list_answersit doesn't cleanup historical data which will be done in a follow up PR.analytics_logsandanalytics_summaryon.devit looks like the data was updated as expected 🥳