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.

Sign up for GitHub

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

Form error improvements #5560

Merged
merged 6 commits into from
Jul 12, 2023
Merged

Form error improvements #5560

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
a2bb427
Factored out FormError class
grzesiek2010 Apr 24, 2023
6c39e17
Use FormError to display error dialog
grzesiek2010 May 5, 2023
566079f
Resrtore the error dialog only if it's a fatal error
grzesiek2010 May 5, 2023
215f2de
Added tests
grzesiek2010 May 5, 2023
1afd28c
Make message in FormError nullable
grzesiek2010 May 5, 2023
7e88617
Fixed error after resolving conflicts
grzesiek2010 Jul 7, 2023
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
1 change: 1 addition & 0 deletions collect_app/build.gradle
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -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
Expand Down
27 changes: 25 additions & 2 deletions ...droidTest/java/org/odk/collect/android/feature/formentry/CatchFormDesignExceptionsTest.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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

Expand All @@ -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()
Expand All @@ -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)
}
}
65 changes: 30 additions & 35 deletions collect_app/src/main/java/org/odk/collect/android/activities/FormFillingActivity.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -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;
Expand Down Expand Up @@ -459,7 +460,7 @@ public void onCreate(Bundle savedInstanceState) {
}
swipeHandler = new SwipeHandler(this, settingsProvider.getUnprotectedSettings());

errorMessage = null;
formError = null;

questionHolder = findViewById(R.id.questionholder);

Expand Down Expand Up @@ -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();
}
});
Expand All @@ -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);
Expand Down Expand Up @@ -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);
Expand All @@ -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;
}

Expand Down Expand Up @@ -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);

Expand Down Expand Up @@ -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:
Expand Down Expand Up @@ -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);
}
Expand All @@ -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);
}
Expand Down Expand Up @@ -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()));
}
}

Expand Down Expand Up @@ -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()));
}
}
}
Expand Down Expand Up @@ -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;
Expand Down Expand Up @@ -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;
}
Expand Down Expand Up @@ -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)));
}
}

Expand Down Expand Up @@ -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)));
}
}
});
Expand Down
51 changes: 7 additions & 44 deletions collect_app/src/main/java/org/odk/collect/android/formentry/FormEntryViewModel.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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 {
Expand Down Expand Up @@ -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()));
}
}

Expand All @@ -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()));
}
}

Expand Down Expand Up @@ -192,7 +191,7 @@ public void moveForward(HashMap<FormIndex, IAnswerData> 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
Expand All @@ -213,7 +212,7 @@ public void moveBackward(HashMap<FormIndex, IAnswerData> answers) {
try {
formController.stepToPreviousScreenEvent();
} catch (JavaRosaException e) {
error.setValue(new NonFatal(e.getCause().getMessage()));
error.setValue(new FormError.NonFatal(e.getCause().getMessage()));
return;
}

Expand Down Expand Up @@ -242,7 +241,7 @@ public boolean saveScreenAnswersToFormController(HashMap<FormIndex, IAnswerData>
return false;
}
} catch (JavaRosaException e) {
error.postValue(new NonFatal(e.getMessage()));
error.postValue(new FormError.NonFatal(e.getMessage()));
return false;
}

Expand Down Expand Up @@ -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;
Expand All @@ -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);
}
Expand Down
13 changes: 13 additions & 0 deletions collect_app/src/main/java/org/odk/collect/android/formentry/FormError.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -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()
}
Loading