Skip to content

End of Form Error Handling #1560

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

Merged
merged 13 commits into from
Apr 10, 2024
Merged

End of Form Error Handling #1560

Show file tree
Hide file tree
Changes from 11 commits
Commits
Show all changes
13 commits
Select commit Hold shift + click to select a range
6988241
feat: handle misses where next step doesn't exist in available option…
Jtang-1 Mar 22, 2024
7f49b49
bug: propogate checked exceptions such that the message isn't lost an…
Jtang-1 Mar 22, 2024
767f3be
bug: handle edge case where step is empty or all items in step has al…
Jtang-1 Mar 22, 2024
4ef0364
adds count to processedStepsCount when step is processed via a QueryS…
Jtang-1 Mar 22, 2024
1374573
bug: only throws error if we're looking for a Command_ID that isn't a…
Jtang-1 Mar 22, 2024
d60c1a0
bug: COMMAND_IDs for "claim_command..." ids are not expected to be ha…
Jtang-1 Mar 22, 2024
bf6e37e
refactor: extract check for valid command_id in options
Jtang-1 Mar 23, 2024
8c1f72b
extract this change for a separate PR as more scrutiny is needed
Jtang-1 Mar 24, 2024
2d7f8b6
catch scenario where entityScreen reference map does not contain the …
Jtang-1 Apr 1, 2024
8268223
feat: catches error scenario where steps do not contain a valid CaseI…
Jtang-1 Apr 2, 2024
9d7c88a
constructs more helpful error message
Jtang-1 Apr 2, 2024
8b47055
style remove unneeded syntax
Jtang-1 Apr 8, 2024
6754f61
extracts creation of exception message into separate function
Jtang-1 Apr 8, 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
4 changes: 2 additions & 2 deletions src/main/java/org/commcare/formplayer/application/FormSubmissionHelper.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -212,7 +212,7 @@ private Optional<SubmitResponseBean> executeStep(HttpServletRequest request, Pro
return Optional.of(response);
}

private SubmitResponseBean validateAnswers(FormSubmissionContext context) {
private SubmitResponseBean validateAnswers(FormSubmissionContext context) throws Exception {
Map<String, ErrorBean> errors = categoryTimingHelper.timed(
Constants.TimingCategories.VALIDATE_SUBMISSION,
() -> validateSubmitAnswers(context),
Expand Down Expand Up @@ -348,7 +348,7 @@ private void processXmlInner(FormSubmissionContext context) throws Exception {
}

@Trace
private SubmitResponseBean doEndOfFormNav(FormSubmissionContext context) {
private SubmitResponseBean doEndOfFormNav(FormSubmissionContext context) throws Exception{
Object nextScreen = categoryTimingHelper.timed(
Constants.TimingCategories.END_OF_FORM_NAV,
() -> {
Expand Down
2 changes: 1 addition & 1 deletion src/main/java/org/commcare/formplayer/services/CategoryTimingHelper.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -86,7 +86,7 @@ public <T> T timed(String category, CheckedSupplier<T> timed) {
}

@SneakyThrows
public <T> T timed(String category, CheckedSupplier<T> timed, Map<String, String> extras) {
public <T> T timed(String category, CheckedSupplier<T> timed, Map<String, String> extras) throws Exception{
SimpleTimer timer = new SimpleTimer();
timer.start();
try {
Expand Down
76 changes: 76 additions & 0 deletions src/main/java/org/commcare/formplayer/services/MenuSessionFactory.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,8 @@
import org.commcare.formplayer.objects.SerializableMenuSession;
import org.commcare.formplayer.session.MenuSession;
import org.commcare.session.CommCareSession;
import org.commcare.session.SessionFrame;
import org.commcare.suite.model.EntityDatum;
import org.commcare.suite.model.MenuDisplayable;
import org.commcare.suite.model.RemoteQueryDatum;
import org.commcare.suite.model.SessionDatum;
Expand All @@ -33,8 +35,12 @@
import java.io.IOException;
import java.net.URI;
import java.net.URISyntaxException;
import java.util.ArrayList;
import java.util.Collection;
import java.util.List;
import java.util.StringJoiner;
import java.util.Vector;
import java.util.stream.Collectors;

import datadog.trace.api.Trace;

Expand Down Expand Up @@ -82,6 +88,7 @@ public void rebuildSessionFromFrame(MenuSession menuSession, CaseSearchHelper ca
boolean respectRelevancy)
throws CommCareSessionException, RemoteInstanceFetcher.RemoteInstanceException {
Vector<StackFrameStep> steps = menuSession.getSessionWrapper().getFrame().getSteps();
List<StackFrameStep> processedSteps = new ArrayList<>();
menuSession.resetSession();
EntityScreenContext entityScreenContext = new EntityScreenContext();
Screen screen = menuSession.getNextScreen(false, entityScreenContext);
Expand All @@ -101,11 +108,15 @@ public void rebuildSessionFromFrame(MenuSession menuSession, CaseSearchHelper ca
for (StackFrameStep step : steps) {
if (step.getId().equals(options[i].getCommandID())) {
currentStep = String.valueOf(i);
processedSteps.add(step);
Copy link
Contributor

Choose a reason for hiding this comment

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

I notice this loop has no break statement in it. I think it should since I can't think of a scenario where more than one step would match.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I was thinking that too. I will make that change in this separate PR

Aside note: I was speaking to Shubham about commit 4ef0364 and he suggested doing this in a separate PR with more scrutiny (doing QA). So may as well have this extra check done on the break statement too.

Copy link
Contributor

Choose a reason for hiding this comment

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

In that case, should this line be moved out of the for loop since only the last matching step would be 'processed'. I don't think it should be possible for multiple steps to match but I can't prove that.

// final step, needs to init fully to show to screen
needsFullInit = ++processedStepsCount == steps.size();
}
}
}
if (currentStep == null && processedStepsCount != steps.size()) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Could you push that in its own method? rebuildSessionFromFrame is quite long already

Copy link
Contributor Author

Choose a reason for hiding this comment

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

bf6e37e

checkAndThrowCommandIDMatchError(steps, processedSteps, options);
}
} else if (screen instanceof EntityScreen) {
EntityScreen entityScreen = (EntityScreen)screen;
entityScreen.initReferences(menuSession.getSessionWrapper());
Expand All @@ -114,7 +125,19 @@ public void rebuildSessionFromFrame(MenuSession menuSession, CaseSearchHelper ca
if (step.getId().equals(neededDatum.getDataId())) {
if (entityScreen.referencesContainStep(step.getValue())) {
currentStep = step.getValue();
processedSteps.add(step);
needsFullInit = ++processedStepsCount == steps.size();
} else {
// This block constructs the message to display then throws the exception
List<String> refsList = entityScreen.getReferences().stream()
.map(ref -> EntityScreen.getReturnValueFromSelection(ref, (EntityDatum) neededDatum, entityScreen.getEvalContext()))
.collect(Collectors.toList());

String referencesString = String.join(",\n ", refsList);
String nodeSetString = ((EntityDatum) neededDatum).getNodeset().toString();

throw new CommCareSessionException(String.format("Could not get %s=%s from entity screen.\nNode set: %s\nReferences: \n[%s]",
neededDatum.getDataId(), step.getValue(), nodeSetString, referencesString));
}
break;
}
Expand All @@ -132,6 +155,9 @@ public void rebuildSessionFromFrame(MenuSession menuSession, CaseSearchHelper ca
screen = menuSession.getNextScreen(needsFullInit, entityScreenContext);
continue;
}
if (currentStep == null && processedStepsCount != steps.size()) {
checkAndThrowCaseIDMatchError(steps, processedSteps, neededDatum.getDataId());
}
} else if (screen instanceof QueryScreen) {
QueryScreen queryScreen = (QueryScreen)screen;
RemoteQueryDatum neededDatum = (RemoteQueryDatum) menuSession.getSessionWrapper().getNeededDatum();
Expand All @@ -140,6 +166,7 @@ public void rebuildSessionFromFrame(MenuSession menuSession, CaseSearchHelper ca
URI uri = null;
try {
uri = new URI(step.getValue());
processedSteps.add(step);
} catch (URISyntaxException e) {
e.printStackTrace();
throw new CommCareSessionException("Query URL format error: " + e.getMessage(), e);
Expand Down Expand Up @@ -228,4 +255,53 @@ private MenuSession performInstall(InstallRequestBean bean) throws Exception {
bean.getPreview()
);
}

private void checkAndThrowCommandIDMatchError(Vector<StackFrameStep> steps, List<StackFrameStep> processedSteps,
MenuDisplayable[] options) throws CommCareSessionException {
Vector<StackFrameStep> unprocessedSteps = new Vector<>();
for (StackFrameStep step : steps) {
if (!processedSteps.contains(step)) {
unprocessedSteps.add(step);
}
}
for (StackFrameStep unprocessedStep : unprocessedSteps) {
if (unprocessedStep.getType().equals(SessionFrame.STATE_COMMAND_ID) &&
!unprocessedStep.getId().startsWith("claim_command")) {
StringJoiner optionsIDJoiner = new StringJoiner(", ", "[", "]");
StringJoiner stepIDJoiner = new StringJoiner(", ", "[", "]");
for (MenuDisplayable option : options) {
optionsIDJoiner.add(option.getCommandID());
}
for (StackFrameStep step : steps) {
stepIDJoiner.add(step.getId());
}
throw new CommCareSessionException(
"Match Error: Steps " + stepIDJoiner.toString() +
" do not contain a valid option " + optionsIDJoiner.toString()
);
}
}
}

private void checkAndThrowCaseIDMatchError(Vector<StackFrameStep> steps, List<StackFrameStep> processedSteps,
String neededDatumID) throws CommCareSessionException {
Vector<StackFrameStep> unprocessedSteps = new Vector<>();
for (StackFrameStep step : steps) {
if (!processedSteps.contains(step)) {
unprocessedSteps.add(step);
}
}
for (StackFrameStep unprocessedStep : unprocessedSteps) {
if (unprocessedStep.getType().equals(SessionFrame.STATE_DATUM_VAL)) {
StringJoiner stepIDJoiner = new StringJoiner(", ", "[", "]");
for (StackFrameStep step : steps) {
stepIDJoiner.add(step.getId());
}
throw new CommCareSessionException(
"Match Error: Steps " + stepIDJoiner.toString() +
" do not contain a valid datum ID " + neededDatumID
);
}
}
}
}