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

feat(messages): added new webhook, viber, and sms parameters #921

Merged
merged 2 commits into from
Mar 20, 2024

Conversation

manchuck
Copy link
Contributor

@manchuck manchuck commented Mar 1, 2024

Description

Added webhook parameters to all messages
Updated viber video and file parameters
Added SMS encoding and options to the SMS class
Removed conversations dependency for server-client

Motivation and Context

Testing Details

Example Output or Screenshots (if appropriate)

Types of changes

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to change)

Checklist

  • My code follows the code style of this project.
  • My change requires a change to the documentation.
  • I have updated the documentation accordingly.
  • I have read the CONTRIBUTING document.
  • I have added tests to cover my changes.
  • All new and existing tests passed.

@manchuck manchuck requested a review from dragonmantank March 1, 2024 15:35
@codecov-commenter
Copy link

codecov-commenter commented Mar 1, 2024

Codecov Report

Attention: Patch coverage is 85.71429% with 1 lines in your changes are missing coverage. Please review.

Project coverage is 84.62%. Comparing base (fbe1c27) to head (0489db3).
Report is 2 commits behind head on 3.x.

❗ Current head 0489db3 differs from pull request most recent head 67c5200. Consider uploading reports for the commit 67c5200 to get more accurate results

Files Patch % Lines
packages/messages/lib/classes/SMS/SMS.ts 80.00% 1 Missing ⚠️
Additional details and impacted files
@@           Coverage Diff           @@
##              3.x     #921   +/-   ##
=======================================
  Coverage   84.61%   84.62%           
=======================================
  Files         122      122           
  Lines        1775     1782    +7     
  Branches      363      367    +4     
=======================================
+ Hits         1502     1508    +6     
- Misses        273      274    +1     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@manchuck manchuck marked this pull request as draft March 1, 2024 15:48
Added webhook parameters to all messages
Updated viber video and file parameters
Added SMS encoding and options to the SMS class
Removed conversations dependency for server-client
@manchuck manchuck force-pushed the feat-message-updates branch from 866f68b to 0489db3 Compare March 5, 2024 20:22
@manchuck manchuck changed the title feat(messages): updating viber video and file parameters feat(messages): added new webhook, viber, and sms parameters Mar 5, 2024
@manchuck manchuck requested a review from superchilled March 5, 2024 20:23
@manchuck manchuck marked this pull request as ready for review March 5, 2024 20:23
@manchuck manchuck force-pushed the 3.x branch 5 times, most recently from a610acb to 35efc13 Compare March 15, 2024 20:13
Copy link
Contributor

@superchilled superchilled left a comment

Choose a reason for hiding this comment

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

Looks good overall. Just a few things I noticed/ wanted to query:

  • In DEVX-7950 for MMS Outbound Messages, it outlines a new caption property for the MMS vCard message type. However, in the current implementation of the Node SDK, it looks like MMSVcardParams is an intersection of MessageParams and MessageVcardType, neither of which contains a caption property. I can't see anything in this PR that changes that.

  • In [DEVX-7950] (https://jira.vonage.com/browse/DEVX-7950), for WhatsApp file message type, ity states "Add optional name property (string) to file object", but I can't see this in the existing implementation or that it's added in this PR. (It looks like WhatsAppFileParams just exports MessageParamsFile which sets file to MessageFileType

  • In DEVX-7950 for Viber, is outlines a new action property as part of the viber_service object, but this should be for text and image types only. In the current implementation it looks like ViberVideoParams also sets viberService to intersect with ViberActionParams (see

    )

Copy link
Contributor

@superchilled superchilled left a comment

Choose a reason for hiding this comment

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

Thanks for making the updates. I think this is good to go.

Added missing params for files and vCard
@manchuck manchuck force-pushed the feat-message-updates branch from 67c5200 to de1cdf2 Compare March 20, 2024 18:13
@manchuck manchuck merged commit 691a796 into 3.x Mar 20, 2024
13 checks passed
@manchuck manchuck deleted the feat-message-updates branch March 20, 2024 18:38
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.

3 participants