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: change database save in DatasourceEditor #9255

Merged
merged 2 commits into from
Mar 10, 2020

Conversation

mistercrunch
Copy link
Member

This addresses the issue where pointing a datasource to another database
in the datasource editor is not reflected.

Also addresses:

  • a minor cosmetic issue in the datasource editor.
  • user/owners list not getting populated

CATEGORY

Choose one

  • Bug Fix
  • Enhancement (new features, refinement)
  • Refactor
  • Add tests
  • Build / Development Environment
  • Documentation

This addresses the issue where pointing a datasource to another database
in the datasource editor is not reflected.

Also addresses:
- a minorcosmetic issue in the datasource editor.
- user/owners list not getting populated
@codecov-io
Copy link

Codecov Report

Merging #9255 into master will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##           master    #9255   +/-   ##
=======================================
  Coverage   58.92%   58.92%           
=======================================
  Files         373      373           
  Lines       12016    12016           
  Branches     2948     2948           
=======================================
  Hits         7081     7081           
  Misses       4756     4756           
  Partials      179      179
Impacted Files Coverage Δ
...erset-frontend/src/datasource/DatasourceEditor.jsx 61.25% <ø> (ø) ⬆️

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 89109a1...c125195. Read the comment docs.

@@ -745,6 +745,7 @@ def import_dashboards(self):
try:
dashboard_import_export.import_dashboards(db.session, f.stream)
except DatabaseNotFound as e:
logger.exception(e)
Copy link
Member

Choose a reason for hiding this comment

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

👍

@@ -268,7 +268,7 @@ def create_fake_db(self):
cls=models.Database,
criteria={"database_name": database_name},
session=db.session,
sqlalchemy_uri="sqlite://test",
sqlalchemy_uri="sqlite:///:memory:",
Copy link
Member

Choose a reason for hiding this comment

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

👍

@@ -18,6 +18,7 @@
datasource_post = {
"id": None,
"column_formats": {"ratio": ".2%"},
"database": {"id": 1},
Copy link
Member

Choose a reason for hiding this comment

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

Eventually it would be good to move these to randomly generated IDs (note to self)

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes, actually here I replace it in the test itself. Not sure what the proper approach here, a function that receives an object and returns a fixture? I'm guessing with DAO abstraction we can get more creative on generating proper fixtures.

@mistercrunch mistercrunch merged commit 116200c into apache:master Mar 10, 2020
@mistercrunch mistercrunch deleted the fix_db_save branch March 10, 2020 16:20
@mistercrunch mistercrunch added 🏷️ bot A label used by `supersetbot` to keep track of which PR where auto-tagged with release labels 🚢 0.36.0 labels Feb 28, 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 size/S 🚢 0.36.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants