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

Jump to bottom

fix: JSONForm Select field validate empty string as an invalid value #13993

Merged
merged 1 commit into from
May 25, 2022
Merged

fix: JSONForm Select field validate empty string as an invalid value #13993

merged 1 commit into from
May 25, 2022

Conversation

ashit-rath
Copy link
Contributor

@ashit-rath ashit-rath commented May 20, 2022 •
edited by github-actions bot

Description

The Select field only considers null and undefined as invalid values.
But when we consider the select widget, it considers empty string as invalid value along with null and undefined.
This PR aligns the implementation and makes the JSON Form's select field consider and empty value as an invalid value when the Required property is enabled.

Fixes #13981

Type of change

  • Bug fix (non-breaking change which fixes an issue)

How Has This Been Tested?

Jest Test:
The validation function is tested in 3 test cases

  1. When required is enabled and the values are either valid or invalid -> It should return valid
  2. When required is enabled and the values are valid -> It should return valid
  3. When required is enabled and the values are invalid -> It should return invalid

Checklist:

  • My code follows the style guidelines of this project
  • I have performed a self-review of my own code
  • I have commented my code, particularly in hard-to-understand areas
  • I have made corresponding changes to the documentation
  • My changes generate no new warnings
  • I have added tests that prove my fix is effective or that my feature works
  • New and existing unit tests pass locally with my changes

Test coverage results 🧪

🟢 Total coverage has increased 
// Code coverage diff between base branch:release and head branch: fix/13981-jf-select-validation
Status File % Stmts % Branch % Funcs % Lines
🟢 total 56.62 (0) 38.64 (0.01) 35.95 (0.01) 56.87 (0)
🟢 app/client/src/utils/autocomplete/TernServer.ts 52.94 (0.23) 41.67 (0.84) 36.21 (0) 56.99 (0.25)
🟢 app/client/src/widgets/JSONFormWidget/fields/SelectField.tsx 37.7 (2.7) 10.2 (6.03) 14.29 (14.29) 39.29 (1.79)

@vercel
Copy link

vercel bot commented May 20, 2022

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Updated
appsmith ✅ Ready (Inspect) Visit Preview May 20, 2022 at 2:02PM (UTC)

@github-actions github-actions bot added App Viewers Pod This label assigns issues to the app viewers pod Bug Something isn't working High This issue blocks a user from building or impacts a lot of users JSON Form Issue / features related to the JSON form wiget UI Building Pod labels May 20, 2022
@ashit-rath
Copy link
Contributor Author

/ok-to-test sha=ef10ef7

@github-actions
Copy link

Unable to find test scripts. Please add necessary tests to the PR.

@github-actions
Copy link

Tests running at: https://github.com/appsmithorg/appsmith/actions/runs/2358744620.
Workflow: Appsmith External Integration Test Workflow.
Commit: ef10ef7.
PR: 13993.

@github-actions
Copy link

UI Performance test run logs and artifacts: https://github.com/appsmithorg/appsmith/actions/runs/2358744620.
Commit: ef10ef7.
Results:

Click to view performance test results

Run 1 Run 2 Run 3 Run 4 Run 5 Median Mean SD.Sample SD.Population
SELECT_WIDGET_MENU_OPEN
scripting 2189.05 2114.29 2092.3 2060.45 2091.71 2092.3 2109.56 2.29 2.05
painting 11.53 9.92 14.4 13.5 19.92 13.5 13.85 27.51 24.62
rendering 821.5 703.02 692.38 710.38 721.18 710.38 729.69 7.18 6.42
SELECT_WIDGET_SELECT_OPTION
scripting 335.32 353.26 337.67 348.03 343.75 343.75 343.61 2.14 1.91
painting 5 8.78 5.86 4.53 7.01 5.86 6.24 27.40 24.52
rendering 22.42 26.7 21.24 20.31 21.51 21.51 22.44 11.14 9.98

1 similar comment
@github-actions
Copy link

UI Performance test run logs and artifacts: https://github.com/appsmithorg/appsmith/actions/runs/2358744620.
Commit: ef10ef7.
Results:

Click to view performance test results

Run 1 Run 2 Run 3 Run 4 Run 5 Median Mean SD.Sample SD.Population
SELECT_WIDGET_MENU_OPEN
scripting 2189.05 2114.29 2092.3 2060.45 2091.71 2092.3 2109.56 2.29 2.05
painting 11.53 9.92 14.4 13.5 19.92 13.5 13.85 27.51 24.62
rendering 821.5 703.02 692.38 710.38 721.18 710.38 729.69 7.18 6.42
SELECT_WIDGET_SELECT_OPTION
scripting 335.32 353.26 337.67 348.03 343.75 343.75 343.61 2.14 1.91
painting 5 8.78 5.86 4.53 7.01 5.86 6.24 27.40 24.52
rendering 22.42 26.7 21.24 20.31 21.51 21.51 22.44 11.14 9.98

@ashit-rath
Copy link
Contributor Author

/ok-to-test sha=ef10ef7

@github-actions
Copy link

Tests running at: https://github.com/appsmithorg/appsmith/actions/runs/2369036479.
Workflow: Appsmith External Integration Test Workflow.
Commit: ef10ef7.
PR: 13993.

@github-actions
Copy link

UI Performance test run logs and artifacts: https://github.com/appsmithorg/appsmith/actions/runs/2369036479.
Commit: ef10ef7.
Results:

Click to view performance test results

