dev/core#1753 - Attachments aren't deleted when deleting activity

[demeritcowboy]

Overview

https://lab.civicrm.org/dev/core/-/issues/1753

1. Create an activity, e.g. a meeting or one more likely to have an attachment like an email activity.
2. Add an attachment.
3. Delete the activity, e.g. from your contact's activity tab.
4. Look in sites/default/files/civicrm/custom. The attachment file is still there.
5. Also the entries are still there in civicrm_file and civicrm_entity_file even though the entry in civicrm_activity isn't there anymore.

Also in the added test the last three asserts all fail.

Before

Deleting an activity leaves the attachments orphaned in the filesystem and database.

After

Deleting an activity also deletes the attachments.

Technical Details

Oversight?

Possibly there's a reason it didn't delete attachments - anybody know of one? The function call I've added checks the reference count in case it's shared with another entity, so it's only deleted from the filesystem if this is the last reference.

Comments

I thought I had seen this before a long time ago but (a) in CiviCase you never delete, and (b) it doesn't really come up for me personally for regular activities either, and (c) it's not something users would notice and report.

[civibot]

(Standard links)

• If this is your first pull-request for CiviCRM, please browse CONTRIBUTING.md for information about the development and testing processes.
• If you are reviewing this pull-request, you may wish to consult the test sites and the Review Standards (long template, short template).

[demeritcowboy]

jenkins retest this please

[demeritcowboy]

jenkins retest this please

[totten]

Possibly there's a reason it didn't delete attachments - anybody know of one? The function call I've added checks the reference count in case it's shared with another entity, so it's only deleted from the filesystem if this is the last reference.

Yeah, IMHO, if the civicrm_file reference-count is zero, then I can't imagine a case for keeping it.

There are other cases which may sound similar - but don't apply, eg

• If we were talking about contact images, then references and locations can be really bonkers. But civicrm_file is more locked down.
• If we were talking about garbage-collection after-the-fact (e.g. deleting all orphaned files of unknown origin), then that might give pause. But this is deleting files that were attached (like 2 milliseconds ago) and now become orphans.

[kcristiano]

I have replicated the issue and done an r-run on this. The attachments are deleted when the activity is deleted.

I did test with CiviCase:

• add an activity to a Case
• Follow Up
• attach a document
• delete activity from within manage case

Attachment is not deleted. @demeritcowboy is this expected?

[demeritcowboy]

@kcristiano Thanks for testing and thanks for thinking of civicase. Yes it is expected because in civicase nothing is ever fully deleted. In the activities section on manage case there's a semi-hidden accordion called "Search Filters" above the table and in there is a "Deleted Activities" checkbox. If you click that you'll see the activity you deleted.

If you want to see how that matches up in the code it's just a few lines down from the change I've made, around line 197 in the else block that handles "moveToTrash".

[kcristiano]

Perfect. That makes sense and this looks good to merge.

[Stoob]

jenkins retest this please

[Stoob]

I have tested this PR.

The file is removed when the activity is deleted.

Attached screenshot is the result if you try to access it.

I can't personally think of a reason that deleting an entity in CiviCRM with an attached file wouldn't remove the file from the filesystem as well. This is the same behavior of Drupal when a node is deleted for what it's worth. But that doesn't mean that someone hasn't come to expect or use a file behavior in their workflow one way or the other. I seems unlikely they would, but I am not certain with whom to even check.

[jaapjansma]

@demeritcowboy what do you think of the comment posted by @Stoob? If you agree we can ask @eileenmcnaughton or @mattwire to merge this.

[eileenmcnaughton]

I'm +1 on merging this - I think people have kicked the tyres on it pretty well & tried to think of the edge cases.

@kcristiano has given it 'merge-ready' which should mean 'this is mergeable but giving a little more time for people to comment' - so I'll solicit last comments - really last objections on chat in case anyone has some

[demeritcowboy]

test this please

[demeritcowboy]

Thanks everybody!