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

chore: refactor SqlEditor to functional component #21320

Merged
merged 11 commits into from
Sep 20, 2022
Merged

chore: refactor SqlEditor to functional component #21320

merged 11 commits into from
Sep 20, 2022

Conversation

EugeneTorap
Copy link
Contributor

SUMMARY

Converted SqlEditor class component into functional component.

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

@codecov
Copy link

codecov bot commented Sep 3, 2022

Codecov Report

Merging #21320 (5bf09a3) into master (8c16806) will increase coverage by 0.00%.
The diff coverage is 53.28%.

@@           Coverage Diff           @@
##           master   #21320   +/-   ##
=======================================
  Coverage   66.66%   66.67%           
=======================================
  Files        1793     1793           
  Lines       68490    68501   +11     
  Branches     7275     7274    -1     
=======================================
+ Hits        45662    45676   +14     
+ Misses      20966    20965    -1     
+ Partials     1862     1860    -2     
Flag Coverage Δ
javascript 52.85% <53.28%> (+0.02%) ⬆️

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

Impacted Files Coverage Δ
...frontend/src/SqlLab/components/SqlEditor/index.jsx 52.74% <51.36%> (+1.33%) ⬆️
superset-frontend/src/SqlLab/constants.ts 100.00% <100.00%> (ø)
...et-frontend/src/components/TableSelector/index.tsx 80.00% <0.00%> (+4.00%) ⬆️

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

@kgabryje
Copy link
Member

kgabryje commented Sep 5, 2022

@jinghua-qa Could you please help testing?

@kgabryje
Copy link
Member

kgabryje commented Sep 5, 2022

/testenv up

@github-actions
Copy link
Contributor

github-actions bot commented Sep 5, 2022

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

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.

I found a few things that need adjustment, as well as these items that I was unable to tag separately:

@EugeneTorap EugeneTorap requested review from lyndsiWilliams and removed request for stephenLYZ, kgabryje, hughhhh and geido September 6, 2022 18:43
@kgabryje
Copy link
Member

kgabryje commented Sep 7, 2022

/testenv up

@github-actions
Copy link
Contributor

github-actions bot commented Sep 7, 2022

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

@EugeneTorap
Copy link
Contributor Author

@lyndsiWilliams @andrey-zayats Can you review/test it?

@andrey-zayats
Copy link

User can't update the saved query. The issue is not reproduced in the latest master

upd.mp4

@andrey-zayats
Copy link

andrey-zayats commented Sep 13, 2022

An error message occurs when user selects a schema from "schema" dropdown. There is also no data in the table schema dropdown. I checked it locally and on eph env, reproduced on both

error.mp4

screen

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.

Good catch @andrey-zayats, that bug will need to be fixed before this PR can be merged.

@EugeneTorap
Copy link
Contributor Author

@lyndsiWilliams master branch has this bug

@lyndsiWilliams
Copy link
Member

There's a fix being worked on for this now, let's wait until that fix is in so QA can properly test this PR.

@EugeneTorap
Copy link
Contributor Author

@lyndsiWilliams @jinghua-qa @andrey-zayats Can you review/test it?

@jinghua-qa
Copy link
Member

/testenv up

@github-actions
Copy link
Contributor

@jinghua-qa Ephemeral environment spinning up at http://35.92.222.234:8080. Credentials are admin/admin. Please allow several minutes for bootstrapping and startup.

@EugeneTorap
Copy link
Contributor Author

@jinghua-qa @andrey-zayats Can you test it?

@andrey-zayats
Copy link

@jinghua-qa @andrey-zayats Can you test it?

I'll take a look tomorrow morning

@andrey-zayats
Copy link

andrey-zayats commented Sep 20, 2022

@EugeneTorap QA verified. LGTM

UDP:
FYI. There is an error message occurs when user saves a chart using chart power query. This issue is already in master

empty.query.mp4

@lyndsiWilliams lyndsiWilliams merged commit 2224ebe into apache:master Sep 20, 2022
@EugeneTorap EugeneTorap deleted the refactor/sqlEditor-to-fc branch September 20, 2022 13:55
@github-actions
Copy link
Contributor

Ephemeral environment shutdown and build artifacts deleted.

mayurnewase added a commit to mayurnewase/incubator-superset that referenced this pull request Sep 22, 2022
justinpark added a commit to justinpark/superset that referenced this pull request Sep 30, 2022
sadpandajoe added a commit to preset-io/superset that referenced this pull request Oct 3, 2022
sadpandajoe added a commit to preset-io/superset that referenced this pull request Oct 17, 2022
sadpandajoe added a commit to preset-io/superset that referenced this pull request Oct 17, 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 size/XL 🚢 2.1.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants