diff --git a/collect_app/src/androidTest/java/org/odk/collect/android/feature/smoke/BadServerTest.java b/collect_app/src/androidTest/java/org/odk/collect/android/feature/smoke/BadServerTest.java index bcc48c466cc..0a47702c5da 100644 --- a/collect_app/src/androidTest/java/org/odk/collect/android/feature/smoke/BadServerTest.java +++ b/collect_app/src/androidTest/java/org/odk/collect/android/feature/smoke/BadServerTest.java @@ -7,17 +7,17 @@ import org.junit.rules.RuleChain; import org.junit.runner.RunWith; import org.odk.collect.android.R; -import org.odk.collect.android.support.rules.CollectTestRule; import org.odk.collect.android.support.TestDependencies; -import org.odk.collect.android.support.rules.TestRuleChain; import org.odk.collect.android.support.TranslatedStringBuilder; +import org.odk.collect.android.support.pages.MainMenuPage; +import org.odk.collect.android.support.rules.CollectTestRule; +import org.odk.collect.android.support.rules.TestRuleChain; import java.util.Arrays; @RunWith(AndroidJUnit4.class) public class BadServerTest { - private final CollectTestRule rule = new CollectTestRule(false); private final TestDependencies testDependencies = new TestDependencies(); @@ -63,4 +63,26 @@ public void whenMediaFileHasMissingPrefix_showsAsUpdated() { .clickGetBlankForm() .assertText(R.string.newer_version_of_a_form_info); } + + @Test + /* + A server that doesn't return hashes based on the md5 of the file for media files + would fool Collect into thinking there was a new one each time. Collect should still redownload + the file in this case (there's nothing else it can do), but it should only identify the form + as being updated if the file actually changed. + */ + public void whenMediaFileHasUnstableHash_butIsIdentical_doesNotShowAsUpdatedAfterRedownload() { + testDependencies.server.returnRandomMediaFileHash(); + testDependencies.server.addForm("One Question", "one_question", "1", "one-question.xml", Arrays.asList("fruits.csv")); + + rule.withProject(testDependencies.server.getURL()) + .copyForm("one-question.xml", Arrays.asList("fruits.csv"), false, testDependencies.server.getHostName()) + .clickGetBlankForm() + .assertText(R.string.newer_version_of_a_form_info) + .clickGetSelected() + .clickOKOnDialog(new MainMenuPage()) + .clickFillBlankForm() + .assertTextThatContainsDoesNoExist("Updated on") + .assertTextThatContainsExists("Added on"); + } } diff --git a/collect_app/src/androidTest/java/org/odk/collect/android/support/StubOpenRosaServer.java b/collect_app/src/androidTest/java/org/odk/collect/android/support/StubOpenRosaServer.java index b9ac4400933..7c5f5d0006b 100644 --- a/collect_app/src/androidTest/java/org/odk/collect/android/support/StubOpenRosaServer.java +++ b/collect_app/src/androidTest/java/org/odk/collect/android/support/StubOpenRosaServer.java @@ -19,6 +19,7 @@ import org.odk.collect.android.openrosa.OpenRosaConstants; import org.odk.collect.android.openrosa.OpenRosaHttpInterface; import org.odk.collect.shared.strings.Md5; +import org.odk.collect.shared.strings.RandomString; import java.io.ByteArrayInputStream; import java.io.File; @@ -43,6 +44,7 @@ public class StubOpenRosaServer implements OpenRosaHttpInterface { private boolean fetchingFormsError; private boolean noHashInFormList; private boolean noHashPrefixInMediaFiles; + private boolean randomHash; @NonNull @Override @@ -150,6 +152,10 @@ public void removeMediaFileHashPrefix() { noHashPrefixInMediaFiles = true; } + public void returnRandomMediaFileHash() { + randomHash = true; + } + public String getURL() { return "https://" + HOST; } @@ -233,7 +239,13 @@ private InputStream getManifestResponse(@NonNull URI uri) throws IOException { for (String mediaFile : xformItem.getMediaFiles()) { AssetManager assetManager = InstrumentationRegistry.getInstrumentation().getContext().getAssets(); - String mediaFileHash = Md5.getMd5Hash(assetManager.open("media/" + mediaFile)); + String mediaFileHash; + + if (randomHash) { + mediaFileHash = RandomString.randomString(8); + } else { + mediaFileHash = Md5.getMd5Hash(assetManager.open("media/" + mediaFile)); + } stringBuilder .append("") diff --git a/collect_app/src/androidTest/java/org/odk/collect/android/support/pages/Page.kt b/collect_app/src/androidTest/java/org/odk/collect/android/support/pages/Page.kt index f58875c91f0..2c4642db7ad 100644 --- a/collect_app/src/androidTest/java/org/odk/collect/android/support/pages/Page.kt +++ b/collect_app/src/androidTest/java/org/odk/collect/android/support/pages/Page.kt @@ -109,6 +109,7 @@ abstract class Page> { return this as T } + @JvmOverloads fun assertTextThatContainsExists(text: String, index: Int = 0): T { onView( withIndex( @@ -123,6 +124,21 @@ abstract class Page> { return this as T } + @JvmOverloads + fun assertTextThatContainsDoesNoExist(text: String, index: Int = 0): T { + onView( + withIndex( + withText( + containsString( + text + ) + ), + index + ) + ).check(doesNotExist()) + return this as T + } + fun checkIsTranslationDisplayed(vararg text: String?): T { for (s in text) { try { diff --git a/collect_app/src/main/java/org/odk/collect/android/formmanagement/FormMediaDownloader.kt b/collect_app/src/main/java/org/odk/collect/android/formmanagement/FormMediaDownloader.kt index 65c443e196b..51dbb38a343 100644 --- a/collect_app/src/main/java/org/odk/collect/android/formmanagement/FormMediaDownloader.kt +++ b/collect_app/src/main/java/org/odk/collect/android/formmanagement/FormMediaDownloader.kt @@ -20,7 +20,6 @@ class FormMediaDownloader( @Throws(IOException::class, FormSourceException::class, InterruptedException::class) fun download( formToDownload: ServerFormDetails, - files: List, tempMediaPath: String, tempDir: File, stateListener: OngoingWorkListener @@ -28,21 +27,33 @@ class FormMediaDownloader( var atLeastOneNewMediaFileDetected = false val tempMediaDir = File(tempMediaPath).also { it.mkdir() } - files.forEachIndexed { i, mediaFile -> + formToDownload.manifest!!.mediaFiles.forEachIndexed { i, mediaFile -> stateListener.progressUpdate(i + 1) val tempMediaFile = File(tempMediaDir, mediaFile.filename) - searchForExistingMediaFile(formToDownload, mediaFile).let { + val existingFile = searchForExistingMediaFile(formToDownload, mediaFile) + existingFile.let { if (it != null) { - copyFile(it, tempMediaFile) + if (getMd5Hash(it).contentEquals(mediaFile.hash)) { + copyFile(it, tempMediaFile) + } else { + val existingFileHash = getMd5Hash(it) + val file = formSource.fetchMediaFile(mediaFile.downloadUrl) + interuptablyWriteFile(file, tempMediaFile, tempDir, stateListener) + + if (!getMd5Hash(tempMediaFile).contentEquals(existingFileHash)) { + atLeastOneNewMediaFileDetected = true + } + } } else { - atLeastOneNewMediaFileDetected = true val file = formSource.fetchMediaFile(mediaFile.downloadUrl) interuptablyWriteFile(file, tempMediaFile, tempDir, stateListener) + atLeastOneNewMediaFileDetected = true } } } + return atLeastOneNewMediaFileDetected } @@ -51,12 +62,12 @@ class FormMediaDownloader( mediaFile: MediaFile ): File? { val allFormVersions = formsRepository.getAllByFormId(formToDownload.formId) - return allFormVersions.map { form: Form -> + return allFormVersions.sortedByDescending { + it.date + }.map { form: Form -> File(form.formMediaPath, mediaFile.filename) }.firstOrNull { file: File -> - val currentFileHash = getMd5Hash(file) - val downloadFileHash = mediaFile.hash - file.exists() && currentFileHash.contentEquals(downloadFileHash) + file.exists() } } } diff --git a/collect_app/src/main/java/org/odk/collect/android/formmanagement/ServerFormDownloader.java b/collect_app/src/main/java/org/odk/collect/android/formmanagement/ServerFormDownloader.java index eda34cf5aa9..ff86971b2ef 100644 --- a/collect_app/src/main/java/org/odk/collect/android/formmanagement/ServerFormDownloader.java +++ b/collect_app/src/main/java/org/odk/collect/android/formmanagement/ServerFormDownloader.java @@ -98,7 +98,7 @@ private void processOneForm(ServerFormDetails fd, OngoingWorkListener stateListe // download media files if there are any if (fd.getManifest() != null && !fd.getManifest().getMediaFiles().isEmpty()) { FormMediaDownloader mediaDownloader = new FormMediaDownloader(formsRepository, formSource); - newAttachmentsDetected = mediaDownloader.download(fd, fd.getManifest().getMediaFiles(), tempMediaPath, tempDir, stateListener); + newAttachmentsDetected = mediaDownloader.download(fd, tempMediaPath, tempDir, stateListener); } } catch (FormDownloadException.DownloadingInterrupted | InterruptedException e) { Timber.i(e); diff --git a/collect_app/src/test/java/org/odk/collect/android/formmanagement/FormMediaDownloaderTest.kt b/collect_app/src/test/java/org/odk/collect/android/formmanagement/FormMediaDownloaderTest.kt new file mode 100644 index 00000000000..ea95992beef --- /dev/null +++ b/collect_app/src/test/java/org/odk/collect/android/formmanagement/FormMediaDownloaderTest.kt @@ -0,0 +1,97 @@ +package org.odk.collect.android.formmanagement + +import org.hamcrest.MatcherAssert.assertThat +import org.hamcrest.Matchers.equalTo +import org.junit.Test +import org.mockito.kotlin.doReturn +import org.mockito.kotlin.mock +import org.odk.collect.forms.FormSource +import org.odk.collect.forms.ManifestFile +import org.odk.collect.forms.MediaFile +import org.odk.collect.formstest.FormFixtures +import org.odk.collect.formstest.InMemFormsRepository +import org.odk.collect.shared.TempFiles +import org.odk.collect.shared.strings.Md5 +import java.io.File + +class FormMediaDownloaderTest { + + @Test + fun `returns false when there is an existing copy of a media file and an older one`() { + // Save forms + val formsRepository = InMemFormsRepository() + val form1 = FormFixtures.form( + version = "1", + date = 1, + mediaFiles = listOf(Pair("file", "old")) + ) + formsRepository.save(form1) + + val form2 = FormFixtures.form( + version = "2", + date = 2, + mediaFiles = listOf(Pair("file", "existing")) + ) + formsRepository.save(form2) + + // Set up same media file on server + val existingMediaFileHash = Md5.getMd5Hash(File(form2.formMediaPath, "file"))!! + val mediaFile = MediaFile("file", existingMediaFileHash, "downloadUrl") + val manifestFile = ManifestFile(null, listOf(mediaFile)) + val serverFormDetails = + ServerFormDetails(null, null, "formId", "3", null, false, true, manifestFile) + val formSource = mock { + on { fetchMediaFile(mediaFile.downloadUrl) } doReturn "existing".toByteArray() + .inputStream() + } + + val formMediaDownloader = FormMediaDownloader(formsRepository, formSource) + val result = formMediaDownloader.download( + serverFormDetails, + File(TempFiles.createTempDir(), "temp").absolutePath, + TempFiles.createTempDir(), + mock() + ) + + assertThat(result, equalTo(false)) + } + + @Test + fun `returns false when there is an existing copy of a media file and an older one and server hash doesn't match existing copy`() { + // Save forms + val formsRepository = InMemFormsRepository() + val form1 = FormFixtures.form( + version = "1", + date = 1, + mediaFiles = listOf(Pair("file", "old")) + ) + formsRepository.save(form1) + + val form2 = FormFixtures.form( + version = "2", + date = 2, + mediaFiles = listOf(Pair("file", "existing")) + ) + formsRepository.save(form2) + + // Set up same media file on server + val mediaFile = MediaFile("file", "somethingElse", "downloadUrl") + val manifestFile = ManifestFile(null, listOf(mediaFile)) + val serverFormDetails = + ServerFormDetails(null, null, "formId", "3", null, false, true, manifestFile) + val formSource = mock { + on { fetchMediaFile(mediaFile.downloadUrl) } doReturn "existing".toByteArray() + .inputStream() + } + + val formMediaDownloader = FormMediaDownloader(formsRepository, formSource) + val result = formMediaDownloader.download( + serverFormDetails, + File(TempFiles.createTempDir(), "temp").absolutePath, + TempFiles.createTempDir(), + mock() + ) + + assertThat(result, equalTo(false)) + } +} diff --git a/collect_app/src/test/java/org/odk/collect/android/widgets/items/SelectChoicesMapDataTest.kt b/collect_app/src/test/java/org/odk/collect/android/widgets/items/SelectChoicesMapDataTest.kt index a11c5166c73..ce5bc567caa 100644 --- a/collect_app/src/test/java/org/odk/collect/android/widgets/items/SelectChoicesMapDataTest.kt +++ b/collect_app/src/test/java/org/odk/collect/android/widgets/items/SelectChoicesMapDataTest.kt @@ -12,8 +12,8 @@ import org.junit.Test import org.junit.runner.RunWith import org.odk.collect.android.R import org.odk.collect.android.support.MockFormEntryPromptBuilder -import org.odk.collect.android.widgets.support.FormFixtures.selectChoice -import org.odk.collect.android.widgets.support.FormFixtures.treeElement +import org.odk.collect.android.widgets.support.FormElementFixtures.selectChoice +import org.odk.collect.android.widgets.support.FormElementFixtures.treeElement import org.odk.collect.androidtest.getOrAwaitValue import org.odk.collect.geo.selection.MappableSelectItem.IconifiedText import org.odk.collect.maps.MapPoint diff --git a/collect_app/src/test/java/org/odk/collect/android/widgets/items/SelectOneFromMapDialogFragmentTest.kt b/collect_app/src/test/java/org/odk/collect/android/widgets/items/SelectOneFromMapDialogFragmentTest.kt index cfa00597410..ef24751ebbd 100644 --- a/collect_app/src/test/java/org/odk/collect/android/widgets/items/SelectOneFromMapDialogFragmentTest.kt +++ b/collect_app/src/test/java/org/odk/collect/android/widgets/items/SelectOneFromMapDialogFragmentTest.kt @@ -31,8 +31,8 @@ import org.odk.collect.android.support.MockFormEntryPromptBuilder import org.odk.collect.android.utilities.Appearances import org.odk.collect.android.widgets.items.SelectOneFromMapDialogFragment.Companion.ARG_FORM_INDEX import org.odk.collect.android.widgets.items.SelectOneFromMapDialogFragment.Companion.ARG_SELECTED_INDEX -import org.odk.collect.android.widgets.support.FormFixtures.selectChoice -import org.odk.collect.android.widgets.support.FormFixtures.treeElement +import org.odk.collect.android.widgets.support.FormElementFixtures.selectChoice +import org.odk.collect.android.widgets.support.FormElementFixtures.treeElement import org.odk.collect.android.widgets.support.NoOpMapFragment import org.odk.collect.async.Scheduler import org.odk.collect.fragmentstest.FragmentScenarioLauncherRule diff --git a/collect_app/src/test/java/org/odk/collect/android/widgets/items/SelectOneFromMapWidgetTest.kt b/collect_app/src/test/java/org/odk/collect/android/widgets/items/SelectOneFromMapWidgetTest.kt index 7104aea5ef2..2f37b0ddd6b 100644 --- a/collect_app/src/test/java/org/odk/collect/android/widgets/items/SelectOneFromMapWidgetTest.kt +++ b/collect_app/src/test/java/org/odk/collect/android/widgets/items/SelectOneFromMapWidgetTest.kt @@ -25,7 +25,7 @@ import org.odk.collect.android.preferences.GuidanceHint import org.odk.collect.android.support.CollectHelpers import org.odk.collect.android.support.MockFormEntryPromptBuilder import org.odk.collect.android.support.WidgetTestActivity -import org.odk.collect.android.widgets.support.FormFixtures.selectChoice +import org.odk.collect.android.widgets.support.FormElementFixtures.selectChoice import org.odk.collect.android.widgets.support.NoOpMapFragment import org.odk.collect.android.widgets.support.QuestionWidgetHelpers.mockValueChangedListener import org.odk.collect.android.widgets.support.QuestionWidgetHelpers.promptWithAnswer diff --git a/collect_app/src/test/java/org/odk/collect/android/widgets/support/FormFixtures.kt b/collect_app/src/test/java/org/odk/collect/android/widgets/support/FormElementFixtures.kt similarity index 96% rename from collect_app/src/test/java/org/odk/collect/android/widgets/support/FormFixtures.kt rename to collect_app/src/test/java/org/odk/collect/android/widgets/support/FormElementFixtures.kt index fda8896ff87..77b9dc05f27 100644 --- a/collect_app/src/test/java/org/odk/collect/android/widgets/support/FormFixtures.kt +++ b/collect_app/src/test/java/org/odk/collect/android/widgets/support/FormElementFixtures.kt @@ -4,7 +4,7 @@ import org.javarosa.core.model.SelectChoice import org.javarosa.core.model.data.StringData import org.javarosa.core.model.instance.TreeElement -object FormFixtures { +object FormElementFixtures { fun selectChoice( value: String = "foo", diff --git a/formstest/src/main/java/org/odk/collect/formstest/FormFixtures.kt b/formstest/src/main/java/org/odk/collect/formstest/FormFixtures.kt new file mode 100644 index 00000000000..d3c8e216ae7 --- /dev/null +++ b/formstest/src/main/java/org/odk/collect/formstest/FormFixtures.kt @@ -0,0 +1,30 @@ +package org.odk.collect.formstest + +import org.odk.collect.forms.Form +import org.odk.collect.shared.TempFiles +import java.io.File + +object FormFixtures { + fun form( + formId: String = "formId", + version: String = "1", + date: Long = 1, + mediaFiles: List> = emptyList() + ): Form { + val formFilesPath = TempFiles.createTempDir().absolutePath + val mediaFilePath = TempFiles.createTempDir().absolutePath + + mediaFiles.forEach { (name, contents) -> + File(mediaFilePath, name).also { it.writeBytes(contents.toByteArray()) } + } + + return Form.Builder() + .displayName("Test Form") + .formId(formId) + .version(version) + .formFilePath(FormUtils.createFormFixtureFile(formId, version, formFilesPath)) + .formMediaPath(mediaFilePath) + .date(date) + .build() + } +} diff --git a/formstest/src/main/java/org/odk/collect/formstest/FormUtils.kt b/formstest/src/main/java/org/odk/collect/formstest/FormUtils.kt index fc66b3d2b6e..0c388d33f16 100644 --- a/formstest/src/main/java/org/odk/collect/formstest/FormUtils.kt +++ b/formstest/src/main/java/org/odk/collect/formstest/FormUtils.kt @@ -4,7 +4,6 @@ import org.apache.commons.io.FileUtils import org.odk.collect.forms.Form import java.io.File import java.io.IOException -import java.lang.RuntimeException import java.nio.charset.Charset object FormUtils { @@ -49,6 +48,22 @@ object FormUtils { xform: String = createXFormBody(formId, version), autosend: String? = null ): Form.Builder { + val formFilePath = createFormFixtureFile(formId, version, formFilesPath, xform) + + return Form.Builder() + .displayName("Test Form") + .formId(formId) + .version(version) + .formFilePath(formFilePath) + .autoSend(autosend) + } + + fun createFormFixtureFile( + formId: String, + version: String?, + formFilesPath: String, + xform: String = createXFormBody(formId, version) + ): String { val fileName = formId + "-" + version + "-" + Math.random() val formFile = File("$formFilesPath/$fileName.xml") @@ -58,11 +73,6 @@ object FormUtils { throw RuntimeException(e) } - return Form.Builder() - .displayName("Test Form") - .formFilePath(formFile.absolutePath) - .formId(formId) - .version(version) - .autoSend(autosend) + return formFile.absolutePath } }