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 media file type to determine entity lists #6249

Merged
merged 9 commits into from
Jul 11, 2024

Conversation

seadowg
Copy link
Member

@seadowg seadowg commented Jul 3, 2024

Closes #6161
Closes #6229

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

Not a lot to discuss here! I just updated how we parse the form manifest.

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 does remove the ability to manually add entity lists manually to make things less confusing, but it will mean that this (and any future entity features/changes) will require testing against a version of Central that supports the type attribute. This change hasn't been released yet, but should be available at https://staging.getodk.cloud.

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 changed the title Use mediaFile type to determine entity lists Use mediaFile type to determine entity lists Jul 3, 2024
@seadowg seadowg changed the title Use mediaFile type to determine entity lists Use media file type to determine entity lists Jul 3, 2024
@seadowg seadowg marked this pull request as ready for review July 4, 2024 08:55
@seadowg seadowg requested a review from grzesiek2010 July 4, 2024 08:55
@@ -71,4 +71,29 @@ class OpenRosaResponseParserImplTest {
val formList = OpenRosaResponseParserImpl().parseManifest(Document())
assertThat(formList, equalTo(null))
}

@Test
fun `parseManifest() when media file has type entityList returns isEntityList as true`() {
Copy link
Member

Choose a reason for hiding this comment

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

We should have a test to check if it returns false otherwise.

@@ -941,8 +941,7 @@
<!--Text for button that deletes all entities for the project-->
<string name="clear_entities">Poista kaikki</string>
<!--Text for button that adds a new entity list-->
Copy link
Member

Choose a reason for hiding this comment

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

You should also get rid of the explanation text above.

Copy link
Member Author

Choose a reason for hiding this comment

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

I've removed it for the string in values, but won't touch it for the translated files as that should happen automatically for when new translations are added.

@grzesiek2010 grzesiek2010 merged commit 9094205 into getodk:master Jul 11, 2024
6 checks passed
@seadowg seadowg deleted the entity-lists branch July 11, 2024 10:15
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
2 participants