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

Store shared entities for follow up forms #6108

Merged
merged 41 commits into from
May 8, 2024
Merged
Show file tree
Hide file tree
Changes from 34 commits
Commits
Show all changes
41 commits
Select commit Hold shift + click to select a range
96200a0
Add failing test around consistent entity lists
seadowg Apr 11, 2024
1b58dd3
Add ability to create blank entity lists in browser
seadowg Apr 11, 2024
4c9933b
Add helper to add entity list in tests
seadowg Apr 15, 2024
6df93d5
Share instance finalization code between finalization paths
seadowg Apr 15, 2024
06c3fa7
Only save entities if they're dataset exists
seadowg Apr 15, 2024
822a964
Update switch project test
seadowg Apr 15, 2024
5c9e999
Don't validate form twice
seadowg Apr 15, 2024
adefa58
Rename method
seadowg Apr 16, 2024
7007f4f
Remove test only code that doesn't seem to be needed
seadowg Apr 16, 2024
dd353b3
Move form download code to one package
seadowg Apr 16, 2024
2e75373
Use FormsDataService when manually downloading forms
seadowg Apr 16, 2024
67eaeae
Combine use cases
seadowg Apr 16, 2024
cbcfa5d
Spike out preloading entities on form download
seadowg Apr 23, 2024
7a01eae
Update tests for entities processing
seadowg Apr 23, 2024
6621684
Handle entity version updates from the server
seadowg Apr 24, 2024
6b1ebba
Simplify update logic
seadowg Apr 24, 2024
97f5722
Add method for grabbing entities directly
seadowg Apr 24, 2024
5c38ca9
Remove unneeded test
seadowg Apr 24, 2024
2bed805
Extend ExternalInstanceParser to prevent needlessly parsing entity lists
seadowg Apr 24, 2024
1f34eb3
Validate entity CSV before updating local
seadowg Apr 24, 2024
322130e
Avoid crashes if there is a clash between a dataset and another media…
seadowg Apr 24, 2024
1cc9f97
Correct property update behaviour
seadowg Apr 24, 2024
c9532cc
Include version in entity follow up forms
seadowg Apr 25, 2024
1ffc276
Update for new JavaRosa version
seadowg Apr 26, 2024
9497a37
Remove get() to prevent performance problems in JSON implementation
seadowg Apr 26, 2024
251f0e9
Allow multiple entities to be saved at once
seadowg Apr 27, 2024
6b70a0b
Reduce repository accesses when updating local entities
seadowg Apr 27, 2024
d577bdc
Remove need for array conversion
seadowg Apr 27, 2024
5de51a4
Use FileInstanceParser interface
seadowg Apr 27, 2024
4afc2b5
Update settings text for entities
seadowg Apr 28, 2024
998871f
Reorganize entity tests
seadowg Apr 28, 2024
d792337
Fix imports
seadowg Apr 29, 2024
bda9c3e
Just use inline dialog for add entity UI
seadowg May 2, 2024
73f533a
Fix imports
seadowg May 2, 2024
9142929
Use node constants
seadowg May 2, 2024
8421443
Make test title clearer
seadowg May 2, 2024
4d4b8d3
Parse form details from test form if not specified
seadowg May 7, 2024
42d8478
Make stub server behave like Central for entity lists
seadowg May 7, 2024
3caca3a
Consolidate server form use cases
seadowg May 7, 2024
6ac28fd
Use constant
seadowg May 7, 2024
a9f9e50
Fix entity list item height
seadowg May 7, 2024
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@ import android.os.Bundle
import androidx.fragment.app.DialogFragment
import androidx.fragment.app.FragmentManager
import timber.log.Timber
import kotlin.reflect.KClass

object DialogFragmentUtils {

Expand Down Expand Up @@ -86,4 +87,8 @@ object DialogFragmentUtils {
}
}
}

fun <T : DialogFragment> FragmentManager.showIfNotShowing(dialogClass: KClass<T>) {
showIfNotShowing(dialogClass.java, this)
}
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,44 @@
package org.odk.collect.android.feature.entitymanagement

import org.junit.Rule
import org.junit.Test
import org.junit.rules.RuleChain
import org.odk.collect.android.support.TestDependencies
import org.odk.collect.android.support.pages.FormEntryPage
import org.odk.collect.android.support.rules.CollectTestRule
import org.odk.collect.android.support.rules.TestRuleChain

