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

Add trunkVersion and branchId support #6276

Merged
merged 15 commits into from
Jul 31, 2024
Merged

Conversation

seadowg
Copy link
Member

@seadowg seadowg commented Jul 22, 2024

Closes #6245
Blocked by #6267

As well as adding support for forms to receive __trunkVersion and __branchId and keeping track of these values correctly, this also changes how we increment __version. Before, Collect would parse the baseVersion, increment it and then store the result. This meant that a form might manipulate the baseVersion in an unexpected way which would Collect would honour. After chatting with @lognaturel, we've decided that this was actually an incorrect implementation of the following line in the spec:

MUST increment the __version of its local entity representation by 1 when an update is successfully applied

Instead, we now increment our own representation of __version (Entity#version) and ignore the baseVersion in submissions.

I've also cleaned up how Collect treats forms that have both an update and a create action and added tests to define that behaviour.

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

Not a lot to discuss here! I'll put comments inline for anything that seems worth mentioning.

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 is mostly all additional, but there's a risk that changes to the way we increment version and to how we treat forms that have both an update and create action might alter how we expect Collect to behave. It'd be worth testing both of these out and checking we're happy with how everything works.

Before submitting this PR, please make sure you have:

  • added or modified tests for any new or changed behavior
  • run ./gradlew connectedAndroidTest (or ./gradlew testLab) and confirmed all checks still pass
  • added a comment above any new strings describing it for translators
  • added any new strings with date formatting to DateFormatsTest
  • verified that any code or assets from external sources are properly credited in comments and/or in the about file.
  • verified that any new UI elements use theme colors. UI Components Style guidelines

@seadowg seadowg force-pushed the trunk-version branch 2 times, most recently from aefaf8c to 70c6280 Compare July 22, 2024 19:37
@seadowg seadowg changed the title Add trunkVersion and branchId support Add trunkVersion and branchId support Jul 23, 2024
@seadowg seadowg force-pushed the trunk-version branch 2 times, most recently from afee21c to 81b419a Compare July 29, 2024 15:49
@seadowg seadowg removed the blocked label Jul 30, 2024
@seadowg seadowg marked this pull request as ready for review July 30, 2024 08:57
@seadowg seadowg requested a review from grzesiek2010 July 30, 2024 08:57
@seadowg seadowg requested a review from grzesiek2010 July 31, 2024 13:12
@grzesiek2010 grzesiek2010 merged commit 4741fec into getodk:master Jul 31, 2024
6 checks passed
@seadowg seadowg deleted the trunk-version branch July 31, 2024 15:05
@WKobus
Copy link

WKobus commented Aug 6, 2024

Tested with success

Verified on device with Android 14

Verified cases:

  • Create or update entity forms
  • Regression check on entity forms
  • Local entities disabled/enabled

@dbemke
Copy link

dbemke commented Aug 6, 2024

Tested with success

Verified on device with Android 10

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Collect should provide trunkVersion and branchId to servers
4 participants