Skip to content
This repository has been archived by the owner on Dec 10, 2021. It is now read-only.

feat: add certification icon to metrics #748

Merged
merged 1 commit into from
Aug 20, 2020
Merged

feat: add certification icon to metrics #748

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
67 changes: 67 additions & 0 deletions packages/superset-ui-chart-controls/src/components/CertifiedIconWithTooltip.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,67 @@
/**
* Licensed to the Apache Software Foundation (ASF) under one
* or more contributor license agreements. See the NOTICE file
* distributed with this work for additional information
* regarding copyright ownership. The ASF licenses this file
* to you under the Apache License, Version 2.0 (the
* "License"); you may not use this file except in compliance
* with the License. You may obtain a copy of the License at
*
* http://www.apache.org/licenses/LICENSE-2.0
*
* Unless required by applicable law or agreed to in writing,
* software distributed under the License is distributed on an
* "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
* KIND, either express or implied. See the License for the
* specific language governing permissions and limitations
* under the License.
*/
import React from 'react';
import { t } from '@superset-ui/translation';
import { supersetTheme } from '@superset-ui/style';
import { Tooltip, OverlayTrigger } from 'react-bootstrap';
import { kebabCase } from 'lodash';

const tooltipStyle: React.CSSProperties = { wordWrap: 'break-word' };

interface CertifiedIconWithTooltipProps {
certifiedBy?: string | null;
details?: string | null;
metricName: string;
}

function CertifiedIconWithTooltip({
certifiedBy,
details,
metricName,
}: CertifiedIconWithTooltipProps) {
return (
<OverlayTrigger
placement="top"
overlay={
<Tooltip id={`${kebabCase(metricName)}-tooltip`} style={tooltipStyle}>
{certifiedBy && <div>{t('Certified by %s', certifiedBy)}</div>}
<div>{details}</div>
</Tooltip>
}
>
{/* TODO: move Icons to superset-ui to remove duplicated icon code here */}
<svg
xmlns="http://www.w3.org/2000/svg"
enableBackground="new 0 0 24 24"
height="16"
viewBox="0 0 24 24"
width="16"
>
<g>
<path
fill={supersetTheme.colors.primary.base}
d="M23,12l-2.44-2.79l0.34-3.69l-3.61-0.82L15.4,1.5L12,2.96L8.6,1.5L6.71,4.69L3.1,5.5L3.44,9.2L1,12l2.44,2.79l-0.34,3.7 l3.61,0.82L8.6,22.5l3.4-1.47l3.4,1.46l1.89-3.19l3.61-0.82l-0.34-3.69L23,12z M9.38,16.01L7,13.61c-0.39-0.39-0.39-1.02,0-1.41 l0.07-0.07c0.39-0.39,1.03-0.39,1.42,0l1.61,1.62l5.15-5.16c0.39-0.39,1.03-0.39,1.42,0l0.07,0.07c0.39,0.39,0.39,1.02,0,1.41 l-5.92,5.94C10.41,16.4,9.78,16.4,9.38,16.01z"
/>
</g>
</svg>
</OverlayTrigger>
);
}

export default CertifiedIconWithTooltip;
29 changes: 24 additions & 5 deletions packages/superset-ui-chart-controls/src/components/MetricOption.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,21 +16,33 @@
* specific language governing permissions and limitations
* under the License.
*/
/* eslint-disable camelcase */
import React from 'react';
import styled from '@superset-ui/style';
import InfoTooltipWithTrigger from './InfoTooltipWithTrigger';
import { ColumnTypeLabel } from './ColumnTypeLabel';
import CertifiedIconWithTooltip from './CertifiedIconWithTooltip';

const FlexRowContainer = styled.div`
align-items: center;
display: flex;

> svg {
margin-right: ${({ theme }) => theme.gridUnit}px;
}
`;

export interface MetricOptionProps {
metric: {
// eslint-disable-next-line camelcase
verbose_name: string;
// eslint-disable-next-line camelcase
metric_name: string;
label: string;
description: string;
// eslint-disable-next-line camelcase
warning_text: string;
expression: string;
is_certified?: boolean;
certified_by?: string | null;
Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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)

Copy link
Contributor

Choose a reason for hiding this comment

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

@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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

certification_details?: string | null;
};
openInNewWindow: boolean;
showFormula: boolean;
Expand All @@ -54,8 +66,15 @@ export function MetricOption({
verbose
);
return (
<div className="metric-option">
<FlexRowContainer className="metric-option">
{showType && <ColumnTypeLabel type="expression" />}
{metric.is_certified && (
<CertifiedIconWithTooltip
metricName={metric.metric_name}
certifiedBy={metric.certified_by}
details={metric.certification_details}
/>
)}
<span className="option-label">{link}</span>
{metric.description && (
<InfoTooltipWithTrigger
Expand All @@ -81,6 +100,6 @@ export function MetricOption({
label={`warn-${metric.metric_name}`}
/>
)}
</div>
</FlexRowContainer>
);
}