Allow deleting moved messages #219
Conversation
There was a problem hiding this comment.
Pull Request Overview
This PR adds support for deleting moved messages while refining the moved message processing workflow. Key changes include updates to message data handling (switching to a MessageData.scrape method and using files instead of attachments), the addition of test cases for snowflake extraction and moved message author detection, and the introduction of a context menu command for deleting moved messages.
Reviewed Changes
Copilot reviewed 8 out of 8 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| tests/test_message_moving.py | Adds comprehensive tests for snowflake extraction and moved message author logic. |
| app/utils/webhooks.py | Updates moved message formatting, replaces attachment handling with files, and renames a parameter. |
| app/utils/message_data.py | Switches to a MessageData subclass with asynchronous file scraping. |
| app/utils/init.py | Updates imports to reflect changes in message data handling. |
| app/core.py | Minor comment update for consistency. |
| app/components/zig_codeblocks.py | Adjusts call to move_message_via_webhook with a flag to disable move markers. |
| app/components/move_message.py | Introduces a context menu command to delete moved messages and updates permissions handling. |
| app/components/github_integration/mentions/resolution.py | Updates comment capitalization for consistency. |
|
I'll deal with the CI failure tomorrow (I didn't catch it before because Ruff 0.9.10, which is what I use, handles it correctly, while Ruff 0.9.6, which is what the CI uses as it is what is in the lockfile, doesn't). I would like a suggestion on what to do:
|
|
Let's bump ruff to v0.11. |
12c005a to
858c6e2
Compare
f46ba14 to
7a8789e
Compare
5d27463 to
f20a330
Compare
This comment was marked as outdated.
This comment was marked as outdated.
ac73a22 to
878fa83
Compare
14bb084 to
9a9b0a5
Compare
727fa5a to
44aa13b
Compare
|
Note: there is currently a known issue, where moved moved messages cannot be deleted, but I don't want to fix it now since it's an exceptionally rare case and it requires parsing the subtext, which I started in another commit and am not in the mood to do the commit surgery to reorder into this one. |
c1bf257 to
4a51c7b
Compare
249627a to
0d04362
Compare
017819a to
13d287b
Compare
| # WARNING: get_moved_message_author_id() makes a lot of assumptions about | ||
| # the structure of the subtext; be careful when editing this as | ||
| # invalidating the previous expected structure can break editing and | ||
| # deletion of messages moved before the change was merged or ALLOW THE | ||
| # WRONG PERSON TO EDIT OR DELETE A MOVED MESSAGE, *even if* that function | ||
| # is updated to match the new structure, as this subtext change won't | ||
| # retroactively apply to previously moved messages on Discord's servers. |
There was a problem hiding this comment.
I'm wondering whether we could counteract this neatly, e.g. by versioning the format somehow and deciding which format to check based on message timestamp. Obviously the ALLOW THE WRONG PERSON TO EDIT OR DELETE A MOVED MESSAGE scenario isn't very likely to happen, but it's still something we can consider I think.
There was a problem hiding this comment.
Message timestamp checks does sound like a good idea! It would definitely make the future changes for editing moved messages less fragile. An alternate I can think of is inserting zero-width spaces in the subtext, though that does sound hacky.
There was a problem hiding this comment.
Oh, I just realised, one problem with timestamp checks is that the timestamp would need to be updated when the bot is redeployed rather than when the change is made, which would be annoying to do correctly…
So should I use timestamps or something else?
There was a problem hiding this comment.
which would be annoying to do correctly
I think it would be best if I pushed a change to update the timestamp right before deploying (maybe I could move it a minute or two forward to account for the redeploy time). Worst case scenario there would be a small X-minute window of wrong edits/deletions being allowed, but users don't even know when a redeploy happens I believe.
There was a problem hiding this comment.
The hard part is remembering 😛.
425efa8 to
097709d
Compare
Co-authored-by: trag1c <dev@jakubr.me>
Closes half of section three of #155, and lays the groundwork for the other half.
It's not perfect, but I doubt the few edge cases would ever occur.