End of Form Error Handling #1560

Jtang-1 · 2024-03-22T05:34:48Z

Product Description

This PR does two things:

Displays errors while rebuilding session from frame that were previously caught but not displayed. Namely, errors resulting from end of form navigation but that is not the only feature where rebuilding session is done.
Adds error handling for when no matches are found between steps and menu options or steps and datum ID. It also handles if the needed step value is not available in the nodeset.

Technical Summary

Jira Ticket

Safety Assurance

Safety story

tested locally, will test on staging

Automated test coverage

Basic end of form navigation tests exists and the response is as expected

QA Plan

no QA

Special deploy instructions

This PR can be deployed after merge with no further considerations.

Rollback instructions

This PR can be reverted after deploy with no further considerations.

Review

The set of people pinged as reviewers is appropriate for the level of risk of the change.

… of menus

…d is handled by callers

…ready been processed

Jtang-1 · 2024-03-22T08:02:52Z

@snopoke There's an edge case causing test failures and I can't figure out how to best handle it. Could I get your thoughts? From what I understand, the situation is:
Steps [m1, case_id, case_id_new_child_case_0]. We just processed case_id (via EntityScreen) and move to the next screen which is a MenuScreen['Case Claim' with choices [Entry with id m1-f0, Menu with id m2]].

case_id_new_child_case_0 is a datum for a new case id (<datum id="case_id_new_child_case_0" function="uuid()"/>). The step is (CASE_ID case_id_new_child_case_0 : 81e0aa64-7aea-4134-b000-43f04fd80f2d) (where the case_id is a generated uuid). The logic before this PR is that this step is ignored (if this loop has no matches, currentStep stays null and we break out of the while loop.)

But with this PR, the non-match throws an exception. A solution for this is: if we encounter a match "miss", we check if the remaining steps are datums for new case ids. If true, we don't throw an exception. However, I don't see a way to know if the step is associated with creating a new case_id.

But similarly, if the last step is (SELECTED_ENTITIES selected_cases : ...), it causes the same issue. So maybe an additional condition before throwing the error should be that the step we're processing is a COMMAND_ID.

snopoke · 2024-03-22T11:08:54Z

I did some debugging and it took a while to figure out what was going on but I think your suggestion to check that the current step we're processing is a COMMAND_ID makes sense.

This might be a challenge to implement without completely refactoring that method since we don't actually track which step is the current one. Personally I've wanted to refactor that function many times but never managed to push it all the way through.

MartinRiese · 2024-03-22T16:22:06Z

src/main/java/org/commcare/formplayer/services/MenuSessionFactory.java

@@ -106,6 +107,17 @@ public void rebuildSessionFromFrame(MenuSession menuSession, CaseSearchHelper ca
                        }
                    }
                }
+                if (currentStep == null && processedStepsCount != steps.size()) {
+                    StringJoiner optionsIDJoiner = new StringJoiner(", ", "[", "]");


I cannot believe StringJoiner exists since 1.8 and I never heard of it. Pretty handy.

MartinRiese · 2024-03-22T16:23:16Z

src/main/java/org/commcare/formplayer/services/MenuSessionFactory.java

@@ -106,6 +107,17 @@ public void rebuildSessionFromFrame(MenuSession menuSession, CaseSearchHelper ca
                        }
                    }
                }
+                if (currentStep == null && processedStepsCount != steps.size()) {


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

…creen

… valid option In the steps, there can be situations such as case id generation datum where it is not intended to be processed. This leads to a non-match between the step and available option. In this case, the exception should not be thrown.

…ndled here "claim_command..." commands are not processed here so these not being matched should not throw an error.

codecov · 2024-03-23T03:26:04Z

Codecov Report

Attention: Patch coverage is 70.58824% with 15 lines in your changes are missing coverage. Please review.

Project coverage is 70.15%. Comparing base (852284c) to head (6754f61).
Report is 2 commits behind head on master.

Files	Patch %	Lines
...mmcare/formplayer/services/MenuSessionFactory.java	70.58%	14 Missing and 1 partial ⚠️

Additional details and impacted files

@@             Coverage Diff              @@
##             master    #1560      +/-   ##
============================================
- Coverage     70.15%   70.15%   -0.01%     
- Complexity     1993     2009      +16     
============================================
  Files           252      252              
  Lines          7781     7833      +52     
  Branches        699      713      +14     
============================================
+ Hits           5459     5495      +36     
- Misses         2054     2069      +15     
- Partials        268      269       +1

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Jtang-1 · 2024-03-23T03:33:19Z

Thank you for taking the time to look into this for me @snopoke. I ended up tracking what steps were processed, and checking that the remaining unprocessed steps contained a COMMAND_ID type (minus the special value claim_command...) that was not matched to an available option. This seems to be more conservative to reduce chances of falsely throwing the exception.
From testing, I found tracking the step we're trying to process is tricky (as you mentioned). It seems like there's steps that we just skip and don't handle within rebuildSessionFromFrame, such as the claim_command... and the "new case id` datums. Unfortunately, I'll also be passing this along and not refactor it at this moment.

snopoke · 2024-03-23T07:12:27Z

src/main/java/org/commcare/formplayer/services/MenuSessionFactory.java

@@ -101,11 +106,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);


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.

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.

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.

