Store shared entities for follow up forms #6108

seadowg · 2024-04-24T13:40:33Z

Closes #6029
~~Blocked by getodk/javarosa#756~~

The big change here is that we now combine the server's entity list with locally created entities when downloading a form (or update). The EntitiesRepository is then used to create the secondary instance used in follow up forms (and the actual CSV is ignored).

As the issue mentions, the big caveat here is that you have to manually create the entity list via the entity browser (in experimental settings) to get anything working. In the future, this will likely not have to happen as which media files are entity lists will hopefully be flagged to us by the server.

Something else to point out here is that all entities (including the ones from the server) will be visible in the entity browser now. I could have filtered the "online" ones out, but it felt more useful to see them all anyway.

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

The approach here was discussed in the issue.

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 users with entity forms and only in the cases where they have manually added an entity list via experimental settings (Settings > Experimental > View entity lists > Options menu > Add entity list). As the issue points out, it's pretty hard to simulate the problems solved here without using manual form management (as match exactly will download all updates at once).

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

It'd be a good idea to test entity forms including create forms, update forms and follow up forms.

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

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

Property merging is handled by EntitiesRepository

… file

grzesiek2010 · 2024-05-02T12:11:28Z

collect_app/src/androidTest/java/org/odk/collect/android/feature/formentry/EntityFormTest.kt

-            .clickRefresh()
-            .clickOnForm("One Question Entity Update")
+            .clickGetBlankForm()
+            .clickClearAll()


Why those forms are both selected here? one-question-entity-update.xml is already downloaded here so you shouldn't need to click Clear all and select one-question-entity-follow-up.xml.

I want to specifically download 'one-question-entity-follow-up.xmlso that I check that downloadingupdated-people.csv` updates the list. I guess a potentially simpler way to test this is to just download both forms at once and check they have the same list, but I'd prefer to test the extreme case here: forms being downloaded individually with different entity lists. Does that make sense?

Yes, it makes sense but my question was about something else. Once you download the first form and open the list of forms to download again the one you already have should be unselected by default. It seems like both are selected as if the first one was removed or not downloaded at all yet. You shouldn't need to click the clear button and then select that form because the form you want to download should be the only one selected.

Ah got you! I think I'd still prefer the "clear all" to make it explicit I'm selecting that form.

Ah got you!

No, I think you don't. Please run that test entityListSecondaryInstancesAreConsistentBetweenFollowUpForms and add a breakpoint in line 126 before you call clearAll(). I was asking if it's a bug that both forms are selected by default after opening the list of forms to download when one of them is already downloaded (line 119).

Sorry! That'll be the brain fog last week. I see what you mean: it's weird that both forms are selected. I'm guessing this is a problem with StubOpenRosaServer rather an actual bug, but I'll look into it.

Yeah it's because StubOpenRosaServer uses the file path as the form ID for the form list, whereas Collect doesn't trust the server and parses it out of the form itself. That means we end up in a scenario where the form gets downloaded and saved with the ID in the XML, but the server is still using the "fake form ID". I've fixed it so that StubOpenRosaServer will parse the form ID out of the XForm if it's not given an explicit form ID.

entities/src/main/java/org/odk/collect/entities/AddEntityListDialogFragment.kt

grzesiek2010 · 2024-05-02T12:57:26Z

collect_app/src/main/java/org/odk/collect/android/formentry/FormEntryUseCases.kt

