feat(trust & safety): [SOCIALPLAT-766] Immediately remove public copy of media when status is deleted #54

nina-py · 2023-11-28T05:08:45Z

Immediately remove public copy of media when status is deleted.

Updated destroy method in StatusesController
Updated unit tests

JIRA reference: https://mozilla-hub.atlassian.net/browse/SOCTRUST-31

Immediately remove public copy of media when status is deleted. - Updated `destroy` method in StatusesController - Updated unit tests JIRA reference: https://mozilla-hub.atlassian.net/browse/SOCIALPLAT-766

aaga · 2023-11-28T16:33:27Z

app/controllers/api/v1/admin/statuses_controller.rb

+
+  def destroy_associated_media!
+    @status.ordered_media_attachments.each do |media|
+      media.destroy


I believe this only removes the Ruby object that keeps track of the media. In order to actually delete the media from S3/filesystem/wherever it is stored, we need to use the delete method from AttachmentBatch, similar to how vacuum_orphaned_records does.

aaga · 2023-11-28T16:36:51Z

app/controllers/api/v1/admin/statuses_controller.rb

+      if @status.with_media?
+        # Immediately remove public copy of media instead of waiting for
+        # the vacuum_orphaned_records job to take care of it later on
+        destroy_associated_media


This same logic needs to also happen in status_actions_controller.rb when type is delete. See the original API design doc for why the delete action is duplicated in two places.

Kat and I were discussing the possibility of pulling this function out (e.g. it could be another option in the RemovalWorker) so that it can be used in both places. Alternatively I don't think it's a huge deal if it's just repeated in both files since I think it can ultimately be a one-liner: AttachmentBatch.new(MediaAttachment, @status.media_attachments).delete (see comment below)

Refactored this into a Worker and added tests - do you think there's any benefit in abstracting the if statement into the Worker class, too? I was thinking it doesn't need to know anything about the status, only attachments, so kept it simple in the new class and used the conditional statement in both places in the code where attachments need to be deleted immediately.

aaga · 2023-11-28T16:51:51Z

app/controllers/api/v1/admin/statuses_controller.rb

@@ -48,4 +55,11 @@ def unsensitive
  def set_status
    @status = Status.find(params[:id])
  end
+
+  def destroy_associated_media!
+    @status.ordered_media_attachments.each do |media|


nit: doesn't need to be ordered_

toufali

Thanks @nina-py ! I'm missing context so this review might not hold up, but looks good apart from my questions below...

toufali · 2023-12-04T21:56:51Z

app/models/admin/status_batch_action.rb

+
+        # Immediately remove public copy of media instead of waiting for
+        # the vacuum_orphaned_records job to take care of it later on
+        Admin::MediaAttachmentDeletionWorker.perform_inline(status.media_attachments)


Should this function be perform() instead of perform_inline()? I don't see perform_inline() defined in the class, but maybe it's some sort of Rails sorcery?

Thanks Amri for having a look! We want the action to trigger immediately, so it should be perform_inline. The worker class itself has one method - perform, and it can be invoked with perform_inline for sync action or perform_async for asynchronous action behind the scenes.

There are a couple more methods like perform_in but they delay action rather than speed it up, so perform_inline is the best we can access, with the caveat that it will hold up that entire transaction though.

https://www.rubydoc.info/gems/sidekiq/Sidekiq/Job/ClassMethods

Ah – thanks for the education!

app/controllers/api/v1/admin/statuses_controller.rb

aaga · 2023-12-05T16:05:30Z

app/workers/admin/media_attachment_deletion_worker.rb

+  sidekiq_options queue: 'pull', lock: :until_executed, lock_ttl: 1.week.to_i
+
+  def perform(media_attachments)
+    AttachmentBatch.new(MediaAttachment, media_attachments).clear


@nina-py why did you choose clear here instead of delete ?

aaga · 2023-12-05T16:09:08Z

app/workers/admin/media_attachment_deletion_worker.rb

+class Admin::MediaAttachmentDeletionWorker
+  include Sidekiq::Worker
+
+  sidekiq_options queue: 'pull', lock: :until_executed, lock_ttl: 1.week.to_i


1 week seems too long for a Worker that will mostly be executed synchronously?

feat(trust&safety): [SOCIALPLAT-766] Remove media when status is deleted

e7fd119

Immediately remove public copy of media when status is deleted. - Updated `destroy` method in StatusesController - Updated unit tests JIRA reference: https://mozilla-hub.atlassian.net/browse/SOCIALPLAT-766

nina-py force-pushed the SOCIALPLAT-766-immediately-remove-public-copy-of-media branch from de1fad1 to e7fd119 Compare November 28, 2023 05:11

nina-py changed the title ~~feat(trust & safety): [SOCIALSocialplat 766 immediately remove public copy of media~~ feat(trust & safety): [SOCIALPLAT-766] Immediately remove public copy of media when status is deleted Nov 28, 2023

nina-py marked this pull request as ready for review November 28, 2023 05:28

aaga reviewed Nov 28, 2023

View reviewed changes

nina-py force-pushed the SOCIALPLAT-766-immediately-remove-public-copy-of-media branch from 2e2e63e to 12e6e51 Compare November 30, 2023 06:54

Refactor media deletion code into a Worker

93250ea

nina-py force-pushed the SOCIALPLAT-766-immediately-remove-public-copy-of-media branch from 12e6e51 to 93250ea Compare November 30, 2023 07:06

Add unit tests for MediaAttachmentDeletionWorker

3111e59

nina-py force-pushed the SOCIALPLAT-766-immediately-remove-public-copy-of-media branch from 054832d to 3111e59 Compare November 30, 2023 07:36

Deleting media attachments in status batch actions, too

09bfd22

toufali reviewed Dec 4, 2023

View reviewed changes

toufali approved these changes Dec 5, 2023

View reviewed changes

nina-py merged commit d0e1834 into main Dec 5, 2023
41 of 60 checks passed

aaga reviewed Dec 5, 2023

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(trust & safety): [SOCIALPLAT-766] Immediately remove public copy of media when status is deleted #54

feat(trust & safety): [SOCIALPLAT-766] Immediately remove public copy of media when status is deleted #54

nina-py commented Nov 28, 2023 •

edited

Loading

aaga Nov 28, 2023

aaga Nov 28, 2023 •

edited

Loading

nina-py Nov 30, 2023

aaga Nov 28, 2023

toufali left a comment

toufali Dec 4, 2023

nina-py Dec 5, 2023 •

edited

Loading

toufali Dec 5, 2023

aaga Dec 5, 2023

aaga Dec 5, 2023

feat(trust & safety): [SOCIALPLAT-766] Immediately remove public copy of media when status is deleted #54

feat(trust & safety): [SOCIALPLAT-766] Immediately remove public copy of media when status is deleted #54

Conversation

nina-py commented Nov 28, 2023 • edited Loading

aaga Nov 28, 2023

Choose a reason for hiding this comment

aaga Nov 28, 2023 • edited Loading

Choose a reason for hiding this comment

nina-py Nov 30, 2023

Choose a reason for hiding this comment

aaga Nov 28, 2023

Choose a reason for hiding this comment

toufali left a comment

Choose a reason for hiding this comment

toufali Dec 4, 2023

Choose a reason for hiding this comment

nina-py Dec 5, 2023 • edited Loading

Choose a reason for hiding this comment

toufali Dec 5, 2023

Choose a reason for hiding this comment

aaga Dec 5, 2023

Choose a reason for hiding this comment

aaga Dec 5, 2023

Choose a reason for hiding this comment

nina-py commented Nov 28, 2023 •

edited

Loading

aaga Nov 28, 2023 •

edited

Loading

nina-py Dec 5, 2023 •

edited

Loading