…step value

…D datum

Jtang-1 · 2024-04-04T19:41:58Z

Bumping for review

snopoke

Nothing blocking.

snopoke · 2024-04-05T07:34:49Z

src/main/java/org/commcare/formplayer/services/MenuSessionFactory.java

@@ -101,11 +106,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);


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.

src/main/java/org/commcare/formplayer/services/MenuSessionFactory.java

Jtang-1 · 2024-04-08T05:01:32Z

@snopoke, I couldn't reply to that comment thread

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.

I'm not entirely sure if it's possible either but I see needsFullInit = ++processedStepsCount == steps.size(); is within the for loop so I kept it within the same level to be on the safe side.

Jtang-1 added 2 commits March 21, 2024 21:54

feat: handle misses where next step doesn't exist in available option…

6988241

… of menus

bug: propogate checked exceptions such that the message isn't lost an…

7f49b49

…d is handled by callers

Jtang-1 requested review from snopoke, MartinRiese and shubham1g5 March 22, 2024 05:34

Jtang-1 marked this pull request as ready for review March 22, 2024 05:34

snopoke approved these changes Mar 22, 2024

View reviewed changes

bug: handle edge case where step is empty or all items in step has al…

767f3be

…ready been processed

MartinRiese reviewed Mar 22, 2024

View reviewed changes

Jtang-1 added 4 commits March 22, 2024 13:50

adds count to processedStepsCount when step is processed via a QueryS…

4ef0364

…creen

bug: COMMAND_IDs for "claim_command..." ids are not expected to be ha…

d60c1a0

…ndled here "claim_command..." commands are not processed here so these not being matched should not throw an error.

refactor: extract check for valid command_id in options

bf6e37e

snopoke reviewed Mar 23, 2024

View reviewed changes

extract this change for a separate PR as more scrutiny is needed

8c1f72b

Jtang-1 requested a review from snopoke March 26, 2024 23:52

Jtang-1 added 3 commits April 1, 2024 16:53

catch scenario where entityScreen reference map does not contain the …

2d7f8b6

…step value

feat: catches error scenario where steps do not contain a valid CaseI…

8268223

…D datum

constructs more helpful error message

9d7c88a

snopoke approved these changes Apr 5, 2024

View reviewed changes

Jtang-1 added 2 commits April 8, 2024 00:48

style remove unneeded syntax

8b47055

extracts creation of exception message into separate function

6754f61

snopoke approved these changes Apr 8, 2024

View reviewed changes

Jtang-1 merged commit a2b00e1 into master Apr 10, 2024
5 of 6 checks passed

Jtang-1 deleted the jt/EoF-error-handling branch April 10, 2024 04:36

This was referenced Apr 26, 2024

End of Form Error Handling #1576

Merged

Log instead of blocking app for EoF errors #1578

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

End of Form Error Handling #1560

End of Form Error Handling #1560

Jtang-1 commented Mar 22, 2024 •

edited

Loading

Jtang-1 commented Mar 22, 2024

snopoke commented Mar 22, 2024

MartinRiese Mar 22, 2024

MartinRiese Mar 22, 2024

Jtang-1 Mar 23, 2024

codecov bot commented Mar 23, 2024 •

edited

Loading

Jtang-1 commented Mar 23, 2024

snopoke Mar 23, 2024

Jtang-1 Mar 24, 2024

snopoke Apr 5, 2024

Jtang-1 commented Apr 4, 2024

snopoke left a comment

snopoke Apr 5, 2024

Jtang-1 commented Apr 8, 2024

End of Form Error Handling #1560

End of Form Error Handling #1560

Conversation

Jtang-1 commented Mar 22, 2024 • edited Loading

Product Description

Technical Summary

Safety Assurance

Safety story

Automated test coverage

QA Plan

Special deploy instructions

Rollback instructions

Review

Jtang-1 commented Mar 22, 2024

snopoke commented Mar 22, 2024

MartinRiese Mar 22, 2024

Choose a reason for hiding this comment

MartinRiese Mar 22, 2024

Choose a reason for hiding this comment

Jtang-1 Mar 23, 2024

Choose a reason for hiding this comment

codecov bot commented Mar 23, 2024 • edited Loading

Codecov Report

Jtang-1 commented Mar 23, 2024

snopoke Mar 23, 2024

Choose a reason for hiding this comment

Jtang-1 Mar 24, 2024

Choose a reason for hiding this comment

snopoke Apr 5, 2024

Choose a reason for hiding this comment

Jtang-1 commented Apr 4, 2024

snopoke left a comment

Choose a reason for hiding this comment

snopoke Apr 5, 2024

Choose a reason for hiding this comment

Jtang-1 commented Apr 8, 2024

Jtang-1 commented Mar 22, 2024 •

edited

Loading

codecov bot commented Mar 23, 2024 •

edited

Loading