Delete xform linked resources upon xform deletion #2136
Conversation
WinnyTroy
force-pushed
the
xform_deletion_cleanup
branch
from
f01fe31
to
6e693d6
Compare
onadata/apps/logger/models/xform.py
Outdated
| for metadata in self.metadata_set.all(): | ||
| # Only target media metadata | ||
| if metadata.data_type == 'media': | ||
| metadata.delete() |
There was a problem hiding this comment.
We should look into soft-deleting metadata perhaps? This seems like it would cause issues when the soft-deleted XForm is restored...
There was a problem hiding this comment.
I considered this, and I didnt see enough precedence to introduce soft_delete functionality on the MetaData model. I could be wrong on this though, hear me out.
Metadata holds form extras link enketo links and submission review enabling/disabling. These are often never deleted.
Its only now we are introducing deleting of media files associated with a form, and I dont believe we would want to restore these?,
For example, if a user creates a google sheet connection, we store the user google credentials within the form metadata. If the user requests to have the form's google sheet connection re-created, we just delete these form metadata. We wouldnt ever need to restore the old metadata.
There was a problem hiding this comment.
I think it'll lead to a bad user experience... Users should be aware that deleting their data does have some consequences but yea I'm not entirely sure about this
Thoughts @denniswambua ?
There was a problem hiding this comment.
@WinnyTroy Not all metadata can be regenerated. So we will need the soft delete on metadata.
There was a problem hiding this comment.
@denniswambua made this change
| @@ -953,8 +953,17 @@ def soft_delete(self, user=None): | |||
| update_fields.append('deleted_by') | |||
|
|
|||
| self.save(update_fields=update_fields) | |||
| # Delete associated filtered datasets | |||
| for dataview in self.dataview_set.all(): | |||
| dataview.soft_delete(user) | |||
There was a problem hiding this comment.
I wonder if we have a static method that can be used to restore an XForm? Seems like a nice to have IMO since this complicates the recovery process a bit
There was a problem hiding this comment.
Yeah, so these changes do increase complexity if a form is to be restored. I agree that we may need a management command that can reverse this, maybe a different issue can capture this?
To uncomplicate this though, I felt the inline comments help one understand the difference of linked datasets and merged datasets.
I also only removed the form media files, I left the other form metadata e.g enketo links, in the event a user may want to have the form restored.
There was a problem hiding this comment.
Yea, we don't have to implement a migration command for this... I think an instance method should do the trick. I'm fine with this being included in a separate PR.
I think re-creating the Enketo links is something we can easily do on the restoration of the form... The media files I'm iffy on
WinnyTroy
force-pushed
the
xform_deletion_cleanup
branch
4 times, most recently
from
a96c15d
to
ce74aeb
Compare
| form_metadata = [ | ||
| mt.data_type for mt in self.xform.metadata_set.filter( | ||
| deleted_at__isnull=True)] | ||
| self.assertNotIn('media', form_metadata) |
There was a problem hiding this comment.
Shouldn't we be checking that the response from the viewset in this case is empty ?
WinnyTroy
force-pushed
the
xform_deletion_cleanup
branch
from
ce74aeb
to
e2b9061
Compare
WinnyTroy
force-pushed
the
xform_deletion_cleanup
branch
2 times, most recently
from
da15874
to
8da71a4
Compare
WinnyTroy
force-pushed
the
xform_deletion_cleanup
branch
2 times, most recently
from
1c33192
to
8d19e16
Compare
WinnyTroy
force-pushed
the
xform_deletion_cleanup
branch
2 times, most recently
from
1e0ff06
to
cb9ed34
Compare
onadata/apps/logger/models/xform.py
Outdated
| for merged_dataset in self.mergedxform_ptr.all(): | ||
| merged_dataset.soft_delete(user) | ||
| # Delete associated Form Media Files | ||
| for metadata in self.metadata_set.all(): | ||
| metadata.soft_delete() |
There was a problem hiding this comment.
Since we are using soft delete, could you please filter by deleted_at___isnull=True?
There was a problem hiding this comment.
Great catch! Thanks
Made this change.
WinnyTroy
force-pushed
the
xform_deletion_cleanup
branch
from
cb9ed34
to
b6b5d33
Compare
…iles upon form deletion. Add documentation on clean up of filtered datasets
WinnyTroy
force-pushed
the
xform_deletion_cleanup
branch
from
b6b5d33
to
3e16c44
Compare
…iles upon form deletion. Add documentation on clean up of filtered datasets (#2136)
* test on charts by grouping multiple fields * test on charts with content type * update the format to json when content type has been provided * styling fix * styling fix * Update onadata/apps/api/tests/viewsets/test_charts_viewset.py Co-authored-by: Davis Raymond <[email protected]> * Clean out Merged Datasets upon xform deletion. Clean out Form Media Files upon form deletion. Add documentation on clean up of filtered datasets (#2136) * Query data endpoint by submission review. Filter `NULL` review status (#2144) * Add failing tests * Include ability to generate sql statement for NULL query strings. * Update data endpoint documentation * Remove unnecessary test * test on charts by grouping multiple fields * test on charts with content type * update the format to json when content type has been provided * styling fix * styling fix * Update onadata/apps/api/tests/viewsets/test_charts_viewset.py Co-authored-by: Davis Raymond <[email protected]> * Remove unnecessary test Co-authored-by: Davis Raymond <[email protected]> Co-authored-by: Winnie Kiragu <[email protected]>
Changes / Features implemented
Before submitting this PR for review, please make sure you have:
Closes #1029