+    ) {
+        formController.finalizeForm()
+        formController.getEntities().forEach { entity ->
+            if (entitiesRepository.getDatasets().contains(entity.dataset)) {


Wouldn't it be better to check this on a repository level? I mean always call save but in save verify if the dataset exists.

I'm tempted by that. I think the restriction of needing to have declared an entity list is probably temporary though. Later, you'll be able to have a registration form without any follow up forms and have entities created locally for that (that are only available via a browser). @lognaturel does that sound correct?

I don't want to block you because I will be off till the end of this week. It seems to be the last thing to fix but you can do that later.

collect_app/src/test/java/org/odk/collect/android/formmanagement/ServerFormUseCasesTest.kt

collect_app/src/main/java/org/odk/collect/android/formmanagement/ServerFormUseCases.kt

entities/src/main/java/org/odk/collect/entities/LocalEntityUseCases.kt

entities/src/test/java/org/odk/collect/entities/LocalEntitiesFileInstanceParserTest.kt

grzesiek2010 · 2024-05-02T14:22:11Z

...pp/src/androidTest/java/org/odk/collect/android/feature/entitymanagement/ViewEntitiesTest.kt

+        .around(rule)
+
+    @Test
+    fun canViewLocallyCreatedEntitiesInBrowser() {


I think we started using different names... online and offline entities are I think the clearest options but here we have local entities. The test title below is also not clear it's about online entities.

The reason I ended up using local is because we know store both the "offline" and "online" entities in one place ("local"), but can create or update them locally (rather than those creates/updates coming from the server). It's definitely got me a little twisted up. Do you think we should use "offline" here?

I reckon this will become easier again with #6011, as we'll need to start tracking if local entities are "offline" or "online" so we know whether to delete them or not if they're not in the "online" lists any longer.

It can be local instead of offline (maybe it's even better) but it would require reviewing the code and fixing other occurrences of offline.

Could we leave this for #6011? I think, it'll be best to consolidate the naming there.

grzesiek2010 · 2024-05-02T14:53:01Z

collect_app/src/androidTest/java/org/odk/collect/android/feature/formentry/EntityFormTest.kt

@@ -21,15 +22,20 @@ class EntityFormTest {
        .around(rule)

    @Test
-    fun fillingEntityRegistrationForm_createsEntityInTheBrowser() {
+    fun fillingEntityRegistrationForm_beforeCreatingEntityList_doesNotCreateEntityForFollowUpForms() {


This title sounds confusing to me similar to #6108 (comment)
I'm not sure what CreatingEntityList means. First, you create local entities using the registration form and then you open a form that has online entities. So CreatingEntityList means opening a form with online entities?

You "create an entity list" in the browser using the "Add entity list" flow. Does that make sense or do you think it needs updated?

You "create an entity list" in the browser using the "Add entity list" flow.

But you don't do that here?

But you don't do that here?

Right. That's the "beforeCreatingEntityList" part. I'm testing that we don't create entities if we haven't manually created an entity list yet.

collect_app/src/androidTest/java/org/odk/collect/android/feature/formentry/EntityFormTest.kt

grzesiek2010 · 2024-05-02T16:53:55Z

strings/src/main/res/values/strings.xml


    <!-- Text for button that deletes all entities for the project -->
    <string name="clear_entities">Clear all</string>

+    <!-- Text for button that adds a new entity list -->
+    <string name="add_entity_list">Add entity list</string>


Entity list is the same as dataset right? Here the naming is also confusing.

That was a change in terminology that's happened since we added the first entity code to Collect. The spec now states:

In this specification, collections of Entities are referred to as Datasets. The term “Entity List” is generally recommended instead of “Dataset” in text that is intended for users rather than developers.

We could just call them "entity lists" everywhere in Collect and avoid "datasets" entirely?

Yeah, it would be better to use consistent naming.

Again, I think we should consolidate this in #6011. Agreed that we want to use "entity list" everywhere instead of data sets though.

grzesiek2010 · 2024-05-02T16:55:19Z

There is something wrong with the browser because if there are multiple elements in the list it looks like this:

seadowg · 2024-05-03T08:48:16Z

There is something wrong with the browser because if there are multiple elements in the list it looks like this

I'm not able to reproduce. Do you have more specific steps?

grzesiek2010 · 2024-05-06T20:15:36Z

I'm not able to reproduce. Do you have more specific steps?

I'm just adding 2 entity lists in the Experimental section.

grzesiek2010

There is a couple of new comments.

seadowg added 18 commits April 24, 2024 13:22

Add failing test around consistent entity lists

96200a0

Add ability to create blank entity lists in browser

1b58dd3

Add helper to add entity list in tests

4c9933b

Share instance finalization code between finalization paths

6df93d5

Only save entities if they're dataset exists

06c3fa7

Update switch project test

822a964

Don't validate form twice

5c9e999

Rename method

adefa58

Remove test only code that doesn't seem to be needed

7007f4f

Move form download code to one package

dd353b3

Use FormsDataService when manually downloading forms

2e75373

Combine use cases

67eaeae

Spike out preloading entities on form download

cbcfa5d

Update tests for entities processing

7a01eae

Handle entity version updates from the server

6621684

Simplify update logic

6b1ebba

Add method for grabbing entities directly

97f5722

Remove unneeded test

5c38ca9

Property merging is handled by EntitiesRepository

seadowg added the blocked label Apr 25, 2024

seadowg added 4 commits April 25, 2024 13:51

Extend ExternalInstanceParser to prevent needlessly parsing entity lists

2bed805

Validate entity CSV before updating local

1f34eb3

Avoid crashes if there is a clash between a dataset and another media…

322130e

… file

Correct property update behaviour

1cc9f97

seadowg force-pushed the entity-lists branch from f2acf4e to 1cc9f97 Compare April 25, 2024 12:54

seadowg added blocked and removed blocked labels Apr 25, 2024

seadowg added 2 commits April 25, 2024 16:53

Include version in entity follow up forms

c9532cc

Update for new JavaRosa version

1ffc276

seadowg mentioned this pull request Apr 26, 2024

Crash of the app after sending a draft version of trees update form #6109

Closed

Remove get() to prevent performance problems in JSON implementation

9497a37

seadowg added 3 commits April 27, 2024 13:04

Use FileInstanceParser interface

5de51a4

Update settings text for entities

4afc2b5

Reorganize entity tests

998871f

seadowg removed the blocked label Apr 29, 2024

Fix imports

d792337

seadowg marked this pull request as ready for review April 29, 2024 10:02

seadowg requested a review from grzesiek2010 April 29, 2024 10:02

grzesiek2010 requested changes May 2, 2024

View reviewed changes

grzesiek2010 reviewed May 2, 2024

View reviewed changes

seadowg added 2 commits May 2, 2024 16:05

Just use inline dialog for add entity UI

bda9c3e

Fix imports

73f533a

grzesiek2010 reviewed May 2, 2024

View reviewed changes

collect_app/src/androidTest/java/org/odk/collect/android/feature/formentry/EntityFormTest.kt Outdated Show resolved Hide resolved

seadowg added 2 commits May 2, 2024 16:13

Use node constants

9142929

Make test title clearer

8421443

seadowg requested a review from grzesiek2010 May 2, 2024 15:29

grzesiek2010 reviewed May 2, 2024

View reviewed changes

seadowg requested a review from grzesiek2010 May 3, 2024 08:48

grzesiek2010 requested changes May 6, 2024

View reviewed changes

seadowg added 5 commits May 7, 2024 10:40

Parse form details from test form if not specified

4d4b8d3

Make stub server behave like Central for entity lists

42d8478

Consolidate server form use cases

3caca3a

Use constant

6ac28fd

Fix entity list item height

a9f9e50

seadowg requested a review from grzesiek2010 May 7, 2024 12:23

grzesiek2010 approved these changes May 8, 2024

View reviewed changes

grzesiek2010 merged commit 916629c into getodk:master May 8, 2024
6 checks passed

seadowg deleted the entity-lists branch May 9, 2024 07:30

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Store shared entities for follow up forms #6108

Store shared entities for follow up forms #6108

seadowg commented Apr 24, 2024 •

edited

Loading

grzesiek2010 May 2, 2024

seadowg May 2, 2024

grzesiek2010 May 2, 2024

seadowg May 3, 2024

grzesiek2010 May 6, 2024

seadowg May 7, 2024

seadowg May 7, 2024

grzesiek2010 May 2, 2024

seadowg May 2, 2024

grzesiek2010 May 8, 2024

grzesiek2010 May 2, 2024

seadowg May 2, 2024

grzesiek2010 May 2, 2024

seadowg May 7, 2024

grzesiek2010 May 2, 2024

seadowg May 2, 2024

grzesiek2010 May 2, 2024

seadowg May 3, 2024

grzesiek2010 May 2, 2024

seadowg May 3, 2024

grzesiek2010 May 6, 2024

seadowg May 7, 2024

grzesiek2010 commented May 2, 2024

seadowg commented May 3, 2024

grzesiek2010 commented May 6, 2024

grzesiek2010 left a comment

Store shared entities for follow up forms #6108

Store shared entities for follow up forms #6108

Conversation

seadowg commented Apr 24, 2024 • edited Loading

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

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?

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

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

Before submitting this PR, please make sure you have:

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

grzesiek2010 commented May 2, 2024

seadowg commented May 3, 2024

grzesiek2010 commented May 6, 2024

grzesiek2010 left a comment

Choose a reason for hiding this comment

seadowg commented Apr 24, 2024 •

edited

Loading