fix: Make sheet_name into a `ValidationInputError` #16056

hughhhh · 2021-08-04T05:20:17Z

SUMMARY

Change the input type for sheet name to ValidationInputError to allow us to show specific errors for this param. We are going to allow user to create a google sheet engine if there is no catalog set.

BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF

Empty URL

Empty Sheet Name

TESTING INSTRUCTIONS

ADDITIONAL INFORMATION

Has associated issue:
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 · 2021-08-04T05:51:40Z

Codecov Report

Merging #16056 (be37328) into master (69c5cd7) will decrease coverage by 0.09%.
The diff coverage is 37.50%.

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

@@            Coverage Diff             @@
##           master   #16056      +/-   ##
==========================================
- Coverage   76.89%   76.79%   -0.10%     
==========================================
  Files         995      995              
  Lines       52869    52881      +12     
  Branches     6712     6720       +8     
==========================================
- Hits        40651    40608      -43     
- Misses      11992    12047      +55     
  Partials      226      226

Flag	Coverage Δ
hive	`81.29% <72.72%> (-0.06%)`	⬇️
mysql	`81.54% <72.72%> (-0.06%)`	⬇️
postgres	`81.61% <72.72%> (+0.02%)`	⬆️
presto	`?`
python	`81.98% <72.72%> (-0.17%)`	⬇️
sqlite	`81.25% <72.72%> (+0.02%)`	⬆️

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

Impacted Files	Coverage Δ
...ters/FiltersConfigModal/FiltersConfigForm/utils.ts	`90.47% <ø> (ø)`
...FormattingControl/ConditionalFormattingControl.tsx	`21.66% <0.00%> (-0.75%)`	⬇️
...onalFormattingControl/FormattingPopoverContent.tsx	`29.26% <0.00%> (-4.07%)`	⬇️
superset-frontend/src/reports/actions/reports.js	`10.86% <0.00%> (ø)`
...c/views/CRUD/data/database/DatabaseModal/styles.ts	`81.57% <ø> (ø)`
superset/viz.py	`56.57% <ø> (ø)`
superset-frontend/src/views/CRUD/hooks.ts	`47.80% <16.66%> (ø)`
.../ReportModal/HeaderReportActionsDropdown/index.tsx	`24.32% <25.00%> (-1.39%)`	⬇️
.../database/DatabaseModal/DatabaseConnectionForm.tsx	`54.00% <33.33%> (ø)`
superset/db_engine_specs/gsheets.py	`74.73% <50.00%> (-1.93%)`	⬇️
... and 9 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 69c5cd7...3b3c5ae. Read the comment docs.

betodealmeida

@hughhhh do you mind if we loop @yousoph on this? I don't think we should force users to enter a name:URL pair to set up the connection.

betodealmeida · 2021-08-04T20:03:09Z

superset/db_engine_specs/gsheets.py

                    error_type=SupersetErrorType.CONNECTION_MISSING_PARAMETERS_ERROR,
                    level=ErrorLevel.WARNING,
-                    extra={"invalid": ["catalog"], "name": "", "url": ""},
+                    extra={"catalog": {"idx": 0}},


This makes the API inconsistent — if there's no catalog then there's no index 0. It would be better to return None here, and add a special case to the UI to highlight the first input pair in that case.

betodealmeida · 2021-08-04T20:03:49Z

superset-frontend/src/views/CRUD/hooks.ts

+                        catalog: {
+                          name: string;
+                          url: string;
+                          idx: string;


This should be a number. if idx being passed around as a string we should convert it to a number, since that's how we're using it.

betodealmeida

betodealmeida · 2021-08-09T16:42:16Z

superset/db_engine_specs/gsheets.py

+        idx = 0
        for name, url in table_catalog.items():


Nit:

Suggested change

idx = 0

for name, url in table_catalog.items():

for ixd, (name, url) in enumerate(table_catalog.items()):

betodealmeida · 2021-08-09T16:42:40Z

superset/db_engine_specs/gsheets.py

                    ),
                )
+            idx += 1


If using enumerate above.

Suggested change

idx += 1

rosemarie-chiu · 2021-08-13T21:30:31Z

🏷 2021.31

* setup validates for name * add error type * fix linting * fix test * remove errors * fix number * fix test

hughhhh added 2 commits August 4, 2021 00:32

setup validates for name

3e6b97b

add error type

38bbfde

pull-request-size bot added the size/M label Aug 4, 2021

fix linting

35113dd

fix test

ecb9c6a

betodealmeida requested changes Aug 4, 2021

View reviewed changes

remove errors

9a623c8

pull-request-size bot added size/L and removed size/M labels Aug 4, 2021

hughhhh added 2 commits August 4, 2021 17:00

fix number

1fa713c

fix test

3b3c5ae

betodealmeida approved these changes Aug 9, 2021

View reviewed changes

hughhhh merged commit fd80ae3 into master Aug 10, 2021

superset-github-bot bot added the preset:2021.31 label Aug 13, 2021

yousoph removed the preset:2021.31 label Aug 14, 2021

opus-42 pushed a commit to opus-42/incubator-superset that referenced this pull request Nov 14, 2021

fix: Make sheet_name into a ValidationInputError (apache#16056)

7e1e9b9

* setup validates for name * add error type * fix linting * fix test * remove errors * fix number * fix test

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

fix: Make sheet_name into a ValidationInputError (apache#16056)

35e3344

* setup validates for name * add error type * fix linting * fix test * remove errors * fix number * fix test

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

mistercrunch deleted the hugh/gsheets-name-validation branch March 26, 2024 17:56

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix: Make sheet_name into a `ValidationInputError` #16056

fix: Make sheet_name into a `ValidationInputError` #16056

hughhhh commented Aug 4, 2021 •

edited

codecov bot commented Aug 4, 2021 •

edited

betodealmeida left a comment

betodealmeida Aug 4, 2021

betodealmeida Aug 4, 2021

betodealmeida left a comment

betodealmeida Aug 9, 2021

betodealmeida Aug 9, 2021

rosemarie-chiu commented Aug 13, 2021

	idx = 0
	for name, url in table_catalog.items():
	for ixd, (name, url) in enumerate(table_catalog.items()):

fix: Make sheet_name into a ValidationInputError #16056

fix: Make sheet_name into a ValidationInputError #16056

Conversation

hughhhh commented Aug 4, 2021 • edited

SUMMARY

BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF

Empty URL

Empty Sheet Name

TESTING INSTRUCTIONS

ADDITIONAL INFORMATION

codecov bot commented Aug 4, 2021 • edited

Codecov Report

betodealmeida left a comment

Choose a reason for hiding this comment

betodealmeida Aug 4, 2021

Choose a reason for hiding this comment

betodealmeida Aug 4, 2021

Choose a reason for hiding this comment

betodealmeida left a comment

Choose a reason for hiding this comment

betodealmeida Aug 9, 2021

Choose a reason for hiding this comment

betodealmeida Aug 9, 2021

Choose a reason for hiding this comment

rosemarie-chiu commented Aug 13, 2021

fix: Make sheet_name into a `ValidationInputError` #16056

fix: Make sheet_name into a `ValidationInputError` #16056

hughhhh commented Aug 4, 2021 •

edited

codecov bot commented Aug 4, 2021 •

edited