class ViewEntitiesTest {

private val rule = CollectTestRule(useDemoProject = false)
private val testDependencies = TestDependencies()

@get:Rule
val ruleChain: RuleChain = TestRuleChain.chain(testDependencies)
.around(rule)

@Test
fun canViewLocallyCreatedEntitiesInBrowser() {
Copy link
Member

Choose a reason for hiding this comment

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

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.

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

Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link
Member Author

Choose a reason for hiding this comment

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

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

testDependencies.server.addForm("one-question-entity-registration.xml")

rule.withMatchExactlyProject(testDependencies.server.url)
.addEntityListInBrowser("people")
.startBlankForm("One Question Entity Registration")
.fillOutAndFinalize(FormEntryPage.QuestionAndAnswer("Name", "Logan Roy"))
.openEntityBrowser()
.clickOnDataset("people")
.assertEntity("Logan Roy", "full_name: Logan Roy")
}

@Test
fun canViewListEntitiesInBrowser() {
testDependencies.server.addForm("one-question-entity-follow-up.xml", listOf("people.csv"))

rule.withMatchExactlyProject(testDependencies.server.url)
.addEntityListInBrowser("people")
.refreshForms()
.openEntityBrowser()
.clickOnDataset("people")
.assertEntity("Roman Roy", "full_name: Roman Roy")
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,7 @@ import org.junit.rules.RuleChain
import org.junit.runner.RunWith
import org.odk.collect.android.support.TestDependencies
import org.odk.collect.android.support.pages.FormEntryPage
import org.odk.collect.android.support.pages.MainMenuPage
import org.odk.collect.android.support.rules.CollectTestRule
import org.odk.collect.android.support.rules.TestRuleChain

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

@Test
fun fillingEntityRegistrationForm_createsEntityInTheBrowser() {
fun fillingEntityRegistrationForm_beforeCreatingEntityList_doesNotCreateEntityForFollowUpForms() {
Copy link
Member

Choose a reason for hiding this comment

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

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?

Copy link
Member Author

Choose a reason for hiding this comment

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

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?

Copy link
Member

Choose a reason for hiding this comment

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

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

But you don't do that here?

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

testDependencies.server.addForm("one-question-entity-registration.xml")
testDependencies.server.addForm("one-question-entity-update.xml", listOf("people.csv"))

rule.withMatchExactlyProject(testDependencies.server.url)
.enableLocalEntitiesInForms()

.startBlankForm("One Question Entity Registration")
.fillOutAndFinalize(FormEntryPage.QuestionAndAnswer("Name", "Logan Roy"))
.openEntityBrowser()
.clickOnDataset("people")
.assertEntity("Logan Roy", "full_name: Logan Roy")

.startBlankForm("One Question Entity Update")
.assertQuestion("Select person")
.assertText("Roman Roy")
.assertTextDoesNotExist("Logan Roy")
}

@Test
Expand All @@ -38,7 +44,7 @@ class EntityFormTest {
testDependencies.server.addForm("one-question-entity-update.xml", listOf("people.csv"))

rule.withMatchExactlyProject(testDependencies.server.url)
.enableLocalEntitiesInForms()
.setupEntities("people")

.startBlankForm("One Question Entity Registration")
.fillOutAndFinalize(FormEntryPage.QuestionAndAnswer("Name", "Logan Roy"))
Expand All @@ -55,7 +61,7 @@ class EntityFormTest {
testDependencies.server.addForm("one-question-entity-update.xml", listOf("people.csv"))

rule.withMatchExactlyProject(testDependencies.server.url)
.enableLocalEntitiesInForms()
.setupEntities("people")

.startBlankForm("One Question Entity Update") // Open to create cached form def
.pressBackAndDiscardForm()
Expand All @@ -74,7 +80,7 @@ class EntityFormTest {
testDependencies.server.addForm("one-question-entity-update.xml", listOf("people.csv"))

rule.withMatchExactlyProject(testDependencies.server.url)
.enableLocalEntitiesInForms()
.setupEntities("people")

.startBlankForm("One Question Entity Update")
.assertQuestion("Select person")
Expand All @@ -90,54 +96,39 @@ class EntityFormTest {
}

@Test
fun fillingEntityFollowUpForm_whenOnlineDuplicateHasHigherVersion_showsOnlineVersion() {
testDependencies.server.addForm("one-question-entity-update.xml", listOf("people.csv"))

val mainMenuPage = rule.withMatchExactlyProject(testDependencies.server.url)
fun entityListsAreConsistentBetweenFollowUpForms() {
seadowg marked this conversation as resolved.
Show resolved Hide resolved
testDependencies.server.apply {
addForm(
"one-question-entity-update.xml",
listOf("people.csv")
)

addForm(
"one-question-entity-follow-up.xml",
mapOf("people.csv" to "updated-people.csv")
)
}

rule.withProject(testDependencies.server)
.enableLocalEntitiesInForms()
.addEntityListInBrowser("people")

.clickGetBlankForm()
.clickClearAll()
.clickForm("one-question-entity-update.xml")
.clickGetSelected()
.clickOK(MainMenuPage())
.startBlankForm("One Question Entity Update")
.assertQuestion("Select person")
.clickOnText("Roman Roy")
.swipeToNextQuestion("Name")
.answerQuestion("Name", "Romulus Roy")
.swipeToEndScreen()
.clickFinalize()

testDependencies.server.updateMediaFile(
"one-question-entity-update.xml",
"people.csv",
"updated-people.csv"
)
.assertText("Roman Roy")
.pressBackAndDiscardForm()

mainMenuPage.clickFillBlankForm()
.clickRefresh()
.clickOnForm("One Question Entity Update")
.clickGetBlankForm()
.clickClearAll()
Copy link
Member

Choose a reason for hiding this comment

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

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.

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

Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link
Member Author

Choose a reason for hiding this comment

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

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

Copy link
Member

Choose a reason for hiding this comment

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

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).

Copy link
Member Author

Choose a reason for hiding this comment

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

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.

Copy link
Member Author

Choose a reason for hiding this comment

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

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.

.clickForm("one-question-entity-follow-up.xml")
.clickGetSelected()
.clickOK(MainMenuPage())
.startBlankForm("One Question Entity Update")
.assertText("Ro-Ro Roy")
.assertTextDoesNotExist("Romulus Roy")
.assertTextDoesNotExist("Roman Roy")
}

@Test
fun fillingEntityFollowUpForm_whenOfflineDuplicateHasHigherVersion_showsOfflineVersion() {
testDependencies.server.addForm(
"one-question-entity-update.xml",
mapOf("people.csv" to "updated-people.csv")
)

rule.withMatchExactlyProject(testDependencies.server.url)
.enableLocalEntitiesInForms()

.startBlankForm("One Question Entity Update")
.assertQuestion("Select person")
.clickOnText("Ro-Ro Roy")
.swipeToNextQuestion("Name")
.answerQuestion("Name", "Romulus Roy")
.swipeToEndScreen()
.clickFinalize()

.startBlankForm("One Question Entity Update")
.assertText("Romulus Roy")
.assertTextDoesNotExist("Ro-Ro Roy")
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -76,6 +76,7 @@ class SwitchProjectTest {
.clickOKOnDialog(MainMenuPage())

// Fill form
.addEntityListInBrowser("people")
.startBlankForm("One Question Entity Registration")
.fillOutAndFinalize(FormEntryPage.QuestionAndAnswer("Name", "Alice"))
.clickSendFinalizedForm(1)
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -6,16 +6,28 @@
import static androidx.test.espresso.matcher.ViewMatchers.isDisplayed;
import static androidx.test.espresso.matcher.ViewMatchers.withText;

import org.odk.collect.strings.R.string;

public class GetBlankFormPage extends Page<GetBlankFormPage> {

@Override
public GetBlankFormPage assertOnPage() {
onView(withText(getTranslatedString(org.odk.collect.strings.R.string.get_forms))).check(matches(isDisplayed()));
onView(withText(getTranslatedString(string.get_forms))).check(matches(isDisplayed()));
return this;
}

public FormsDownloadResultPage clickGetSelected() {
onView(withText(getTranslatedString(org.odk.collect.strings.R.string.download))).perform(click());
onView(withText(getTranslatedString(string.download))).perform(click());
return new FormsDownloadResultPage().assertOnPage();
}

public GetBlankFormPage clickClearAll() {
clickOnString(string.clear_all);
return this;
}

public GetBlankFormPage clickForm(String formName) {
clickOnText(formName);
return this;
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,7 @@
import org.odk.collect.android.support.StorageUtils;
import org.odk.collect.android.support.TestScheduler;
import org.odk.collect.android.support.WaitFor;
import org.odk.collect.strings.R.string;

import java.io.IOException;
import java.util.List;
Expand Down Expand Up @@ -287,6 +288,30 @@ public EntitiesPage openEntityBrowser() {
return new EntitiesPage().assertOnPage();
}

public MainMenuPage addEntityListInBrowser(String entityList) {
return openEntityBrowser()
.clickOptionsIcon(string.add_entity_list)
.clickOnTextInPopup(string.add_entity_list)
.inputText(entityList)
.clickOnTextInDialog(string.add)
.assertText(entityList)
.pressBack(new ExperimentalPage())
.pressBack(new ProjectSettingsPage())
.pressBack(new MainMenuPage());
}

public MainMenuPage refreshForms() {
return clickFillBlankForm()
.clickRefresh()
.pressBack(new MainMenuPage());
}

public MainMenuPage setupEntities(String entityList) {
return enableLocalEntitiesInForms()
.addEntityListInBrowser(entityList)
.refreshForms();
}

@NotNull
public MainMenuPage enableLocalEntitiesInForms() {
return openProjectSettingsDialog()
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -39,16 +39,17 @@
import org.odk.collect.android.adapters.FormDownloadListAdapter;
import org.odk.collect.android.formentry.RefreshFormListDialogFragment;
import org.odk.collect.android.formlists.sorting.FormListSortingOption;
import org.odk.collect.android.formmanagement.FormDownloadException;
import org.odk.collect.android.formmanagement.FormDownloader;
import org.odk.collect.android.formmanagement.FormSourceExceptionMapper;
import org.odk.collect.android.formmanagement.FormsDataService;
import org.odk.collect.android.formmanagement.ServerFormDetails;
import org.odk.collect.android.formmanagement.ServerFormsDetailsFetcher;
import org.odk.collect.android.formmanagement.download.FormDownloadException;
import org.odk.collect.android.fragments.dialogs.FormsDownloadResultDialog;
import org.odk.collect.android.injection.DaggerUtils;
import org.odk.collect.android.listeners.DownloadFormsTaskListener;
import org.odk.collect.android.listeners.FormListDownloaderListener;
import org.odk.collect.android.openrosa.HttpCredentialsInterface;
import org.odk.collect.android.projects.ProjectsDataService;
import org.odk.collect.android.tasks.DownloadFormListTask;
import org.odk.collect.android.tasks.DownloadFormsTask;
import org.odk.collect.android.utilities.ApplicationConstants;
Expand Down Expand Up @@ -128,7 +129,10 @@ public class FormDownloadListActivity extends FormListActivity implements FormLi
NetworkStateProvider connectivityProvider;

@Inject
FormDownloader formDownloader;
FormsDataService formsDataService;

@Inject
ProjectsDataService projectsDataService;

@SuppressWarnings("unchecked")
@Override
Expand Down Expand Up @@ -395,7 +399,7 @@ private void startFormsDownload(@NonNull ArrayList<ServerFormDetails> filesToDow
// show dialog box
DialogFragmentUtils.showIfNotShowing(RefreshFormListDialogFragment.class, getSupportFragmentManager());

downloadFormsTask = new DownloadFormsTask(formDownloader);
downloadFormsTask = new DownloadFormsTask(projectsDataService.getCurrentProject().getUuid(), formsDataService);
downloadFormsTask.setDownloaderListener(this);

if (viewModel.getUrl() != null) {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -11,7 +11,7 @@ import org.javarosa.xform.util.XFormUtils
import org.odk.collect.android.dynamicpreload.DynamicPreloadXFormParserFactory
import org.odk.collect.android.entities.EntitiesRepositoryProvider
import org.odk.collect.android.logic.actions.setgeopoint.CollectSetGeopointActionHandler
import org.odk.collect.entities.OfflineEntitiesExternalInstanceParserFactory
import org.odk.collect.entities.LocalEntitiesExternalInstanceParserFactory
import org.odk.collect.metadata.PropertyManager
import org.odk.collect.settings.SettingsProvider
import org.odk.collect.settings.keys.ProjectKeys
Expand Down Expand Up @@ -46,11 +46,11 @@ class JavaRosaInitializer(

XFormUtils.setXFormParserFactory(dynamicPreloadXFormParserFactory)

val offlineEntitiesExternalInstanceParserFactory = OfflineEntitiesExternalInstanceParserFactory(
val localEntitiesExternalInstanceParserFactory = LocalEntitiesExternalInstanceParserFactory(
entitiesRepositoryProvider::get,
{ settingsProvider.getUnprotectedSettings().getBoolean(ProjectKeys.KEY_LOCAL_ENTITIES) }
)

XFormUtils.setExternalInstanceParserFactory(offlineEntitiesExternalInstanceParserFactory)
XFormUtils.setExternalInstanceParserFactory(localEntitiesExternalInstanceParserFactory)
}
}
Loading