migrate data pivot to visual #1114
Conversation
There was a problem hiding this comment.
This is a hefty PR and covers a lot of changes, great stuff. Most of this looks good. I noticed the LabeledItems are not migrating correctly, and the LabeledItemListView (and admin list view) are still trying to prefetch information for the old labels via the nonexistent DataPivot models. We could just exclude the old labels in the queryset manager (.exclude(content_type__model__in=["datapivotquery", "datapivotupload"])) or perhaps overwrite the prefetching to avoid these content types. Otherwise, looks good to me.
|
|
||
| for label in LabeledItem.objects.filter(object_id=dp.id, content_type_id=dpq_ct): | ||
| new_label = LabeledItem.objects.create( | ||
| object_id=visualization.id, content_type_id=dpq_ct, label_id=label.label_id |
There was a problem hiding this comment.
Labels are not transferring from the old datapivots to the new ones. Changing the content_type_id to the Visual content type (not the datapivot type) fixes this.
|
|
||
| for label in LabeledItem.objects.filter(object_id=dp.id, content_type_id=dpu_ct): | ||
| new_label = LabeledItem.objects.create( | ||
| object_id=visualization.id, content_type_id=dpu_ct, label_id=label.label_id |
There was a problem hiding this comment.
See above comment; content_type_id needs to be changed from datapivot to visual content type id
There was a problem hiding this comment.
Looks good! Numbers seem to match up correctly after migrations (ie Visual and Dataset counts) and views seem to be working correctly. My only comment is that I don't see any use for the dp_id field being kept around; we should consider removing this field unless we want to keep it for posterity / have future need of it.
|
|
||
| Visual.objects.bulk_update(dps, fields=("created", "last_updated")) | ||
|
|
||
| pd.DataFrame(data=mapping, columns="model|old_id|new_id".split("|")).to_csv( |
There was a problem hiding this comment.
Not sure if this excel was generated just for debugging purposes, but if you intend to keep this code here consider adding the generated excel file to gitignore.
The DataPivot was a separate visualization type that even had a multi-table inheritance pattern, but in reality it is just a specific visualization type. This PR migrates all data pivots to be a specific visualization type, instead of a standalone model.
The main reason for the migration was delete a bunch of redundant code, and simplify cases where we want to show visualizations and data pivots on the same page, which would require 5+ queries instead of just a single query.
Migrate from this:
To this:
One of the trickiest parts of the migration was migrating a few additional settings from DataPivot into the
prefiltersfield in the Visual model; there's a little extra complexity in setting up the DataPivotQuery form that effectively unpacks and repacks this information when building the user-interface. I think/hope I added enough tests to cover the logic.Data Pivot File are now migrated to datasets instead of a excel-file on a visual. Timestamps were preserved with the migration.
A few additional changes were also made with this migration:
fieldsinstead ofexclude