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(embedding-sdk): emit data-mask events through embedded sdk to iframe parent #31331

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

MohamedHalat
Copy link

@MohamedHalat MohamedHalat commented Dec 6, 2024

SUMMARY

Added the ability to emit data-mask events to the parent. Still need to figure out how to define what events can be emitted and if this should be a middleware/some other redux operation instead of subscribe

TESTING INSTRUCTIONS

  • Enable EMBEDDED_SUPERSET_EMIT_DATA_MASKS feature flag
  • Embed dashboard onto a page
  • call getDataMasks and pass in a callback
  • Validate data comes in and click a chart filterable field to view changes

ADDITIONAL INFORMATION

  • Has associated issue:
  • Required feature flags: EMBEDDED_SUPERSET_EMIT_DATA_MASKS
  • Changes UI
  • Includes DB Migration (follow approval process in SIP-59)
  • Introduces new feature or API
  • Removes existing feature or API

@MohamedHalat MohamedHalat changed the title feat: allow event emit through embedded sdk to iframe parent feat(embedding-sdk): Add event emit through embedded sdk to iframe parent Dec 6, 2024
Copy link
Contributor

@github-actions github-actions bot left a comment

Choose a reason for hiding this comment

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

Congrats on making your first PR and thank you for contributing to Superset! 🎉 ❤️

We hope to see you in our Slack community too! Not signed up? Use our Slack App to self-register.

@pull-request-size pull-request-size bot added size/M and removed size/S labels Dec 9, 2024
@MohamedHalat MohamedHalat changed the title feat(embedding-sdk): Add event emit through embedded sdk to iframe parent feat(embedding-sdk): emit data-mask events through embedded sdk to iframe parent Dec 9, 2024
@MohamedHalat MohamedHalat marked this pull request as ready for review December 9, 2024 15:32
@MohamedHalat MohamedHalat force-pushed the embedded-sdk-event-emitter branch from f5d42c0 to 438c989 Compare December 9, 2024 16:23
@rusackas
Copy link
Member

rusackas commented Dec 9, 2024

This seems like a reasonable feature, but leaves me with a couple of questions:

  1. I defer to @yousoph / @eschutho to assign a reviewer who might have a good embedded test setup. We need to figure out who are the best owners/reviewers for Embedded SDK changes, going forward.

  2. Regarding the feature flag, do you think we need to have a flag at all? Do you perceive a risk (security or otherwise) that would warrant people disabling it?

  3. If it's to be configurable, should the config happen at the Superset level, or can it be contained as part of the SDK itself?

@MohamedHalat
Copy link
Author

MohamedHalat commented Dec 9, 2024

This seems like a reasonable feature, but leaves me with a couple of questions:

  1. I defer to @yousoph / @eschutho to assign a reviewer who might have a good embedded test setup. We need to figure out who are the best owners/reviewers for Embedded SDK changes, going forward.
  2. Regarding the feature flag, do you think we need to have a flag at all? Do you perceive a risk (security or otherwise) that would warrant people disabling it?
  3. If it's to be configurable, should the config happen at the Superset level, or can it be contained as part of the SDK itself?

Hey @rusackas, thanks for the initial followup. To answer your questions:

  1. I'm hesitant to remove the feature flag as I'd rather not add a store.subscribe that does nothing.
  2. I'm all for having configuration be apart of the SDK, as long as the data passed is not too large. For now, my only requirement is to have data masks emitted - which is why I renamed the feature flag. I can imagine it may be useful to have more information about the charts passed to the window parent.

I'll come up with a small list of data to emit, and If we are in agreement I'll add that data as well (possibly configured through SDK), for now I feel this amount is enough.

Copy link

codecov bot commented Dec 9, 2024

Codecov Report

Attention: Patch coverage is 0% with 5 lines in your changes missing coverage. Please review.

Project coverage is 70.95%. Comparing base (76d897e) to head (438c989).
Report is 1160 commits behind head on master.

Files with missing lines Patch % Lines
superset-frontend/src/embedded/index.tsx 0.00% 5 Missing ⚠️
Additional details and impacted files
@@             Coverage Diff             @@
##           master   #31331       +/-   ##
===========================================
+ Coverage   60.48%   70.95%   +10.47%     
===========================================
  Files        1931     2000       +69     
  Lines       76236    80874     +4638     
  Branches     8568     9213      +645     
===========================================
+ Hits        46114    57387    +11273     
+ Misses      28017    21247     -6770     
- Partials     2105     2240      +135     
Flag Coverage Δ
hive 48.77% <ø> (-0.39%) ⬇️
javascript 58.99% <0.00%> (+1.27%) ⬆️
mysql 76.53% <ø> (?)
postgres 76.59% <ø> (?)
presto 53.27% <ø> (-0.54%) ⬇️
python 83.81% <ø> (+20.32%) ⬆️
sqlite 76.05% <ø> (?)
unit 60.92% <ø> (+3.29%) ⬆️

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

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants