[SIP-15] Adding initial framework #8398

john-bodley · 2019-10-15T22:17:01Z

SUMMARY

This PR provides the initial framework for SIP-15 which is currently behind a disabled feature flag. Note this is still WIP as SIP-15A also needs to be addressed prior to this feature being live.

Given that SIP-15 requires a rollout period (where users can preview and correct changes as necessary) the premise of this PR is to add the necessary wiring to allow users to:

Explicitly see how the current time range endpoints behave. Note this is only when the chart references explicit times as opposed to now, 7 days ago, etc.
Preview via link (restricted to owners) how their chart will behave when the [start, end] (or equivalent) interval is changed to [start, end).

Note the backend logic is behind a feature flag whereas the frontend logic which leverages the endpoints is not, i.e., the the endpoints prop is simply undefined. These endpoints will persist after SIP-15 has been rolled out (to provide more transparency as to how the time range endpoints behave though we'll simply hard code these).

BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF

Current

SIP-15 enabled but chart not updated

SIP-15 enabled chart preview link

SIP-15 enabled and chart updated (via preview or save)

TEST PLAN

ADDITIONAL INFORMATION

Has associated issue: [SIP-15] Transparent and Consistent Time Intervals #6360
Changes UI
Requires DB Migration.
Confirm DB Migration upgrade and downgrade tested.
Introduces new feature or API
Removes existing feature or API

REVIEWERS

to: @betodealmeida @etr2460 @michellethomas @mistercrunch @villebro
cc: @sylvia-tomiyama

codecov-io · 2019-10-16T02:03:09Z

Codecov Report

Merging #8398 into master will decrease coverage by 0.02%.
The diff coverage is 58.33%.

@@            Coverage Diff             @@
##           master    #8398      +/-   ##
==========================================
- Coverage   66.58%   66.56%   -0.03%     
==========================================
  Files         449      449              
  Lines       22525    22567      +42     
  Branches     2364     2367       +3     
==========================================
+ Hits        14999    15021      +22     
- Misses       7388     7408      +20     
  Partials      138      138

Impacted Files	Coverage Δ
superset/viz.py	`71.29% <ø> (ø)`	⬆️
...rset/assets/src/messageToasts/components/Toast.jsx	`93.75% <ø> (ø)`	⬆️
...rset/assets/src/explore/controlPanels/sections.jsx	`100% <ø> (ø)`	⬆️
superset/utils/core.py	`88.65% <100%> (+0.09%)`	⬆️
superset/config.py	`89.34% <100%> (+0.16%)`	⬆️
superset/assets/src/explore/controls.jsx	`42.18% <100%> (+0.91%)`	⬆️
superset/views/utils.py	`76.41% <15.78%> (-13.36%)`	⬇️
superset/views/core.py	`72.15% <60%> (-0.07%)`	⬇️
superset/connectors/sqla/models.py	`84.87% <83.33%> (-0.09%)`	⬇️
.../explore/components/controls/DateFilterControl.jsx	`40% <87.5%> (+2.09%)`	⬆️

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 f7f0be5...8cf290b. Read the comment docs.

superset/connectors/sqla/models.py

etr2460

a few comments on the JS stuff

etr2460 · 2019-10-16T21:45:55Z

superset/config.py

+# Note currently SIP-15 feature is WIP and should not be enabled.
+SIP_15_ENABLED = False
+SIP_15_DEFAULT_TIME_RANGE_ENDPOINTS = ["unknown", "inclusive"]
+SIP_15_TOAST_MESSAGE = 'Preview then save your chart using the new time range endpoints <a href="{url}" class="alert-link">here</a>.'


Maybe prefixing this with Action Required: or something like that to make it more apparent what's needed?

@etr2460 one issue I ran into is that any more text would wrap in the toast which looked a tad weird. Do you know if these toasts can be resized?

You could always increase the max size of the toasts. It seems like a reasonable change

etr2460 · 2019-10-16T21:46:18Z

superset/assets/src/messageToasts/components/Toast.jsx

@@ -96,7 +97,7 @@ class Toast extends React.Component {
          toastType === DANGER_TOAST && 'toast--danger',
        )}
      >
-        {text}
+        <div dangerouslySetInnerHTML={{ __html: dompurify.sanitize(text) }} />


we should really avoid dangerously setting html. Why not encode the URL in another way and then pull it out of the string? I know this is just to make flash work since it only accepts a string. But instead i'd send a flash something like:

Preview then save your chart using the new time range endpoints $$$SUPER_UNIQUE_TOKEN_FOR_ENCODING_LINK_FOR_HERE$$$http://superset.../

Then you could just do in a helper function:

const parts = str.split('$$$SUPER_UNIQUE_TOKEN_FOR_ENCODING_LINK_FOR_HERE$$$'); if (parts.length > 1) { return <div>{parts.length[0]} <a href={parts.length[1]}>here</a>.</div>; } else { return <div>{parts.length[0]}</div>; }

But in general, this is super hacky.... maybe there's a way to do this without needing to use flash?

@williaster thoughts?

etr2460 · 2019-10-16T21:48:36Z

superset/assets/src/explore/components/controls/DateFilterControl.jsx

@@ -494,7 +495,9 @@ export default class DateFilterControl extends React.Component {
  }
  render() {
    let value = this.props.value || defaultProps.value;
-    value = value.split(SEPARATOR).map((v, idx) => v.replace('T00:00:00', '') || (idx === 0 ? '-∞' : '∞')).join(SEPARATOR);
+    const endpoints = this.props.endpoints;
+    const iso8601 = /^\d{4}-\d{2}-\d{2}T\d{2}:\d{2}:\d{2}$/;


I'd pull this out to the top of the file and name is something like ISO_8601_REGEX_MATCH

etr2460 · 2019-10-21T21:19:31Z

docs/installation.rst

+SIP-15
+------
+
+`SIP-15 <https://github.com/apache/incubator-superset/issues/6360>`_ aims to ensure that time intervals are handled in a consistent and transparent manner for both the Druid and SQLAlchemy connectors.


nit: remove extra underscore here

@etr2460 see http://docutils.sourceforge.net/docs/user/rst/quickref.html#contents.

etr2460 · 2019-10-21T21:21:00Z

superset/assets/src/explore/controls.jsx

+    type: 'HiddenControl',
+    label: t('Time range endpoints'),
+    hidden: true,
+    description: t('Time range endpoints (SIP-15)'),


Should we elaborate more about SIP-15 here? Provide a link or something?

@etr2460 this control is always hidden and only exists to ensure that associated form-data is preserved.

etr2460 · 2019-10-21T21:21:44Z

superset/config.py

+# Note currently SIP-15 feature is WIP and should not be enabled.
+SIP_15_ENABLED = False
+SIP_15_DEFAULT_TIME_RANGE_ENDPOINTS = ["unknown", "inclusive"]
+SIP_15_TOAST_MESSAGE = 'Preview then save your chart using the new time range endpoints <a href="{url}" class="alert-link">here</a>.'


You could always increase the max size of the toasts. It seems like a reasonable change

john-bodley · 2019-10-24T19:00:26Z

@etr2460 I addressed your comments.

john-bodley · 2019-10-26T16:30:09Z

@etr2460 would you mind reviewing this again?

etr2460

a few more comments

etr2460 · 2019-10-28T17:42:15Z

docs/installation.rst

+SIP-15
+------
+
+`SIP-15 <https://github.com/apache/incubator-superset/issues/6360>`_ aims to ensure that time intervals are handled in a consistent and transparent manner for both the Druid and SQLAlchemy connectors.


etr2460 · 2019-10-28T17:44:07Z

superset/assets/src/explore/components/controls/DateFilterControl.jsx

      return (
        <OverlayTrigger
          key={timeFrame}
          placement="left"
          overlay={
            <Tooltip id={`tooltip-${timeFrame}`}>
-              {nextState.since}<br />{nextState.until}
+              {nextState.since} {endpoints ? `(${endpoints[0]})` : ''}<br />{nextState.until} {endpoints ? `(${endpoints[1]})` : ''}


these can be {endpoints && `(${endpoints[0]})`} instead of the ternary with the empty string

etr2460 · 2019-10-28T17:44:59Z

superset/assets/src/explore/components/controls/DateFilterControl.jsx

@@ -494,7 +497,8 @@ export default class DateFilterControl extends React.Component {
  }
  render() {
    let value = this.props.value || defaultProps.value;
-    value = value.split(SEPARATOR).map((v, idx) => v.replace('T00:00:00', '') || (idx === 0 ? '-∞' : '∞')).join(SEPARATOR);
+    const endpoints = this.props.endpoints;
+    value = value.split(SEPARATOR).map((v, idx) => ISO_8601_REGEX_MATCH.test(v) ? v.replace('T00:00:00', '') + (endpoints ? ` (${endpoints[idx]})` : '') : v || (idx === 0 ? '-∞' : '∞')).join(SEPARATOR);


same note here about the ternary. some newlines might also make this more readable.

etr2460 · 2019-10-28T17:46:10Z

superset/config.py

+
+
+# Note currently SIP-15 feature is WIP and should not be enabled.
+SIP_15_ENABLED = False


i think this should all be above line 682, otherwise you'll be overriding custom configs here

john-bodley · 2019-10-28T18:45:33Z

@etr2460 I addressed your comments.

etr2460

lgtm!

* [sip-15] Adding initial framework * [toast] Addressing etr2460's comments * [fix] Addressing etr2460's comments

pull-request-size bot added the size/L label Oct 15, 2019

john-bodley changed the title ~~[sip-15] Adding initial framework~~ [SIP-15] Adding initial framework Oct 15, 2019

john-bodley force-pushed the john-bodley--sip-15-messaging branch 7 times, most recently from 81ae5e0 to 1f285af Compare October 16, 2019 01:53

villebro reviewed Oct 16, 2019

View reviewed changes

superset/connectors/sqla/models.py Show resolved Hide resolved

john-bodley force-pushed the john-bodley--sip-15-messaging branch from 1f285af to d1d05b4 Compare October 16, 2019 04:47

etr2460 requested changes Oct 16, 2019

View reviewed changes

john-bodley force-pushed the john-bodley--sip-15-messaging branch 2 times, most recently from dfc2925 to f8b4b63 Compare October 18, 2019 17:23

etr2460 reviewed Oct 21, 2019

View reviewed changes

john-bodley force-pushed the john-bodley--sip-15-messaging branch 3 times, most recently from a8852e6 to d051e8c Compare October 24, 2019 23:48

etr2460 reviewed Oct 28, 2019

View reviewed changes

etr2460 approved these changes Oct 28, 2019

View reviewed changes

John Bodley added 3 commits October 28, 2019 13:07

[sip-15] Adding initial framework

0a6fb53

[toast] Addressing etr2460's comments

34d4b20

[fix] Addressing etr2460's comments

8cf290b

john-bodley force-pushed the john-bodley--sip-15-messaging branch from 7c92647 to 8cf290b Compare October 28, 2019 20:11

john-bodley merged commit 8b74745 into apache:master Oct 28, 2019

john-bodley deleted the john-bodley--sip-15-messaging branch October 28, 2019 21:23

graceguo-supercat pushed a commit that referenced this pull request Nov 13, 2019

[SIP-15] Adding initial framework (#8398)

c42960f

* [sip-15] Adding initial framework * [toast] Addressing etr2460's comments * [fix] Addressing etr2460's comments

dpgaspar added v0.35 and removed v0.35 labels Dec 20, 2019

mistercrunch added 🏷️ bot A label used by `supersetbot` to keep track of which PR where auto-tagged with release labels 🚢 0.36.0 labels Feb 28, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[SIP-15] Adding initial framework #8398

[SIP-15] Adding initial framework #8398

john-bodley commented Oct 15, 2019 •

edited

Loading

codecov-io commented Oct 16, 2019 •

edited

Loading

etr2460 left a comment

etr2460 Oct 16, 2019

john-bodley Oct 17, 2019

etr2460 Oct 21, 2019

etr2460 Oct 16, 2019

etr2460 Oct 16, 2019

john-bodley Oct 17, 2019

etr2460 Oct 16, 2019

etr2460 Oct 21, 2019

john-bodley Oct 22, 2019

etr2460 Oct 28, 2019

etr2460 Oct 21, 2019

john-bodley Oct 22, 2019

etr2460 Oct 21, 2019

john-bodley commented Oct 24, 2019

john-bodley commented Oct 26, 2019

etr2460 left a comment

etr2460 Oct 28, 2019

etr2460 Oct 28, 2019 •

edited by john-bodley

Loading

etr2460 Oct 28, 2019

etr2460 Oct 28, 2019

john-bodley commented Oct 28, 2019

etr2460 left a comment



		# Note currently SIP-15 feature is WIP and should not be enabled.
		SIP_15_ENABLED = False

[SIP-15] Adding initial framework #8398

[SIP-15] Adding initial framework #8398

Conversation

john-bodley commented Oct 15, 2019 • edited Loading

CATEGORY

SUMMARY

BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF

Current

SIP-15 enabled but chart not updated

SIP-15 enabled chart preview link

SIP-15 enabled and chart updated (via preview or save)

TEST PLAN

ADDITIONAL INFORMATION

REVIEWERS

codecov-io commented Oct 16, 2019 • edited Loading

Codecov Report

etr2460 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

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

john-bodley commented Oct 24, 2019

john-bodley commented Oct 26, 2019

etr2460 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

etr2460 Oct 28, 2019 • edited by john-bodley Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

john-bodley commented Oct 28, 2019

etr2460 left a comment

Choose a reason for hiding this comment

john-bodley commented Oct 15, 2019 •

edited

Loading

codecov-io commented Oct 16, 2019 •

edited

Loading

etr2460 Oct 28, 2019 •

edited by john-bodley

Loading