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

fix: save dataset and repopulate state #20965

Merged

Conversation

eschutho
Copy link
Member

@eschutho eschutho commented Aug 3, 2022

SUMMARY

This PR fixes a bug in the sqllab to explore chart with a query flow where upon saving, there was an error saying that the form fields had not been transferred when they had.

BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF

Before:
Clipboard 2022-29-07 at 11 00 05 AM

After:
_DEV__Superset

TESTING INSTRUCTIONS

Repro steps
1, query in sql and click create chart
2, create chart in explore and save chart
3, in save chart modal when chart power=query, remove dataset name and leave it blank
4, click save button

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 Aug 3, 2022

Codecov Report

Merging #20965 (8af9fe7) into master (eb5369f) will decrease coverage by 11.52%.
The diff coverage is 30.68%.

❗ Current head 8af9fe7 differs from pull request most recent head bdf20f9. Consider uploading reports for the commit bdf20f9 to get more accurate results

@@             Coverage Diff             @@
##           master   #20965       +/-   ##
===========================================
- Coverage   66.38%   54.86%   -11.53%     
===========================================
  Files        1767     1767               
  Lines       67232    67284       +52     
  Branches     7138     7140        +2     
===========================================
- Hits        44633    36916     -7717     
- Misses      20773    28541     +7768     
- Partials     1826     1827        +1     
Flag Coverage Δ
hive 53.15% <14.03%> (-0.05%) ⬇️
mysql ?
postgres ?
presto 53.05% <14.03%> (-0.05%) ⬇️
python 57.78% <14.03%> (-23.80%) ⬇️
sqlite ?
unit 50.47% <8.77%> (-0.06%) ⬇️

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

Impacted Files Coverage Δ
...set-ui-core/src/ui-overrides/ExtensionsRegistry.ts 100.00% <ø> (ø)
...frontend/src/SqlLab/components/SaveQuery/index.tsx 74.35% <ø> (ø)
...ols/MetricControl/AdhocMetricEditPopover/index.jsx 75.53% <ø> (ø)
...et-frontend/src/explore/reducers/exploreReducer.js 40.00% <0.00%> (ø)
superset-frontend/src/utils/datasourceUtils.js 100.00% <ø> (ø)
superset/models/helpers.py 36.09% <6.66%> (-4.70%) ⬇️
...rset-frontend/src/explore/components/SaveModal.tsx 37.61% <10.00%> (ø)
superset/models/sql_lab.py 72.58% <25.00%> (-6.17%) ⬇️
...set-frontend/src/explore/actions/exploreActions.ts 58.33% <50.00%> (ø)
superset/common/query_context_processor.py 63.06% <66.66%> (-26.94%) ⬇️
... and 300 more

📣 Codecov can now indicate which changes are the most critical in Pull Requests. Learn more

@eschutho eschutho force-pushed the elizabeth/chart-power-query-save-fix branch from 532561f to ef1b78e Compare August 3, 2022 22:01
metrics?: string[];
columns?: string[];
};
}
Copy link
Member Author

Choose a reason for hiding this comment

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

This is a first attempt at creating a set of types of that map to the api. cc @hughhhh cc @eric-briscoe. We talked about auto-generating this with Marshmallow, but this seemed like a decent start. Thoughts?

@eschutho eschutho force-pushed the elizabeth/chart-power-query-save-fix branch 2 times, most recently from 081228c to 18e3c12 Compare August 4, 2022 00:11
@yousoph
Copy link
Member

yousoph commented Aug 4, 2022

/testenv up

@github-actions
Copy link
Contributor

github-actions bot commented Aug 4, 2022

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

@yousoph
Copy link
Member

yousoph commented Aug 4, 2022

image
Seeing that if a chart name and dashboard are selected but no dataset, the Save & Go to Dashboard button is enabled - it should be disabled when the Dataset name is empty

@eschutho
Copy link
Member Author

eschutho commented Aug 4, 2022

@yousoph thanks for catching that. I just pushed up a fix for the dashboard button too.

