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

[improvement] WhatsApp file uploading/sending and error handling #2072

Merged
merged 10 commits into from
Nov 19, 2024

Conversation

mattwr18
Copy link
Contributor

@mattwr18 mattwr18 commented Nov 7, 2024

TODO:

  • Add specs for statuses/errors in spec/requests/whats_app/three_sixty_dialog_webhook_spec.rb
  • refactor app/adapters/whats_app_adapter/three_sixty_dialog_outbound/file.rb to remove disabling rubocop

Specs for statuses/errors will be added in the PR that actually handles statuses.

@mattwr18 mattwr18 force-pushed the fix_whats_app_image_sending branch from 693071c to a37caf6 Compare November 7, 2024 16:22
payload: text_payload(
recipient, message.text
),
message_id: message.id)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think it might be a better UX to upload the file, if one is attached, then send out the file and after it was sent successfully. Alternatively, I've since clarified that we can send the message text as a caption with a limit of 1024 characters. So, we could potentially do that. For telegram, we check if the message text is greater than the limit and if it is we send it separately.

Copy link
Collaborator

@roschaefer roschaefer left a comment

Choose a reason for hiding this comment

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

This PR is looking good! 👍

We can do the caption idea that you mention, but we can also keep things simple stupid and just send files immediately followed by a text message, no? I do this often myself.

),
message_id: message.id)
else
files.each do |_file|
Copy link
Collaborator

Choose a reason for hiding this comment

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

We used to enqueue UploadFileJobs for every _file. Does it matter that we enqueue only one job now?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This actually was an oversight. We iterated through every file attached to a message to schedule a job to upload the file, but then iterated through a request's files to actually upload them so we were uploading many more files than were necessary.

@mattwr18 mattwr18 force-pushed the fix_whats_app_image_sending branch from a77b7da to 0f70a5e Compare November 18, 2024 11:10
@mattwr18 mattwr18 changed the title Send out the text of a message that has an image [improvement] WhatsApp file uploading/sending and error handling Nov 19, 2024
@mattwr18 mattwr18 force-pushed the fix_whats_app_image_sending branch from c9ac345 to 8637544 Compare November 19, 2024 11:21
@mattwr18 mattwr18 merged commit 60ee87f into main Nov 19, 2024
1 check passed
@mattwr18 mattwr18 deleted the fix_whats_app_image_sending branch November 19, 2024 11:25
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.

2 participants