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

chore: add changed_on_delta_humanized field on dashboard schema #15542

Merged
merged 2 commits into from
Jul 7, 2021

Conversation

zhaoyongjie
Copy link
Member

SUMMARY

added changed_on_delta_humanized field on dashboard schema. ensure rest API has this response data.

related PR: #15186

BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF

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

@zhaoyongjie zhaoyongjie requested a review from suddjian July 5, 2021 10:06
@zhaoyongjie zhaoyongjie requested a review from a team July 5, 2021 10:06
@@ -159,6 +159,7 @@ class DashboardGetResponseSchema(Schema):
owners = fields.List(fields.Nested(UserSchema))
roles = fields.List(fields.Nested(RolesSchema))
table_names = fields.String() # legacy nonsense
changed_on_humanized = fields.String(data_key="changed_on_delta_humanized")
Copy link
Member Author

Choose a reason for hiding this comment

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

  1. superset.models.helps.AuditMixinNullable.changed_on_delta_humanized is a instance method, unable to serializes.
  2. ensure DashboardRestApi list_column and dashboard metadata are the same

Copy link
Member

Choose a reason for hiding this comment

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

to ensure 2. we should add show_columns = list_columns on the REST API class, and follow the same existing pattern

Copy link
Member Author

Choose a reason for hiding this comment

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

I will follow this PR, thanks, Daniel.

@codecov
Copy link

codecov bot commented Jul 5, 2021

Codecov Report

Merging #15542 (dda6fb7) into master (000a038) will increase coverage by 18.03%.
The diff coverage is n/a.

Impacted file tree graph

@@             Coverage Diff             @@
##           master   #15542       +/-   ##
===========================================
+ Coverage   58.93%   76.96%   +18.03%     
===========================================
  Files         373      976      +603     
  Lines       12014    51298    +39284     
  Branches     2945     6912     +3967     
===========================================
+ Hits         7080    39480    +32400     
- Misses       4755    11599     +6844     
- Partials      179      219       +40     
Flag Coverage Δ
hive 81.33% <ø> (?)
mysql 81.60% <ø> (?)
postgres 81.62% <ø> (?)
presto 81.32% <ø> (?)
python 82.14% <ø> (?)
sqlite 81.24% <ø> (?)

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

Impacted Files Coverage Δ
...nd/src/explore/components/ExploreViewContainer.jsx 2.15% <0.00%> (-44.33%) ⬇️
superset-frontend/src/explore/store.js 57.14% <0.00%> (-35.17%) ⬇️
superset-frontend/src/featureFlags.ts 66.66% <0.00%> (-25.00%) ⬇️
...set-frontend/src/dashboard/reducers/datasources.js 36.36% <0.00%> (-20.78%) ⬇️
superset-frontend/src/SqlLab/components/App.jsx 57.14% <0.00%> (-20.64%) ⬇️
...t-frontend/src/dashboard/reducers/sliceEntities.js 87.50% <0.00%> (-12.50%) ⬇️
superset-frontend/src/explore/controls.jsx 28.57% <0.00%> (-11.58%) ⬇️
...ashboard/components/gridComponents/ChartHolder.jsx 68.68% <0.00%> (-10.98%) ⬇️
superset-frontend/src/components/ListView/utils.ts 85.12% <0.00%> (-9.33%) ⬇️
...c/dashboard/components/gridComponents/Markdown.jsx 82.82% <0.00%> (-8.96%) ⬇️
... and 1119 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 83d50dd...dda6fb7. Read the comment docs.

@zhaoyongjie zhaoyongjie added the need:review Requires a code review label Jul 5, 2021
Copy link

@fintzd fintzd left a comment

Choose a reason for hiding this comment

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

looks good to me!

@junlincc junlincc requested a review from nytai July 6, 2021 22:29
@stephenLYZ
Copy link
Member

LGTM!

@suddjian suddjian merged commit ae160f7 into apache:master Jul 7, 2021
cccs-RyanS pushed a commit to CybercentreCanada/superset that referenced this pull request Dec 17, 2021
…he#15542)

* chore: add changed_on_delta_humanized field on dashboard schema

* fix ut
QAlexBall pushed a commit to QAlexBall/superset that referenced this pull request Dec 29, 2021
…he#15542)

* chore: add changed_on_delta_humanized field on dashboard schema

* fix ut
cccs-rc pushed a commit to CybercentreCanada/superset that referenced this pull request Mar 6, 2024
…he#15542)

* chore: add changed_on_delta_humanized field on dashboard schema

* fix ut
@mistercrunch mistercrunch added 🏷️ bot A label used by `supersetbot` to keep track of which PR where auto-tagged with release labels 🚢 1.3.0 labels Mar 12, 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 need:review Requires a code review size/XS 🚢 1.3.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants