Update annotation model to have JSON Metadata field #5745

hughhhh · 2018-08-27T21:29:36Z

Adding an additional field to Annotation Model, to allow users to store additional information about this record.

codecov-io · 2018-08-27T23:59:40Z

Codecov Report

Merging #5745 into master will decrease coverage by 0.03%.
The diff coverage is 100%.

@@            Coverage Diff             @@
##           master    #5745      +/-   ##
==========================================
- Coverage   63.45%   63.41%   -0.04%     
==========================================
  Files         362      366       +4     
  Lines       22978    25574    +2596     
  Branches     2554     2844     +290     
==========================================
+ Hits        14580    16219    +1639     
- Misses       8383     9340     +957     
  Partials       15       15

Impacted Files	Coverage Δ
superset/views/annotations.py	`76.08% <100%> (+0.53%)`	⬆️
superset/models/annotations.py	`93.1% <100%> (+0.24%)`	⬆️
superset/assets/src/explore/controls.jsx	`41.26% <0%> (-5.01%)`	⬇️
superset/assets/src/explore/visTypes.jsx	`53.33% <0%> (-3.81%)`	⬇️
superset/assets/src/modules/colors.js	`77.14% <0%> (-0.41%)`	⬇️
...uperset/assets/src/datasource/DatasourceEditor.jsx	`72.81% <0%> (-0.12%)`	⬇️
...ssets/src/visualizations/deckgl/layers/polygon.jsx	`0% <0%> (ø)`	⬆️
superset/assets/src/visualizations/country_map.js	`0% <0%> (ø)`	⬆️
superset/assets/src/visualizations/partition.js	`0% <0%> (ø)`	⬆️
...ssets/src/visualizations/deckgl/layers/scatter.jsx	`0% <0%> (ø)`	⬆️
... and 29 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 8d01c84...8463a01. Read the comment docs.

mistercrunch

For renaming the column in the db it'll come handy to have the downgrade method defined in your migration. First define the downgrade method with the old table name, then run superset db downgrade, then rename the table, ...

mistercrunch · 2018-08-28T00:00:06Z

superset/migrations/versions/40a0a483dd12_add_metadata_column_to_annotation_model_.py

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


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

mistercrunch · 2018-08-28T00:06:20Z

superset/models/annotations.py

@@ -42,6 +42,7 @@ class Annotation(Model, AuditMixinNullable):
    layer = relationship(
        AnnotationLayer,
        backref='annotation')
+    annotation_metadata = Column(Text)


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.

mistercrunch

For renaming the column in the db it'll come handy to have the downgrade method defined in your migration. First define the downgrade method with the old table name, then run superset db downgrade, then rename the table, ...

mistercrunch

For renaming the column in the db it'll come handy to have the downgrade method defined in your migration. First define the downgrade method with the old table name, then run superset db downgrade, then rename the table, ...

mistercrunch

For renaming the column in the db it'll come handy to have the downgrade method defined in your migration. First define the downgrade method with the old table name, then run superset db downgrade, then rename the table, ...

mistercrunch

For renaming the column in the db it'll come handy to have the downgrade method defined in your migration. First define the downgrade method with the old table name, then run superset db downgrade, then rename the table, ...

mistercrunch · 2018-08-28T00:56:18Z

Sorry github was down for a moment and I refreshed the page a few times, review got posted many times

mistercrunch · 2018-08-30T15:40:36Z

superset/views/annotations.py

@@ -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']


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.

mistercrunch · 2018-08-31T16:41:30Z

LGTM

* add column to annotation model * update migration file * change to Text * remove old migration file * add migration file * remove JSON * add new column to view * add comma back * linting * lint some more * missing comma * rename columns * add degrade and new migration file * update version * fixe changed name * remove json from list columns (cherry picked from commit 5e3f833)

* add column to annotation model * update migration file * change to Text * remove old migration file * add migration file * remove JSON * add new column to view * add comma back * linting * lint some more * missing comma * rename columns * add degrade and new migration file * update version * fixe changed name * remove json from list columns

* add column to annotation model * update migration file * change to Text * remove old migration file * add migration file * remove JSON * add new column to view * add comma back * linting * lint some more * missing comma * rename columns * add degrade and new migration file * update version * fixe changed name * remove json from list columns (cherry picked from commit 5e3f833)

* add column to annotation model * update migration file * change to Text * remove old migration file * add migration file * remove JSON * add new column to view * add comma back * linting * lint some more * missing comma * rename columns * add degrade and new migration file * update version * fixe changed name * remove json from list columns

hughhhh added 8 commits August 27, 2018 13:59

add column to annotation model

6c25f54

update migration file

d3e83b0

change to Text

770af9a

remove old migration file

b23cfd3

add migration file

cf017aa

remove JSON

3e68b4e

add new column to view

6220966

add comma back

7691d96

hughhhh changed the title ~~[WIP] Update annotation model~~ Update annotation model to have JSON Metadata field Aug 27, 2018

hughhhh added 3 commits August 27, 2018 15:48

linting

70116b2

lint some more

0a90aed

missing comma

578b62d

mistercrunch requested changes Aug 28, 2018

View reviewed changes

hughhhh added 3 commits August 28, 2018 10:59

rename columns

9e9de8e

add degrade and new migration file

855a789

update version

e44df93

mistercrunch reviewed Aug 30, 2018

View reviewed changes

hughhhh added 2 commits August 30, 2018 12:54

fixe changed name

b20dd92

remove json from list columns

8463a01

mistercrunch approved these changes Aug 31, 2018

View reviewed changes

hughhhh merged commit 5e3f833 into master Aug 31, 2018

kristw deleted the update-annotation-model branch March 6, 2019 16:46

mistercrunch added 🏷️ bot A label used by `supersetbot` to keep track of which PR where auto-tagged with release labels 🚢 0.28.0 labels Feb 27, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Update annotation model to have JSON Metadata field #5745

Update annotation model to have JSON Metadata field #5745

hughhhh commented Aug 27, 2018 •

edited

Loading

codecov-io commented Aug 27, 2018 •

edited

Loading

mistercrunch left a comment

mistercrunch Aug 28, 2018

mistercrunch Aug 28, 2018

mistercrunch left a comment

mistercrunch left a comment

mistercrunch left a comment

mistercrunch left a comment

mistercrunch commented Aug 28, 2018

mistercrunch Aug 30, 2018

mistercrunch commented Aug 31, 2018



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

Update annotation model to have JSON Metadata field #5745

Update annotation model to have JSON Metadata field #5745

Conversation

hughhhh commented Aug 27, 2018 • edited Loading

codecov-io commented Aug 27, 2018 • edited Loading

Codecov Report

mistercrunch left a comment

Choose a reason for hiding this comment

mistercrunch Aug 28, 2018

Choose a reason for hiding this comment

mistercrunch Aug 28, 2018

Choose a reason for hiding this comment

mistercrunch left a comment

Choose a reason for hiding this comment

mistercrunch left a comment

Choose a reason for hiding this comment

mistercrunch left a comment

Choose a reason for hiding this comment

mistercrunch left a comment

Choose a reason for hiding this comment

mistercrunch commented Aug 28, 2018

mistercrunch Aug 30, 2018

Choose a reason for hiding this comment

mistercrunch commented Aug 31, 2018

hughhhh commented Aug 27, 2018 •

edited

Loading

codecov-io commented Aug 27, 2018 •

edited

Loading