feat: add certification to metrics #10630

etr2460 · 2020-08-18T20:28:11Z

SUMMARY

Makes use of the extra column in sql_metrics to add certification labels to metrics. This shows the certification label in the datasource editor, and future work will add it to the metric control in superset-ui.

further implementation of #10591

BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF

TEST PLAN

No icon appears when no certifiers or details are set
An icon appears when either a certifier or certification detail is set
Modifying the json string in the FAB CRUD view also works

ADDITIONAL INFORMATION

to: @nytai @graceguo-supercat @mistercrunch @serenajiang

serenajiang

I think this requires a db migration?

Also, maybe we should add this to BaseMetric instead of SQLMetric since it's nice-to-have for all metrics.

etr2460 · 2020-08-18T20:46:36Z

Nope, no db migration as we've already added this column to the sql_metrics table in a previous PR.

Also, we only added it to sql_metrics and not the druid metric table metrics. I'm not sure if we want to go through the work to add this to the native druid datasources too, as I believe they're in the process of being deprecated anyway now that Druid supports SQL

serenajiang · 2020-08-18T20:50:51Z

superset/connectors/sqla/models.py

+    def is_certified(self) -> bool:
+        try:
+            extra_object = json.loads(self.extra)
+            return bool(extra_object.get("certification", False))


Suggested change

return bool(extra_object.get("certification", False))

return bool(extra_object.get("certification"))

🐍

serenajiang · 2020-08-18T20:51:39Z

superset/connectors/sqla/models.py

+    def certified_by(self) -> Optional[str]:
+        try:
+            extra_object = json.loads(self.extra)
+            return extra_object.get("certification", {}).get("certified_by", None)


Suggested change

return extra_object.get("certification", {}).get("certified_by", None)

return extra_object.get("certification", {}).get("certified_by")

🐍

serenajiang · 2020-08-18T20:51:59Z

superset/connectors/sqla/models.py

+    def certification_details(self) -> Optional[str]:
+        try:
+            extra_object = json.loads(self.extra)
+            return extra_object.get("certification", {}).get("details", None)


Suggested change

return extra_object.get("certification", {}).get("details", None)

return extra_object.get("certification", {}).get("details")

🐍

serenajiang · 2020-08-18T20:56:50Z

superset/connectors/sqla/models.py

+    @property
+    def data(self) -> Dict[str, Any]:
+        attrs = ("is_certified", "certified_by", "certification_details")
+        attr_dict = {s: getattr(self, s) for s in attrs}


It's a little weird that we have these related attributes floating around as separate attributes in the attr_dict. Does data need to be a flat dict, or can we keep these all in one "certification" object, sorta like how they're stored in extra? So, .data could return

{ "certification": { "is_certified": ..., "certified_by": ..., "certification_details": ... }, ... }

hmm, that's a good point. Unfortunately, the way the CRUD view works on the frontend is that these fields are expected to be in a flat dict here. It's possible I could modify it though, I'll take a look

There's a decent bit of magic in the CRUD JS code, so I think keeping as a flat dict is the easiest for now. I'm going to leave this as is

codecov-commenter · 2020-08-18T23:11:35Z

Codecov Report

Merging #10630 into master will decrease coverage by 4.10%.
The diff coverage is 77.77%.

@@            Coverage Diff             @@
##           master   #10630      +/-   ##
==========================================
- Coverage   64.22%   60.12%   -4.11%     
==========================================
  Files         778      779       +1     
  Lines       36709    36835     +126     
  Branches     3461     3492      +31     
==========================================
- Hits        23578    22148    -1430     
- Misses      13022    14498    +1476     
- Partials      109      189      +80

Flag	Coverage Δ
#cypress	`?`
#javascript	`60.67% <68.96%> (+0.04%)`	⬆️
#python	`59.80% <88.00%> (+0.04%)`	⬆️

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

Impacted Files	Coverage Δ
superset/connectors/sqla/views.py	`67.92% <ø> (-13.76%)`	⬇️
...perset-frontend/src/datasource/DatasourceModal.tsx	`73.91% <50.00%> (-23.31%)`	⬇️
...ontend/src/components/CertifiedIconWithTooltip.tsx	`70.00% <70.00%> (ø)`
superset/connectors/sqla/models.py	`89.41% <88.00%> (+0.35%)`	⬆️
superset-frontend/src/CRUD/CollectionTable.tsx	`69.81% <100.00%> (-17.93%)`	⬇️
superset-frontend/src/components/Icon/index.tsx	`100.00% <100.00%> (ø)`
...erset-frontend/src/datasource/DatasourceEditor.jsx	`69.58% <100.00%> (-1.42%)`	⬇️
superset-frontend/src/SqlLab/App.jsx	`0.00% <0.00%> (-100.00%)`	⬇️
superset-frontend/src/explore/App.jsx	`0.00% <0.00%> (-100.00%)`	⬇️
superset-frontend/src/dashboard/App.jsx	`0.00% <0.00%> (-100.00%)`	⬇️
... and 158 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 692266f...260c782. Read the comment docs.

ktmud · 2020-08-19T00:51:58Z

If it's certified by Taylor Swift then it must be good.

mistercrunch · 2020-08-19T04:16:13Z

etr2460 · 2020-08-19T17:10:27Z

superset-frontend/src/CRUD/Field.jsx

