feat: deleting media when redacted events get removed #225
Conversation
jason-famedly
left a comment
jason-famedly
left a comment
There was a problem hiding this comment.
A few small nit things, a bigger thing and a question. Looking good though
| # Create media worker and it does not run the background tasks | ||
| media_worker = self.make_worker_hs( | ||
| "synapse.app.generic_worker", | ||
| {"worker_name": "media_worker_1", "run_background_tasks": False}, |
There was a problem hiding this comment.
| {"worker_name": "media_worker_1", "run_background_tasks": False}, | |
| {"worker_name": "media_worker_1", "run_background_tasks_on": "master"}, |
The setting should be run_background_tasks_on. It defaults to master if not declared. Let's match what you have up at line 990, explicit is probably better than relying on defaults
There was a problem hiding this comment.
Actually, for both this line and line 990 above, import MAIN_PROCESS_INSTANCE_NAME and assign it as that, then if it ever gets changed from master to main in the future we are covered(something upstream wanted since forever-ago)
| return config | ||
|
|
||
| def prepare(self, reactor: MemoryReactor, clock: Clock, hs: HomeServer) -> None: | ||
| self.profile_handler = self.hs.get_profile_handler() |
There was a problem hiding this comment.
Can lose this, these tests don't use the profile handler
| shorthand=False, | ||
| access_token=self.user_tok, | ||
| ) | ||
| assert channel.code == 404, channel.json_body |
There was a problem hiding this comment.
🤔 This one took me a minute. Of course this is returning a 404. You are requesting media endpoints on the main process instead of the media worker. The main process doesn't have media endpoints loaded, so returns a 404. I bet the response dict has a errcode: M_UNRECOGNIZED bit in it too. Let's try that with the media worker and see what it does
| deleted_media = self.get_success( | ||
| self.hs.get_datastores().main.get_local_media(media_id) | ||
| ) | ||
| assert deleted_media is None, deleted_media |
There was a problem hiding this comment.
Should we be checking if the file got deleted too?
Handles the deletion part of the media