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

Allow forms with parse errors to be downloaded, show errors at form open time #6428

Merged
merged 19 commits into from
Oct 1, 2024
Merged
Show file tree
Hide file tree
Changes from 15 commits
Commits
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 @@ -43,17 +43,15 @@ class CatchFormDesignExceptionsTest {
fun whenFormHasNonFatalErrors_explanationDialogShouldBeDisplayedAndTheFormShouldNotBeClosedAfterClickingOK() {
rule.startAtMainMenu()
.copyForm("g6Error.xml")
.startBlankFormWithError("g6Error")
.assertText(org.odk.collect.strings.R.string.error_occured)
.startBlankFormWithError("g6Error", false)
.clickOK(FormEntryPage("g6Error"))
}

@Test
fun whenFormHasNonFatalErrors_explanationDialogShouldNotSurviveActivityRecreation() {
rule.startAtMainMenu()
.copyForm("g6Error.xml")
.startBlankFormWithError("g6Error")
.assertText(org.odk.collect.strings.R.string.error_occured)
.startBlankFormWithError("g6Error", false)
.clickOK(FormEntryPage("g6Error"))
.rotateToLandscape(FormEntryPage("g6Error"))
.assertTextDoesNotExist(org.odk.collect.strings.R.string.error_occured)
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,7 @@ 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
import org.odk.collect.strings.R

@RunWith(AndroidJUnit4::class)
class EntityFormTest {
Expand Down Expand Up @@ -190,4 +191,39 @@ class EntityFormTest {
.assertText("Roman Roy")
.assertTextDoesNotExist("Logan Roy")
}

@Test
fun manualEntityFormDownload_withUnsupportedSpecVersion_completesSuccessfully_butThrowsAnErrorAfterOpeningIt() {
Copy link
Member

Choose a reason for hiding this comment

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

Do we really need all three of these? I feel like we could maybe use our knowledge of the shared metadata parsing implementation and only test one of the paths given that these tests are so slow to run.

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 thought about that and keeping just one test, but since I didn't really favor any option, I decided to add all three. For me, the time required to run these tests has never been a significant issue, as I don’t run them in bulk locally. I always use Firebase and get results in 4-5 minutes. Additionally, we have recently removed many tests from the regression package—see the last part: #6383 so I believe we have fewer tests overall. Generally, I think it’s better to have such tests, even if they take some time to run. However, as I mentioned, I was unsure about keeping or removing them, so I'm fine with keeping just one test if that’s your preference, just let me know.

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

rule.withProject(testDependencies.server)
.clickGetBlankForm()
.clickClearAll()
.clickForm("One Question Entity Registration")
.clickGetSelected()
.clickOK(MainMenuPage())
.startBlankFormWithError("One Question Entity Registration", true)
.assertTextInDialog(R.string.unrecognized_entity_version, "2020.1.0")
.clickOKOnDialog(MainMenuPage())
}

@Test
fun automaticEntityFormDownload_withUnsupportedSpecVersion_completesSuccessfully_butThrowsAnErrorAfterOpeningIt() {
testDependencies.server.addForm("one-question-entity-registration-v2020.1.xml")

rule.withMatchExactlyProject(testDependencies.server.url)
.startBlankFormWithError("One Question Entity Registration", true)
.assertTextInDialog(R.string.unrecognized_entity_version, "2020.1.0")
.clickOKOnDialog(MainMenuPage())
}

@Test
fun syncEntityFormFromDisc_withUnsupportedSpecVersion_completesSuccessfully_butThrowsAnErrorAfterOpeningIt() {
rule.startAtFirstLaunch()
.clickTryCollect()
.copyForm("one-question-entity-registration-v2020.1.xml")
.startBlankFormWithError("One Question Entity Registration", true)
.assertTextInDialog(R.string.unrecognized_entity_version, "2020.1.0")
.clickOKOnDialog(MainMenuPage())
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -19,12 +19,16 @@ class InvalidFormTest {
var copyFormChain: RuleChain = chain().around(rule)

@Test
fun brokenForm_shouldNotBeVisibleOnFormList() {
fun brokenForm_isVisibleOnFormList_butThrowsAnErrorAfterOpeningIt() {
rule.startAtMainMenu()
.copyForm("invalid-form.xml")
.clickFillBlankForm()
.checkIsSnackbarErrorVisible("org.javarosa.xform.parse.XFormParseException: Cycle detected in form's relevant and calculation logic!")
.assertTextDoesNotExist("invalid-form")
.startBlankFormWithError("invalid-form", true)
.assertTextInDialog("An unknown error has occurred. Please ask your project leadership to email support@getodk.org with information about this form.\n" +
lognaturel marked this conversation as resolved.
Show resolved Hide resolved
"\n" +
"Cycle detected in form's relevant and calculation logic!\n" +
"The following nodes are likely involved in the loop:\n" +
"/asasas/q2")
.clickOKOnDialog(MainMenuPage())
}

@Test
Expand All @@ -41,7 +45,7 @@ class InvalidFormTest {
fun app_ShouldNotCrash_whenFillingFormsWithRepeatInFieldList() {
rule.startAtMainMenu()
.copyForm("repeat_in_field_list.xml")
.startBlankFormWithError("repeat_in_field_list")
.startBlankFormWithError("repeat_in_field_list", false)
.clickOK(FormEntryPage("repeat_in_field_list"))
.swipeToEndScreen()
.clickFinalize()
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -162,8 +162,7 @@ class FormUpdateTest {
.clickGetBlankForm()
.clickGetSelected()
.clickOKOnDialog(MainMenuPage())
.startBlankFormWithError("external select")
.assertText(org.odk.collect.strings.R.string.error_occured)
.startBlankFormWithError("external select", true)
.clickOKOnDialog(MainMenuPage())

testDependencies.server.addForm(
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -324,7 +324,7 @@ public void typeMismatchErrorMessage_shouldBeDisplayed() {
.startBlankForm("validate")
.longPressOnQuestion("year")
.removeResponse()
.swipeToNextQuestionWithError()
.swipeToNextQuestionWithError(false)
.checkIsTextDisplayedOnDialog("The value \"-01-01\" can't be converted to a date.");
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -7,10 +7,9 @@
import androidx.annotation.NonNull;
import androidx.annotation.Nullable;

import org.javarosa.xform.parse.XFormParser;
import org.jetbrains.annotations.NotNull;
import org.kxml2.kdom.Document;
import org.kxml2.kdom.Element;
import org.odk.collect.android.formmanagement.metadata.FormMetadata;
import org.odk.collect.android.formmanagement.metadata.FormMetadataParser;
import org.odk.collect.android.openrosa.CaseInsensitiveEmptyHeaders;
import org.odk.collect.android.openrosa.CaseInsensitiveHeaders;
import org.odk.collect.android.openrosa.HttpCredentialsInterface;
Expand All @@ -26,7 +25,6 @@
import java.io.File;
import java.io.IOException;
import java.io.InputStream;
import java.io.InputStreamReader;
import java.net.URI;
import java.util.ArrayList;
import java.util.HashMap;
Expand Down Expand Up @@ -317,33 +315,8 @@ private InputStream getMediaFile(URI uri) throws IOException {
}

private void addFormFromInputStream(String formXML, List<MediaFileItem> mediaFiles, InputStream formDefStream) {
try {
Document doc = XFormParser.getXMLDocument(new InputStreamReader(formDefStream));
Element head = doc.getRootElement().getElement(null, "head");
String title = (String) head.getElement(null, "title").getChild(0);

Element model = head.getElement(null, "model");

Element mainInstance = null;
for (int i = 0; i < model.getChildCount(); i++) {
Element child = model.getElement(i);
if (child == null) {
continue;
}

if (child.getName().equals("instance") && child.getAttributeCount() == 0) {
mainInstance = child;
}
}

Element mainInstanceRoot = mainInstance.getElement(0);
String id = mainInstanceRoot.getAttributeValue(null, "id");
String version = mainInstanceRoot.getAttributeValue(null, "version");

forms.add(new XFormItem(title, formXML, id, version, mediaFiles));
} catch (IOException e) {
throw new RuntimeException(e);
}
FormMetadata formMetadata = FormMetadataParser.readMetadata(formDefStream);
forms.add(new XFormItem(formMetadata.getTitle(), formXML, formMetadata.getId(), formMetadata.getVersion(), mediaFiles));
}

private static class XFormItem {
Expand Down

This file was deleted.

Original file line number Diff line number Diff line change
@@ -0,0 +1,15 @@
package org.odk.collect.android.support.pages

import org.odk.collect.strings.R

class ErrorDialog : OkDialog() {
fun assertOnPage(isFatal: Boolean): ErrorDialog {
assertOnPage()
if (isFatal) {
assertText(R.string.form_cannot_be_used)
} else {
assertText(R.string.error_occured)
}
return this
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -145,9 +145,9 @@ public FormEndPage swipeToEndScreen() {
return WaitFor.waitFor(() -> new FormEndPage(formName).assertOnPage());
}

public ErrorDialog swipeToNextQuestionWithError() {
public ErrorDialog swipeToNextQuestionWithError(boolean isFatal) {
flingLeft();
return new ErrorDialog().assertOnPage();
return new ErrorDialog().assertOnPage(isFatal);
}

public FormEntryPage swipeToNextQuestionWithConstraintViolation(int constraintText) {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -54,9 +54,9 @@ public AddNewRepeatDialog startBlankFormWithRepeatGroup(String formName, String
return new AddNewRepeatDialog(repeatName).assertOnPage();
}

public ErrorDialog startBlankFormWithError(String formName) {
public ErrorDialog startBlankFormWithError(String formName, boolean isFatal) {
goToBlankForm(formName);
return new ErrorDialog().assertOnPage();
return new ErrorDialog().assertOnPage(isFatal);
}

public OkDialog startBlankFormWithDialog(String formName) {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -491,8 +491,8 @@ abstract class Page<T : Page<T>> {
return this as T
}

fun assertTextInDialog(text: Int): T {
return assertTextInDialog(getTranslatedString(text))
fun assertTextInDialog(text: Int, vararg formatArgs: Any): T {
return assertTextInDialog(getTranslatedString(text, *formatArgs))
}

fun closeSnackbar(): T {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -1512,7 +1512,11 @@ private void createErrorDialog(FormError error) {
formError = error;

alertDialog = new MaterialAlertDialogBuilder(this).create();
alertDialog.setTitle(getString(org.odk.collect.strings.R.string.error_occured));
if (formError instanceof FormError.Fatal) {
alertDialog.setTitle(getString(org.odk.collect.strings.R.string.form_cannot_be_used));
} else {
alertDialog.setTitle(getString(org.odk.collect.strings.R.string.error_occured));
}
alertDialog.setMessage(formError.getMessage());
DialogInterface.OnClickListener errorListener = new DialogInterface.OnClickListener() {
@Override
Expand Down

This file was deleted.

Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@ import kotlinx.coroutines.flow.Flow
import org.odk.collect.android.formmanagement.download.FormDownloadException
import org.odk.collect.android.formmanagement.download.ServerFormDownloader
import org.odk.collect.android.formmanagement.matchexactly.ServerFormsSynchronizer
import org.odk.collect.android.formmanagement.metadata.FormMetadataParser
import org.odk.collect.android.notifications.Notifier
import org.odk.collect.android.projects.ProjectDependencyModule
import org.odk.collect.android.state.DataKeys
Expand Down Expand Up @@ -213,7 +214,7 @@ private fun formDownloader(
projectDependencyModule.formsRepository,
File(projectDependencyModule.cacheDir),
projectDependencyModule.formsDir,
FormMetadataParser(),
FormMetadataParser,
clock,
projectDependencyModule.entitiesRepository
)
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,8 @@ package org.odk.collect.android.formmanagement
import android.database.SQLException
import org.javarosa.xform.parse.XFormParser
import org.odk.collect.android.application.Collect
import org.odk.collect.android.formmanagement.metadata.FormMetadata
import org.odk.collect.android.formmanagement.metadata.FormMetadataParser
import org.odk.collect.android.utilities.FileUtils
import org.odk.collect.androidshared.utils.Validator
import org.odk.collect.forms.Form
Expand Down Expand Up @@ -213,9 +215,8 @@ object LocalFormUseCases {
// Probably someone overwrite the file on the sdcard
// So re-parse it and update it's information
val builder = Form.Builder()
val fields: HashMap<String, String>
fields = try {
FileUtils.getMetadataFromFormDefinition(formDefFile)
val formMetadata: FormMetadata = try {
FormMetadataParser.readMetadata(formDefFile!!)
} catch (e: RuntimeException) {
throw IllegalArgumentException(formDefFile!!.name + " :: " + e.toString())
} catch (e: XFormParser.ParseException) {
Expand All @@ -225,7 +226,7 @@ object LocalFormUseCases {
// update date
val now = System.currentTimeMillis()
builder.date(now)
val title = fields[FileUtils.TITLE]
val title = formMetadata.title
if (title != null) {
builder.displayName(title)
} else {
Expand All @@ -238,7 +239,7 @@ object LocalFormUseCases {
)
)
}
val formid = fields[FileUtils.FORMID]
val formid = formMetadata.id
if (formid != null) {
builder.formId(formid)
} else {
Expand All @@ -251,11 +252,11 @@ object LocalFormUseCases {
)
)
}
val version = fields[FileUtils.VERSION]
val version = formMetadata.version
if (version != null) {
builder.version(version)
}
val submission = fields[FileUtils.SUBMISSIONURI]
val submission = formMetadata.submissionUri
if (submission != null) {
if (Validator.isUrlValid(submission)) {
builder.submissionUri(submission)
Expand All @@ -269,13 +270,13 @@ object LocalFormUseCases {
)
}
}
val base64RsaPublicKey = fields[FileUtils.BASE64_RSA_PUBLIC_KEY]
val base64RsaPublicKey = formMetadata.base64RsaPublicKey
if (base64RsaPublicKey != null) {
builder.base64RSAPublicKey(base64RsaPublicKey)
}
builder.autoDelete(fields[FileUtils.AUTO_DELETE])
builder.autoSend(fields[FileUtils.AUTO_SEND])
builder.geometryXpath(fields[FileUtils.GEOMETRY_XPATH])
builder.autoDelete(formMetadata.autoDelete)
builder.autoSend(formMetadata.autoSend)
builder.geometryXpath(formMetadata.geometryXPath)

// Note, the path doesn't change here, but it needs to be included so the
// update will automatically update the .md5 and the cache path.
Expand Down
Loading