Run 1 Run 2 Run 3 Run 4 Run 5 Median Mean SD.Sample SD.Population
SELECT_WIDGET_MENU_OPEN
scripting 2569.9 2336.8 2571.17 2450.76 2487.75 2487.75 2483.28 3.91 3.50
painting 22.54 13.66 13.44 18.51 42.61 18.51 22.15 54.36 48.62
rendering 894.79 821.3 888.99 830.13 927.61 888.99 872.56 5.20 4.65
SELECT_WIDGET_SELECT_OPTION
scripting 404.26 661.78 377.69 364.73 398.98 398.98 441.49 28.13 25.16
painting 13.78 4.49 7.34 4.91 5.88 5.88 7.28 52.20 46.57
rendering 23.16 21.3 23.01 24.61 22.83 23.01 22.98 5.13 4.57

1 similar comment
@github-actions
Copy link

UI Performance test run logs and artifacts: https://github.com/appsmithorg/appsmith/actions/runs/2369036479.
Commit: ef10ef7.
Results:

Click to view performance test results

Run 1 Run 2 Run 3 Run 4 Run 5 Median Mean SD.Sample SD.Population
SELECT_WIDGET_MENU_OPEN
scripting 2569.9 2336.8 2571.17 2450.76 2487.75 2487.75 2483.28 3.91 3.50
painting 22.54 13.66 13.44 18.51 42.61 18.51 22.15 54.36 48.62
rendering 894.79 821.3 888.99 830.13 927.61 888.99 872.56 5.20 4.65
SELECT_WIDGET_SELECT_OPTION
scripting 404.26 661.78 377.69 364.73 398.98 398.98 441.49 28.13 25.16
painting 13.78 4.49 7.34 4.91 5.88 5.88 7.28 52.20 46.57
rendering 23.16 21.3 23.01 24.61 22.83 23.01 22.98 5.13 4.57

@chandannkumar
Copy link

chandannkumar commented May 24, 2022 •
edited by loom-gif-previews bot

Observation:

  • When changing name field to Select field type and then emptying value from source could not results error state when isRequired is enabled and vice versa emptying value first and then changing field to select type results error. refer video @ashit-rath
    LOOM DEMO

@ashit-rath
Copy link
Contributor Author

@chandannkumar Yes this problem still exists and we will fix this in the JSON Form Next steps epic as to fix that we have to upgrade the version of this widget.
This issue is only present in edit mode and only when you change the field type. If you refresh in edit mode or deploy the app, it should work correctly.

@ashit-rath
Copy link
Contributor Author

/ok-to-test sha=ef10ef7

@github-actions
Copy link

Tests running at: https://github.com/appsmithorg/appsmith/actions/runs/2375645712.
Workflow: Appsmith External Integration Test Workflow.
Commit: ef10ef7.
PR: 13993.

@github-actions
Copy link

UI Performance test run logs and artifacts: https://github.com/appsmithorg/appsmith/actions/runs/2375645712.
Commit: ef10ef7.
Results:

Click to view performance test results

Run 1 Run 2 Run 3 Run 4 Run 5 Median Mean SD.Sample SD.Population
SELECT_WIDGET_MENU_OPEN
scripting 2084.75 2170.89 1998.9 2037.63 2038.3 2038.3 2066.09 3.19 2.86
painting 16.3 18.94 14.68 12.51 18.3 16.3 16.15 16.35 14.61
rendering 725.77 752.14 706.99 732.85 725.42 725.77 728.63 2.23 2.00
SELECT_WIDGET_SELECT_OPTION
scripting 322.72 347.32 352.86 308.02 297.65 322.72 325.71 7.38 6.60
painting 4.01 4.03 14.4 12.53 6.29 6.29 8.25 59.27 53.09
rendering 21.18 21.87 21.84 23.49 33.57 21.87 24.39 21.32 19.07

1 similar comment
@github-actions
Copy link

UI Performance test run logs and artifacts: https://github.com/appsmithorg/appsmith/actions/runs/2375645712.
Commit: ef10ef7.
Results:

Click to view performance test results

Run 1 Run 2 Run 3 Run 4 Run 5 Median Mean SD.Sample SD.Population
SELECT_WIDGET_MENU_OPEN
scripting 2084.75 2170.89 1998.9 2037.63 2038.3 2038.3 2066.09 3.19 2.86
painting 16.3 18.94 14.68 12.51 18.3 16.3 16.15 16.35 14.61
rendering 725.77 752.14 706.99 732.85 725.42 725.77 728.63 2.23 2.00
SELECT_WIDGET_SELECT_OPTION
scripting 322.72 347.32 352.86 308.02 297.65 322.72 325.71 7.38 6.60
painting 4.01 4.03 14.4 12.53 6.29 6.29 8.25 59.27 53.09
rendering 21.18 21.87 21.84 23.49 33.57 21.87 24.39 21.32 19.07

@chandannkumar
Copy link

Tested this PR and working as expected

@ashit-rath ashit-rath merged commit 6d591c4 into release May 25, 2022
@ashit-rath ashit-rath deleted the fix/13981-jf-select-validation branch May 25, 2022 10:04
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@sbalaji1192 sbalaji1192 sbalaji1192 approved these changes

Assignees
No one assigned
Labels
App Viewers Pod This label assigns issues to the app viewers pod Bug Something isn't working High This issue blocks a user from building or impacts a lot of users JSON Form Issue / features related to the JSON form wiget
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

[Bug]: JSONForm select field consider empty string as invalid value when isRequired is enabled.
3 participants
@ashit-rath @chandannkumar @sbalaji1192