refactor: move CTAS/CVAS field II #13877

hughhhh · 2021-03-30T23:08:26Z

SUMMARY

Moved CTAS/CVAS Schema Field Closer to the "Allow Create Table As" In Edit Datasource Modal. This required nested conditional rendering, so I had to use some useState/useEffect in order to hold that state at a higher level for all functions to run properly.

BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF

Field before:

#### Field after:

TEST PLAN

Tests will ensure that all elements are rendered in their respectively appropriate conditions.

ADDITIONAL INFORMATION

codecov · 2021-03-31T06:20:02Z

Codecov Report

Merging #13877 (72768d8) into master (4187d9e) will increase coverage by 0.44%.
The diff coverage is 88.23%.

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

@@            Coverage Diff             @@
##           master   #13877      +/-   ##
==========================================
+ Coverage   77.91%   78.36%   +0.44%     
==========================================
  Files         934      934              
  Lines       47350    47369      +19     
  Branches     5941     5946       +5     
==========================================
+ Hits        36894    37121     +227     
+ Misses      10308    10104     -204     
+ Partials      148      144       -4

Flag	Coverage Δ
cypress	`56.03% <74.41%> (+2.80%)`	⬆️
javascript	`66.38% <86.27%> (+0.09%)`	⬆️

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

Impacted Files	Coverage Δ
...end/src/views/CRUD/data/database/DatabaseModal.tsx	`78.94% <86.95%> (+19.20%)`	⬆️
...-frontend/src/components/IndeterminateCheckbox.tsx	`100.00% <100.00%> (+5.26%)`	⬆️
...set-frontend/src/dashboard/util/getDropPosition.js	`90.90% <0.00%> (-1.52%)`	⬇️
superset-frontend/src/SqlLab/actions/sqlLab.js	`63.26% <0.00%> (+0.20%)`	⬆️
superset-frontend/src/reduxUtils.ts	`81.01% <0.00%> (+1.26%)`	⬆️
...-frontend/src/dashboard/actions/dashboardLayout.js	`98.03% <0.00%> (+1.96%)`	⬆️
...frontend/src/dashboard/reducers/dashboardLayout.js	`99.00% <0.00%> (+2.00%)`	⬆️
superset-frontend/src/views/routes.tsx	`56.81% <0.00%> (+2.27%)`	⬆️
...-frontend/src/dashboard/reducers/dashboardState.js	`85.71% <0.00%> (+3.57%)`	⬆️
...et-frontend/src/components/EditableTitle/index.tsx	`75.30% <0.00%> (+3.70%)`	⬆️
... and 19 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 4187d9e...cedabe2. Read the comment docs.

…/move-ctas-cvas-fields

eschutho · 2021-03-31T20:02:48Z

superset-frontend/spec/javascripts/sqllab/fixtures.ts

@@ -480,6 +480,8 @@ export const initialState = {
      DEFAULT_SQLLAB_LIMIT: 1000,
      SQL_MAX_ROW: 100000,
      DISPLAY_MAX_ROW: 100,
+      SQLALCHEMY_DOCS_URL: 'test_SQLALCHEMY_DOCS_URL',
+      SQLALCHEMY_DISPLAY_TEXT: 'test_SQLALCHEMY_DISPLAY_TEXT',


wasn't this part of a previous PR?

not sure but we wanted to update the fixture to make sure it has all the config items

eschutho · 2021-03-31T20:05:22Z

superset-frontend/spec/javascripts/views/CRUD/data/database/DatabaseModal_spec.jsx

+      // When clicked, "Expose in SQL Lab" becomes unchecked
+      userEvent.click(exposeInSqlLab);
+
+      // While unchecked, only "Expose in SQL Lab" should display


Are the comments still aligned with the code? This says unchecked, while the assertion below is checked.

eschutho · 2021-03-31T20:05:39Z

superset-frontend/spec/javascripts/views/CRUD/data/database/DatabaseModal_spec.jsx

+      expect(exposeInSqlLab).toBeChecked();
+      const checkboxes = screen
+        .getAllByRole('checkbox')
+        .filter(checkbox => !checkbox.checked);


eschutho · 2021-03-31T20:07:03Z

superset-frontend/spec/javascripts/views/CRUD/data/database/DatabaseModal_spec.jsx

+      // Uncheck both "Allow CTAS" and "Allow CVAS" to hide schema field again
+      userEvent.click(allowCTAS);
+      // Both checkboxes go unchecked, so the field should no longer render
+      userEvent.click(allowCVAS);


nit, but I would move this up a line with the other click to be more in line with the comments.

eschutho · 2021-03-31T20:28:44Z

We changed some functionality of the indeterminate checkbox, so we'll need to test across the site cc @rusackas @junlincc
I'm doing some visual testing now.

eschutho · 2021-03-31T23:40:04Z

Screen.Recording.2021-03-31.mov

Other checkboxes:
existing:

new:

existing:

new:

eschutho · 2021-03-31T23:41:55Z

@hughhhh @lyndsiWilliams indeterminate checkboxes are only on this modal and the list views. The list views looked ok. I would just make sure that all of the checkboxes in the modal are passing in the label and then just do a check on updating any styles.

hughhhh · 2021-04-02T17:56:22Z

/testenv up

github-actions · 2021-04-02T17:56:34Z

@hughhhh Ephemeral environment creation is currently limited to committers.

hughhhh · 2021-04-02T19:14:55Z

/testenv up

github-actions · 2021-04-02T19:16:58Z

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

eschutho · 2021-04-02T23:44:39Z

looks great! I rebased this recently and saw that an earlier commit had a few trims on database_name in the DatabaseModal file. Do we need to add those in?
db.database_name.trim()
Here's the PR if you want to make sure that they changes are represented here: https://github.com/apache/superset/pull/13800/files

eschutho · 2021-04-02T23:47:44Z

tested in ephemeral environment, too, and it looks good. I wasn't able to test save functionality b/c of sqllite but I had tested it locally previously.

hughhhh · 2021-04-05T20:40:01Z

looks great! I rebased this recently and saw that an earlier commit had a few trims on database_name in the DatabaseModal file. Do we need to add those in?
db.database_name.trim()
Here's the PR if you want to make sure that they changes are represented here: #13800 (files)

Those changes we need are currently on the branch already, the other database_name reference don't need to have trim. We can add them if want the consistency

betodealmeida

Nice!

github-actions · 2021-04-05T21:33:50Z

Ephemeral environment shutdown and build artifacts deleted.

Co-authored-by: lyndsiWilliams <kcatgirl@gmail.com>

lyndsiWilliams and others added 16 commits March 24, 2021 15:44

Implemented testing on DatabaseModal_spec

d0372fe

Implemented changes on IndeterminateCheckbox

ce546b5

Finished copying code over from old branch

5d9c9fc

Updated jest-dom dependency

b452ed4

Tests fixed and all passing!

7233003

Forgot the package-lock.json

0077f88

Fixing merge conflicts

9892804

Fixing merge conflicts

b27e36f

Fixing merge conflicts

3bfbf7b

Fixing tests

0f0f48e

Fixing tests

03156f4

fix test

45f105b

remove consolde

9b07e36

fix test

ad8f671

fix 1st test

8251f90

working updated test

791226f

pull-request-size bot added the size/XL label Mar 30, 2021

lyndsiWilliams mentioned this pull request Mar 30, 2021

refactor: move CTAS/CVAS field #13810

Closed

6 tasks

hughhhh changed the title ~~Implemented testing on DatabaseModal_spec~~ refactor: move CTAS/CVAS field #13810 Mar 30, 2021

hughhhh changed the title ~~refactor: move CTAS/CVAS field #13810~~ refactor: move CTAS/CVAS field II Mar 30, 2021

hughhhh closed this Mar 31, 2021

hughhhh reopened this Mar 31, 2021

AAfghahi approved these changes Mar 31, 2021

View reviewed changes

Merge branch 'master' of https://github.com/apache/superset into hugh…

bbd695f

…/move-ctas-cvas-fields

eschutho reviewed Mar 31, 2021

View reviewed changes

hughhhh added 6 commits April 1, 2021 10:59

update comments

488d0a0

change form height

093d946

updated test file

69815e9

move to labeltext

2f89e08

remove margin right

7608883

update cypress test

e9d9be7

hughhhh force-pushed the hugh/move-ctas-cvas-fields branch from aca7984 to e9d9be7 Compare April 2, 2021 16:35

hughhhh added 2 commits April 2, 2021 13:02

update dashboard list

e327537

fixe merge conflicts

9febdce

hughhhh force-pushed the hugh/move-ctas-cvas-fields branch from 5faef21 to 9febdce Compare April 2, 2021 18:04

hughhhh added 2 commits April 2, 2021 15:10

package.json

573ee7e

fix

cedabe2

betodealmeida approved these changes Apr 5, 2021

View reviewed changes

hughhhh merged commit d006178 into master Apr 5, 2021

hughhhh deleted the hugh/move-ctas-cvas-fields branch April 5, 2021 20:57

lyndsiWilliams added a commit to preset-io/superset that referenced this pull request Apr 7, 2021

refactor: move CTAS/CVAS field II (apache#13877)

673ad37

Co-authored-by: lyndsiWilliams <kcatgirl@gmail.com>

allanco91 pushed a commit to allanco91/superset that referenced this pull request May 21, 2021

refactor: move CTAS/CVAS field II (apache#13877)

e491c83

Co-authored-by: lyndsiWilliams <kcatgirl@gmail.com>

QAlexBall pushed a commit to QAlexBall/superset that referenced this pull request Dec 29, 2021

refactor: move CTAS/CVAS field II (apache#13877)

dc61f9a

Co-authored-by: lyndsiWilliams <kcatgirl@gmail.com>

mistercrunch added 🏷️ bot A label used by `supersetbot` to keep track of which PR where auto-tagged with release labels 🚢 1.2.0 labels Mar 12, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

refactor: move CTAS/CVAS field II #13877

refactor: move CTAS/CVAS field II #13877

hughhhh commented Mar 30, 2021 •

edited

Loading

codecov bot commented Mar 31, 2021 •

edited

Loading

eschutho Mar 31, 2021

hughhhh Apr 1, 2021

eschutho Mar 31, 2021

eschutho Mar 31, 2021

eschutho Mar 31, 2021

eschutho commented Mar 31, 2021

eschutho commented Mar 31, 2021 •

edited

Loading

eschutho commented Mar 31, 2021

hughhhh commented Apr 2, 2021

github-actions bot commented Apr 2, 2021

hughhhh commented Apr 2, 2021

github-actions bot commented Apr 2, 2021

eschutho commented Apr 2, 2021 •

edited

Loading

eschutho commented Apr 2, 2021

hughhhh commented Apr 5, 2021

betodealmeida left a comment

github-actions bot commented Apr 5, 2021

refactor: move CTAS/CVAS field II #13877

refactor: move CTAS/CVAS field II #13877

Conversation

hughhhh commented Mar 30, 2021 • edited Loading

SUMMARY

BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF

Field before:

TEST PLAN

ADDITIONAL INFORMATION

codecov bot commented Mar 31, 2021 • edited Loading

Codecov Report

eschutho Mar 31, 2021

Choose a reason for hiding this comment

hughhhh Apr 1, 2021

Choose a reason for hiding this comment

eschutho Mar 31, 2021

Choose a reason for hiding this comment

eschutho Mar 31, 2021

Choose a reason for hiding this comment

eschutho Mar 31, 2021

Choose a reason for hiding this comment

eschutho commented Mar 31, 2021

eschutho commented Mar 31, 2021 • edited Loading

eschutho commented Mar 31, 2021

hughhhh commented Apr 2, 2021

github-actions bot commented Apr 2, 2021

hughhhh commented Apr 2, 2021

github-actions bot commented Apr 2, 2021

eschutho commented Apr 2, 2021 • edited Loading

eschutho commented Apr 2, 2021

hughhhh commented Apr 5, 2021

betodealmeida left a comment

Choose a reason for hiding this comment

github-actions bot commented Apr 5, 2021

hughhhh commented Mar 30, 2021 •

edited

Loading

codecov bot commented Mar 31, 2021 •

edited

Loading

eschutho commented Mar 31, 2021 •

edited

Loading

eschutho commented Apr 2, 2021 •

edited

Loading