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

refactor: serialize extra json in state #21523

Merged
merged 3 commits into from
Oct 14, 2022

Conversation

eschutho
Copy link
Member

SUMMARY

refactoring the extra json state in the database connection modal so that it is always serialized, as the api will expect it. Each component will deserialize and push it back to the reducer, which will then serialize it again. This should reduce some duplication of types and state.

This is also in preparation for adding a databricks form which will need to have extra json property as a field.

BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF

No visual changes

TESTING INSTRUCTIONS

Adding and removing extra json in the advanced section should work as expected

ADDITIONAL INFORMATION

  • Has associated issue:
  • Required feature flags:
  • Changes UI
  • Includes DB Migration (follow approval process in SIP-59)
    • Migration is atomic, supports rollback & is backwards-compatible
    • Confirm DB migration upgrade and downgrade tested
    • Runtime estimates and downtime expectations provided
  • Introduces new feature or API
  • Removes existing feature or API

@eschutho
Copy link
Member Author

/testenv up

@github-actions
Copy link
Contributor

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

@codecov
Copy link

codecov bot commented Sep 20, 2022

Codecov Report

Merging #21523 (9cf9ea1) into master (bd3166b) will increase coverage by 0.02%.
The diff coverage is 45.16%.

@@            Coverage Diff             @@
##           master   #21523      +/-   ##
==========================================
+ Coverage   66.88%   66.90%   +0.02%     
==========================================
  Files        1802     1805       +3     
  Lines       68987    69051      +64     
  Branches     7345     7366      +21     
==========================================
+ Hits        46139    46197      +58     
- Misses      20951    20952       +1     
- Partials     1897     1902       +5     
Flag Coverage Δ
javascript 53.29% <45.16%> (+0.06%) ⬆️

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 41.99% <41.66%> (+7.86%) ⬆️
.../CRUD/data/database/DatabaseModal/ExtraOptions.tsx 75.00% <57.14%> (-11.96%) ⬇️
...ontrols/DateFilterControl/utils/dateFilterUtils.ts 39.39% <0.00%> (-60.61%) ⬇️
.../explore/exploreUtils/getParsedExploreURLParams.ts 83.33% <0.00%> (-8.10%) ⬇️
...nts/controls/DateFilterControl/DateFilterLabel.tsx 35.71% <0.00%> (-5.70%) ⬇️
...iews/CRUD/data/dataset/AddDataset/Header/index.tsx 81.81% <0.00%> (-5.69%) ⬇️
...perset-frontend/src/views/CRUD/chart/ChartList.tsx 56.61% <0.00%> (-2.36%) ⬇️
...ontrols/DndColumnSelectControl/DndFilterSelect.tsx 44.03% <0.00%> (-1.01%) ⬇️
...et-ui-chart-controls/src/shared-controls/index.tsx 53.44% <0.00%> (-0.79%) ⬇️
superset-frontend/src/explore/constants.ts 100.00% <0.00%> (ø)
... and 40 more

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

@geido
Copy link
Member

geido commented Sep 21, 2022

@eschutho when opening the "Other" tab under "Advanced" the page crashes. I checked it on master and it is working fine, the bug appears only on the ephemeral env. I think it is worth having a look

@eschutho
Copy link
Member Author

@eschutho when opening the "Other" tab under "Advanced" the page crashes. I checked it on master and it is working fine, the bug appears only on the ephemeral env. I think it is worth having a look

Thanks for testing! I'll take a look.

@eschutho eschutho force-pushed the elizabeth/extra-json branch from eaeac1c to 2b55053 Compare September 22, 2022 00:10
@eschutho eschutho force-pushed the elizabeth/extra-json branch 2 times, most recently from ebe32f3 to 0c1938b Compare September 22, 2022 20:37
@eschutho eschutho mentioned this pull request Sep 23, 2022
9 tasks
Copy link
Member

@betodealmeida betodealmeida left a comment

Choose a reason for hiding this comment

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

This looks amazing and a lot of work — thanks for refactoring this!

@andrey-zayats
Copy link

The "Allow this database to be explored" setting should be checked by default in the Database Connection UI. I can't repro the issue in latest master.

db_settings.mp4

@andrey-zayats
Copy link

andrey-zayats commented Sep 28, 2022

"Cancel query on window unload event" shouldn't be checked by default in db settings (Advanced -> Performance). I can't repro the issue in latest master.

image

UPD:
User can't even uncheck the checkbox

@andrey-zayats
Copy link

User can't create a dataset using newly connected gsheet.

gsheet.mp4

@eschutho eschutho force-pushed the elizabeth/extra-json branch 8 times, most recently from bcc2c9b to b8f1125 Compare October 8, 2022 00:08
@eschutho eschutho force-pushed the elizabeth/extra-json branch from b8f1125 to 9bba719 Compare October 11, 2022 19:48
@eschutho
Copy link
Member Author

/testenv up

@github-actions
Copy link
Contributor

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

@eschutho eschutho force-pushed the elizabeth/extra-json branch from 9bba719 to 6269700 Compare October 13, 2022 23:38
@eschutho eschutho force-pushed the elizabeth/extra-json branch from 6269700 to da02b71 Compare October 14, 2022 18:07
paramatersCatalog[item.name] = item.value;
});

if (action.payload.type?.startsWith('catalog')) {
Copy link
Member

Choose a reason for hiding this comment

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

why have this be a nested loop?

Copy link
Member Author

Choose a reason for hiding this comment

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

Thanks for calling that out. I had originally written logic for the case where action.payload.type?.startsWith('catalog') && trimmedState.catalog === undefined, thus the nested logic, but now I see that a form with a catalog will always have a defined catalog state, so I removed the logic and added some comments.

@eschutho eschutho merged commit 196c367 into apache:master Oct 14, 2022
@github-actions
Copy link
Contributor

Ephemeral environment shutdown and build artifacts deleted.

@eschutho eschutho deleted the elizabeth/extra-json branch October 21, 2022 20:59
@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 size/XL 🚢 2.1.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants