Submission attachments list is not updated correctly after replacing an old image/file with a new one.

[JohnMwashuma]

Environment (local, stage, preview, production)

All

Expected behaviour

When an image/file in a submission is replaced, the old image/file should be replaced with the new one and deleted from the attachments list.

Actual behaviour

The old image/file is replaced successfully but the old image/file is still retained in the attachments list.

Steps to reproduce the behaviour

Use Enketo to edit a submission that has an attachment. Check the attachments for that submission and you will notice the previous attachment is still available.

---

Implementation plan

Blocking or related onadata issues

Aha! Link: https://ona.aha.io/features/PROD-213

[WinnyTroy]

The current implementation when editing submissions with new attachment files does not delete the old attachment, but creates a new attachment to the same submission alongside the old attachment files.

This could be helpful when keeping track of the submission history, however, as we never delete attachment files, this approach could fill up our storage unnecessarily.

@ukanga @pld @ivermac @JohnMwashuma @lincmba is there a different approach we could use, as in this instance when one edits a submission with a new media file?

[pld]

since this data is going in s3, and it's the way we're doing it now, I'm not particularly concerned about storage space.

If we can ignore attachments that are connected to submissions that are marked as deleted, would that be sufficient to address this error?

[JohnMwashuma]

So i found a way of filtering the attachments list from the submission api endpoint response. The response has the list of attachment fields with the attachment names. I used these names to filter the attachments.

[pld]

Does this issue affect exports or attachment downloads?

…

On Oct 4, 2019, at 4:14 AM, John Kirigha Mwashuma ***@***.***> wrote: So i found a way of filtering the attachments list from the submission api endpoint response. The response has the list of attachment fields with the attachment names. I used these names to filter the attachments. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#1696?email_source=notifications&email_token=AAAMMETCIHT7K3URGIMLQILQM33MZA5CNFSM4I4V52AKYY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOEAK3WQA#issuecomment-538295104>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAAMMEQ4FFTADRCHVFK74SDQM33MZANCNFSM4I4V52AA>.

[JohnMwashuma]

yes it affects attachments, all attachments are downloaded even the ones that had been replaced.

[pld]

sounds like we need a general fix then that only surfaces attachments to instance that do have deleted_at set to null

[ukanga]

We may need to rethink this again - it will be more like we should not expose in any way the attachments that have been replaced. Hence the files/attachments endpoint should not expose these records at all. I like the idea of deleted_at, but I hesitate exposing that information on the API at all since it would serve no purpose, the endpoint should use the field only to return attachments that are not replaced or deleted.

[pld]

Yes agree, that's how we use the deleted_at column in other tables

[WinnyTroy]

QA for this issue is proving to be abit tricky.
At the moment, deleting a submission cascade deletes all the attachments for the given submission.
However, when replacing the attachments in a submission from Enketo, we do not at any point interact with the AttachmentViewSet(And also the viewset is readonly), thus we do not trigger deletion of the replaced attachment.

After discussions with @DavisRayM and @ivermac , we discussed catching this update, while saving the attachments to the instance on this file

onadata/onadata/libs/utils/logger_tools.py

Lines 217 to 244 in 3faabd3

	def save_attachments(xform, instance, media_files):
	"""
	Saves attachments for the given instance/submission.
	"""
	# upload_path = os.path.join(instance.xform.user.username, 'attachments')
	for f in media_files:
	filename, extension = os.path.splitext(f.name)
	extension = extension.replace('.', '')
	content_type = u'text/xml' \
	if extension == Attachment.OSM else f.content_type
	if extension == Attachment.OSM and not xform.instances_with_osm:
	xform.instances_with_osm = True
	xform.save()
	filename = os.path.basename(f.name)
	media_in_submission = (
	filename in instance.get_expected_media() or
	[instance.xml.decode('utf-8').find(filename) != -1 if
	isinstance(instance.xml, bytes) else
	instance.xml.find(filename) != -1])
	if media_in_submission:
	Attachment.objects.get_or_create(
	instance=instance,
	media_file=f,
	mimetype=content_type,
	name=filename,
	extension=extension)
	update_attachment_tracking(instance)

At this point we could check the existing attachments for that instance and compare with the list of media files received from the update request and call the soft_delete action to delete the replaced attachments.
CC: @pld @ukanga

[pld]

how would we compare with the list of media files received?

is this that if we replace an attachment for an instance, we'd only want to delete the existing attachment for that same field?

in that case is the check that we see if there's an existing attachment for the field we're replacing the attachment in, and delete that existing one if there is?

I'm trying to avoid a loop over all attachments if know specifically what attachments are relevant.

[WinnyTroy]

is this that if we replace an attachment for an instance, we'd only want to delete the existing attachment for that same field?

We would be deleting existing attachments to an instance that have been replaced (i.e if they are no longer in the media files received with that instance).
If the attachment is still present in the media files received, then it wasnt replaced, so those we wont delete.

in that case is the check that we see if there's an existing attachment for the field we're replacing the attachment in, and delete that existing one if there is?

yes, this is what we had in mind.
We can identify new attachments for a submission as so:

onadata/onadata/libs/utils/logger_tools.py

Lines 354 to 359 in 3faabd3

	attachment_names = [
	a.media_file.name.split('/')[-1]
	for a in Attachment.objects.filter(instance=instance)
	]
	media_files = [f for f in media_files
	if f.name not in attachment_names]

We can then identify the attachments that have been replaced and soft_delete those.

replaced_attachments = [a for a in Attachment.objects.filter(instance=instance) 
                if a.media_file.name.split('/')[-1] not in media_files]

I'm trying to avoid a loop over all attachments if know specifically what attachments are relevant.

True. Alternatively, we could use generator expressions as opposed to list comprehensions to filter these replaced_attachments, so that we dont load all the attachments to memory. This could be less expensive imo.

[pld]

This sounds good, let's pursue your final comment below and try to do this without a loop.

True. Alternatively, we could use generator expressions as opposed to list comprehensions to filter these replaced_attachments, so that we dont load all the attachments to memory. This could be less expensive imo.

[WNjihia]

LGTM 🙂
Tested that deleted attachments do not appear on the submissions attachments when replaced
Tested that deleted attachments do not appear on the submissions attachments when deleted and not replaced
Tested this for audio files too

[ivermac]

@WNjihia I think you want this comment and QA+ label on the PR rather than the issue