Skip to content

Send attachments improvements#1209

Merged
kwzrd merged 4 commits into
python-discord:masterfrom
wookie184:send_attachments-improvements-new
Oct 8, 2020
Merged

Send attachments improvements#1209
kwzrd merged 4 commits into
python-discord:masterfrom
wookie184:send_attachments-improvements-new

Conversation

@wookie184
Copy link
Copy Markdown
Contributor

@wookie184 wookie184 commented Oct 2, 2020 •
edited
Loading

Closes #1137 and closes #1139
I think this also closes #909

I just changed the default for use_cached to False, and didn't change it to set to True manually anywhere as I couldn't find anywhere that it was needed, antispam was the main one I checked but doesn't seem to try to fetch deleted messages from what I can see. Would be nice if somebody could confirm this

@wookie184 wookie184 requested a review from a team as a code owner October 2, 2020 19:16
@wookie184 wookie184 requested review from Senjan21 and scragly and removed request for a team October 2, 2020 19:16
@ghost ghost added the needs 2 approvals label Oct 2, 2020
MarkKoz
MarkKoz requested changes Oct 2, 2020
View reviewed changes
Copy link
Copy Markdown
Contributor

@MarkKoz MarkKoz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You're correct that antispam sends the attachments before deleting the messages. However, setting use_cached to false decreases the likelihood of successfully sending deleted attachments for all callers of the function. This is because a message can be deleted by the author while the bot is trying to process/handle the message. That being said, I don't know if it's really worth potentially making two requests per attachment via the idea to try the proxy and then the normal if the proxy fails.

Comment thread bot/exts/moderation/dm_relay.py
@ghost ghost added s: waiting for author Waiting for author to address a review or respond to a comment and removed needs 2 approvals labels Oct 2, 2020
@MarkKoz MarkKoz added a: backend Related to internal functionality and utilities (error_handler, logging, security, utils and core) a: moderation Related to community moderation functionality: (moderation, defcon, verification) t: bug Something isn't working p: 2 - normal Normal Priority labels Oct 2, 2020
@ghost ghost added needs 2 approvals and removed s: waiting for author Waiting for author to address a review or respond to a comment labels Oct 7, 2020
kwzrd
kwzrd approved these changes Oct 8, 2020
View reviewed changes
Copy link
Copy Markdown
Contributor

@kwzrd kwzrd left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks alright. I think it's a little risky to forward **kwargs because of the destination union type, but I guess it would be the caller's responsibility to make sure they don't pass an invalid kwarg.

@ghost ghost removed the needs 1 approval label Oct 8, 2020
@kwzrd kwzrd merged commit af9ab45 into python-discord:master Oct 8, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@MarkKoz MarkKoz MarkKoz approved these changes

@scragly scragly Awaiting requested review from scragly

@Senjan21 Senjan21 Awaiting requested review from Senjan21

+1 more reviewer

@kwzrd kwzrd kwzrd approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

a: backend Related to internal functionality and utilities (error_handler, logging, security, utils and core) a: moderation Related to community moderation functionality: (moderation, defcon, verification) p: 2 - normal Normal Priority t: bug Something isn't working

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Some attachments being ignored in dm-log ID not displayed in dm_relay when attachment sent 415 error for attachment re-upload

3 participants

@wookie184 @MarkKoz @kwzrd