-
Notifications
You must be signed in to change notification settings - Fork 31
Add Case Management to Vellum #1179
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
base: master
Are you sure you want to change the base?
Conversation
|
Aware that the changes here cause existing tests to fail. I modified the default test options to use case management, which means that any XML that gets generated will include a case management block in its XML. I'm still deciding how best to handle this. |
687fc82 to
ce30095
Compare
src/caseManagement.js
Outdated
| questions.splice(prevIndex, 1); | ||
| if (questions.length === 0) { | ||
| // delete the old case property questions | ||
| delete this.form.mappings[prev]; |
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 !!prev always true at this point? Looks like we test that up on the first line of this function, but it's not clear that that is true here. If it is not always true, what happens here when it is false?
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.
I might be missing the question? prev never changes in this function. questions is initialized to a list of the mappings for the previous case property. We then search those mappings for the first occurence of the questionPath. Assuming the old case property was mapped to the question, we then remove the question from these mappings, and then delete the mappings for the old case property entirely if there are no mappings remaining for this case property, after the removal.
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.
The question was not clear. Let me try again.
The ternary expression on the first line of this function implies that !!prev may be either true or false. On this line prev is being used as a key to lookup an object attribute. Is this line safe when !!prev is false?
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.
Got it. This is safe because this code will never be executed if !!prev is false. When prev is false, questions is initialized to an empty array. We then search that empty array for questionPath in our parameters. Because its an empty array, this always returns -1, and so the prevIndex !== -1 will always be false, and we'll then never execute the code that depends on prev being true
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.
Basically, the condition is if (prev && (prevIndex = this.form.caseMappings[prev].findIndex(...))) but that seems even more confusing. Curious if you see a more intuitive way to phrase this
tests/caseManagement.js
Outdated
| // this question should be mapped to both 'one' and 'two', but since 'one' is first, that is expected | ||
| assert.equal(selectedOption.val(), "one"); | ||
|
|
||
| chai.expect(messages.text()).to.include( |
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.
Why use assert-style in some places and BDD-style (chai.expect) in others?
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.
Yeah, this would ideally be consistent. I think I was using examples from our other tests, where some used one style and others used another (one example is hidden value tests, which use both. I'll take a look at the effort required to stick to one vs the other. Is your preference to stick to assert-style? That seems to be more common.
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 your preference to stick to
assert-style?
Yes.
tests/caseManagement.js
Outdated
| chai.expect(mappedQuestions.attr("question_path")).to.equal("/data/question1"); | ||
| }); | ||
|
|
||
| it("should preserve additional question attributes", function () { |
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.
What are additional question attributes used for?
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.
The only one I'm aware of, currently, is update_mode. Right now, the extra attributes have absolutely no impact on Vellum, and aren't displayed outside of looking at the XML. There could be an argument made to hide these from Vellum and instead resolve them during a diff resolution process on the server, but I'm a bit worried about the edge cases that could create. I didn't think it would be a big deal if the user used the Edit XML modal to add custom attributes, as they would just be ignored on the server end. But I should check whether submitting an invalid update_mode could break a form
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.
I am in favor of hiding them from Vellum if they are not used here. However, I don't think I know enough about the possible edge cases to understand the argument against. Can you say more about that?
| <question1 /> | ||
| </data> | ||
| </instance> | ||
| <bind vellum:nodeset="#form/question1" nodeset="/data/question1" type="xsd:string" /> |
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.
Rather than including the new and problematic case_mapping element, what do you think of adding vellum:case_properties="property_name" to the bind element?
- If there is no mapping, the attribute is not present.
- If there are multiple case properties for a single question, the value is a space-delimited sequence of property names (case property names cannot contain spaces).
- If multiple questions map to the same case property, each of their respective bind elements would have a case properties attribute. I think this is represented as a conflict, and cannot be edited in Vellum?
- Extra data could be added if it is needed, although the need is currently unclear so I'm refraining from specifying how that might be included at this time.
It would be easier to update the XForm spec because we would be extending an existing construct rather than inventing an entirely new part of the form.
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.
Given that a future PR doesn't marshal the mappings inside the XML (HQ sends them as the initial properties, and receives them as a diff), I'm curious if you still feel this is a concern? The Edit XML modal will still display them, but that's mainly for copy/paste convenience. One argument I have against this is the difficulty in accepting data without this bind property. In the future PR, if the user pastes in XML without case_mappings, we try our best to preserve any existing mappings, rather than remove all mappings. With question-specific bindings, that scenario becomes more difficult. If I want to support "preserve" mode, then I have to treat the absence of the bind as preservation, which means that any question would need to explicitly have vellum:case_properties="", which feels verbose.
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.
Given that a future PR doesn't marshal the mappings inside the XML (HQ sends them as the initial properties, and receives them as a diff), I'm curious if you still feel this is a concern?
I still think they would be better as bind attributes rather than introduce a new top-level case_mapping element.
If I want to support "preserve" mode, then I have to treat the absence of the bind as preservation, which means that any question would need to explicitly have
vellum:case_properties="", which feels verbose.
Agreed, adding vellum:case_properties="" to every bind element with no mapping would be verbose and ugly.
Another way to handle that would be to add a vellum:case_mappings="true" attribute to the parent <model> element. If a form pasted into Vellum is missing the vellum:case_mappings="true" attribute then invoke "preserve" mode.
src/caseManagement.js
Outdated
| // multiple questions no longer are assigned to this case property, | ||
| // so we can remove the conflict message | ||
| const mugWithConflict = this.form.getMugByPath(questions[0].question_path); | ||
| mugWithConflict.dropMessage('case_property', CONFLICT_MSG_KEY); |
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.
Presumably there were two mugs that had conflict messages. Does this remove the message from both? If not, will there be a lingering/incorrect conflict message on another mug?
The caseManagement plugin is now active for all tests (due to caseManagement.properties being defined in tests/options.js). This plugin adds a <case_mappings/> element via contributeToAdditionalXML() after the writer completes. When the ignoreButRetain plugin is also active, it must re-parse the entire XML document (including the newly added case_mappings element) in order to re-insert ignored nodes at the correct positions. This re-parsing causes formatting changes: 1. <h:html> → <h:xdoc> (parseXML hack to force jQuery XML mode) 2. Whitespace/indentation removed (browser XMLSerializer normalization) 3. Spaces before "/>" removed (browser serialization style) 4. <case_mappings/> element now present in output
* make function a const * Add missing case_mappings tag * Adjust test fixture attribute ordering
3e2b836 to
edce5e9
Compare
Product Description
Technical Summary
Opening this as a draft for now. This adds the a case management section to Vellum that gives users a second path to store their questions as case properties. It is intended to be an easy-accessible, more limited form of the Case Management screen. This is still a WIP.
Feature Flag
Safety Assurance
Safety story
Automated test coverage
QA Plan
This will go through QA.
Rollback instructions
Labels & Review