feat(dashboard): embedded dashboard UI configuration #17175

lilykuang · 2021-10-20T21:53:10Z

Add a provider to hide chart controls for dashboard
More work will be done later for embedded dashboard

SUMMARY

BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF

TESTING INSTRUCTIONS

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

suddjian

Awesome work! I love how clean this refactor is. Left some nits/opinions, mostly about variable names :)

suddjian · 2021-10-27T17:26:05Z

superset-frontend/src/constants.ts

@@ -27,6 +27,10 @@ export const URL_PARAMS = {
    name: 'standalone',
    type: 'number',
  },
+  embedded: {


Let's add a comment here about how this works

suddjian · 2021-10-27T17:27:26Z

superset-frontend/src/components/Emmbedded/index.tsx

+  config: number;
+}
+
+export const EmbeddedContext = createContext<EmbeddedConfigType>({


I think we should call this UiConfigContext, so that it can be used for things other than embedded dashboards. I think that better describes what this context is to be used for.

suddjian · 2021-10-27T17:27:52Z

superset-frontend/src/components/Emmbedded/index.tsx

+  hideChartFilter: false,
+});
+
+export const useEmbedded = () => useContext(EmbeddedContext);


and let's call this useUiConfig

suddjian · 2021-10-27T17:28:27Z

superset-frontend/src/components/Emmbedded/index.tsx

