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

Use raw values, not display text, for Entity property values #6514

Merged
merged 3 commits into from
Nov 19, 2024

Conversation

grzesiek2010
Copy link
Member

@grzesiek2010 grzesiek2010 commented Nov 15, 2024

Closes #6510

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

There is no other solution to discuss. If we want to use plain, unformatted strings, we need to uncast the value and read the string.
There is one issue that needs to be addressed in javarosa I belive: https://github.com/getodk/collect/pull/6514/files#r1844165858

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?

Regardless of the data type used to store entities, the values saved to them should be plain, unformatted text, identical to what is stored in XML submissions. I think testing dates here is sufficient; we don't need to test other data types. Even if this issue affects other data types, the PR should address all cases.

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

I used:
entities_with_dates_registration.xlsx

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

No.

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

@grzesiek2010 grzesiek2010 changed the title Collect 6510 Do not format properties that are build using XPath expressions Nov 15, 2024
@grzesiek2010 grzesiek2010 changed the title Do not format properties that are build using XPath expressions Do not entity values that are build using XPath expressions Nov 15, 2024
@grzesiek2010 grzesiek2010 changed the title Do not entity values that are build using XPath expressions Do not format entity values that are build using XPath expressions Nov 15, 2024
.clickFinalize()

.startBlankForm("Entities With Dates Follow Up")
// .assertText("2024-11-15")
Copy link
Member Author

@grzesiek2010 grzesiek2010 Nov 15, 2024

Choose a reason for hiding this comment

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

There is a problem with saving dates as labels. I fixed it here: https://github.com/getodk/collect/pull/6514/files#diff-dcf4e4fc03ff048616e3d821f4dc1ff53ea090480626d6516d0ac054b35c1446R35 but the problem is that such an element is identified as DateTime data type not just Date. It seems to be a bug in javarosa so I'm going to file an issue there.

Copy link
Member Author

Choose a reason for hiding this comment

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

The issue is here: getodk/javarosa#807

@grzesiek2010 grzesiek2010 marked this pull request as ready for review November 15, 2024 16:48
@lognaturel lognaturel requested a review from seadowg November 18, 2024 15:12
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.

The solution looks good as far as I can tell, although I wasn't familiar with the uncast method.

I think we could test this at the unit level for EntityFormFinalizationProcessor and EntityFormParser instead of needing a connected test though. @grzesiek2010 could you make that change in a follow-up? That way we're not blocking this for test changes.

@grzesiek2010
Copy link
Member Author

@grzesiek2010 could you make that change in a follow-up? That way we're not blocking this for test changes.

Ok I will do that.

@seadowg seadowg merged commit bae5116 into getodk:master Nov 19, 2024
6 checks passed
@lognaturel lognaturel changed the title Do not format entity values that are build using XPath expressions Use raw values, not display text, for Entity property values Nov 19, 2024
@srujner
Copy link

srujner commented Nov 19, 2024

Verified on device with Android 14

@dbemke
Copy link

dbemke commented Nov 19, 2024

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.

Dates saved to offline Entity properties have the wrong format when used in XPath expressions
4 participants