diff --git a/collect_app/build.gradle b/collect_app/build.gradle index 1a05682f0e4..98e781c2c00 100644 --- a/collect_app/build.gradle +++ b/collect_app/build.gradle @@ -1,6 +1,7 @@ apply plugin: 'com.android.application' apply from: '../config/quality.gradle' apply plugin: 'kotlin-android' +apply plugin: 'kotlin-parcelize' apply plugin: "com.google.android.gms.oss-licenses-plugin" import com.android.ddmlib.DdmPreferences diff --git a/collect_app/src/androidTest/java/org/odk/collect/android/feature/formentry/CatchFormDesignExceptionsTest.kt b/collect_app/src/androidTest/java/org/odk/collect/android/feature/formentry/CatchFormDesignExceptionsTest.kt index d231237e546..eb9769df038 100644 --- a/collect_app/src/androidTest/java/org/odk/collect/android/feature/formentry/CatchFormDesignExceptionsTest.kt +++ b/collect_app/src/androidTest/java/org/odk/collect/android/feature/formentry/CatchFormDesignExceptionsTest.kt @@ -5,8 +5,9 @@ import org.junit.Rule import org.junit.Test import org.junit.rules.RuleChain import org.junit.runner.RunWith -import org.odk.collect.android.R +import org.odk.collect.android.support.pages.FormEntryPage import org.odk.collect.android.support.pages.MainMenuPage +import org.odk.collect.android.support.pages.OkDialog import org.odk.collect.android.support.rules.CollectTestRule import org.odk.collect.android.support.rules.TestRuleChain @@ -19,7 +20,7 @@ class CatchFormDesignExceptionsTest { val ruleChain: RuleChain = TestRuleChain.chain().around(rule) @Test // https://github.com/getodk/collect/issues/4750 - fun whenFormHasDesignErrors_explanationDialogShouldBeDisplayedAndTheFormShouldBeClosed() { + fun whenFormHasFatalErrors_explanationDialogShouldBeDisplayedAndSurviveActivityRecreationAndTheFormShouldBeClosedAfterClickingOK() { rule.startAtMainMenu() .copyForm("form_design_error.xml") .clickFillBlankForm() @@ -32,7 +33,29 @@ class CatchFormDesignExceptionsTest { // value of a non-relevant node throws an exception. .answerQuestion(2, "D") .assertTextInDialog(org.odk.collect.strings.R.string.update_widgets_error) + .rotateToLandscape(OkDialog()) + .assertTextInDialog(org.odk.collect.strings.R.string.update_widgets_error) .clickOKOnDialog() .assertOnPage(MainMenuPage()) } + + @Test + fun whenFormHasNonFatalErrors_explanationDialogShouldBeDisplayedAndTheFormShouldNotBeClosedAfterClickingOK() { + rule.startAtMainMenu() + .copyForm("g6Error.xml") + .startBlankFormWithError("g6Error") + .assertText(org.odk.collect.strings.R.string.error_occured) + .clickOK(FormEntryPage("g6Error")) + } + + @Test + fun whenFormHasNonFatalErrors_explanationDialogShouldNotSurviveActivityRecreation() { + rule.startAtMainMenu() + .copyForm("g6Error.xml") + .startBlankFormWithError("g6Error") + .assertText(org.odk.collect.strings.R.string.error_occured) + .clickOK(FormEntryPage("g6Error")) + .rotateToLandscape(FormEntryPage("g6Error")) + .assertTextDoesNotExist(org.odk.collect.strings.R.string.error_occured) + } } diff --git a/collect_app/src/main/java/org/odk/collect/android/activities/FormFillingActivity.java b/collect_app/src/main/java/org/odk/collect/android/activities/FormFillingActivity.java index 3e5eccd2932..25cdea9aa47 100644 --- a/collect_app/src/main/java/org/odk/collect/android/activities/FormFillingActivity.java +++ b/collect_app/src/main/java/org/odk/collect/android/activities/FormFillingActivity.java @@ -103,6 +103,7 @@ import org.odk.collect.android.formentry.FormEndViewModel; import org.odk.collect.android.formentry.FormEntryMenuDelegate; import org.odk.collect.android.formentry.FormEntryViewModel; +import org.odk.collect.android.formentry.FormError; import org.odk.collect.android.formentry.FormIndexAnimationHandler; import org.odk.collect.android.formentry.FormIndexAnimationHandler.Direction; import org.odk.collect.android.formentry.FormLoadingDialogFragment; @@ -273,7 +274,7 @@ public class FormFillingActivity extends LocalizedActivity implements AnimationL private SwipeHandler.View currentView; private AlertDialog alertDialog; - private String errorMessage; + private FormError formError; private boolean shownAlertDialogIsGroupRepeat; private FormLoaderTask formLoaderTask; @@ -459,7 +460,7 @@ public void onCreate(Bundle savedInstanceState) { } swipeHandler = new SwipeHandler(this, settingsProvider.getUnprotectedSettings()); - errorMessage = null; + formError = null; questionHolder = findViewById(R.id.questionholder); @@ -540,8 +541,8 @@ private void setupViewModels(FormEntryViewModelFactory formEntryViewModelFactory formEntryViewModel.setAnswerListener(this::onAnswer); formEntryViewModel.getError().observe(this, error -> { - if (error instanceof FormEntryViewModel.NonFatal) { - createErrorDialog(((FormEntryViewModel.NonFatal) error).getMessage(), false); + if (error instanceof FormError.NonFatal) { + createErrorDialog(error); formEntryViewModel.errorDisplayed(); } }); @@ -555,7 +556,7 @@ private void setupViewModels(FormEntryViewModelFactory formEntryViewModelFactory getCurrentViewIfODKView().highlightWidget(failedValidationResult.getIndex()); } } catch (RepeatsInFieldListException e) { - createErrorDialog(e.getMessage(), false); + createErrorDialog(new FormError.NonFatal(e.getMessage())); } swipeHandler.setBeenSwiped(false); @@ -636,7 +637,7 @@ private void setupFields(Bundle savedInstanceState) { newForm = savedInstanceState.getBoolean(NEWFORM, true); } if (savedInstanceState.containsKey(KEY_ERROR)) { - errorMessage = savedInstanceState.getString(KEY_ERROR); + formError = savedInstanceState.getParcelable(KEY_ERROR); } if (savedInstanceState.containsKey(KEY_AUTO_SAVED)) { autoSaved = savedInstanceState.getBoolean(KEY_AUTO_SAVED); @@ -653,8 +654,8 @@ private void loadForm() { // If a parse error message is showing then nothing else is loaded // Dialogs mid form just disappear on rotation. - if (errorMessage != null) { - createErrorDialog(errorMessage, true); + if (formError instanceof FormError.Fatal) { + createErrorDialog(formError); return; } @@ -836,7 +837,7 @@ protected void onSaveInstanceState(Bundle outState) { nonblockingCreateSavePointData(); } outState.putBoolean(NEWFORM, false); - outState.putString(KEY_ERROR, errorMessage); + outState.putParcelable(KEY_ERROR, formError); outState.putBoolean(KEY_AUTO_SAVED, autoSaved); outState.putBoolean(KEY_LOCATION_PERMISSIONS_GRANTED, locationPermissionsPreviouslyGranted); @@ -904,7 +905,7 @@ protected void onActivityResult(int requestCode, int resultCode, final Intent in } } catch (JavaRosaException e) { Timber.e(e); - createErrorDialog(e.getCause().getMessage(), false); + createErrorDialog(new FormError.NonFatal(e.getCause().getMessage())); } break; case RequestCodes.DRAW_IMAGE: @@ -1179,11 +1180,10 @@ private SwipeHandler.View createView(int event, boolean advancingPage) { // this is badness to avoid a crash. try { event = formController.stepToNextScreenEvent(); - createErrorDialog(e.getMessage(), false); + createErrorDialog(new FormError.NonFatal(e.getMessage())); } catch (JavaRosaException e1) { Timber.d(e1); - createErrorDialog(e.getMessage() + "\n\n" + e1.getCause().getMessage(), - false); + createErrorDialog(new FormError.NonFatal(e.getMessage() + "\n\n" + e1.getCause().getMessage())); } return createView(event, advancingPage); } @@ -1203,10 +1203,10 @@ private SwipeHandler.View createView(int event, boolean advancingPage) { // this is badness to avoid a crash. try { event = formController.stepToNextScreenEvent(); - createErrorDialog(getString(org.odk.collect.strings.R.string.survey_internal_error), true); + createErrorDialog(new FormError.Fatal(getString(org.odk.collect.strings.R.string.survey_internal_error))); } catch (JavaRosaException e) { Timber.d(e); - createErrorDialog(e.getCause().getMessage(), true); + createErrorDialog(new FormError.Fatal(e.getCause().getMessage())); } return createView(event, advancingPage); } @@ -1254,9 +1254,9 @@ private SwipeHandler.View createViewForFormBeginning(FormController formControll } catch (JavaRosaException e) { Timber.d(e); if (e.getMessage().equals(e.getCause().getMessage())) { - createErrorDialog(e.getMessage(), false); + createErrorDialog(new FormError.NonFatal(e.getMessage())); } else { - createErrorDialog(e.getMessage() + "\n\n" + e.getCause().getMessage(), false); + createErrorDialog(new FormError.NonFatal(e.getMessage() + "\n\n" + e.getCause().getMessage())); } } @@ -1525,7 +1525,7 @@ public void showView(SwipeHandler.View next, FormAnimationType from) { } } } catch (RepeatsInFieldListException e) { - createErrorDialog(e.getMessage(), false); + createErrorDialog(new FormError.NonFatal(e.getMessage())); } } } @@ -1628,24 +1628,19 @@ public void run() { /** * Creates and displays dialog with the given errorMsg. */ - private void createErrorDialog(String errorMsg, final boolean shouldExit) { - if (alertDialog != null && alertDialog.isShowing()) { - errorMsg = errorMessage + "\n\n" + errorMsg; - errorMessage = errorMsg; - } else { - alertDialog = new MaterialAlertDialogBuilder(this).create(); - errorMessage = errorMsg; - } + private void createErrorDialog(FormError error) { + formError = error; + alertDialog = new MaterialAlertDialogBuilder(this).create(); alertDialog.setTitle(getString(org.odk.collect.strings.R.string.error_occured)); - alertDialog.setMessage(errorMsg); + alertDialog.setMessage(formError.getMessage()); DialogInterface.OnClickListener errorListener = new DialogInterface.OnClickListener() { @Override public void onClick(DialogInterface dialog, int i) { switch (i) { case BUTTON_POSITIVE: - if (shouldExit) { - errorMessage = null; + if (formError instanceof FormError.Fatal) { + formError = null; exit(); } break; @@ -1900,9 +1895,9 @@ protected void onResume() { updateNavigationButtonVisibility(); } - if (errorMessage != null) { + if (formError instanceof FormError.Fatal) { if (alertDialog != null && !alertDialog.isShowing()) { - createErrorDialog(errorMessage, true); + createErrorDialog(formError); } else { return; } @@ -2220,9 +2215,9 @@ private void showFormLoadErrorAndExit(String errorMsg) { DialogFragmentUtils.dismissDialog(FormLoadingDialogFragment.class, getSupportFragmentManager()); if (errorMsg != null) { - createErrorDialog(errorMsg, true); + createErrorDialog(new FormError.Fatal(errorMsg)); } else { - createErrorDialog(getString(org.odk.collect.strings.R.string.parse_error), true); + createErrorDialog(new FormError.Fatal(getString(org.odk.collect.strings.R.string.parse_error))); } } @@ -2471,10 +2466,10 @@ public void onLayoutChange(View v, int left, int top, int right, int bottom, int } }); } catch (RepeatsInFieldListException e) { - createErrorDialog(e.getMessage(), false); + createErrorDialog(new FormError.NonFatal(e.getMessage())); } catch (Exception | Error e) { Timber.e(e); - createErrorDialog(getString(org.odk.collect.strings.R.string.update_widgets_error), true); + createErrorDialog(new FormError.Fatal(getString(org.odk.collect.strings.R.string.update_widgets_error))); } } }); diff --git a/collect_app/src/main/java/org/odk/collect/android/formentry/FormEntryViewModel.java b/collect_app/src/main/java/org/odk/collect/android/formentry/FormEntryViewModel.java index fbbc59daf6a..202e4c8e14c 100644 --- a/collect_app/src/main/java/org/odk/collect/android/formentry/FormEntryViewModel.java +++ b/collect_app/src/main/java/org/odk/collect/android/formentry/FormEntryViewModel.java @@ -33,7 +33,6 @@ import java.io.FileNotFoundException; import java.util.HashMap; import java.util.List; -import java.util.Objects; import java.util.function.Supplier; public class FormEntryViewModel extends ViewModel implements SelectChoiceLoader { @@ -129,14 +128,14 @@ public void addRepeat() { try { formController.newRepeat(); } catch (RuntimeException e) { - error.setValue(new NonFatal(e.getCause() != null ? e.getCause().getMessage() : e.getMessage())); + error.setValue(new FormError.NonFatal(e.getCause() != null ? e.getCause().getMessage() : e.getMessage())); } if (!formController.indexIsInFieldList()) { try { formController.stepToNextScreenEvent(); } catch (JavaRosaException e) { - error.setValue(new NonFatal(e.getCause() != null ? e.getCause().getMessage() : e.getMessage())); + error.setValue(new FormError.NonFatal(e.getCause() != null ? e.getCause().getMessage() : e.getMessage())); } } @@ -155,7 +154,7 @@ public void cancelRepeatPrompt() { try { this.formController.stepToNextScreenEvent(); } catch (JavaRosaException exception) { - error.setValue(new NonFatal(exception.getCause().getMessage())); + error.setValue(new FormError.NonFatal(exception.getCause().getMessage())); } } @@ -192,7 +191,7 @@ public void moveForward(HashMap answers, Boolean evaluat try { formController.stepToNextScreenEvent(); } catch (JavaRosaException e) { - error.setValue(new NonFatal(e.getCause().getMessage())); + error.setValue(new FormError.NonFatal(e.getCause().getMessage())); } formController.getAuditEventLogger().flush(); // Close events waiting for an end time @@ -213,7 +212,7 @@ public void moveBackward(HashMap answers) { try { formController.stepToPreviousScreenEvent(); } catch (JavaRosaException e) { - error.setValue(new NonFatal(e.getCause().getMessage())); + error.setValue(new FormError.NonFatal(e.getCause().getMessage())); return; } @@ -242,7 +241,7 @@ public boolean saveScreenAnswersToFormController(HashMap return false; } } catch (JavaRosaException e) { - error.postValue(new NonFatal(e.getMessage())); + error.postValue(new FormError.NonFatal(e.getMessage())); return false; } @@ -299,7 +298,7 @@ public void validate() { try { result = formController.validateAnswers(true); } catch (JavaRosaException e) { - error.setValue(new NonFatal(e.getMessage())); + error.setValue(new FormError.NonFatal(e.getMessage())); } return result; @@ -314,42 +313,6 @@ public void validate() { ); } - public abstract static class FormError { - - } - - public static class NonFatal extends FormError { - - private final String message; - - public NonFatal(String message) { - this.message = message; - } - - public String getMessage() { - return message; - } - - @Override - public boolean equals(Object o) { - if (this == o) { - return true; - } - - if (o == null || getClass() != o.getClass()) { - return false; - } - - NonFatal nonFatal = (NonFatal) o; - return Objects.equals(message, nonFatal.message); - } - - @Override - public int hashCode() { - return Objects.hash(message); - } - } - public interface AnswerListener { void onAnswer(FormIndex index, IAnswerData answer); } diff --git a/collect_app/src/main/java/org/odk/collect/android/formentry/FormError.kt b/collect_app/src/main/java/org/odk/collect/android/formentry/FormError.kt new file mode 100644 index 00000000000..83ac738ae59 --- /dev/null +++ b/collect_app/src/main/java/org/odk/collect/android/formentry/FormError.kt @@ -0,0 +1,13 @@ +package org.odk.collect.android.formentry + +import android.os.Parcelable +import kotlinx.android.parcel.Parcelize + +@Parcelize +sealed class FormError : Parcelable { + abstract val message: String? + + data class NonFatal(override val message: String?) : FormError() + + data class Fatal(override val message: String?) : FormError() +} diff --git a/collect_app/src/test/java/org/odk/collect/android/formentry/FormEntryViewModelTest.java b/collect_app/src/test/java/org/odk/collect/android/formentry/FormEntryViewModelTest.java index 813e523d0c3..4a81304e2d0 100644 --- a/collect_app/src/test/java/org/odk/collect/android/formentry/FormEntryViewModelTest.java +++ b/collect_app/src/test/java/org/odk/collect/android/formentry/FormEntryViewModelTest.java @@ -12,7 +12,6 @@ import static org.mockito.Mockito.never; import static org.mockito.Mockito.verify; import static org.mockito.Mockito.when; -import static org.odk.collect.android.formentry.FormEntryViewModel.NonFatal; import static org.odk.collect.androidtest.LiveDataTestUtilsKt.getOrAwaitValue; import androidx.arch.core.executor.testing.InstantTaskExecutorRule; @@ -83,7 +82,7 @@ public void addRepeat_whenThereIsAnErrorCreatingRepeat_setsErrorWithMessage() { doThrow(new RuntimeException(new IOException("OH NO"))).when(formController).newRepeat(); viewModel.addRepeat(); - assertThat(viewModel.getError().getValue(), equalTo(new NonFatal("OH NO"))); + assertThat(viewModel.getError().getValue(), equalTo(new FormError.NonFatal("OH NO"))); } @Test @@ -95,7 +94,7 @@ public void addRepeat_whenThereIsAnErrorCreatingRepeat_setsErrorWithoutCause() { doThrow(runtimeException).when(formController).newRepeat(); viewModel.addRepeat(); - assertThat(viewModel.getError().getValue(), equalTo(new NonFatal("Unknown issue occurred while adding a new group"))); + assertThat(viewModel.getError().getValue(), equalTo(new FormError.NonFatal("Unknown issue occurred while adding a new group"))); } @Test @@ -103,7 +102,7 @@ public void addRepeat_whenThereIsAnErrorSteppingToNextScreen_setsErrorWithMessag when(formController.stepToNextScreenEvent()).thenThrow(new JavaRosaException(new IOException("OH NO"))); viewModel.addRepeat(); - assertThat(viewModel.getError().getValue(), equalTo(new NonFatal("OH NO"))); + assertThat(viewModel.getError().getValue(), equalTo(new FormError.NonFatal("OH NO"))); } @Test @@ -115,7 +114,7 @@ public void addRepeat_whenThereIsAnErrorSteppingToNextScreen_setsErrorWithoutCau when(formController.stepToNextScreenEvent()).thenThrow(javaRosaException); viewModel.addRepeat(); - assertThat(viewModel.getError().getValue(), equalTo(new NonFatal("Unknown issue occurred while adding a new group"))); + assertThat(viewModel.getError().getValue(), equalTo(new FormError.NonFatal("Unknown issue occurred while adding a new group"))); } @Test @@ -142,7 +141,7 @@ public void cancelRepeatPrompt_whenThereIsAnErrorSteppingToNextScreen_setsErrorW when(formController.stepToNextScreenEvent()).thenThrow(new JavaRosaException(new IOException("OH NO"))); viewModel.cancelRepeatPrompt(); - assertThat(viewModel.getError().getValue(), equalTo(new NonFatal("OH NO"))); + assertThat(viewModel.getError().getValue(), equalTo(new FormError.NonFatal("OH NO"))); } @Test @@ -223,7 +222,7 @@ public void moveForward_whenThereIsAnErrorSteppingToNextEvent_setErrorWithMessag viewModel.moveForward(new HashMap<>()); scheduler.runBackground(); - assertThat(viewModel.getError().getValue(), equalTo(new NonFatal("OH NO"))); + assertThat(viewModel.getError().getValue(), equalTo(new FormError.NonFatal("OH NO"))); } @Test @@ -269,7 +268,7 @@ public void moveForward_whenThereIsAnErrorSaving_setsErrorWithMessage() throws E viewModel.moveForward(new HashMap<>()); scheduler.runBackground(); - assertThat(viewModel.getError().getValue(), equalTo(new NonFatal("OH NO"))); + assertThat(viewModel.getError().getValue(), equalTo(new FormError.NonFatal("OH NO"))); } @Test @@ -337,7 +336,7 @@ public void moveBackward_whenThereIsAnErrorSteppingToPreviousEvent_setErrorWithM viewModel.moveBackward(new HashMap<>()); scheduler.runBackground(); - assertThat(viewModel.getError().getValue(), equalTo(new NonFatal("OH NO"))); + assertThat(viewModel.getError().getValue(), equalTo(new FormError.NonFatal("OH NO"))); } @Test @@ -347,7 +346,7 @@ public void moveBackward_whenThereIsAnErrorSaving_setsErrorWithMessage() throws viewModel.moveBackward(new HashMap<>()); scheduler.runBackground(); - assertThat(viewModel.getError().getValue(), equalTo(new NonFatal("OH NO"))); + assertThat(viewModel.getError().getValue(), equalTo(new FormError.NonFatal("OH NO"))); } @Test @@ -388,6 +387,6 @@ public void validate_whenThereIsAnErrorValidating_setsError() throws Exception { viewModel.validate(); scheduler.runBackground(); - assertThat(viewModel.getError().getValue(), equalTo(new NonFatal("OH NO"))); + assertThat(viewModel.getError().getValue(), equalTo(new FormError.NonFatal("OH NO"))); } }