Tagging frontend #7418

betodealmeida · 2019-04-30T22:24:50Z

SUMMARY

This PR introduces the frontend for tags, behind a feature flag:

DEFAULT_FEATURE_FLAGS = {                                                                                              │
    'TAGGING_SYSTEM': True,                                                                                            │
}

When the feature is enabled, owners can tag charts:

And dashboards:

Note that tags can be autocompleted.

A user who's not an owner will see non-editable tags:

When clicking on a tag, the user is taken to a page with content that has that tag:

TEST PLAN

Tested locally. We've also been using this at Lyft for months.

ADDITIONAL INFORMATION

REVIEWERS

@mistercrunch @williaster @khtruong

codecov-io · 2019-05-01T17:48:15Z

Codecov Report

Merging #7418 into master will decrease coverage by 0.15%.
The diff coverage is 27.21%.

@@            Coverage Diff             @@
##           master    #7418      +/-   ##
==========================================
- Coverage   65.08%   64.92%   -0.16%     
==========================================
  Files         429      434       +5     
  Lines       21002    21261     +259     
  Branches     2338     2365      +27     
==========================================
+ Hits        13669    13804     +135     
- Misses       7217     7334     +117     
- Partials      116      123       +7

Impacted Files	Coverage Δ
superset/assets/src/welcome/App.jsx	`0% <0%> (ø)`	⬆️
superset/assets/src/components/ObjectTags.jsx	`10.52% <10.52%> (ø)`
superset/assets/src/utils/tags.js	`100% <100%> (ø)`
superset/assets/src/featureFlags.ts	`88.88% <100%> (+1.38%)`	⬆️
superset/assets/src/tags.js	`11.76% <11.76%> (ø)`
superset/assets/src/welcome/TagsTable.jsx	`15.38% <15.38%> (ø)`
superset/assets/src/welcome/Welcome.jsx	`52.63% <41.93%> (-27.37%)`	⬇️
...uperset/assets/src/dashboard/components/Header.jsx	`44.34% <83.33%> (+2.14%)`	⬆️
...sets/src/explore/components/ExploreChartHeader.jsx	`56.81% <87.5%> (+6.81%)`	⬆️
... and 5 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 46579b1...ae40ee6. Read the comment docs.

1AB9502 · 2019-06-17T15:41:38Z

@betodealmeida ,

Any plans on getting this merged?

betodealmeida · 2019-06-17T19:04:39Z

@1AB9502 sorry for the delay with this. I need to add some integration tests. Let me put this up for review in the meantime.

betodealmeida · 2019-06-17T19:05:13Z

cc: @khtruong

khtruong · 2019-06-17T19:40:02Z

superset/assets/src/components/ObjectTags.less

+ * specific language governing permissions and limitations
+ * under the License.
+ */
+/**


Let's add a quick comment of what this div element is.

1AB9502 · 2019-06-17T20:29:11Z

@betodealmeida

No worries! We are just really looking forward to getting this feature!

kristw · 2019-06-18T16:24:21Z

superset/assets/src/components/ObjectTags.less

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


Is it necessary to be a less file? From quick skimming, a *.css extension should be enough.

kristw · 2019-06-18T16:26:51Z

superset/assets/src/tags.js

+  QUERY: 'query',
+});
+
+export function fetchTags({ objectType, objectId, includeTypes = false }, callback, error) {


Prefer returning Promise over callback. SupersetClient.get() already returns Promise which can be chained.

kristw · 2019-06-18T16:29:24Z

superset/assets/src/welcome/TagsTable.jsx

+  renderTable(type) {
+    const data = this.state.objects[type].map(o => ({
+      [type]: <a href={o.url}>{o.name}</a>,
+      creator: unsafe(o.creator),


What does unsafe do?

kristw · 2019-06-18T16:30:02Z

superset/assets/src/welcome/TagsTable.jsx

+          { key: 'creator', label: 'Creator' },
+          { key: 'modified', label: 'Modified' },
+        ]}
+        defaultSort={{ column: 'modified', direction: 'desc' }}


create const outside class to avoid creating new object every time.

kristw · 2019-06-18T16:31:43Z

superset/assets/src/explore/components/ExploreChartHeader.jsx

+  constructor(props) {
+    super(props);
+
+     this.fetchTags = fetchTags.bind(this, {


same parameter with addTag()

kristw · 2019-06-18T16:32:42Z

superset/assets/src/explore/components/ExploreChartHeader.jsx

+
+     this.fetchTags = fetchTags.bind(this, {
+      objectType: OBJECT_TYPES.CHART,
+      objectId: props.chart.id,


What if the props and chart change? This id will not be updated.

kristw · 2019-06-18T16:33:31Z

superset/assets/src/components/ObjectTags.jsx

+    return (
+      <div className="react-tags-rw">
+        {this.state.tags.map(tag => (
+          <Label bsStyle="info">


missing key

kristw · 2019-06-18T16:34:49Z

superset/assets/src/components/ObjectTags.jsx

+  }
+
+  renderEditableTags() {
+    const Tag = props => (


This component should be declared somewhere else instead of redeclaring everytime in this render function. Can also be its own file.

etr2460

I might be missing context from when this was originally proposed, but I had some higher level questions here:

Is there a reason why you choose to create individual API functions instead of a set of redux actions to fetch and update tags? It makes sense to pull the tagging actions into its own file because it would be shared between both Explore and Dashboard views, but I think that maintaining the state of tags in its own redux store would be more typical and follow the patterns i've seen throughout superset already.

I would recommend creating a new directory in assets/src/tags to keep the component files, the main tags page, the redux actions and the reducers and then wire up the required state to both explore and dashboard components. It would make tagging consistent with the rest of the app, clearer, and easier to maintain

etr2460 · 2019-06-18T21:37:03Z

package-lock.json

@@ -0,0 +1,16 @@
+{


Should this file be here?

My mistake, let me remove it.

etr2460 · 2019-06-18T21:43:56Z

superset/assets/src/welcome/Welcome.jsx

+    const defaultTab = isFeatureEnabled(FeatureFlag.TAGGING_SYSTEM)
+      ? 'tags'
+      : 'favorites';
+    const parsedUrl = new URI(window.location);


This seems odd that we're adding the tags page as a subpage of welcome. Why not create a new page at /superset/tags?

Also, if all of this is only required for the TAGGING_SYSTEM feature flag, then can't we wrap all the logic in that flag? Right now even if you turn off the feature flag, all this code is executed, which seems to remove the safety that feature flags provide

Good point. I added the feature flag later (since we've been using this for a while), will improve the wrapped logic.

etr2460 · 2019-06-18T21:46:42Z

superset/assets/src/welcome/Welcome.jsx

+  updateURL(key, value) {
+    const parsedUrl = new URI(window.location);
+    parsedUrl.search(data => ({ ...data, [key]: value }));
+    window.history.pushState({ value }, value, parsedUrl.href());


manually adjusting the window's history is really error prone... i'm worried that there are a bunch of edge cases here

Yeah. If we opt to do this (I saw your comment below) I can write comprehensive unit tests.

etr2460 · 2019-06-18T21:47:07Z

superset/assets/src/welcome/Welcome.jsx

+  }
+  handleSelect(key) {
+    // store selected tab in URL
+    window.history.pushState({ tab: key }, key, `#${key}`);


Using the URL to maintain application state seems like an antipattern. Why not use the redux store?

This is so we can deep link to a particular tag search when the user clicks a tag (though this could be done encoding the info on the path), and also so that the user can share search results for a tag.

betodealmeida · 2019-06-19T18:51:52Z

@etr2460

Is there a reason why you choose to create individual API functions instead of a set of redux actions to fetch and update tags? It makes sense to pull the tagging actions into its own file because it would be shared between both Explore and Dashboard views, but I think that maintaining the state of tags in its own redux store would be more typical and follow the patterns i've seen throughout superset already.

Great question. Yeah, we've been running this code at Lyft for maybe a year now, so it's definitely outdated and would be written differently from scratch today.

IIRC we went with API functions since the state lives outside the applications (explore, dashboards, SQL Lab). Maybe @mistercrunch remembers better?

I would recommend creating a new directory in assets/src/tags to keep the component files, the main tags page, the redux actions and the reducers and then wire up the required state to both explore and dashboard components. It would make tagging consistent with the rest of the app, clearer, and easier to maintain.

+1

stale · 2019-08-18T19:04:42Z

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions. For admin, please label this issue .pinned to prevent stale bot from closing the issue.

senduren · 2019-09-27T17:44:26Z

Looks like this PR has gone stale, any idea on what version we can expect this feature?

mrshu · 2020-01-07T09:40:14Z

Echoing @senduren's comment above, it has been about half a year since the last commit in this PR. Based on @betodealmeida's last comment, does it look like a full rewrite will be necessary? Is it this feature still on the roadmap for future versions of Superset?

Thanks a lot!

1AB9502 · 2020-01-07T14:18:58Z

Chiming in as well. Our users love this feature. During the latest upgrade we had to add everything back manually. Would love to see this feature merged!

mfyuce · 2020-05-10T01:28:00Z

Hi All,

First thanks for the nice feature implementation.

I have synced the source files for this PR with the latest master branch on incubator-superset and sent a PR to lyft/incubator-superset fork (lyft#98). If it is accepted we can continue with that.

Or, I can also send another PR to incubator-superset if it is requested.

We think the functionality is really nice to have.

All the best,

kalimuthu123

but tagging (grouping) the dashboard is a good feature but the expection was is it saved in backend Db

mfyuce · 2020-07-02T09:16:37Z

in this PR, and the one send to lyft/incubator-superset fork (lyft#98), it is being saved to db.

betodealmeida added 5 commits April 8, 2019 10:27

Fix for tagging backend

7c34c24

WIP: tag frontend, needs feature flag

32aea34

Put tags behind feature flag

69d7563

Merge

974ff49

Remove whatwg-fetch

1b827dd

pull-request-size bot added the size/XL label Apr 30, 2019

betodealmeida mentioned this pull request Apr 30, 2019

A tagging system for dashboards, charts and queries #5524

Closed

1AB9502 mentioned this pull request May 1, 2019

Organising Charts List #7367

Closed

betodealmeida added 3 commits May 1, 2019 08:36

Fix tests

9871cf4

Small fixes

d8900c0

Fix unit test

ae40ee6

Use less for stylesheet

6e1333e

betodealmeida changed the title ~~[WIP] Tagging frontend~~ Tagging frontend May 22, 2019

betodealmeida added enhancement:request Enhancement request submitted by anyone from the community .js lyft Related to Lyft labels May 22, 2019

Fix stylesheet

7865afb

betodealmeida requested review from kristw and mistercrunch June 17, 2019 19:05

khtruong approved these changes Jun 17, 2019

View reviewed changes

mistercrunch approved these changes Jun 18, 2019

View reviewed changes

kristw requested changes Jun 18, 2019

View reviewed changes

etr2460 requested changes Jun 18, 2019

View reviewed changes

stale bot added the inactive Inactive for >= 30 days label Aug 18, 2019

mistercrunch added the .pinned Draws attention label Aug 19, 2019

stale bot removed the inactive Inactive for >= 30 days label Aug 19, 2019

kevinpostlewaite mentioned this pull request Apr 16, 2020

Ability to Group Charts And Dashboards via Tags or Folders #9551

Closed

kalimuthu123 approved these changes May 7, 2020

View reviewed changes

kalimuthu123 approved these changes May 19, 2020

View reviewed changes

junlincc mentioned this pull request Sep 23, 2020

Feature request: Using tags to help sort and search for item #11022

Closed

betodealmeida closed this Oct 8, 2020

kevinpostlewaite mentioned this pull request Mar 28, 2022

Ability to Group Charts And Dashboards via Tags or Folders #19398

Closed

cccs-nik mentioned this pull request Jun 6, 2022

CLDN-1279 Tagging feature CybercentreCanada/superset#147

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Tagging frontend #7418

Tagging frontend #7418

betodealmeida commented Apr 30, 2019 •

edited

Loading

codecov-io commented May 1, 2019 •

edited

Loading

1AB9502 commented Jun 17, 2019

betodealmeida commented Jun 17, 2019

betodealmeida commented Jun 17, 2019

khtruong Jun 17, 2019

1AB9502 commented Jun 17, 2019

kristw Jun 18, 2019

kristw Jun 18, 2019

kristw Jun 18, 2019

kristw Jun 18, 2019

kristw Jun 18, 2019

kristw Jun 18, 2019

kristw Jun 18, 2019

kristw Jun 18, 2019

etr2460 left a comment

etr2460 Jun 18, 2019

betodealmeida Jun 19, 2019

etr2460 Jun 18, 2019

etr2460 Jun 18, 2019

betodealmeida Jun 19, 2019

etr2460 Jun 18, 2019

betodealmeida Jun 19, 2019

etr2460 Jun 18, 2019

betodealmeida Jun 19, 2019

betodealmeida commented Jun 19, 2019

stale bot commented Aug 18, 2019

senduren commented Sep 27, 2019

mrshu commented Jan 7, 2020

1AB9502 commented Jan 7, 2020

mfyuce commented May 10, 2020 •

edited

Loading

kalimuthu123 left a comment

mfyuce commented Jul 2, 2020

Tagging frontend #7418

Tagging frontend #7418

Conversation

betodealmeida commented Apr 30, 2019 • edited Loading

CATEGORY

SUMMARY

TEST PLAN

ADDITIONAL INFORMATION

REVIEWERS

codecov-io commented May 1, 2019 • edited Loading

Codecov Report

1AB9502 commented Jun 17, 2019

betodealmeida commented Jun 17, 2019

betodealmeida commented Jun 17, 2019

Choose a reason for hiding this comment

1AB9502 commented Jun 17, 2019

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

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

betodealmeida commented Jun 19, 2019

stale bot commented Aug 18, 2019

senduren commented Sep 27, 2019

mrshu commented Jan 7, 2020

1AB9502 commented Jan 7, 2020

mfyuce commented May 10, 2020 • edited Loading

kalimuthu123 left a comment

Choose a reason for hiding this comment

mfyuce commented Jul 2, 2020

betodealmeida commented Apr 30, 2019 •

edited

Loading

codecov-io commented May 1, 2019 •

edited

Loading

mfyuce commented May 10, 2020 •

edited

Loading