-
Notifications
You must be signed in to change notification settings - Fork 19
Add support for setvalue action and odk-instance-first-load, odk-new-repeat, xforms-value-changed events #563
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: main
Are you sure you want to change the base?
Conversation
🦋 Changeset detectedLatest commit: e76c3ef The changes in this PR will be included in the next version bump. This PR includes changesets to release 2 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
| describe('[odk-instance-load] instance load event', () => { | ||
| it.fails('fires event on first load', async () => { | ||
| describe('odk-instance-load event', () => { | ||
| // ported from: https://github.com/getodk/javarosa/blob/2dd8e15e9f3110a86f8d7d851efc98627ae5692e/src/test/java/org/javarosa/core/model/actions/InstanceLoadEventsTest.java#L28 |
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'm trying out this method of recording where the test was ported from instead of having cryptic punctuation characters. Let me know if this works for you and I'll keep rolling it out.
| }); | ||
| }); | ||
| }); | ||
| expect(scenario.attributeOf('/data/element', 'attr')).toStartWith('uuid:'); |
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 added this attributeOf function which makes the implementation much easier. Let me know if I'm just being lazy! The previous approach split on the @ symbol but because Attributes are a special kind of Node the typing had to be unioned which ended up more messy than this IMO.
|
|
||
| readonly contextReference = (): string => { | ||
| return '@' + this.definition.qualifiedName.getPrefixedName(); | ||
| return this.owner.contextReference() + '/@' + this.definition.qualifiedName.getPrefixedName(); |
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 testing exposed this bug from the attribute implementation.
| } | ||
| } | ||
| if (action.events.includes(SET_ACTION_EVENTS.xformsValueChanged)) { | ||
| createValueChangedCalculation(context, setValue, action); |
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.
This is the one that isn't like the others... xforms-value-changed events are defined on the element that triggers them whereas the other events are triggered by form actions.
|
|
||
| this.children = this.body.getChildElementDefinitions(form, this, element, childElements); | ||
| this.reference = parseNodesetReference(parent, element, 'ref'); | ||
| this.children = this.body.getChildElementDefinitions(form, this, element, childElements); |
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.
Order is important here!
Closes #303
I have verified this PR works in these browsers (latest versions):
What else has been done to verify that this works as intended?
Enabled many scenario tests
Why is this the best possible solution? Were any other approaches considered?
How does this change affect users? Describe intentional changes to behavior and behavior that could have accidentally been affected by code changes. In other words, what are the regression risks?
Do we need any specific form for testing your changes? If so, please attach one.
What's changed