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

Fix property eq expressions for entities #6351

Merged
merged 8 commits into from
Aug 22, 2024

Conversation

seadowg
Copy link
Member

@seadowg seadowg commented Aug 16, 2024

Closes #6304
Closes #6314
Blocked by #6318

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

There's two key fixes here:

  1. Making sure new properties are added to existing local entities (like they would be in the CSV)
  2. Making sure that thing = '' expressions behave as they would in JavaRosa when they end up going through getAllByProperty

It's interestingly up for debate whether the behaviour for 2 is correct or not though in the case where thing is not a node that exists on the filtered nodes: JavaRosa would return all nodes, whereas Enketo would return none. It's likely that Enketo's behaviour is more correct here, but after discussing with @lognaturel we've decided to keep entities and JavaRosa evaluation consistent and then make all the ODK tools consistent later once web forms is dealing with secondary instances.

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?

Firstly, because this PR makes changes to the entities DB but doesn't perform any migrations (as we haven't released the DB yet), the app should be uninstalled/reinstalled or data should be cleared before testing this.

Other than that, checking the bug is fixed and that entity forms (especially those with filtered selects) behave as expected is the most important thing.

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
Copy link
Member Author

seadowg commented Aug 20, 2024

@dbemke could you check for me if this also fixes #6336?

@dbemke
Copy link

dbemke commented Aug 20, 2024

#6336 crashes the app

@seadowg seadowg force-pushed the entity-offline-fix branch 3 times, most recently from 150e726 to 9b05aee Compare August 20, 2024 14:28
@seadowg seadowg removed the blocked label Aug 20, 2024
@seadowg seadowg force-pushed the entity-offline-fix branch from 9b05aee to 51d3999 Compare August 22, 2024 08:19
@seadowg seadowg marked this pull request as ready for review August 22, 2024 08:36
@seadowg seadowg requested a review from grzesiek2010 August 22, 2024 08:36
db/build.gradle.kts Outdated Show resolved Hide resolved
).foldAndClose {
mapCursorRowToEntity(list, it, it.getInt(ROW_ID))
}
} else if (value == "") {
Copy link
Member

Choose a reason for hiding this comment

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

Can properties have blank values like " "? Is it sanitized somewhere? I'm wondering if here we should use value.isBlank() instead.

Copy link
Member Author

Choose a reason for hiding this comment

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

This case only occurs if the property doesn't actually exist for the entities. It's the special case described above:

It's interestingly up for debate whether the behaviour for 2 is correct or not though in the case where thing is not a node that exists on the filtered nodes: JavaRosa would return all nodes, whereas Enketo would return none.

Copy link
Member

Choose a reason for hiding this comment

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

This case only occurs if the property doesn't actually exist for the entities.

I see but isn't it somehow possible that it doesn't exist and the value here is not "" but " "?

Copy link
Member Author

@seadowg seadowg Aug 22, 2024

Choose a reason for hiding this comment

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

I see but isn't it somehow possible that it doesn't exist and the value here is not "" but " "?

Yes, but in that case we'd return an empty list because we'd be searching for nodes with a non-existent property equal to "something" rather than "nothing" and JavaRosa treats non-existent nodes as having the value "". Basically thing = ' ' is the same as thing = 'blah' if thing doesn't exist.

@seadowg seadowg requested a review from grzesiek2010 August 22, 2024 14:17
@grzesiek2010 grzesiek2010 merged commit aa10b3b into getodk:master Aug 22, 2024
6 checks passed
@seadowg seadowg deleted the entity-offline-fix branch August 23, 2024 07:14
@WKobus
Copy link

WKobus commented Aug 26, 2024

Tested with Success

Verified on device with Android 14

Verified cases:

@dbemke
Copy link

dbemke commented Aug 26, 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
4 participants