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

Update annotation model to have JSON Metadata field #5745

Merged
merged 16 commits into from
Aug 31, 2018
Merged

Update annotation model to have JSON Metadata field #5745

Show file tree
Hide file tree
Changes from 11 commits
Commits
Show all changes
16 commits
Select commit Hold shift + click to select a range
6c25f54
add column to annotation model
hughhhh Aug 27, 2018
d3e83b0
update migration file
hughhhh Aug 27, 2018
770af9a
change to Text
hughhhh Aug 27, 2018
b23cfd3
remove old migration file
hughhhh Aug 27, 2018
cf017aa
add migration file
hughhhh Aug 27, 2018
3e68b4e
remove JSON
hughhhh Aug 27, 2018
6220966
add new column to view
hughhhh Aug 27, 2018
7691d96
add comma back
hughhhh Aug 27, 2018
70116b2
linting
hughhhh Aug 27, 2018
0a90aed
lint some more
hughhhh Aug 27, 2018
578b62d
missing comma
hughhhh Aug 27, 2018
9e9de8e
rename columns
hughhhh Aug 28, 2018
855a789
add degrade and new migration file
hughhhh Aug 29, 2018
e44df93
update version
hughhhh Aug 30, 2018
b20dd92
fixe changed name
hughhhh Aug 30, 2018
8463a01
remove json from list columns
hughhhh Aug 31, 2018
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
18 changes: 18 additions & 0 deletions superset/migrations/versions/40a0a483dd12_add_metadata_column_to_annotation_model_.py
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,18 @@
"""add_metadata_column_to_annotation_model.py

Revision ID: 40a0a483dd12
Revises: 1a1d627ebd8e
Create Date: 2018-08-27 14:25:28.079119

"""

# revision identifiers, used by Alembic.
revision = '40a0a483dd12'
down_revision = '1a1d627ebd8e'

from alembic import op
import sqlalchemy as sa


def upgrade():
op.add_column('annotation', sa.Column('annotation_metadata', sa.Text(), nullable=True))
Copy link
Member

Choose a reason for hiding this comment

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

Migration should have a downgrade too. You can grep and find many other migrations that add_column on upgrade and remove_column on downgarde

1 change: 1 addition & 0 deletions superset/models/annotations.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -42,6 +42,7 @@ class Annotation(Model, AuditMixinNullable):
layer = relationship(
AnnotationLayer,
backref='annotation')
annotation_metadata = Column(Text)
Copy link
Member

Choose a reason for hiding this comment

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

Knowing that this is the Annotation model, the pattern we've been following is to not repeat the entity name in the column names, as in just id and not annotation_id (since we know all columns in the table has to be annotation-related). There's different school of thought here and different school of data modeling will suggest different column naming schemes, but let's be consistent with other models in Superset.

Also could be nice to have json in the column name to let people know this column contains json text. So maybe something like extra_json or json_metadata.


__table_args__ = (
Index('ti_dag_state', layer_id, start_dttm, end_dttm),
Expand Down
13 changes: 11 additions & 2 deletions superset/views/annotations.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22,9 +22,12 @@ class AnnotationModelView(SupersetModelView, DeleteMixin): # noqa
add_title = _('Add Annotation')
edit_title = _('Edit Annotation')

list_columns = ['layer', 'short_descr', 'start_dttm', 'end_dttm']
list_columns = ['layer', 'short_descr', 'start_dttm', 'end_dttm',
'annotation_metadata']
Copy link
Member

Choose a reason for hiding this comment

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

I think annotation_metadata should be json_metadata, though actually we don't want the json in the list view as it may bloat the table, I think I'd just leave it out.

edit_columns = [
'layer', 'short_descr', 'long_descr', 'start_dttm', 'end_dttm']
'layer', 'short_descr', 'long_descr', 'start_dttm', 'end_dttm',
'annotation_metadata']

add_columns = edit_columns

label_columns = {
Expand All @@ -33,6 +36,12 @@ class AnnotationModelView(SupersetModelView, DeleteMixin): # noqa
'start_dttm': _('Start Dttm'),
'end_dttm': _('End Dttm'),
'long_descr': _('Long Descr'),
'annotation_metadata': _('JSON Metadata'),
}

description_columns = {
'annotation_metadata': 'This JSON represents any additional metadata this \
annotation needs to add more context.',
}

def pre_add(self, obj):
Expand Down