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

Make NSAssert simply an if clause to allow mismatched defaultValue #149

Merged
merged 1 commit into from
Sep 11, 2024

Conversation

eschramm
Copy link

@eschramm eschramm commented Sep 10, 2024

Fixes https://github.com/CareEvolution/MyDataHelps-iOS/issues/1314.

While the fix is relatively simple, following the logic upstream to understand risk of regression bugs is not. Regardless, I feel confident this fixes the issue by de-escalating a mismatched defaultValue of an ORK[1]AnswerFormat (which is technically not stored there, but we pluck them out on deserialization) from an NSAssert to a simple check. I ran though the UITests as well which does a battery of tests on basic RK functionality.

Testing

Verify the following survey doesn't crash on iOS. It has a defaultValue ("garbage") which doesn't match any of the TextChoice values.

Also regression testing of survey functionality of iOS would be helpful, in particular the following:

  • surveys with valid defaultValues
  • navigation forward and backward to verify previous answers reappear correctly
  • restoring saved surveys and validating answers restore correctly
{
  "type": "RKStudioOrderedTask",
  "name": "Survey",
  "identifier": "Survey",
  "steps": [
      {
        "identifier": "AFib",
        "type": "QuestionStep",
        "answerFormat": {
          "type": "TextChoiceAnswerFormat",
          "textChoices": [
            {
              "type": "TextChoice",
              "value": "1",
              "text": "Yes"
            },
            {
              "type": "TextChoice",
              "value": "0",
              "text": "No"
            }
          ],
          "style": "Single",
          "customField": "afib",
          "descriptionStyle": "",
          "defaultValue": "garbage"
        },
        "optional": false,
        "text": "Do you have atrial fibrillation (AFib)?"
      }
  ]
}

Copy link
Member

@mmertsock mmertsock left a comment

Choose a reason for hiding this comment

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

LGTM 🚢

Tested with the survey in this PR, and also a more complex survey with additional steps to verify navigation and state restoration.

@mmertsock
Copy link
Member

Also didn't see anything else that we would need to update to go along with this change, and didn't find additional NSAsserts that should be addressed for this particular issue.

@eschramm eschramm merged commit 2993610 into CEVRelease-2.x Sep 11, 2024
1 check passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants