feat: add certifications to tables #11450
Conversation
etr2460
force-pushed
the
erik-ritter--table-certification-details
branch
from
98288f0
to
91c6344
Compare
Codecov Report
@@ Coverage Diff @@
## master #11450 +/- ##
==========================================
- Coverage 66.58% 62.28% -4.31%
==========================================
Files 863 870 +7
Lines 40985 41669 +684
Branches 3694 3802 +108
==========================================
- Hits 27290 25953 -1337
- Misses 13598 15537 +1939
- Partials 97 179 +82
Flags with carried forward coverage won't be shown. Click here to find out more.
Continue to review full report at Codecov.
|
etr2460
force-pushed
the
erik-ritter--table-certification-details
branch
from
91c6344
to
bc163e9
Compare
|
Should we add the verification icon to be after the table name? The alignment looks kind of weird right now. Imaging you have two tables starting with the same prefix and only one of them is verified: |
etr2460
force-pushed
the
erik-ritter--table-certification-details
branch
from
bc163e9
to
edc5800
Compare
|
Design preference has typically been for the verification icon before the entity name. This pattern is used for metrics in superset, and also aligns to specs from @kenchendesign |
etr2460
force-pushed
the
erik-ritter--table-certification-details
branch
2 times, most recently
from
52a08b7
to
013b075
Compare
|
another rebase should fix the broken E2E test. |
etr2460
force-pushed
the
erik-ritter--table-certification-details
branch
from
013b075
to
47938c1
Compare
| description={t( | ||
| 'Extra data to specify table metadata. Currently supports ' + | ||
| 'certification data of the format: `{ "certification": { "certified_by": ' + | ||
| '"Taylor Swift", "details": "This table is the source of truth." ' + |
There was a problem hiding this comment.
Lol, I like how we codified Taylor Swift to the actual user interface. She's certainly not a controversial figure, but should we use a more "business friendly" example? E.g. "certified_by": "Data Platform Team"
There was a problem hiding this comment.
fwiw, i've used her as an example before... https://github.com/apache/incubator-superset/search?q=Taylor+Swift
But happy to change them all if people have strong opinions. I seem to recall design from Preset indicating that Superset should have a friendly tone, so i would say Taylor Swift aligns with that tone.
There was a problem hiding this comment.
+1 on Taylor Swift
Or Batman. Batman may work too.
There was a problem hiding this comment.
Actually, thinking about this for a few more seconds, I vote let's draw the line on examples (where random/funny is ok) and keep tooltips / descriptions more businessy/serious
| @@ -492,6 +492,7 @@ class SqlaTable( # pylint: disable=too-many-public-methods,too-many-instance-at | |||
| sql = Column(Text) | |||
| is_sqllab_view = Column(Boolean, default=False) | |||
| template_params = Column(Text) | |||
| extra = Column(Text) | |||
There was a problem hiding this comment.
We don't need a db migration for this? If so, maybe add a link in this PR to the migration script that has added this column?
There was a problem hiding this comment.
The migration happened ages ago, see here: #10592
etr2460
force-pushed
the
erik-ritter--table-certification-details
branch
from
47938c1
to
27c84be
Compare
mistercrunch
added
🏷️ bot
SUMMARY
Adds certification details to tables in the new CRUD view and allows editing in both the old and new dataset editors
BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF
TEST PLAN
Add extra data to a table, see the certification details
Also test invalid data in extra and see no JS exceptions
ADDITIONAL INFORMATION
to: @nytai @ktmud @john-bodley