Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Support multipart file uploads for interaction responses via webhook builder #178

Open
wants to merge 14 commits into
base: main
Choose a base branch
from

Conversation

robherley
Copy link

@robherley robherley commented Oct 2, 2022

Summary

Currently, if you wish to attach a file to an interaction via the webhook builder it does not work.

Also, today the webhook builder does not support multiple attachments.

The desired behavior would be as follows:

file = File.open('/Users/robherley/Pictures/photo.png')

event.respond(content: 'photo') do |builder|
  builder.attachments = [file]
end

The methods defined in the webhook API only serialized the interaction data for a json payload:

components ||= view
data = builder.to_json_hash
Discordrb::API::Interaction.create_interaction_response(@token, @id, CALLBACK_TYPES[:channel_message], data[:content], tts, data[:embeds], data[:allowed_mentions], flags, components.to_a)

This PR adds support for file uploads via the interaction methods that support it. To avoid breaking changes and compatibility with the current method signature, I added an additional nil-by-default param. However, it would be nice to move these all to keyword arguments eventually.

I also added some specs for the modules that were missing them. They don't have complete coverage of the module, but cover the changes introduced in this PR.


Added

  • Discordrb::Webhooks::Builder#to_payload_hash: returns relevant hash depending on the presence of @file or @attachments
  • spec for Discordrb::API::Interaction
  • spec for Discordrb::Interaction

Changed

  • Discordrb::API::Interaction
    • added attachments as a parameter to #create_interaction_response
  • Discordrb::API::Webhook
    • added attachments as a parameter to #token_edit_message and #token_execute_webhook
  • Discordrb::Interaction
    • for #respond, #update_message, #edit_response, #send_message, #edit_message:
      • using #to_payload_hash instead of #to_json_hash for webhook builder
      • passing attachments param from webhook builder to relevant API method
      • ⚠️ worth noting that file from webhook builder was only used in #send_message (bc of #token_execute_webhook), so it'll be the only method that supports the deprecated param behavior

Deprecated

  • Discordrb::Webhooks::Builder#file: added warning log and appropriate yard doc comments

Removed

None

Fixed

  • Uploading attachments in interaction methods via webhook builder
  • The unrelated failing Channel#delete_all_emoji_reactions test, it was a small mistake with the emoji encoding

@robherley
Copy link
Author

@swarley is this PR good to merge as is? or would you prefer support for attachments vs. the existing single file implementation

@swarley
Copy link
Member

swarley commented Oct 9, 2022

@robherley If you're up to the task we can get the multi file upload going here, otherwise I can take care of expanding it in a separate PR. I'd like to get it working with multiple files before we tag a new release 🙂. Let me know what you want to do

@robherley
Copy link
Author

@swarley I can add the multi-file attachments in this PR! Just a couple impl questions:

  • would you prefer removing file from the webhook builder altogether? or should I just mark it as deprecated and append the single file to attachments
  • a lot of the ::Interaction methods use positional args instead of keyword args which makes invocation a bit awkward like this. are you opposed to changing those func signatures to use keyword arguments instead?

@swarley
Copy link
Member

swarley commented Oct 9, 2022

I would mark as deprecated, and I'd like to change the API methods to use kwargs in the next major release but currently they need to stay as positional for consistency and semver reasons.
I'm actively working on the API portion of v4 now and rest assured we won't have positional stacks like this

@robherley
Copy link
Author

@swarley multi-file attachments should be supported now:

fileA = File.open('/Users/robherley/Pictures/photoA.png')
fileB = File.open('/Users/robherley/Pictures/photoB.png')

event.respond(content: 'photos!') do |builder|
  builder.attachments = [fileA, fileB]
end

file will log out as deprecated and if a user supplies both it just falls back to the deprecated behavior until they switch completely to attachments. I thought about appending the file to the attachments list but that'll probably just create more confusing behavior 😅

also noticed the rubocop complaining about unchanged files 🤷 I can fix those if you want

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants