feat: add warning metadata to tables and metrics #13606
Conversation
|
/testenv up |
|
@etr2460 Ephemeral environment creation is currently limited to committers. |
etr2460
force-pushed
the
erik-ritter--metric-and-table-deprecation
branch
4 times, most recently
from
db80bab to
aeb83f3
Compare
etr2460
force-pushed
the
erik-ritter--metric-and-table-deprecation
branch
from
aeb83f3 to
8302744
Compare
| @@ -482,7 +496,7 @@ class SqlaTable( # pylint: disable=too-many-public-methods,too-many-instance-at | |||
| "fetch_values_predicate", | |||
| "extra", | |||
| ] | |||
| update_from_object_fields = [f for f in export_fields if not f == "database_id"] | |||
| update_from_object_fields = [f for f in export_fields if f != "database_id"] | |||
There was a problem hiding this comment.
ran into this while debugging and got really confused. this seems clearer
Codecov Report
@@ Coverage Diff @@
## master #13606 +/- ##
==========================================
+ Coverage 77.10% 77.25% +0.14%
==========================================
Files 921 922 +1
Lines 46745 46798 +53
Branches 5733 5746 +13
==========================================
+ Hits 36043 36152 +109
+ Misses 10567 10510 -57
- Partials 135 136 +1
Flags with carried forward coverage won't be shown. Click here to find out more.
Continue to review full report at Codecov.
|
|
/testenv up |
|
@ktmud Ephemeral environment creation is currently limited to committers. |
|
@robdiciuccio it seems there's something wrong with the committer detection logic in the Ephemeral environment workflow. |
|
@ktmud yep, turns out your Apache org membership needs to be public to work correctly. There's another possible code fix, but this should hopefully unblock for now. |
|
/testenv up |
|
@etr2460 Ephemeral environment spinning up at http://54.68.109.205:8080. Credentials are |
| <Field | ||
| label={t('Deprecation link')} | ||
| fieldKey="deprecation_link" | ||
| description={t( | ||
| 'A link with more details regarding the deprecation of this metric', | ||
| )} | ||
| control={ | ||
| <TextControl | ||
| controlId="deprecation_link" | ||
| placeholder="https://www.example.com" | ||
| /> | ||
| } | ||
| /> |
There was a problem hiding this comment.
I think having a freetext description field would be more convenient for most orgs. Instead of having a dedicated link field, could we have a general description field in which one can paste links if needed?
There was a problem hiding this comment.
That's a good suggestion, how would you recommend structuring a description field like that? Ideally, we could provide the capability for someone to embed a link and make it clickable/open in a new tab. Would rendering arbitrary markdown be a bad idea?
There was a problem hiding this comment.
I think we should be ok supporting the same markdown allowances we support on the dashboard markdown component:
I might be oversimplifying, but I'd be happy with just a simple freetext field that's then rendered as markdown (not sure if there's a simple way of forcing a new tab, but I suppose it should be doable).
There was a problem hiding this comment.
I don't think forcing a new tab in markdown is possible unless you use html a tags. i can play around a bit with supporting markdown descriptions. it's a bit less structured (which i'm not sure if that's good or bad).
For context, in internal use, the learn more link typically goes to a google doc or something else with lots of detailed, freeform content. So we made the choice in other products to just link out to there instead of making people write more context/details in product
There was a problem hiding this comment.
I think we can allow both the free-text markdown and the "Learn more" link, if it's not too much work.
There was a problem hiding this comment.
I found this, and making the following change seems to send the links to _blank:
diff --git a/superset-frontend/src/dashboard/components/gridComponents/Markdown.jsx b/superset-frontend/src/dashboard/components/gridComponents/Markdown.jsx
index b071a9982..5b30cd400 100644
--- a/superset-frontend/src/dashboard/components/gridComponents/Markdown.jsx
+++ b/superset-frontend/src/dashboard/components/gridComponents/Markdown.jsx
@@ -286,6 +286,7 @@ class Markdown extends React.PureComponent {
isValidNode: node => node.type !== 'script',
}),
]}
+ linkTarget="_blank"
/>
);
}
There was a problem hiding this comment.
Nice feature!
Would it make sense to extend this to more than Deprecation? E.g. It could also be used as a general warning for things like delayed data and suggesting an alternative field:
- "This data has been delayed for x hours. We are working on addressing this issue".
- "This field is for advanced use only. Please use "xxx_xxx" for simpler categorization."
superset/connectors/sqla/models.py
Outdated
| "is_certified", | ||
| "certified_by", | ||
| "certification_details", | ||
| "deprecated_at", |
There was a problem hiding this comment.
it may be nice to have deprecated_by field as well to have point of contact
|
A bunch of good suggestions here, thanks for the comments! I think if we're going to provide the flexibility of free text, then we should let the user add whatever content they want there (links, POCs, descriptions, replacements, etc.) and not worry about structuring it more. However, making this a generic warning might be even better. If we want to take the flexible route, then we could get rid of the deprecated_at field too and just rely on everything to be provided in markdown. Then it's just one simple field that anyone could use for whatever purpose. Super extensible for other users, and would serve the deprecation use case. I'm going to simplify and move forward with a generic |
etr2460
force-pushed
the
erik-ritter--metric-and-table-deprecation
branch
from
8302744 to
3c7995f
Compare
|
/testenv up |
|
@etr2460 Ephemeral environment spinning up at http://54.184.90.208:8080. Credentials are |
etr2460
force-pushed
the
erik-ritter--metric-and-table-deprecation
branch
from
3c7995f to
f8a15af
Compare
|
Here's the new user experience with markdown: |
etr2460
changed the title
etr2460
force-pushed
the
erik-ritter--metric-and-table-deprecation
branch
from
f8a15af to
fb91c83
Compare
There was a problem hiding this comment.
Thanks for the updates! Code LGTM, tested it locally and found a couple of UX issues:
-
Is the extra bottom padding intentional?
-
Just remembered we already have
warning_textfor metrics, can we maybe migrate it to the sameextra.warning_markdownfield?
-
The icon for metric warning markdown does not show up in any of the Dataset panel, Select options, or selected options. Maybe we should add all of them in a separate PR and only do datasource warning in this PR.
It does show up, however, in the Datasource editor modal.
BTW, there are like 3 different warning icons in this screen. Should we consolidate and make them more consistent?
Not related to this PR, but I think we also need some UI adjustment in the datasource editor modal:
- Move "extra" to "Advanced"
- Make the AceEditor for
extrataller and resizable - Move the two column layout to be two rows with Basic options on top and Advanced in the bottom.
- Add two column layout within the Basic and Advanced container
|
Here are the follow ups i'll make here:
Here's what i'll do in future PRs:
|
@etr2460 Thank you, Erik, for opening this PR and adjusting the UI element for consistency. Really appreciate it! |
@junlincc That's part of the user provided string, and isn't actually in the code anywhere. FWIW, we use |
etr2460
force-pushed
the
erik-ritter--metric-and-table-deprecation
branch
from
fb91c83 to
ddd90c8
Compare
|
I've made the changes mentioned above, but it won't build until apache-superset/superset-ui#1011 is merged and we have a new superset-ui version |
etr2460
force-pushed
the
erik-ritter--metric-and-table-deprecation
branch
3 times, most recently
from
b89012d to
c8993ac
Compare
|
@ktmud i think this is ready to go now |
|
etr2460
force-pushed
the
erik-ritter--metric-and-table-deprecation
branch
from
c8993ac to
c124618
Compare
etr2460
force-pushed
the
erik-ritter--metric-and-table-deprecation
branch
from
c124618 to
9318fd6
Compare
|
Ephemeral environment shutdown and build artifacts deleted. |
mistercrunch
added
🏷️ bot
SUMMARY
Similar to the need for certifying metrics and tables, often data should have warnings attached. This provides support for arbitrary warning markdown text to be provided for a metric or table
BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF
TEST PLAN
Verify the icon is displayed whenever the deprecated_at param is set. Also add unit tests
ADDITIONAL INFORMATION
to: @graceguo-supercat @ktmud @junlincc (could you tag the appropriate engineers from Preset here?)