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.

Sign up for GitHub

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

Jump to bottom

Retire dashboard v1 (js and python) #5418

Merged
merged 1 commit into from
Jul 24, 2018
Merged

Retire dashboard v1 (js and python) #5418

merged 1 commit into from
Jul 24, 2018

Conversation

graceguo-supercat
Copy link

@graceguo-supercat graceguo-supercat commented Jul 18, 2018 •
edited
Loading

this PR depends on #5463.

Remove dashboard v1-only and v1 => v2 transition related logic, and update unit tests.

build passed 😂

@williaster @mistercrunch

@graceguo-supercat graceguo-supercat force-pushed the gg-RetireDashboardV1 branch 10 times, most recently from 5d2ce25 to 7838098 Compare July 18, 2018 18:05
@codecov-io
Copy link

codecov-io commented Jul 18, 2018 •
edited
Loading

Codecov Report

Merging #5418 into master will increase coverage by 4.21%.
The diff coverage is 59.09%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #5418      +/-   ##
==========================================
+ Coverage   59.09%   63.31%   +4.21%     
==========================================
  Files         372      349      -23     
  Lines       23747    22096    -1651     
  Branches     2758     2455     -303     
==========================================
- Hits        14033    13989      -44     
+ Misses       9699     8093    -1606     
+ Partials       15       14       -1
Impacted Files Coverage Δ
...rset/assets/src/dashboard/components/Dashboard.jsx 80.64% <ø> (+0.43%) ⬆️
superset/assets/src/logger.js 95.31% <ø> (-0.34%) ⬇️
...rset/assets/src/dashboard/components/SaveModal.jsx 71.11% <ø> (-1.23%) ⬇️
...ssets/src/dashboard/containers/DashboardHeader.jsx 100% <ø> (ø) ⬆️
...et/assets/src/dashboard/reducers/dashboardState.js 94.59% <ø> (ø) ⬆️
...t/assets/src/dashboard/reducers/getInitialState.js 0% <0%> (ø) ⬆️
superset/views/core.py 75.05% <0%> (+1.94%) ⬆️
...uperset/assets/src/dashboard/components/Header.jsx 59.3% <100%> (+0.88%) ⬆️
superset/assets/src/explore/constants.js 100% <100%> (ø) ⬆️
...src/dashboard/components/HeaderActionsDropdown.jsx 81.81% <100%> (+1.81%) ⬆️
... and 3 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 99ce7b7...75019cd. Read the comment docs.

@graceguo-supercat graceguo-supercat force-pushed the gg-RetireDashboardV1 branch 2 times, most recently from 5c80682 to f236971 Compare July 18, 2018 22:31
williaster
williaster reviewed Jul 18, 2018
View reviewed changes
Copy link
Contributor

@williaster williaster left a comment

Choose a reason for hiding this comment

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

couple minor changes but lgtm overall! this is exciting! 🎉

Not sure how you are thinking about merging things but I feel like frontend deprecation shouldn't be merged until after the python migration script is in place. wdyt @john-bodley @mistercrunch ?

superset/assets/src/dashboard/components/Dashboard.jsx Outdated
const version = nextProps.dashboardState.isV2Preview
? 'v2-preview'
: 'v2';
const version = 'v2';
Copy link
Contributor

Choose a reason for hiding this comment

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

nit I wouldn't define a variable for this, you can just define it as part of the object below.

Copy link
Author

Choose a reason for hiding this comment

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

fixed.

superset/assets/src/dashboard/components/SaveModal.jsx Outdated
'Save Dashboard%s',
isV2Preview ? ' (⚠️ all saved dashboards will be V2)' : '',
)}
modalTitle={t('Save Dashboard%s')}
Copy link
Contributor

Choose a reason for hiding this comment

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

extra % I think?

Copy link
Author

Choose a reason for hiding this comment

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

fixed. removed %s

superset/views/core.py Outdated
# Hack to log the dashboard_id properly, even when getting a slug
@log_this
def dashboard(**kwargs): # noqa
pass

# TODO remove extra logging upon v1 deprecation 🎉
dashboard(
Copy link
Contributor

Choose a reason for hiding this comment

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

I think you still need to call the dashboard() method or there will be no logging. just need to remove the v1/2-switch-specific kwargs.

Copy link
Author

Choose a reason for hiding this comment

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

fixed.

@williaster
Copy link
Contributor

I wonder also if we should delete the markdown + separator slice types?

@graceguo-supercat
Copy link
Author

i think if user ever saved a separator or markdown slice before, they should still be able to access by slice_id or etc. Also for geo related viz, we allow user give custom js code as render function. User define their js code in markdown type viz.

So I didn't delete those viz type, but i added a logic in Save to dashboard. if current viz_type is separator or markdown, the save to dash option is disabled (so that this slice can't be added to dashboard from explore view).

williaster
williaster reviewed Jul 23, 2018
View reviewed changes
superset/assets/package.json
@@ -102,7 +102,6 @@
"react-dnd-html5-backend": "^2.5.4",
"react-dom": "^15.6.2",
"react-gravatar": "^2.6.1",
"react-grid-layout": "0.16.6",
Copy link
Contributor

Choose a reason for hiding this comment

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

🎉

williaster
williaster approved these changes Jul 23, 2018
View reviewed changes
Copy link
Contributor

@williaster williaster left a comment

Choose a reason for hiding this comment

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

This LGTM, I would still say the DB migration (in #5463) should be merged before this tho otherwise folks might view an unconverted v1 dashboard which would fail.

@graceguo-supercat graceguo-supercat mentioned this pull request Jul 24, 2018
Merged
@graceguo-supercat graceguo-supercat merged commit 3f2fc8f into apache:master Jul 24, 2018
@graceguo-supercat graceguo-supercat mentioned this pull request Jul 24, 2018
Merged
graceguo-supercat pushed a commit to graceguo-supercat/superset that referenced this pull request Jul 26, 2018
retire dashboard v1 (js and python) (apache#5418)
9f4a4f2 
(cherry picked from commit 3f2fc8f)
@graceguo-supercat graceguo-supercat mentioned this pull request Jul 27, 2018
Merged
@graceguo-supercat graceguo-supercat deleted the gg-RetireDashboardV1 branch August 6, 2018 23:36
wenchma pushed a commit to wenchma/incubator-superset that referenced this pull request Nov 16, 2018
retire dashboard v1 (js and python) (apache#5418)
33d07e2
@smacker smacker mentioned this pull request Oct 22, 2019
Closed
@mistercrunch mistercrunch added 🏷️ bot A label used by `supersetbot` to keep track of which PR where auto-tagged with release labels 🚢 0.28.0 labels Feb 27, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@williaster williaster williaster approved these changes

Assignees
No one assigned
Labels
🏷️ bot A label used by `supersetbot` to keep track of which PR where auto-tagged with release labels 🚢 0.28.0
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@graceguo-supercat @codecov-io @williaster @mistercrunch