[BUG] `delete` does not delete message for recipient

[FloatingMilkshake]

Describe the bug
When a moderator deletes a message sent using ?delete, it disappears from the thread channel but is not deleted from the recipient's DMs with the bot.

Bot Info
Bot version (check with @modmail about): 3.9.4
Host method (Heroku, self-host, etc): Heroku

To Reproduce
Steps to reproduce the behavior:

1. Who can reproduce (ex. anyone, owners)? Anyone
2. Where can it be reproduced (ex. in thread channels, recipient DM's)? Issue is present in recipient DMs, however to reproduce you must use a command in a thread channel
3. Done what to cause the error? Delete a mod-sent message, observe as it is deleted from the thread channel but not from the recipient's DMs
4. Any recently made changes to your bot? No changes, the bot is copied exactly from this repository
5. Errored

Error Logs
https://hastebin.cc/sihuwejobo

Screenshots

Note that this screenshot is from a different server than the attached logs and as such, user/server/bot names and IDs may not match. Issue is the same and is equally reproducible in both servers. Bot is up-to-date in both (version 3.9.4), hosted with Heroku in both.

[Jerrie-Aries]

Please double check, make sure your bot has View Audit Log permission on your guild.

[lorenzo132]

This same issue is for me on a 767k membered guild, has all needed perms including View Audit Log. So this should not be the problem @Jerrie-Aries
I most likely think this has something to do with any form of caching fault

[FloatingMilkshake]

I can confirm that my bot also has the View Audit Log permission.

[StephenDaDev]

Unable to reproduce on the latest version, (the bot we use in the support server), VPS hosted.

[StephenDaDev]

Any logs when it happens?

[Jerrie-Aries]

This same issue is for me on a 767k membered guild, has all needed perms including View Audit Log. So this should not be the problem @Jerrie-Aries
I most likely think this has something to do with any form of caching fault

Sorry, my bad Renzo. I was just telling what the error log says, because it seemed like it came from here in bot.py:

File "/app/bot.py", line 1455, in on_message_delete
    entry = await audit_logs.find(lambda a: a.target == self.user)

And apparently I was wrong, the bot can execute it even without the View Audit Log permission.

I was not able to reproduce this either.

[Erisa]

From my investigation of this issue, I found it can be resolved in my case by essentially reverting beb73d8

In thread.py's delete_message function after that commit it has, among other things, this:

        if not isinstance(message, discord.Message):
            tasks += [message1.delete()]
        elif message2 is not None:
            tasks += [message2.delete()]
        elif message1.embeds[0].author.name.startswith("Persistent Note"):
            tasks += [self.bot.api.delete_note(message1.id)]
        if tasks:
            await asyncio.gather(*tasks)

As best I can tell, this means that if it deletes the first message (the if not isinstance(message, discord.Message):) then it will not move on to checking whether message2 exists and the recipient message is not touched despite it returning success.

Changing that elif message2 is not None: back to if resolves this particular issue by making it actually check and delete both message1 and message2.

The only reason I did not make a PR is because I do not know if this change has any adverse side effect (Possibly re-introducing #2910 ?) and I don't have the means to test it fully.
While I am familiar with development, I have little experience with Python or this bots codebase so I'm hesitant about making potentially breaking changes for other use-cases.

Author of the referenced commit would be @fourjr but I also noticed that it was from an idea by @Jerrie-Aries.

[lorenzo132]

This seems to work, a

From my investigation of this issue, I found it can be resolved in my case by essentially reverting beb73d8

In thread.py's delete_message function after that commit it has, among other things, this:

        if not isinstance(message, discord.Message):
            tasks += [message1.delete()]
        elif message2 is not None:
            tasks += [message2.delete()]
        elif message1.embeds[0].author.name.startswith("Persistent Note"):
            tasks += [self.bot.api.delete_note(message1.id)]
        if tasks:
            await asyncio.gather(*tasks)

As best I can tell, this means that if it deletes the first message (the if not isinstance(message, discord.Message):) then it will not move on to checking whether message2 exists and the recipient message is not touched despite it returning success.

Changing that elif message2 is not None: back to if resolves this particular issue by making it actually check and delete both message1 and message2.

The only reason I did not make a PR is because I do not know if this change has any adverse side effect (Possibly re-introducing #2910 ?) and I don't have the means to test it fully.
While I am familiar with development, I have little experience with Python or this bots codebase so I'm hesitant about making potentially breaking changes for other use-cases.

Author of the referenced commit would be @fourjr but I also noticed that it was from an idea by @Jerrie-Aries.

i'v tried this and can confirm it solves the issue, althought it would give

2021-06-19 15:57:58 __main__[1504] - ERROR: Ignoring exception in on_ready. 2021-06-19 15:57:58 __main__[1505] - ERROR: Unexpected exception: Traceback (most recent call last): File "/root/.local/share/virtualenvs/modmail15-jQmjnF-G/lib/python3.8/site-packages/discord/client.py", line 343, in _run_event await coro(*args, **kwargs) File "bot.py", line 588, in on_ready await thread.close( File "/root/modmails/modmail15/core/thread.py", line 366, in close "closer_id": closer.id, AttributeError: 'NoneType' object has no attribute 'id'

on startup now, atleast in my case.
It also will not return the image deleting error issue.

[Erisa]

I don't get that error in my case, no errors are reported neither at startup nor when issuing !delete.
Thank you for checking and confirming it solves the issue for you though!

[Jerrie-Aries]

Looks like the ?delete command is quite complicated than I thought. As I mentioned in the comment here, the delete_message method in thread.py is called twice if we use ?delete command to delete a thread message, first from the ?delete command itself and the second from the on_message_delete event. And I've explained there a little why that should be changed to elif.

Now as I looked through this issue again, there is no reason for this code in on_message_delete in bot.py:
https://github.com/kyb3r/modmail/blob/5fab5b7977fbc6bfbbabee0a19721490b764f7aa/bot.py#L1451-L1458
to be there. It returns the wrong value most of the time anyways.
The audit log entry couldn't be found if the bot deletes its own message (in this case, thread message or note as we use the ?delete command). Or it could return the wrong value if there was any bot messages deleted from users in any channel before the ?delete command is executed. And if it returns None, the bot would return early without executing the second call which I thought it's supposed to execute.

[lorenzo132]

This seems to work, a

From my investigation of this issue, I found it can be resolved in my case by essentially reverting beb73d8
In thread.py's delete_message function after that commit it has, among other things, this:

        if not isinstance(message, discord.Message):
            tasks += [message1.delete()]
        elif message2 is not None:
            tasks += [message2.delete()]
        elif message1.embeds[0].author.name.startswith("Persistent Note"):
            tasks += [self.bot.api.delete_note(message1.id)]
        if tasks:
            await asyncio.gather(*tasks)

As best I can tell, this means that if it deletes the first message (the if not isinstance(message, discord.Message):) then it will not move on to checking whether message2 exists and the recipient message is not touched despite it returning success.
Changing that elif message2 is not None: back to if resolves this particular issue by making it actually check and delete both message1 and message2.
The only reason I did not make a PR is because I do not know if this change has any adverse side effect (Possibly re-introducing #2910 ?) and I don't have the means to test it fully.
While I am familiar with development, I have little experience with Python or this bots codebase so I'm hesitant about making potentially breaking changes for other use-cases.
Author of the referenced commit would be @fourjr but I also noticed that it was from an idea by @Jerrie-Aries.

i'v tried this and can confirm it solves the issue, althought it would give

2021-06-19 15:57:58 __main__[1504] - ERROR: Ignoring exception in on_ready. 2021-06-19 15:57:58 __main__[1505] - ERROR: Unexpected exception: Traceback (most recent call last): File "/root/.local/share/virtualenvs/modmail15-jQmjnF-G/lib/python3.8/site-packages/discord/client.py", line 343, in _run_event await coro(*args, **kwargs) File "bot.py", line 588, in on_ready await thread.close( File "/root/modmails/modmail15/core/thread.py", line 366, in close "closer_id": closer.id, AttributeError: 'NoneType' object has no attribute 'id'

on startup now, atleast in my case.
It also will not return the image deleting error issue.

the error i had was caused by a plugin, so the revert works. havent had any issues past days

[Erisa]

@lorenzo132

the error i had was caused by a plugin, so the revert works. havent had any issues past days

It's a great hotfix for now, however I have noticed one time where deleting a message said that it failed when it in fact succeeded. Still, I keep it because wonky delete is better than no working delete.

A good longterm solution going forward would be what @Jerrie-Aries mentioned and appears to have committed to some dev fork. (Jerrie-Aries@25aa1d4)