feat: add certification icon to metrics

[etr2460]

🏆 Enhancements
Builds from apache/superset#10630 to add the certification icon into the MetricOption component. Unfortunately there's some code duplication here as the main Icon component is in incubator-superset. In the future we'll want to move icons into superset-ui I think.

Test Plan:
npm link to incubator-superset, test with a certified metric and an uncertified metric

to: @ktmud @graceguo-supercat

[vercel]

This pull request is being automatically deployed with Vercel (learn more).
To see the status of your deployment, click below or on the icon next to each commit.

🔍 Inspect: https://vercel.com/superset/superset-ui/fo73lhje9
✅ Preview: https://superset-ui-git-erik-ritter-certified-metrics.superset.vercel.app

[ktmud]

Does it bother you that the selected pills have different heights?

[etr2460]

Does it bother you that the selected pills have different heights?

I don't know... I noticed that too, but wasn't sure how best to fix it. I don't want to make the icon any smaller as that makes it less noticeable. Perhaps increasing the size of the pill for everything is better? But then it'd be inconsistent with all the other pills in the sidepane controls. In short, I wasn't sure what to do here :P

[john-bodley]

@etr2460 sorry for not picking up on this earlier, but shouldn't certified_by be an integer and refer to the user ID?

[etr2460]

So regardless of the backend structure here, we'd want to pass down all the text necessary to render it in the frontend without an additional api call.

However, we made the choice of a free text certified_by field on the backend for a few reasons:

• It makes iteration easier at this early phase
• It allows entities other than users (like groups of users, team names, certification protocols) to certify metrics
• If a user is deleted (perhaps because of leaving a team) we still keep the record of who certified the metric (although this could be fixed in the future with paranoid deletes)

[john-bodley]

@etr2460 I have some concerns with denormalized data:

• There's no consistency/accuracy with free-form entries as opposed to a typeahead, i.e., the user could be named "John Doe" however it could be incorrectly labelled as certified by "Jon Doe".
• Superset (for right or wrong) only knows about users. It seems somewhat strange to add flexibility for certification (granted this is embedded in a JSON blob and thus there's no type coupling from a foreign key perspective).
• Paranoid deletes should never delete users. The ab_user table has an active column which is how we should make users inactive as opposed to deleting records.

[etr2460]

Yeah, I agree it's not the cleanest solution, but it does provide the most flexibility as we build this feature out. I think it's something that can be expanded upon within the extra column, and then eventually migrated into a full certification column once we land on a full model for certification within Superset.

Although I don't even know if it makes sense to tie certification to users at all, seeing as a single user/owner validating data isn't the most ideal solution; groups of people or standards should own the certification process, not a single person. That's the reason for the certified_by flexibility: anything can go there, not necessarily a person's name

[john-bodley]

@etr2460 as long as we're committed to the fact that there may be additional work required for said feature.

[etr2460]

Don't worry, I'm committed to a decent amount more work here. Definitely don't want to get this to "working" and then ignore the tech debt left behind

[codecov]

Codecov Report

Merging #748 into master will increase coverage by 0.00%.
The diff coverage is 33.33%.

@@           Coverage Diff           @@
##           master     #748   +/-   ##
=======================================
  Coverage   24.38%   24.38%           
=======================================
  Files         340      341    +1     
  Lines        7662     7668    +6     
  Branches      935      937    +2     
=======================================
+ Hits         1868     1870    +2     
- Misses       5721     5724    +3     
- Partials       73       74    +1     

Impacted Files	Coverage Δ
...ntrols/src/components/CertifiedIconWithTooltip.tsx	33.33% <33.33%> (ø)
...-ui-chart-controls/src/components/MetricOption.tsx	53.33% <33.33%> (-5.00%)	⬇️

---

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 b952584...bedb10e. Read the comment docs.

[etr2460]

@ktmud the pills are now the same height!

[williaster]

thanks for making them the same height! ✨