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

feat(report): add tab selector for dashboard reports #19354

Closed

Conversation

graceguo-supercat
Copy link

@graceguo-supercat graceguo-supercat commented Mar 24, 2022

SUMMARY

Add Dashboard tab Selector in alert/report config, following the discussion #18475. This feature's backend support is completed by #17749.

  1. Alerts/Reports List View:

4iS3ZVawuH

  • we will show dashboard layout in a tree structure, and group charts under their parent tab.
  • user can pick single dashboard tab for report
  • Charts included in the report will be automatically checked/selected, but user can not select single chart.
  • if user uncheck all tabs, or dashboard report has no tab selected, we will capture dashboard from its landing page (always default to first tab).
  1. Dashboard Shortcut:

r6Z5DZogBa

  • in dashboard context, we will not show dashboard layout.
  • user can select default tab (tabs when user landing on dashboard), or currently opened tab.

TESTING INSTRUCTIONS

CI and manual tests.

FUTURE WORKS

There are a few advanced use-cases are not covered by this PR:

  1. Currently we only allow 1 screenshot per email report: User can select single dashboard tab for the email report, but not multiple tabs. We found the UI configuration that allow user to select multiple tabs for a dashboard is a little tricky, it might need extra user research and help from design team.
  2. Multiple tabs belong to different parent tabs component: For example, if dashboard layout is:
root 
   ROW_1
       TAB_1a  | TAB_1b  | TAB_1c
   ROW_2   
       TAB_2a | TAB_2b | TAB_2c

User can select child tab TAB_1b or TAB_2b, but not both TAB_1b and TAB_2b. The reason is that currently dashboard anchor link only support 1 component id. In order to support children from multiple layout branches, we nee dot improve the dashboard anchor feature first.

ADDITIONAL INFORMATION

  • Has associated issue: [alert/report] Solution for dashboards with multiple tabs #18475
  • 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

@graceguo-supercat graceguo-supercat force-pushed the gg-tab-selector branch 6 times, most recently from 8ea526e to fdc147a Compare March 25, 2022 05:33
@codecov
Copy link

codecov bot commented Mar 28, 2022

Codecov Report

Merging #19354 (331513b) into master (cadd259) will decrease coverage by 0.32%.
The diff coverage is 61.70%.

❗ Current head 331513b differs from pull request most recent head 867aade. Consider uploading reports for the commit 867aade to get more accurate results

@@            Coverage Diff             @@
##           master   #19354      +/-   ##
==========================================
- Coverage   66.80%   66.47%   -0.33%     
==========================================
  Files        1752     1675      -77     
  Lines       65457    64093    -1364     
  Branches     6912     6519     -393     
==========================================
- Hits        43726    42604    -1122     
+ Misses      19982    19798     -184     
+ Partials     1749     1691      -58     
Flag Coverage Δ
hive ?
mysql 81.91% <100.00%> (?)
postgres 81.95% <100.00%> (-0.51%) ⬇️
presto 52.55% <100.00%> (-1.08%) ⬇️
python 82.17% <100.00%> (-0.65%) ⬇️
sqlite 81.73% <100.00%> (?)
unit ?

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

Impacted Files Coverage Δ
...set-frontend/src/components/ReportModal/styles.tsx 100.00% <ø> (ø)
...src/dashboard/components/FiltersBadge/selectors.ts 68.46% <0.00%> (ø)
...tersConfigModal/FiltersConfigForm/DefaultValue.tsx 9.09% <0.00%> (ø)
...nd/src/dashboard/components/nativeFilters/utils.ts 46.66% <0.00%> (ø)
...end/src/dashboard/util/getChartIdsInFilterScope.ts 0.00% <ø> (ø)
superset/reports/api.py 89.84% <ø> (-0.39%) ⬇️
...frontend/src/views/CRUD/alert/AlertReportModal.tsx 52.38% <36.84%> (+0.29%) ⬆️
...ponents/nativeFilters/FilterCard/useFilterScope.ts 79.62% <75.00%> (ø)
...rset-frontend/src/components/ReportModal/index.tsx 82.08% <100.00%> (+3.75%) ⬆️
...ConfigModal/FiltersConfigForm/FilterScope/state.ts 86.66% <100.00%> (ø)
... and 661 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 cadd259...867aade. Read the comment docs.

@ktmud
Copy link
Member

ktmud commented Apr 13, 2022

Partially fixed the merge conflict but the feature is likely now broken. Will test and update

@ktmud ktmud force-pushed the gg-tab-selector branch from dc7a7c2 to 9f52a89 Compare May 4, 2022 06:33
@ktmud ktmud force-pushed the gg-tab-selector branch 2 times, most recently from 777c78b to 82a8ecd Compare May 4, 2022 17:25
@ktmud ktmud force-pushed the gg-tab-selector branch from 82a8ecd to 0bfd9a6 Compare June 29, 2022 17:34
@graceguo-supercat graceguo-supercat requested a review from a team as a code owner June 29, 2022 17:34
@ktmud ktmud force-pushed the gg-tab-selector branch from 0bfd9a6 to 867aade Compare June 29, 2022 17:38
@ktmud ktmud removed request for a team and michael-s-molina June 29, 2022 17:38
@ktmud ktmud self-assigned this Jun 29, 2022
@ktmud ktmud changed the title feat(report): add dashboard tab selector for Report/Alert feat(report): add tab selector for dashboard reports Jun 29, 2022
@ktmud ktmud force-pushed the gg-tab-selector branch 2 times, most recently from c098b7d to b2c5032 Compare July 12, 2022 23:54
@ktmud ktmud force-pushed the gg-tab-selector branch from b2c5032 to 37025dc Compare July 15, 2022 01:16
@ktmud
Copy link
Member

ktmud commented Jul 20, 2022

I will open a new PR to retain the full iteration history.

@ktmud ktmud closed this Jul 20, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants