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

Fix: Prevent clear of other questions when question name equals other value name #7517

Merged
merged 3 commits into from
Dec 18, 2023
Merged

Conversation

marvin-wtt
Copy link
Contributor

@marvin-wtt marvin-wtt commented Dec 18, 2023

By default, the values of all invisible questions are cleared on complete to exclude the data from the result. As multiple questions may have the same value name, it is checked wherever there is another question with the same value name is visible before clearing the value.
However, this check does not take into account, that the question name is the default value name when no value name is provided. This causes all questions with the same value name as the question name to be cleared.

This PR suggests using getValueName instead, which falls back to the question name if no value name is defined.

Example Survey
{
 "pages": [
  {
   "name": "page1",
   "elements": [
    {
     "type": "boolean",
     "name": "toggle",
     "title": "Toggle",
     "labelTrue": "Bug",
     "labelFalse": "It works"
    },
    {
     "type": "text",
     "name": "username",
     "visibleIf": "{toggle} = false",
     "title": "Username",
     "maxLength": 25
    },
    {
     "type": "text",
     "name": "alternative_username",
     "visibleIf": "{toggle} = true",
     "title": "Alternative Username",
     "valueName": "username"
    }
   ]
  }
 ],
 "showQuestionNumbers": "off"
}

@andrewtelnov
Copy link
Member

@marvin-wtt We need a unit test for this case to make sure we don't have the regression on a code moification. We don't accept PR without a unit test or functional test or visual regression test.
I can add it if you can't add it.

Thank you,
Andrew

@marvin-wtt
Copy link
Contributor Author

@andrewtelnov I can try to add a test. I noticed that your other tests usually link to an issue describing the problem. Should I create an issue for this too?

@andrewtelnov
Copy link
Member

@marvin-wtt Adding a unit test without creating issue is fine.

Thank you,
Andrew

@andrewtelnov andrewtelnov merged commit fb37e49 into surveyjs:master Dec 18, 2023
18 checks 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