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: gsheets editing with dynamic forms #21710

Merged
merged 9 commits into from
Oct 7, 2022
Merged

fix: gsheets editing with dynamic forms #21710

merged 9 commits into from
Oct 7, 2022

Conversation

hughhhh
Copy link
Member

@hughhhh hughhhh commented Oct 5, 2022

SUMMARY

Change reference for grabbing data for gsheets validation. Now, catalog data lives in the root instead of in parameters in our client code.

BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF

TESTING INSTRUCTIONS

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

@hughhhh hughhhh changed the title fix: reference catalog for gsheets validation fix: gsheets editing with dynamic forms Oct 5, 2022
@hughhhh hughhhh force-pushed the fix-gsheet-edits branch 2 times, most recently from cf98ad3 to 19ba4a0 Compare October 5, 2022 23:25
@codecov
Copy link

codecov bot commented Oct 5, 2022 •
edited
Loading

Codecov Report

Merging #21710 (600dec0) into master (97273f5) will decrease coverage by 0.00%.
The diff coverage is 58.33%.

@@            Coverage Diff             @@
##           master   #21710      +/-   ##
==========================================
- Coverage   66.85%   66.84%   -0.01%     
==========================================
  Files        1799     1799              
  Lines       68887    68896       +9     
  Branches     7320     7322       +2     
==========================================
+ Hits        46052    46055       +3     
- Misses      20946    20951       +5     
- Partials     1889     1890       +1
Flag Coverage Δ
hive 52.91% <50.00%> (-0.01%) ⬇️
javascript 53.15% <33.33%> (-0.01%) ⬇️
mysql 78.24% <50.00%> (-0.01%) ⬇️
postgres 78.30% <50.00%> (-0.01%) ⬇️
presto 52.81% <50.00%> (-0.01%) ⬇️
python 81.44% <83.33%> (-0.01%) ⬇️
sqlite 76.80% <50.00%> (-0.01%) ⬇️
unit 50.95% <83.33%> (+<0.01%) ⬆️

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 34.12% <33.33%> (-0.42%) ⬇️
superset/db_engine_specs/gsheets.py 75.91% <80.00%> (-0.21%) ⬇️
superset/databases/schemas.py 98.23% <100.00%> (+<0.01%) ⬆️

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

@hughhhh
Copy link
Member Author

hughhhh commented Oct 5, 2022

/testenv up

@github-actions
Copy link
Contributor

github-actions bot commented Oct 5, 2022

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

@github-actions
Copy link
Contributor

github-actions bot commented Oct 5, 2022

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

eschutho
eschutho approved these changes Oct 6, 2022
View reviewed changes
Copy link
Member

@eschutho eschutho left a comment

Choose a reason for hiding this comment

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

I think we can clean this up more in the future to have the structure match the db model instead of having catalog at the root.

@pull-request-size pull-request-size bot added size/M and removed size/S labels Oct 6, 2022
@hughhhh hughhhh force-pushed the fix-gsheet-edits branch 2 times, most recently from b3b5024 to 28de5f0 Compare October 6, 2022 03:19
@betodealmeida
Copy link
Member

I think we can clean this up more in the future to have the structure match the db model instead of having catalog at the root.

Yeah, I think ideally this would just live inside the extra field in the payload (mimicking how it's stored in the database), and the frontend would move it to a custom field as needed. Having db-specific fields to the root can quickly pollute the interface.

Antonio-RiveroMartnez
Antonio-RiveroMartnez reviewed Oct 7, 2022
View reviewed changes
superset-frontend/src/views/CRUD/data/database/DatabaseModal/index.tsx
@@ -744,7 +746,10 @@ const DatabaseModal: FunctionComponent<DatabaseModalProps> = ({
});
}

setDB({ type: ActionType.addTableCatalogSheet });
if (database_name === 'Google Sheets') {
Copy link
Member

Choose a reason for hiding this comment

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

Shouldn't this be checking for the engine/backend instead of the DB Name which might be changed by users?

Copy link
Member

@eschutho eschutho Oct 7, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

This part happens before the user names it, but I agree.. the engine would also work.

Copy link
Member Author

Choose a reason for hiding this comment

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

i'll make a follow up PR with the enums of the engines so we can leverage that for checking vs. the string

@eschutho eschutho merged commit 882bfb6 into master Oct 7, 2022
hughhhh added a commit to preset-io/superset that referenced this pull request Oct 7, 2022
@hughhhh
fix: gsheets editing with dynamic forms (apache#21710)
cdc69ac 
(cherry picked from commit 882bfb6)
@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
@mistercrunch mistercrunch deleted the fix-gsheet-edits branch March 26, 2024 16:12
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Antonio-RiveroMartnez Antonio-RiveroMartnez Antonio-RiveroMartnez left review comments

@eschutho eschutho eschutho approved these changes

@betodealmeida betodealmeida Awaiting requested review from betodealmeida

Assignees
No one assigned
Labels
🏷️ bot A label used by `supersetbot` to keep track of which PR where auto-tagged with release labels size/M 🚢 2.1.0
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@hughhhh @betodealmeida @eschutho @Antonio-RiveroMartnez @mistercrunch