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

Include locally created entities in follow up forms #5982

Merged
merged 41 commits into from
Mar 29, 2024

Conversation

seadowg
Copy link
Member

@seadowg seadowg commented Feb 22, 2024

Blocked by getodk/javarosa#748
Blocked by getodk/javarosa#749

Closes #5923
Closes #5924
Closes #5972

This introduces a new experimental setting ("Include local entities in forms") that will include entities created or updated locally in follow up forms. The entities will also be viewable in the entity browser ("View local entities") also available from experimental settings.

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

There's a few things to point out about the implementation here:

  • The logic for incrementing the entity "version" on an update lives in JavaRosa rather that Collect. This allows Collect to not actually care whether the entities exposed after form finalization are being created or updated and simply save (or merge) them into it's own repo.
  • The current implementation of EntitiesRepository (JsonFileEntitiesRepository) is not intended to be something that we release with. For the moment, local entities will remain an experimental (beta only) feature. We'll discuss and finalize how we actually store entities when we come to work on Allow entities to be deleted/cleared locally #6012. The idea here is to get to a place where the EntitiesRepository API is pretty much finished before making a decision on storage.
  • Entities are "injected" into forms using an ExternalDataInstanceProcessor (OfflineEntitiesExternalDataInstanceProcessor) that adds entities in EntitiesRepository to an external instance with a matching ID to a dataset name whenever the external instance is parsed. It's likely that we'll modify exactly how this works as we work on issues like Delete local entities that have been deleted/archived/unassigned from server #6011 as we'll potentially need to perform some work to determine which entities are "offline" or "online" whenever the list updated from the server.

A side note for @grzesiek2010: I did have a quick look at using Compose for the entities browser code (replacing the Navigation/Fragment/RecyclerView based implementation), but I found that "lazy lists" (like RecyclerView) aren't the simplest thing to get working and would be a bit much to add on to this already quite large PR (and I'm still kind of wary about a "Compose 2" release down the line). It's maybe something I'll have another go at if I get time. For the moment, using the "old" Fragment with the Navigation graph feels like a totally reasonable way to be building new UI (even if it does seem a bit more boilerplate heavy than Compose).

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 should only affect forms with local entities, but it would be good to check any form with an external secondary instance (both XML and CSV) to check that I haven't introduced any problems/weirdness. It'd also be good to verify that "Include local entities in forms" works as expected when enabled or disabled (it should be disabled by default).

For checking local entities themselves, I'm guessing there's already a good set of example forms that have been used to test entities on Central, but let me know if not! The linked issues describe the behaviour we expect when using forms that update or create entities (and how they work with follow up forms).

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
  • 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


val wines = repository.getEntities("wines")
assertThat(wines.size, equalTo(1))
assertThat(wines[0], equalTo(updatedWine))
Copy link
Member

Choose a reason for hiding this comment

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

You can simplify this and use:
assertThat(repository.getEntities("wines"), contains(updatedWine))
then there is no need to assert the size of the list.

Copy link
Member

Choose a reason for hiding this comment

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

And the test title says save() does not update but it does the update?

Copy link
Member Author

Choose a reason for hiding this comment

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

It's "save() does not update existing entity with matching id but not dataset". We're checking that whisky doesn't get updated even though it has a matching id to wine.

Copy link
Member

Choose a reason for hiding this comment

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

Ok so this part of the test was confusing:

        val wines = repository.getEntities("wines")
        assertThat(wines, contains(updatedWine))

I think this should be a separate test like: save() updates existing entity with matching id and dataset but I think it's already covered by save() updates existing entity with matching id and this is enough.

Copy link
Member Author

Choose a reason for hiding this comment

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

but I think it's already covered by save() updates existing entity with matching id and this is enough.

That test doesn't check that we don't update entities with matching ids in different datasets though.

@seadowg
Copy link
Member Author

seadowg commented Mar 27, 2024

@grzesiek2010 I'm running into a bunch of random Espresso test failures that look like flakes here. I've made this a draft, but go ahead and have another look while I work out what's going on.

@seadowg
Copy link
Member Author

seadowg commented Mar 29, 2024

@grzesiek2010 it looks like the flakes are a master problem so this is no longer a draft.

@seadowg seadowg marked this pull request as ready for review March 29, 2024 11:54

val wines = repository.getEntities("wines")
assertThat(wines.size, equalTo(1))
assertThat(wines[0], equalTo(updatedWine))
Copy link
Member

Choose a reason for hiding this comment

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

Ok so this part of the test was confusing:

        val wines = repository.getEntities("wines")
        assertThat(wines, contains(updatedWine))

I think this should be a separate test like: save() updates existing entity with matching id and dataset but I think it's already covered by save() updates existing entity with matching id and this is enough.

@grzesiek2010
Copy link
Member

@lognaturel do you want to take a look as well? If not we can merge.

@lognaturel
Copy link
Member

Happy to merge! 🎉🎉🎉 I’ve taken a little look here and there. We’ll be layering on more and not exposing this for release builds right away so I don’t feel like we need a super careful review now.

@grzesiek2010 grzesiek2010 merged commit 4429ad9 into getodk:master Mar 29, 2024
6 checks passed
@seadowg seadowg deleted the entity-create branch April 2, 2024 09:57
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.

Entity updates from the server Local entity updates Local entity create
3 participants