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.

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

Delete filtered datasets linked to a form when deleting a form #1307

Merged
merged 14 commits into from
Mar 28, 2018
Merged

Conversation

Wambere
Copy link
Contributor

@Wambere Wambere commented Mar 23, 2018

Fix #964

@Wambere Wambere requested review from pld and ukanga March 26, 2018 08:51
Copy link
Member

@pld pld left a comment

Choose a reason for hiding this comment

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

Since this is soft deleting the dataset, I think it should soft delete the filtered datasets as well (by setting an inactive flag).

Is there a reason not to do that?

Related things

  1. Is there a programmatic way to bring back soft deleted datasets? If there is we'd also want to modify that so brings back related solf deleted filtered datasets
  2. Is the filtered datasets set to accommodate soft deletes? If not we'll need to add a deleted_at, some default filters that ignore rows where that value is set, and a db index on it

def soft_delete(self):
"""
Soft deletes a dataview by adding a deleted_at timestamp and renaming
the dataview by adding a deleted-at and timestamp to the name.
Copy link
Member

Choose a reason for hiding this comment

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

this comment needs to explain why the deleted-at suffix is important, like the XForm soft_delete function's docstring does

@ukanga
Copy link
Member

ukanga commented Mar 27, 2018

Can you also add deleted_by field? So that we can know who deleted it?

@@ -53,6 +53,9 @@ pymongo
dict2xml
lxml

# python 3 compatibility
future
Copy link
Member

Choose a reason for hiding this comment

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

Can we please remove this requirement, I think it can come in the python-2-3 branch, the upstream package that required this onaio/python-json2xlsclient@dc545c7 handles it and as such it is not necessary to have this here.

@@ -57,6 +57,7 @@ fleming==0.4.5 # via django-query-builder
formencode==1.3.1 # via pyxform
funcsigs==1.0.2 # via mock
functools32==3.2.3.post2 # via jsonschema
future==0.16.0
Copy link
Member

Choose a reason for hiding this comment

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

Please remove, see above.

@@ -62,6 +62,7 @@ fleming==0.4.5 # via django-query-builder
formencode==1.3.1 # via pyxform
funcsigs==1.0.2 # via mock
functools32==3.2.3.post2 # via jsonschema
future==0.16.0
Copy link
Member

Choose a reason for hiding this comment

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

Please remove, see above.

- Add deleted_at field to dataview
- Add soft delete method
- Add test for soft delete method
- Sort imports
- Remove test for deleting dataview
- Clear xform_linked_dataview cache after soft deleting dataview
- Add deleted_at field to dataview_serializer
- Add filter to get_data_views function to remove soft deleted dataviews
- Add on_delete, default and blank to the deleted_by field
- Change variable from kwargs to user=None
@ukanga ukanga merged commit 0cd0762 into master Mar 28, 2018
@ukanga ukanga deleted the issue-964 branch March 28, 2018 16:49
@faith-mutua faith-mutua added the QA+ PR passed QA testing label Apr 4, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
QA+ PR passed QA testing
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants