Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Added ability to certify entities with multiple values #224

Merged
merged 6 commits into from
Nov 24, 2022
Merged

Added ability to certify entities with multiple values #224

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
0ca39a8
Added ability to certify entities with multiple values
reesercollins Nov 4, 2022
dcf1f1c
Update description text to reflect new feature
reesercollins Nov 4, 2022
bf5e94d
Merge branch 'cccs-2.0' into feature/CLDN-1777
reesercollins Nov 17, 2022
94282b7
Add tests for multiple certified by values
reesercollins Nov 18, 2022
cf41715
Merge branch 'cccs-2.0' into feature/CLDN-1777
reesercollins Nov 18, 2022
8a19bce
Merge branch 'cccs-2.0' into feature/CLDN-1777
reesercollins Nov 24, 2022
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
9 changes: 9 additions & 0 deletions superset-frontend/src/components/CertifiedBadge/CertifiedBadge.test.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -39,6 +39,15 @@ test('renders with certified by', async () => {
expect(await screen.findByRole('tooltip')).toHaveTextContent(certifiedBy);
});

test('renders with multiple certified by values', async () => {
const certifiedBy = ['Trusted Authority', 'Other Authority'];
render(<CertifiedBadge certifiedBy={certifiedBy} />);
userEvent.hover(screen.getByRole('img'));
expect(await screen.findByRole('tooltip')).toHaveTextContent(
certifiedBy.join(', '),
);
});

test('renders with details', async () => {
const details = 'All requirements have been met.';
render(<CertifiedBadge details={details} />);
Expand Down
8 changes: 5 additions & 3 deletions superset-frontend/src/components/CertifiedBadge/index.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,12 +17,12 @@
* under the License.
*/
import React from 'react';
import { t, useTheme } from '@superset-ui/core';
import { ensureIsArray, t, useTheme } from '@superset-ui/core';
import Icons, { IconType } from 'src/components/Icons';
import { Tooltip } from 'src/components/Tooltip';

export interface CertifiedBadgeProps {
certifiedBy?: string;
certifiedBy?: string | string[];
details?: string;
size?: IconType['iconSize'];
}
Expand All @@ -41,7 +41,9 @@ function CertifiedBadge({
<>
{certifiedBy && (
<div>
<strong>{t('Certified by %s', certifiedBy)}</strong>
<strong>
{t('Certified by %s', ensureIsArray(certifiedBy).join(', '))}
reesercollins marked this conversation as resolved.
Show resolved Hide resolved
</strong>
</div>
)}
<div>{details}</div>
Expand Down
2 changes: 1 addition & 1 deletion superset-frontend/src/components/Datasource/DatasourceEditor.jsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -905,7 +905,7 @@ class DatasourceEditor extends React.PureComponent {
description={t(
'Extra data to specify table metadata. Currently supports ' +
'metadata of the format: `{ "certification": { "certified_by": ' +
'"Data Platform Team", "details": "This table is the source of truth." ' +
'["Data Platform Team", "Engineering Team"], "details": "This table is the source of truth." ' +
'}, "warning_markdown": "This is a warning." }`.',
)}
control={
Expand Down
6 changes: 3 additions & 3 deletions superset/connectors/sqla/views.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -144,7 +144,7 @@ class TableColumnInlineView(
"extra": utils.markdown(
"Extra data to specify column metadata. Currently supports "
'certification data of the format: `{ "certification": "certified_by": '
'"Taylor Swift", "details": "This column is the source of truth." '
'["Taylor Swift", "Harry Styles"], "details": "This column is the source of truth." '
Copy link
Member

Choose a reason for hiding this comment

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

We should also add a test that confirms backward-compatibility (the a single value is passed instead of an array).

"} }`. This should be modified from the edit datasource model in "
"Explore to ensure correct formatting.",
True,
Expand Down Expand Up @@ -236,7 +236,7 @@ class SqlMetricInlineView(
"extra": utils.markdown(
"Extra data to specify metric metadata. Currently supports "
'metadata of the format: `{ "certification": { "certified_by": '
'"Data Platform Team", "details": "This metric is the source of truth." '
'["Data Platform Team", "Engineering Team"], "details": "This metric is the source of truth." '
'}, "warning_markdown": "This is a warning." }`. This should be modified '
"from the edit datasource model in Explore to ensure correct formatting.",
True,
Expand Down Expand Up @@ -443,7 +443,7 @@ class TableModelView( # pylint: disable=too-many-ancestors
"extra": utils.markdown(
"Extra data to specify table metadata. Currently supports "
'metadata of the format: `{ "certification": { "certified_by": '
'"Data Platform Team", "details": "This table is the source of truth." '
'["Data Platform Team", "Engineering Team"], "details": "This table is the source of truth." '
'}, "warning_markdown": "This is a warning." }`.',
True,
),
Expand Down