Skip to content
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

Accept forms with entities version 2023.1 #735

Merged
merged 1 commit into from
Nov 22, 2023

Conversation

lognaturel
Copy link
Member

Related to XLSForm/pyxform#664 (comment)

This accepts forms that specify entity updates but doesn't actually do anything with the update directive. This is ok because for now there's no local concept of permanent entity storage and updates will always be processed server-side.

What has been done to verify that this works as intended?

Added tests

Why is this the best possible solution? Were any other approaches considered?

Previously there was a single supported version stored in a public constant. I changed that to a list of supported versions in a private constant. I think the constant was previously public only for use in tests and it's unlikely it was accessed by any other code so it feels ok to make it private without considering this a breaking change.

I think explicitly enumerating supported versions is the least error-prone way to go.

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?

This one feels very low risk. There's testing to ensure the previous version is still supported.

Do we need any specific form for testing your changes? If so, please attach one.

See tests.

Does this change require updates to documentation? If so, please file an issue here and include the link below.

No

@lognaturel lognaturel requested a review from seadowg November 15, 2023 23:06
@seadowg
Copy link
Member

seadowg commented Nov 20, 2023

If Collect (or any other client) is going to support v2022.1 and v2023.1, that would mean to me that we won't be able to release a version that just support "create" for entities right? My thinking we'd need to have both "create" and "update" as soon as we're doing any offline storage or we'd not properly be supporting v2023.1.

Copy link
Member

@seadowg seadowg left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@lognaturel
Copy link
Member Author

we'd need to have both "create" and "update" as soon as we're doing any offline storage or we'd not properly be supporting v2023.1

I believe we only have to make the decision for ODK Collect v2023.3.1 for now. Because it doesn't have any offline entities support, it is accurate that it supports create and update equally.

When we work on v2024.2 with offline entities support, we could choose to only support create offline. In that case we could release a new JR version that removes support for v2023.1 of the spec to reject forms that have updates defined. If we decide to do that, we could consider having the exception here provide the version from the form so Collect can show a specialized message.

That said, I think we could also say that a version of Collect with offline create but not update does support update just as much as this version here. The most strict approach would be to make offline support for different actions a versioned part of the spec. This would guarantee that all Collect users would have to upgrade (we'd have to think about what it means for web form users). I think we have options and I don't believe that we need to make a decision as part of this upcoming release. I'm not really seeing an alternative here unless we redesign versioning.

More dramatic ideas we could consider are separating out client and server versions or implementing a "capabilities"-style API where clients can say what they're able to do and the server can decide whether or not to proceed. The latter would allow a project manager to do something like toggle between requiring offline-capable clients or accepting ones without offline entities.

@seadowg
Copy link
Member

seadowg commented Nov 21, 2023

When we work on v2024.2 with offline entities support, we could choose to only support create offline. In that case we could release a new JR version that removes support for v2023.1 of the spec to reject forms that have updates defined. If we decide to do that, we could consider having the exception here provide the version from the form so Collect can show a specialized message.

Great point!

That said, I think we could also say that a version of Collect with offline create but not update does support update just as much as this version here. The most strict approach would be to make offline support for different actions a versioned part of the spec. This would guarantee that all Collect users would have to upgrade (we'd have to think about what it means for web form users). I think we have options and I don't believe that we need to make a decision as part of this upcoming release. I'm not really seeing an alternative here unless we redesign versioning.

Yeah I like this. Would that mean just releasing versions of the spec with the same literal XML spec, but "expectations" around behaviour?

More dramatic ideas we could consider are separating out client and server versions or implementing a "capabilities"-style API where clients can say what they're able to do and the server can decide whether or not to proceed. The latter would allow a project manager to do something like toggle between requiring offline-capable clients or accepting ones without offline entities.

Sounds like you've got some plans around avoiding this, so hopefully it's not a road we need to go down.

@lognaturel lognaturel merged commit 72bbbf3 into getodk:master Nov 22, 2023
3 checks passed
@lognaturel lognaturel deleted the add-entities-update-version branch November 22, 2023 16:22
lognaturel added a commit to lognaturel/javarosa that referenced this pull request Nov 28, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants