-
Notifications
You must be signed in to change notification settings - Fork 19
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
Refactor prompt selectors #835
Conversation
The implementation looks correct based on the behavior described in the comments. However, with this behavior, a node could be excluded from both collections. Is that the intent? Example in the dev protocol:
Observed: Anita disappears when dropped. It's been so long since the prompt check worked that this may be what was originally intended, but I'm wondering if |
The sass-lint warning causing the CI failure is because NC-UI uses a slightly different sass-lint config. Since the UI repo's CI now fails if its linting fails, should we just remove UI from linting here (a la Server)? |
Can we update the interface tests to include this specification? RE @bryfox's point if the selector ends up being correct, maybe we ought to make a specific action for adding and removing nodes from prompts to ensure it is handled correctly there. |
Really good catch @bryfox. I initially went to resolve this by ORing the check for additional attributes and prompt ID, which resolves the issue you describe but creates an additional one: If additional attributes is empty, the Because of this, I think we should change these selectors to only compare promptId, but make promptId an array of IDs that this prompt has been nominated on. This causes us to lose the built-in ability to tell which prompt a node was nominated on, but this can be compensated for using additional attributes. An additional benefit of this approach is that it will allow the user to remove the node from the prompt it had initially been nominated on without deleting it, providing it has also been nominated elsewhere. Any thoughts/objections? |
I like the clarity of that approach. I think the interface implementations will need review; notably, the NG List behavior with Finally, the behavior of external data (which may have already contained the additionalAttributes) changes with this, but I think in a way that is more expected (to me). |
Am I right in thinking that the promptId array is necessary because a protocol may have overlapping questions that toggle the same attribute? What happens when a nodes which is on both prompts (which share an attribute) is removed from one of the prompts? Does it loose the attribute, but remain on the other prompt? |
I have a couple of opinions on the code, not specifically related to these changes, but they relate to a lot of what has changed:
Then we have a better model of what we want to happen, without any risk of the two tests diverging. |
}); | ||
} | ||
const nodeWithModelandAttributeData = (modelData, attributeData, defaultAttributeData = {}) => ({ | ||
...omit(modelData, 'promptId'), |
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.
Is this redundant?
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.
nm, ignore that
Yes it does, and in this case that would be by design, I think. At least it is "temporally consistent". |
I agree, but I think (as much as this is obviously bad) we should put that work off. It isn't part of MVP, whereas the fix to the prompt behaviour that spurred this refactor is. |
Fixes makeNetworkNodesForOtherPrompts does not work as expected #833 by storing promptID on nodes as an array, and having
makeNetworkNodesForPrompt
andmakeNetworkNodesForOtherPrompts
reference this, rather than node attribute data.Fixes Initialise all node attributes on node creation #710 by creating default node values based on the variable registry for a given node type.
Removes
ADD_NODES
and replaces withADD_NODE
, which has a correct separation of node model and attribute data. New format isaddNode(modelData, attributeData)
.modelData
can be sourced frommakeGetPromptNodeModelData()
, andattributeData
can come frommakeGetAdditionalAttributes()
.Adds a
BATCH_UPDATE_NODES
action for use on the roster name generator interfaces. Accepts a node list, and static model properties and node attributes to be merged with each member of the node list.Modified
UPDATE_NODE
to correctly handle merging prompt ID data. The new format isupdateNode(nodeId, newModelData, newAttributeData)
, which avoids the old approach of passing the entire existing node in, in favour of just passing in the uid. There is a limitation of this design not addressed here: updating is additive. This can be worked around by merging new node mode/attribute data with the default values (as with addNode).Adds a
REMOVE_NODE_FROM_PROMPT
action, which allows for a single prompt ID to be removed from a node'spromptIDs
model property, whilst also removing any prompt specific additional attributes.