Skip to content
Merged
Show file tree
Hide file tree
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
21 changes: 20 additions & 1 deletion kpi/serializers/v2/paired_data.py
Original file line number Diff line number Diff line change
Expand Up @@ -62,8 +62,11 @@ def get_source__name(self, paired_data):
return source__name

def to_representation(self, instance):
source_deleted = self._source_is_deleted(instance)

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Let's move this line inside __get_source_asset_url() and get_source__name() and return None (change their signature accordingly).

You can decorate _source_is_deleted with @cache_for_request to avoid calling the DB twice.

in both methods, if self._source_is_deleted() returns None, we return right away, in the representation, no need to add conditions anymore. It would be handled inside other methods.

On the side note. That's not great. I think we should move away from the JSON Field and give PairedData its own db model and use FKs. Would you mind to create a linear issue for that?

return {
'source': self.__get_source_asset_url(instance),
'source': (
None if source_deleted else self.__get_source_asset_url(instance)
),
'source__name': self.get_source__name(instance),
'fields': instance.fields,
'filename': instance.filename,
Expand Down Expand Up @@ -120,6 +123,22 @@ def validate_source(self, source):
def update(self, instance, validated_data):
return self.__save(validated_data)

def _source_is_deleted(self, instance) -> bool:
"""
Return whether the source (parent) asset no longer exists.

A deleted source is not cleaned up from the destination's
`paired_data` because there is no cascading FK (see DEV-2034), so it
keeps appearing here. Reuse the `source__names` mapping the viewset
already built (uid absent means the row does not exist) to avoid an
extra query per row on the list endpoint; fall back to a direct
existence check when the context is not populated.
"""
source__names = self.context.get('source__names')
if source__names is not None:
return instance.source_uid not in source__names
return not Asset.objects.filter(uid=instance.source_uid).exists()

def _validate_fields(self, attrs: dict):

if 'fields' not in attrs:
Expand Down
21 changes: 21 additions & 0 deletions kpi/tests/api/v2/test_api_paired_data.py
Original file line number Diff line number Diff line change
Expand Up @@ -289,6 +289,27 @@ def test_create_with_already_used_filename(self):
self.assertEqual('`paired_data` is already used',
str(response.data['filename'][0]))

def test_list_with_deleted_source(self):
# anotheruser pairs their form with someuser's source asset
self.toggle_source_sharing(enabled=True)
self.source_asset.assign_perm(self.anotheruser, PERM_VIEW_SUBMISSIONS)
response = self.paired_data()
self.assertEqual(response.status_code, status.HTTP_201_CREATED)

# someuser deletes the source (parent) project. There is no FK cascade,
# so the destination asset still references it in its `paired_data` dict.
self.source_asset.delete()

# The destination owner lists their paired data. The deleted source must
# be reported with a null `source` (not just a null `source__name`).
self.login_as_other_user('anotheruser', 'anotheruser')
response = self.client.get(self.destination_asset_paired_data_url)
self.assertEqual(response.status_code, status.HTTP_200_OK)
self.assertEqual(response.data['count'], 1)
result = response.data['results'][0]
self.assertIsNone(result['source'])
self.assertIsNone(result['source__name'])

def test_create_paired_data_anonymous(self):
self.toggle_source_sharing(enabled=True)
payload = {
Expand Down
Loading