feat: auto-answer one at a time, not into the future #3764
Conversation
jessicamcinchak
changed the title
|
Removed vultr server and associated DNS entries |
…ss/automate-next-not-future
…ss/automate-next-not-future
…ss/automate-next-not-future
…ss/automate-next-not-future
…ss/automate-next-not-future
…ss/automate-next-not-future
editor.planx.uk/src/pages/FlowEditor/lib/__tests__/advancedAutomations.test.ts
Show resolved
Hide resolved
There was a problem hiding this comment.
Great stuff here! 👏
Again, I really appreciate how much work this PR covers. Tests are clear and comprehensive which is going to be a huge help going forward. Likewise, the level of comments on the code is thorough and helpful.
Comments basically fall into three categories -
- Taking the explicitness a step further by using named callback functions or using alternative variables names in a few places
- Simplifying some nested conditional logic
- Questions around where we should be running this check, especially in the context of automating on inputs - discussed earlier on the dev call
The nested conditions in autoAnswerableOptions() is the bit I had the hardest time reading and understanding here.
| .sort( | ||
| (a, b) => | ||
| // Sort by the most to least number of dot-separated items in data.val (most granular to least) | ||
| String(b.data?.val).split(".").length - | ||
| String(a.data?.val).split(".").length, | ||
| ) |
There was a problem hiding this comment.
| .sort( | |
| (a, b) => | |
| // Sort by the most to least number of dot-separated items in data.val (most granular to least) | |
| String(b.data?.val).split(".").length - | |
| String(a.data?.val).split(".").length, | |
| ) | |
| .sort(byGranularity) |
Another small idea for a named callback function that could help!
|
|
||
| // If we have existing passport value(s) for this fn in an eligible automation format (eg not numbers or plain strings), | ||
| // then proceed through the matching option(s) or the blank option independent if other vals have been seen before | ||
| if (Array.isArray(passportValues) && passportValues.length > 0) { |
There was a problem hiding this comment.
There's a lot of nested conditionality here which is hard to follow, it might be best to reverse conditions into guard clauses below a few times.
| if (Array.isArray(passportValues) && passportValues.length > 0) { | |
| if (!Array.isArray(passportValues) || !passportValues.length) { |
There was a problem hiding this comment.
I honestly just don't have the brain capacity to re-organise this one again right now, but promise to keep this in mind and try to revisit as I draft a Notion page with a human-readable explanation of our auto-answering rules !
| if (type === TYPES.Question) { | ||
| optionsThatCanBeAutoAnswered = optionsThatCanBeAutoAnswered.slice(0, 1); | ||
| } |
There was a problem hiding this comment.
The balance between "what's a component's responsibility" and "what's a store / navigation / graph traversal responsibility" is a tricky one!
This feels like it could comfortably fall into the useEffect() of the component itself, but doesn't seem wrong here either.
Just an observation really - very much happy to keep it here for now. If we find that this list of per-component exceptions grows, that might be a sign it's best handled elsewhere. I suspect what we'll end up with though is a continuation of the pattern you have here - functions responsible for how a single component, or a group of related components, handle auto answering.
There was a problem hiding this comment.
My argument for here rather than within useEffect of the component itself is that it's nice to be able to clearly test length of returned array throughout these logic unit tests ! Eg test cases where I know that there are many matching paths, but I want to confirm that I'm only going to auto-answer the most-granular, left-most option if my node is a Question
High-level goals / summary of changes:
upcomingCardIdsis responsible for graph navigation based on answers so far only and does not check future node's data values for automation potential or queue up nodes based on optimistic automationsauto: truebreadcrumb (Filters & Questions without edges don't leave any breadcrumbs currently)Detailed changes:
upcomingCardIdsautoAnswerableOptions&autoAnswerableFlagwhich takes current nodeId & outputs a list of ids that can be automated (ids are subset of current node'sedgesaka "options" aka "answers" aka "responses")startsWithgranularity for Questions & Checklists_notsTesting:
There's lot of old classic automation demo flows like:
Will also be asking content team to review full services; we need a plan of how to migrate "content work-arounds".
Pay special attention to:
_nots(TODO!)Next steps / followup PRs:
upcomingCardIds- egpreviousCard,setCurrentCard&isFinalCardcollectedFlags&getResultDatapreview store methodsarticle4,flood)