@@ -32,7 +32,7 @@ import './crud.less';
 const propTypes = {
  value: PropTypes.any.isRequired,
  label: PropTypes.string.isRequired,
-  descr: PropTypes.node,


why make this shorter and less clear? Turns out because this file is JSX, some usages of this component set the description prop, which doesn't even work!

I've cleaned it all up to use description instead

ktmud

LGTM with a suggestion on the Icon component

ktmud · 2020-08-19T21:33:08Z

superset-frontend/src/CRUD/Field.jsx

@@ -32,7 +32,7 @@ import './crud.less';
 const propTypes = {
  value: PropTypes.any.isRequired,
  label: PropTypes.string.isRequired,
-  descr: PropTypes.node,


ktmud · 2020-08-19T21:36:42Z

superset-frontend/src/components/CertifiedIconWithTooltip.tsx

+import Icon from 'src/components/Icon';
+import TooltipWrapper from 'src/components/TooltipWrapper';
+
+interface CertifiedIconWithTooltipProps {


Naming/API design nit: is there a variant of CertifiedIcon which doesn't have a tooltip? If no, then WithTooltip is probably not needed; if yes, you can also just make tooltip as an optional prop and add the TooltipWrapper conditionally.

In the Icon file, we do:
import { ReactComponent as CertifiedIcon } from 'images/icons/certified.svg';
so I think there's a general understanding of an "Icon" component only including the icon and nothing else.

ktmud · 2020-08-19T22:58:18Z

superset/connectors/sqla/models.py

+    @property
+    def is_certified(self) -> bool:
+        try:
+            extra_object = json.loads(self.extra)


Not sure what's the practice of other SQLA models, but do you think it makes sense parse self.extra on initialization? Or at least make it a property to DRY.

def __init__(...): ... self._extra_json = None @property def extra_json(self) -> Dict[String, Any]: if self._extra_json: return self._extra_object try: self._extra_object = json.loads(self.extra) return extra_object except (TypeError, json.JSONDecodeError): self._extra_json = {} return self._extra_object @property def is_certified(self) -> bool: return bool(self.extra_json.get("certification")) @property def certified_by(self) -> Optional[str]: return self.extra_json.get("certification", {}).get("certified_by")

Ooo, i like the property idea to parse extra as a dict. I'll make that change

also, looking at other models that do this, we don't bother with the optimization of only parsing once on init

* master: (43 commits) feat: Getting fancier with Storybook (apache#10647) fix: dedup groupby in viz.py while preserving order (apache#10633) feat: bump superset-ui for certified tag (apache#10650) feat: setup react page with submenu for datasources listview (apache#10642) feat: add certification to metrics (apache#10630) feat(viz-plugins): add date formatting to pivot-table (apache#10637) fix: controls scroll issue (apache#10644) feat: Allow tests files in /src (plus Label component tests) (apache#10634) fix: remove duplicated params and cache_timeout from list_columns; add viz_type to list_columns (apache#10643) chore: splitting button stories into separate stories (apache#10631) refactor: remove slice level label_colors from dashboard init load (apache#10603) feat: card view bulk select (apache#10607) style: Label styling/storybook touchups (apache#10627) fix: removing unsupported modal sizes (apache#10625) feat(datasource): remove deleted columns and update column type on metadata refresh (apache#10619) improve documentation for country maps (apache#10621) chore: npm audit fix as of 2020-08-15 (apache#10613) feat: dataset REST API for distinct values (apache#10595) chore: bump react-redux to 5.1.2, whittling console noise (apache#10602) fixing console error about bad html attribute (apache#10604) ... # Conflicts: # superset-frontend/src/explore/components/ExploreViewContainer.jsx # superset-frontend/src/views/App.tsx # superset/config.py

pull-request-size bot added the size/L label Aug 18, 2020

serenajiang reviewed Aug 18, 2020

View reviewed changes

etr2460 force-pushed the erik-ritter--certified-metrics branch from bbf345d to 2fd9766 Compare August 18, 2020 20:43

serenajiang reviewed Aug 18, 2020

View reviewed changes

etr2460 force-pushed the erik-ritter--certified-metrics branch from 2fd9766 to 260c782 Compare August 18, 2020 22:52

etr2460 force-pushed the erik-ritter--certified-metrics branch from 260c782 to e3deabf Compare August 19, 2020 17:09

etr2460 commented Aug 19, 2020

View reviewed changes

ktmud approved these changes Aug 19, 2020

View reviewed changes

feat: add certification to metrics

20faa16

etr2460 force-pushed the erik-ritter--certified-metrics branch from e3deabf to 20faa16 Compare August 19, 2020 23:47

etr2460 merged commit 38da552 into apache:master Aug 20, 2020

etr2460 mentioned this pull request Aug 20, 2020

feat: add certification icon to metrics apache-superset/superset-ui#748

Merged

etr2460 pushed a commit to airbnb/superset-fork that referenced this pull request Aug 20, 2020

feat: add certification to metrics (apache#10630)

a085f84

Ofeknielsen pushed a commit to ofekisr/incubator-superset that referenced this pull request Oct 5, 2020

feat: add certification to metrics (apache#10630)

9d09eda

auxten pushed a commit to auxten/incubator-superset that referenced this pull request Nov 20, 2020

feat: add certification to metrics (apache#10630)

0f4ab1c

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

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: add certification to metrics #10630

feat: add certification to metrics #10630

etr2460 commented Aug 18, 2020

serenajiang left a comment

etr2460 commented Aug 18, 2020

serenajiang Aug 18, 2020

etr2460 Aug 18, 2020

serenajiang Aug 18, 2020

etr2460 Aug 18, 2020

serenajiang Aug 18, 2020

etr2460 Aug 18, 2020

serenajiang Aug 18, 2020

etr2460 Aug 18, 2020

etr2460 Aug 19, 2020

codecov-commenter commented Aug 18, 2020

ktmud commented Aug 19, 2020

mistercrunch commented Aug 19, 2020 •

edited

Loading

etr2460 Aug 19, 2020

ktmud Aug 19, 2020

ktmud left a comment

ktmud Aug 19, 2020

ktmud Aug 19, 2020

etr2460 Aug 19, 2020

ktmud Aug 19, 2020

etr2460 Aug 19, 2020

etr2460 Aug 19, 2020

	return bool(extra_object.get("certification", False))
	return bool(extra_object.get("certification"))

	return extra_object.get("certification", {}).get("certified_by", None)
	return extra_object.get("certification", {}).get("certified_by")

feat: add certification to metrics #10630

feat: add certification to metrics #10630

Conversation

etr2460 commented Aug 18, 2020

SUMMARY

BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF

TEST PLAN

ADDITIONAL INFORMATION

serenajiang left a comment

Choose a reason for hiding this comment

etr2460 commented Aug 18, 2020

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-commenter commented Aug 18, 2020

Codecov Report

ktmud commented Aug 19, 2020

mistercrunch commented Aug 19, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ktmud 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

mistercrunch commented Aug 19, 2020 •

edited

Loading