@eschutho eschutho force-pushed the elizabeth/chart-power-query-save-fix branch from 574fe39 to f38811d Compare August 4, 2022 17:32
@yousoph
Copy link
Member

yousoph commented Aug 4, 2022

/testenv up

inner_to_dttm or to_dttm,
)
]
if isinstance(dttm_col, dict):
Copy link
Member

Choose a reason for hiding this comment

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

@@ -1331,7 +1334,8 @@ def get_sqla_query( # pylint: disable=too-many-arguments,too-many-locals,too-ma

columns_by_name: Dict[str, "TableColumn"] = {
col.get("column_name"): col
for col in self.columns # col.column_name: col for col in self.columns
# col.column_name: col for col in self.columns
for col in self.columns
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 for col in self.columns: it's missing the colon

Copy link
Member Author

Choose a reason for hiding this comment

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

It looks like it's the end of a dictionary comprehension, not a for loop, so no colon needed. The comment change btw was just part of my automated linting on line length. I'm not actually sure what the comment means, too. :)


return json_success(
json.dumps(
{"table_id": table.id, "data": sanitize_datasource_data(table.data)}
Copy link
Member

Choose a reason for hiding this comment

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

👌🏾

@yousoph
Copy link
Member

yousoph commented Aug 4, 2022

/testenv up

@github-actions
Copy link
Contributor

github-actions bot commented Aug 4, 2022

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

@github-actions
Copy link
Contributor

github-actions bot commented Aug 4, 2022

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

@eschutho eschutho force-pushed the elizabeth/chart-power-query-save-fix branch from f38811d to 9a43dd2 Compare August 4, 2022 21:08
@@ -119,7 +120,7 @@ export const hydrateExplore =
controls: initialControls,
form_data: initialFormData,
slice: initialSlice,
controlsTransferred: [],
controlsTransferred: explore.controlsTransferred,
Copy link
Member Author

Choose a reason for hiding this comment

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

@michael-s-molina do you see any problems with us pulling in state here on hydrate? It's not a property that exists in the persisted db.

Copy link
Member

Choose a reason for hiding this comment

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

Yes. The hydrateExplore is the action responsible for mounting the Explore state so it shouldn't have a reference to itself. It will only work because there's a known problem where the save action is firing hydrateExplore again but it shouldn't. We're calling hydrateExplore on save intentionally because during the SPA work we wanted to avoid reloading the page but many of the existing logic depended on that. It would require a major refactor to change this behavior. Currently, explore.controlsTransferred is being updated by UPDATE_FORM_DATA_BY_DATASOURCE action. You'll probably won't need all the logic inside that action, so I suggest creating a new action to update this piece of state and call this new action from your code.

Ping me or @kgabryje if you need more assistance on this.

fetchMock.post(saveDatasetEndpoint, saveDatasetResponse);
const dispatch = sinon.spy();
const getState = sinon.spy(() => ({ explore: { datasource } }));
const dataset = await saveDataset(SAVE_DATASET_POST_ARGS)(dispatch);
Copy link
Member

Choose a reason for hiding this comment

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

nice way to test this!

@pkdotson
Copy link
Member

pkdotson commented Aug 5, 2022

looks good to me baring any of Michael's feedback! I'l approve!

@eschutho
Copy link
Member Author

eschutho commented Aug 5, 2022

/testenv up

@github-actions
Copy link
Contributor

github-actions bot commented Aug 5, 2022

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

@jinghua-qa
Copy link
Member

LGTM

@eschutho eschutho merged commit 463406f into apache:master Aug 5, 2022
@github-actions
Copy link
Contributor

github-actions bot commented Aug 5, 2022

Ephemeral environment shutdown and build artifacts deleted.

jinghua-qa pushed a commit to preset-io/superset that referenced this pull request Aug 5, 2022
* save dataset and repopulate state

* disable dashboard button if dataset is missing

* fix error message

* fix tests

(cherry picked from commit 463406f)
@yousoph yousoph assigned yousoph and unassigned yousoph Aug 8, 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 preset:2022.31 size/L 🚢 2.1.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants