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: make database connection modal ace fields uncontrolled #22350

Merged
merged 2 commits into from
Dec 12, 2022

Conversation

eschutho
Copy link
Member

@eschutho eschutho commented Dec 6, 2022

SUMMARY

I recently refactored the extraJson field #21523 so that we didn't have to do any data munging on database connection save, but rather can use the state that is in the local component and just push it to the api. The tricky situation here is that ace expects a string value, but we are pasting stringified objects into the field, so when we have to parse it later to add to the extra field, it also parses the ace contents. This pr makes the ace editor and input fields in the extra options component uncontrolled so that we don't have to parse the state back into a string again.

BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF

TESTING INSTRUCTIONS

You should be able to edit the engine parameters field in the "Other" section of the database connection form on creation and edit for all databases. There should be no extra characters added to the editor as you type.

ADDITIONAL INFORMATION

@eschutho eschutho changed the title DRAFT: don't deep parse extra json fix: don't deep parse extra json Dec 7, 2022
@eschutho eschutho changed the title fix: don't deep parse extra json fix: don't deep parse extra json in database connection modal Dec 7, 2022
@lyndsiWilliams
Copy link
Member

/testenv up

@github-actions
Copy link
Contributor

github-actions bot commented Dec 7, 2022

@lyndsiWilliams Container image not yet published for this PR. Please try again when build is complete.

@github-actions
Copy link
Contributor

github-actions bot commented Dec 7, 2022

@lyndsiWilliams Ephemeral environment creation failed. Please check the Actions logs for details.

@eschutho eschutho changed the title fix: don't deep parse extra json in database connection modal fix: shallow parse extra json in database connection modal Dec 7, 2022
@pull-request-size pull-request-size bot added size/M and removed size/S labels Dec 7, 2022
@codecov
Copy link

codecov bot commented Dec 7, 2022

Codecov Report

Merging #22350 (ce815d0) into master (605cfa0) will increase coverage by 0.01%.
The diff coverage is 36.36%.

❗ Current head ce815d0 differs from pull request most recent head f3cb414. Consider uploading reports for the commit f3cb414 to get more accurate results

@@            Coverage Diff             @@
##           master   #22350      +/-   ##
==========================================
+ Coverage   66.86%   66.88%   +0.01%     
==========================================
  Files        1847     1847              
  Lines       70574    70580       +6     
  Branches     7748     7749       +1     
==========================================
+ Hits        47190    47204      +14     
+ Misses      21383    21360      -23     
- Partials     2001     2016      +15     
Flag Coverage Δ
javascript 53.86% <36.36%> (+0.02%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
...c/views/CRUD/data/database/DatabaseModal/index.tsx 43.76% <33.33%> (+2.76%) ⬆️
.../CRUD/data/database/DatabaseModal/ExtraOptions.tsx 70.37% <37.50%> (-4.63%) ⬇️

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

@lyndsiWilliams
Copy link
Member

/testenv up

@github-actions
Copy link
Contributor

github-actions bot commented Dec 7, 2022

@lyndsiWilliams Ephemeral environment spinning up at http://34.221.105.24:8080. Credentials are admin/admin. Please allow several minutes for bootstrapping and startup.

@jinghua-qa jinghua-qa added the need:qa-review Requires QA review label Dec 7, 2022
@eschutho
Copy link
Member Author

eschutho commented Dec 7, 2022

/testenv up

@github-actions
Copy link
Contributor

github-actions bot commented Dec 7, 2022

@eschutho Ephemeral environment spinning up at http://35.89.15.116:8080. Credentials are admin/admin. Please allow several minutes for bootstrapping and startup.

@pull-request-size pull-request-size bot added size/L and removed size/M labels Dec 7, 2022
@eschutho eschutho force-pushed the elizabeth/parse-json branch 2 times, most recently from 1d9e1c9 to 64120a0 Compare December 8, 2022 18:36
@pull-request-size pull-request-size bot added size/M and removed size/L labels Dec 10, 2022
@eschutho eschutho changed the title fix: shallow parse extra json in database connection modal fix: make database connection modal ace fields uncontrolled Dec 10, 2022
@pull-request-size pull-request-size bot added size/L and removed size/M labels Dec 12, 2022
Copy link
Member

@lyndsiWilliams lyndsiWilliams left a comment

Choose a reason for hiding this comment

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

Tested locally with a gsheet, every seems to be working properly! Nice fix!

@eschutho eschutho merged commit 608ffcb into apache:master Dec 12, 2022
@github-actions
Copy link
Contributor

Ephemeral environment shutdown and build artifacts deleted.

jinghua-qa pushed a commit to preset-io/superset that referenced this pull request Dec 12, 2022
@mistercrunch mistercrunch added 🏷️ bot A label used by `supersetbot` to keep track of which PR where auto-tagged with release labels 🚢 2.1.0 and removed 🚢 2.1.3 labels Mar 13, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
🏷️ bot A label used by `supersetbot` to keep track of which PR where auto-tagged with release labels need:qa-review Requires QA review preset:2022.49 size/L 🚢 2.1.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants