Add ability to remove attachments #132

goldpbear · 2018-08-05T23:35:36Z

Closes: #59
Addresses: #126

In conjunction with mapseed/platform#983

Create a new view, AttachmentInstanceView, to facilitate PATCH requests to individual attachments. Add a visible parameter to AttachmentModels to allow "deletion" of attachments from the client editor.

TODO:

Add tests

goldpbear · 2018-08-06T19:59:59Z

src/sa_api_v2/tests/test_views.py

@@ -517,7 +517,7 @@ def test_GET_from_cache(self):
        # - SELECT * FROM sa_api_attachment AS a
        #    WHERE a.thing_id IN (<self.place.id>);
        #
-        with self.assertNumQueries(11):
+        with self.assertNumQueries(14):


I don't have a good understanding of why these tests (this one and the next two) need an adjustment. I didn't want to delay the review any longer, but I'll look into the details of the number of queries executed here and hopefully get a better understanding.

I think it is fine to punt this issue for later, but we should file an issue and mark it as "technical-debt", and reference the issue as a TODO a comment on the three diffs below.

FYI here are the three queries that were added:

SELECT * FROM sa_api_place AS p JOIN sa_api_submittedthing as t ON (p.submittedthing_ptr_id = t.id) WHERE p.submittedthing_ptr_id = <self.place.id>; SELECT * from sa_api_dataset AS d WHERE d.id = <self.dataset.id>; SELECT * FROM auth_user as u1 WHERE u1.id = <self.owner.id>;

These queries are being made because we added a new field to our Attachment serializer:

url = AttachmentIdentityField()

where we are querying for the place_id, dataset_slug, and owner_username in AttachmentIdentityField. There should be a way to optimize this and avoid the extra queries, but I think we should punt this and address the issue at a later time, perhaps after the 1.8 upgrade.

goldpbear · 2018-08-06T20:03:37Z

src/sa_api_v2/views/base_views.py

+    def partial_update(self, *args, **kwargs):
+        attachment_id = self.kwargs['attachment_id']
+        obj = self.get_object_or_404(attachment_id)
+        self.model.clear_instance_cache(obj)


I found that we have to explicitly clear the cache on a partial_update; otherwise, deleted attachments will reappear on a page reload.

I'm not sure if this is the best way to handle cache clearing, though. Alternatively, we could overwrite partial_update (and update for that matter) in the CacheClearingModel, from which several other models inherit. I have a feeling this might address #126 too.

I think this solution makes sense for now. Note that you don't have to pass obj to the clear_instance_cache method:

api/src/sa_api_v2/models/caching.py

Line 27 in 47a204c

def clear_instance_cache(self):

I get the following error if I don't pass obj to clear_instance_cache:

TypeError: unbound method clear_instance_cache() must be called with Attachment instance as first argument

Ah, that's right. I think you want to do this:

obj.clear_instance_cache()

It's essentially doing the same thing, but I would prefer this approach because it's more OO versus calling the class's method and passing in the instance.

And maybe instead of obj we can call it something like attachment.

jalMogo

LGTM 👍

I have a few minor suggestions to clean up some of the code. Overall, I think this looks good!

I think we should add a test case for a PATCH on our attachment model, but I'm fine with making an issue for it and addressing it later.

jalMogo · 2018-08-07T10:06:50Z

src/sa_api_v2/views/base_views.py

+    def partial_update(self, *args, **kwargs):
+        attachment_id = self.kwargs['attachment_id']
+        obj = self.get_object_or_404(attachment_id)
+        self.model.clear_instance_cache(obj)


I think this solution makes sense for now. Note that you don't have to pass obj to the clear_instance_cache method:

api/src/sa_api_v2/models/caching.py

Line 27 in 47a204c

def clear_instance_cache(self):

jalMogo · 2018-08-07T20:46:25Z

src/sa_api_v2/serializers.py

-
-class AttachmentSerializer (EmptyModelSerializer, serializers.ModelSerializer):
+
+class AttachmentListSerializer (AttachmentSerializerMixin):
    file = AttachmentFileField()


It looks like this file field is not needed because our Attachment model already has a file field, which is being returned in the 'file': obj.file.storage.url(obj.file.name), field of our AttachmentSerializerMixin

jalMogo · 2018-08-07T20:47:59Z

src/sa_api_v2/serializers.py

    file = AttachmentFileField()
+    url = AttachmentIdentityField()


I think we want a url field for our attachment instance representations, as well as our attachment list representations (eg: http://localhost:8001/api/v2/smartercleanup/datasets/demo/places/3871/attachments/425)

To enable that, I think we should move this field into our AttachmentSerializerMixin.

jalMogo · 2018-08-07T20:56:18Z

src/sa_api_v2/serializers.py

+        }
+        fields = self.get_fields()
+
+        if 'url' in fields:


If we move the url = AttachmentIdentityField() field definition to this class, then we can remove this check

jalMogo · 2018-08-07T21:31:24Z

src/sa_api_v2/serializers.py

@@ -794,7 +811,7 @@ def get_detailed_submission_sets(self, place):
        return details

    def attachments_to_native(self, obj):
-        return [AttachmentSerializer(a).data for a in obj.attachments.all()]
+        return [AttachmentListSerializer(a, context=self.context).data for a in obj.attachments.all()]


Do we need this context kwarg? All of the tests pass, and the app seems to behave fine without it.

jalMogo · 2018-08-08T01:22:00Z

src/sa_api_v2/tests/test_views.py

@@ -517,7 +517,7 @@ def test_GET_from_cache(self):
        # - SELECT * FROM sa_api_attachment AS a
        #    WHERE a.thing_id IN (<self.place.id>);
        #
-        with self.assertNumQueries(11):
+        with self.assertNumQueries(14):


I think it is fine to punt this issue for later, but we should file an issue and mark it as "technical-debt", and reference the issue as a TODO a comment on the three diffs below.

FYI here are the three queries that were added:

SELECT * FROM sa_api_place AS p JOIN sa_api_submittedthing as t ON (p.submittedthing_ptr_id = t.id) WHERE p.submittedthing_ptr_id = <self.place.id>; SELECT * from sa_api_dataset AS d WHERE d.id = <self.dataset.id>; SELECT * FROM auth_user as u1 WHERE u1.id = <self.owner.id>;

These queries are being made because we added a new field to our Attachment serializer:

url = AttachmentIdentityField()

where we are querying for the place_id, dataset_slug, and owner_username in AttachmentIdentityField. There should be a way to optimize this and avoid the extra queries, but I think we should punt this and address the issue at a later time, perhaps after the 1.8 upgrade.

goldpbear added 4 commits August 5, 2018 16:34

WIP: update attachments

3643cfe

fix: docstring

36fca0b

remove unused import

3a7d98d

refactor: clear cache on update

e28fe97

goldpbear mentioned this pull request Aug 6, 2018

Make attachments editable mapseed/platform#983

Merged

goldpbear added 3 commits August 6, 2018 11:34

refactor: better attachment serializer abstraction

4983f7d

chore: add tests

afb6022

chore: tests

021371d

goldpbear commented Aug 6, 2018

View reviewed changes

goldpbear requested a review from modulitos August 6, 2018 20:04

goldpbear self-assigned this Aug 6, 2018

jalMogo approved these changes Aug 8, 2018

View reviewed changes

refactor(AttachmentListSerializer): filter out invisible attachments

4804b45

goldpbear mentioned this pull request Aug 9, 2018

Optimize additional DB queries generated by AttachmentIdentityField #137

Closed

goldpbear added 3 commits August 9, 2018 11:29

fix: tests

2883f09

refactor: incorporate review changes

8642e1d

refactor: review changes

7e22ad0

goldpbear merged commit 0c18080 into master Aug 10, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add ability to remove attachments #132

Add ability to remove attachments #132

goldpbear commented Aug 5, 2018 •

edited

Loading

goldpbear Aug 6, 2018

jalMogo Aug 8, 2018

goldpbear Aug 6, 2018

jalMogo Aug 7, 2018

goldpbear Aug 9, 2018

jalMogo Aug 9, 2018 •

edited

Loading

jalMogo left a comment

jalMogo Aug 7, 2018

jalMogo Aug 7, 2018

jalMogo Aug 7, 2018

jalMogo Aug 7, 2018

jalMogo Aug 7, 2018

jalMogo Aug 8, 2018

Add ability to remove attachments #132

Add ability to remove attachments #132

Conversation

goldpbear commented Aug 5, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jalMogo Aug 9, 2018 • edited Loading

Choose a reason for hiding this comment

jalMogo left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

goldpbear commented Aug 5, 2018 •

edited

Loading

jalMogo Aug 9, 2018 •

edited

Loading