Skip to content

Commit

Permalink
Add metric warning (#3499)
Browse files Browse the repository at this point in the history
* Adding warning text to metrics

* Adding javascript tests

* Fixing downgrade script for warning_text

* Adding merge migration
  • Loading branch information
michellethomas authored and mistercrunch committed Sep 22, 2017
1 parent 9af34ba commit 255ea69
Show file tree
Hide file tree
Showing 7 changed files with 76 additions and 9 deletions.
8 changes: 8 additions & 0 deletions superset/assets/javascripts/components/MetricOption.jsx
Expand Up @@ -27,6 +27,14 @@ export default function MetricOption({ metric }) {
tooltip={metric.expression}
label={`expr-${metric.metric_name}`}
/>
{metric.warning_text &&
<InfoTooltipWithTrigger
className="m-r-5 text-danger"
icon="warning"
tooltip={metric.warning_text}
label={`warn-${metric.metric_name}`}
/>
}
</div>);
}
MetricOption.propTypes = propTypes;
14 changes: 10 additions & 4 deletions superset/assets/spec/javascripts/components/MetricOption_spec.jsx
Expand Up @@ -13,6 +13,7 @@ describe('MetricOption', () => {
verbose_name: 'Foo',
expression: 'SUM(foo)',
description: 'Foo is the greatest metric of all',
warning_text: 'Be careful when using foo',
},
};

Expand All @@ -31,17 +32,22 @@ describe('MetricOption', () => {
expect(lbl).to.have.length(1);
expect(lbl.first().text()).to.equal('Foo');
});
it('shows 2 InfoTooltipWithTrigger', () => {
expect(wrapper.find(InfoTooltipWithTrigger)).to.have.length(2);
it('shows 3 InfoTooltipWithTrigger', () => {
expect(wrapper.find(InfoTooltipWithTrigger)).to.have.length(3);
});
it('shows only 1 InfoTooltipWithTrigger when no descr', () => {
it('shows only 2 InfoTooltipWithTrigger when no descr', () => {
props.metric.description = null;
wrapper = shallow(factory(props));
expect(wrapper.find(InfoTooltipWithTrigger)).to.have.length(1);
expect(wrapper.find(InfoTooltipWithTrigger)).to.have.length(2);
});
it('shows a label with metric_name when no verbose_name', () => {
props.metric.verbose_name = null;
wrapper = shallow(factory(props));
expect(wrapper.find('.option-label').first().text()).to.equal('foo');
});
it('shows only 1 InfoTooltipWithTrigger when no descr and no warning', () => {
props.metric.warning_text = null;
wrapper = shallow(factory(props));
expect(wrapper.find(InfoTooltipWithTrigger)).to.have.length(1);
});
});
5 changes: 4 additions & 1 deletion superset/connectors/base/models.py
Expand Up @@ -265,6 +265,7 @@ class BaseMetric(AuditMixinNullable, ImportMixin):
description = Column(Text)
is_restricted = Column(Boolean, default=False, nullable=True)
d3format = Column(String(128))
warning_text = Column(Text)

"""
The interface should also declare a datasource relationship pointing
Expand All @@ -289,5 +290,7 @@ def expression(self):

@property
def data(self):
attrs = ('metric_name', 'verbose_name', 'description', 'expression')
attrs = (
'metric_name', 'verbose_name', 'description', 'expression',
'warning_text')
return {s: getattr(self, s) for s in attrs}
3 changes: 2 additions & 1 deletion superset/connectors/druid/views.py
Expand Up @@ -85,7 +85,7 @@ class DruidMetricInlineView(CompactCRUDMixin, SupersetModelView): # noqa
list_columns = ['metric_name', 'verbose_name', 'metric_type']
edit_columns = [
'metric_name', 'description', 'verbose_name', 'metric_type', 'json',
'datasource', 'd3format', 'is_restricted']
'datasource', 'd3format', 'is_restricted', 'warning_text']
add_columns = edit_columns
page_size = 500
validators_columns = {
Expand All @@ -109,6 +109,7 @@ class DruidMetricInlineView(CompactCRUDMixin, SupersetModelView): # noqa
'metric_type': _("Type"),
'json': _("JSON"),
'datasource': _("Druid Datasource"),
'warning_text': _("Warning Message"),
}

def post_add(self, metric):
Expand Down
7 changes: 4 additions & 3 deletions superset/connectors/sqla/views.py
Expand Up @@ -108,7 +108,7 @@ class SqlMetricInlineView(CompactCRUDMixin, SupersetModelView): # noqa
list_columns = ['metric_name', 'verbose_name', 'metric_type']
edit_columns = [
'metric_name', 'description', 'verbose_name', 'metric_type',
'expression', 'table', 'd3format', 'is_restricted']
'expression', 'table', 'd3format', 'is_restricted', 'warning_text']
description_columns = {
'expression': utils.markdown(
"a valid SQL expression as supported by the underlying backend. "
Expand All @@ -135,7 +135,8 @@ class SqlMetricInlineView(CompactCRUDMixin, SupersetModelView): # noqa
'expression': _("SQL Expression"),
'table': _("Table"),
'd3format': _("D3 Format"),
'is_restricted': _('Is Restricted')
'is_restricted': _('Is Restricted'),
'warning_text': _('Warning Message'),
}

def post_add(self, metric):
Expand All @@ -151,7 +152,7 @@ def post_update(self, metric):

class TableModelView(DatasourceModelView, DeleteMixin): # noqa
datamodel = SQLAInterface(models.SqlaTable)

list_title = _('List Tables')
show_title = _('Show Table')
add_title = _('Add Table')
Expand Down
@@ -0,0 +1,26 @@
"""Adding metric warning_text
Revision ID: 19a814813610
Revises: ca69c70ec99b
Create Date: 2017-09-15 15:09:40.495345
"""

# revision identifiers, used by Alembic.
revision = '19a814813610'
down_revision = 'ca69c70ec99b'

from alembic import op
import sqlalchemy as sa


def upgrade():
op.add_column('metrics', sa.Column('warning_text', sa.Text(), nullable=True))
op.add_column('sql_metrics', sa.Column('warning_text', sa.Text(), nullable=True))


def downgrade():
with op.batch_alter_table('sql_metrics') as batch_op_sql_metrics:
batch_op_sql_metrics.drop_column('warning_text')
with op.batch_alter_table('metrics') as batch_op_metrics:
batch_op_metrics.drop_column('warning_text')
22 changes: 22 additions & 0 deletions superset/migrations/versions/472d2f73dfd4_.py
@@ -0,0 +1,22 @@
"""empty message
Revision ID: 472d2f73dfd4
Revises: ('19a814813610', 'a9c47e2c1547')
Create Date: 2017-09-21 18:37:30.844196
"""

# revision identifiers, used by Alembic.
revision = '472d2f73dfd4'
down_revision = ('19a814813610', 'a9c47e2c1547')

from alembic import op
import sqlalchemy as sa


def upgrade():
pass


def downgrade():
pass

0 comments on commit 255ea69

Please sign in to comment.