@@ -0,0 +1,57 @@
+/**


typo in the directory

Also, given the other suggested naming changes below, we should probably name the directory UiConfigContext.

suddjian · 2021-10-27T17:33:12Z

superset-frontend/src/components/Emmbedded/index.tsx

+
+export const useEmbedded = () => useContext(EmbeddedContext);
+
+export const EmbeddedProvider: React.FC<EmbeddedProviderProps> = ({


We can call this EmbeddedUiConfigProvider. In the future we could have FullUiConfigProvider, and potentially others that provide the UiConfigContext in different ways.

suddjian · 2021-10-27T17:40:26Z

superset-frontend/src/components/Emmbedded/index.tsx

+    hideTitle: formattedConfig[0] === '1',
+    hideTab: formattedConfig[1] === '1',
+    hideNav: formattedConfig[2] === '1',
+    hideChartFilter: formattedConfig[3] === '1',


This is a more standard way to do bit flags:

Suggested change

hideTitle: formattedConfig[0] === '1',

hideTab: formattedConfig[1] === '1',

hideNav: formattedConfig[2] === '1',

hideChartFilter: formattedConfig[3] === '1',

hideTitle: config & 1 !== 0,

hideTab: config & 2 !== 0,

hideNav: config & 4 !== 0,

hideChartFilter: config & 8 !== 0,

Also, those numbers could be moved out into named constants to increase clarity.

That said, depending on the embedded dashboard implementation we might want to switch to named flags instead of bit flags.

Also can we call the last one hideChartControls since it covers both the kebab menu and the filter?

suddjian · 2021-10-27T17:46:23Z

superset-frontend/src/views/App.tsx

@@ -63,19 +66,23 @@ const RootContextProviders: React.FC = ({ children }) => {
    lastLocationPathname = location.pathname;
  }, [location.pathname]);

+  const config = getUrlParam(URL_PARAMS.embedded);


Suggestion: I would move this line into the EmbeddedProvider for better encapsulation. And/or make the prop optional and use getUrlParam by default if no config is provided.

codecov · 2021-11-08T22:54:41Z

Codecov Report

Merging #17175 (44cfb7e) into master (33a8e87) will increase coverage by 0.00%.
The diff coverage is 80.76%.

@@           Coverage Diff           @@
##           master   #17175   +/-   ##
=======================================
  Coverage   77.14%   77.15%           
=======================================
  Files        1036     1037    +1     
  Lines       55759    55779   +20     
  Branches     7628     7637    +9     
=======================================
+ Hits        43013    43034   +21     
  Misses      12490    12490           
+ Partials      256      255    -1

Flag	Coverage Δ
javascript	`71.43% <80.76%> (+0.02%)`	⬆️

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

Impacted Files	Coverage Δ
superset-frontend/src/constants.ts	`100.00% <ø> (ø)`
superset-frontend/src/views/App.tsx	`0.00% <0.00%> (ø)`
...-frontend/src/components/UiConfigContext/index.tsx	`60.00% <60.00%> (ø)`
superset-frontend/src/components/Menu/Menu.tsx	`70.40% <100.00%> (+0.61%)`	⬆️
...d/components/DashboardBuilder/DashboardBuilder.tsx	`91.12% <100.00%> (+0.29%)`	⬆️
...end/src/dashboard/components/SliceHeader/index.tsx	`93.44% <100.00%> (+0.71%)`	⬆️
...ols/FilterControl/AdhocFilterEditPopover/index.jsx	`66.10% <0.00%> (-3.35%)`	⬇️
...d/src/explore/components/ExploreContentPopover.tsx	`100.00% <0.00%> (ø)`
...components/controls/FixedOrMetricControl/index.jsx	`83.92% <0.00%> (ø)`
...mponents/controls/MetricControl/MetricsControl.jsx	`38.18% <0.00%> (ø)`
... and 2 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 33a8e87...44cfb7e. Read the comment docs.

suddjian · 2021-11-09T18:31:46Z

This looks great. I think rather than merging it into master, though, we should start a long-lived feature branch for embedded and open our PRs into that. What do you think?

lilykuang · 2021-11-09T18:37:19Z

@suddjian feature branch sounds good. I will create feature branch called embedded

* setup embedded provider * update ui configuration * fix test

* feat(dashboard): embedded dashboard UI configuration (#17175) (#17450) * setup embedded provider * update ui configuration * fix test * feat: Guest token (for embedded dashboard auth) (#17517) * generate an embed token * improve existing tests * add some auth setup, and rename token * fix the stuff for compatibility with external request loaders * docs, standard jwt claims, tweaks * black * lint * tests, and safer token decoding * linting * type annotation * prettier * add feature flag * quiet pylint * apparently typing is a problem again * Make guest role name configurable * fake being a non-anonymous user * just one log entry * customizable algo * lint * lint again * 403 works now! * get guest token from header instead of cookie * Revert "403 works now!" This reverts commit df2f49a. * fix tests * Revert "Revert "403 works now!"" This reverts commit 883dff3. * rename method * correct import * feat: entry for embedded dashboard (#17529) * create entry for embedded dashboard in webpack * add cookies * lint * token message handshake * guestTokenHeaderName * use setupClient instead of calling configure * rename the webpack chunk * simplified handshake * embedded entrypoint: render a proper app * make the embedded page accept anonymous connections * format * lint * fix test # Conflicts: # superset-frontend/src/embedded/index.tsx # superset/views/core.py * lint * Update superset-frontend/src/embedded/index.tsx Co-authored-by: David Aaron Suddjian <[email protected]> * comment out origins checks * move embedded for core to dashboard * pylint * isort Co-authored-by: David Aaron Suddjian <[email protected]> Co-authored-by: David Aaron Suddjian <[email protected]> * feat: Authorizing guest access to embedded dashboards (#17757) * helper methods and dashboard access * guest token dashboard authz * adjust csrf exempt list * eums don't work that way * Remove unnecessary import * move row level security tests to their own file * a bit of refactoring * add guest token security tests * refactor tests * clean imports * variable names can be too long apparently * missing argument to get_user_roles * don't redefine builtins * remove unused imports * fix test import * default to global user when getting roles * missing import * mock it * test get_user_roles * infer g.user for ease of tests * remove redundant check * tests for guest user security manager fns * use algo to get rid of warning messages * tweaking access checks * fix guest token security tests * missing imports * more tests * more testing and also some small refactoring * move validation out of parsing * fix dashboard access check again * add more test Co-authored-by: Lily Kuang <[email protected]> * feat: Row Level Security rules for guest tokens (#17836) * helper methods and dashboard access * guest token dashboard authz * adjust csrf exempt list * eums don't work that way * Remove unnecessary import * move row level security tests to their own file * a bit of refactoring * add guest token security tests * refactor tests * clean imports * variable names can be too long apparently * missing argument to get_user_roles * don't redefine builtins * remove unused imports * fix test import * default to global user when getting roles * missing import * mock it * test get_user_roles * infer g.user for ease of tests * remove redundant check * tests for guest user security manager fns * use algo to get rid of warning messages * tweaking access checks * fix guest token security tests * missing imports * more tests * more testing and also some small refactoring * move validation out of parsing * fix dashboard access check again * rls rules for guest tokens * test guest token rls rules * more flexible rls rules * lint * fix tests * fix test * defaults * fix some tests * fix some tests * lint Co-authored-by: Lily Kuang <[email protected]> * SupersetClient guest token test * Apply suggestions from code review Co-authored-by: Lily Kuang <[email protected]> Co-authored-by: Lily Kuang <[email protected]>

* feat(dashboard): embedded dashboard UI configuration (apache#17175) (apache#17450) * setup embedded provider * update ui configuration * fix test * feat: Guest token (for embedded dashboard auth) (apache#17517) * generate an embed token * improve existing tests * add some auth setup, and rename token * fix the stuff for compatibility with external request loaders * docs, standard jwt claims, tweaks * black * lint * tests, and safer token decoding * linting * type annotation * prettier * add feature flag * quiet pylint * apparently typing is a problem again * Make guest role name configurable * fake being a non-anonymous user * just one log entry * customizable algo * lint * lint again * 403 works now! * get guest token from header instead of cookie * Revert "403 works now!" This reverts commit df2f49a. * fix tests * Revert "Revert "403 works now!"" This reverts commit 883dff3. * rename method * correct import * feat: entry for embedded dashboard (apache#17529) * create entry for embedded dashboard in webpack * add cookies * lint * token message handshake * guestTokenHeaderName * use setupClient instead of calling configure * rename the webpack chunk * simplified handshake * embedded entrypoint: render a proper app * make the embedded page accept anonymous connections * format * lint * fix test # Conflicts: # superset-frontend/src/embedded/index.tsx # superset/views/core.py * lint * Update superset-frontend/src/embedded/index.tsx Co-authored-by: David Aaron Suddjian <[email protected]> * comment out origins checks * move embedded for core to dashboard * pylint * isort Co-authored-by: David Aaron Suddjian <[email protected]> Co-authored-by: David Aaron Suddjian <[email protected]> * feat: Authorizing guest access to embedded dashboards (apache#17757) * helper methods and dashboard access * guest token dashboard authz * adjust csrf exempt list * eums don't work that way * Remove unnecessary import * move row level security tests to their own file * a bit of refactoring * add guest token security tests * refactor tests * clean imports * variable names can be too long apparently * missing argument to get_user_roles * don't redefine builtins * remove unused imports * fix test import * default to global user when getting roles * missing import * mock it * test get_user_roles * infer g.user for ease of tests * remove redundant check * tests for guest user security manager fns * use algo to get rid of warning messages * tweaking access checks * fix guest token security tests * missing imports * more tests * more testing and also some small refactoring * move validation out of parsing * fix dashboard access check again * add more test Co-authored-by: Lily Kuang <[email protected]> * feat: Row Level Security rules for guest tokens (apache#17836) * helper methods and dashboard access * guest token dashboard authz * adjust csrf exempt list * eums don't work that way * Remove unnecessary import * move row level security tests to their own file * a bit of refactoring * add guest token security tests * refactor tests * clean imports * variable names can be too long apparently * missing argument to get_user_roles * don't redefine builtins * remove unused imports * fix test import * default to global user when getting roles * missing import * mock it * test get_user_roles * infer g.user for ease of tests * remove redundant check * tests for guest user security manager fns * use algo to get rid of warning messages * tweaking access checks * fix guest token security tests * missing imports * more tests * more testing and also some small refactoring * move validation out of parsing * fix dashboard access check again * rls rules for guest tokens * test guest token rls rules * more flexible rls rules * lint * fix tests * fix test * defaults * fix some tests * fix some tests * lint Co-authored-by: Lily Kuang <[email protected]> * SupersetClient guest token test * Apply suggestions from code review Co-authored-by: Lily Kuang <[email protected]> Co-authored-by: Lily Kuang <[email protected]>

superset-github-bot bot added the preset-io label Oct 20, 2021

pull-request-size bot added the size/L label Oct 20, 2021

lilykuang changed the title ~~[WIP] feat: embedded dashboard~~ feat [WIP]: embedded dashboard Oct 20, 2021

suddjian reviewed Oct 27, 2021

View reviewed changes

lilykuang force-pushed the embedded-provider branch from 0da0662 to 1003256 Compare November 1, 2021 22:14

lilykuang changed the title ~~feat [WIP]: embedded dashboard~~ feat: embedded dashboard UI configuration Nov 1, 2021

lilykuang force-pushed the embedded-provider branch from 1003256 to 5b311f1 Compare November 3, 2021 18:18

lilykuang changed the title ~~feat: embedded dashboard UI configuration~~ feat(dashboard): embedded dashboard UI configuration Nov 4, 2021

lilykuang requested a review from suddjian November 4, 2021 19:12

lilykuang marked this pull request as ready for review November 4, 2021 19:30

Lily Kuang added 2 commits November 8, 2021 09:49

setup embedded provider

ad23ee6

update ui configuration

a55537d

lilykuang force-pushed the embedded-provider branch from 5b311f1 to a55537d Compare November 8, 2021 20:30

fix test

44cfb7e

lilykuang changed the base branch from master to embedded November 9, 2021 18:59

lilykuang merged commit 36246f9 into apache:embedded Nov 9, 2021

lilykuang deleted the embedded-provider branch November 9, 2021 19:52

lilykuang pushed a commit to preset-io/superset that referenced this pull request Nov 16, 2021

feat(dashboard): embedded dashboard UI configuration (apache#17175)

0576572

* setup embedded provider * update ui configuration * fix test

lilykuang pushed a commit that referenced this pull request Nov 16, 2021

feat(dashboard): embedded dashboard UI configuration (#17175) (#17450)

7108e6b

* setup embedded provider * update ui configuration * fix test

lilykuang mentioned this pull request Mar 4, 2022

feat(embedded): add optional dashboard ui configuration #19031

Merged

9 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(dashboard): embedded dashboard UI configuration #17175

feat(dashboard): embedded dashboard UI configuration #17175

lilykuang commented Oct 20, 2021 •

edited

Loading

suddjian left a comment

suddjian Oct 27, 2021

suddjian Oct 27, 2021

suddjian Oct 27, 2021

suddjian Oct 27, 2021

suddjian Oct 27, 2021

suddjian Oct 27, 2021

suddjian Oct 27, 2021

suddjian Oct 27, 2021

codecov bot commented Nov 8, 2021

suddjian commented Nov 9, 2021

lilykuang commented Nov 9, 2021


		export const useEmbedded = () => useContext(EmbeddedContext);

		export const EmbeddedProvider: React.FC<EmbeddedProviderProps> = ({

feat(dashboard): embedded dashboard UI configuration #17175

feat(dashboard): embedded dashboard UI configuration #17175

Conversation

lilykuang commented Oct 20, 2021 • edited Loading

SUMMARY

BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF

TESTING INSTRUCTIONS

ADDITIONAL INFORMATION

suddjian left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov bot commented Nov 8, 2021

Codecov Report

suddjian commented Nov 9, 2021

lilykuang commented Nov 9, 2021

lilykuang commented Oct 20, 2021 •

edited

Loading