-
Notifications
You must be signed in to change notification settings - Fork 1
feat (samples/webhooks): Report 'webhooks' server template from quickstart repository #302
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 (samples/webhooks): Report 'webhooks' server template from quickstart repository #302
Conversation
Dependency Review✅ No vulnerabilities or license issues found.Snapshot WarningsEnsure that dependencies are being submitted on PR branches and consider enabling retry-on-snapshot-warnings. See the documentation for more information and troubleshooting advice. Scanned Manifest Filesexamples/client/pom.xml
examples/getting-started/numbers/rent-first-available-number/pom.xml
examples/getting-started/numbers/search-available/pom.xml
examples/getting-started/sms/respond-to-incoming-message/pom.xml
examples/getting-started/sms/send-sms-message/pom.xml
examples/getting-started/verification/user-verification-using-sms-pin/pom.xml
examples/getting-started/voice/make-a-call/pom.xml
examples/getting-started/voice/respond-to-incoming-call/pom.xml
examples/snippets/pom.xml
examples/tutorials/voice/capture-leads-app/pom.xml
examples/webhooks/pom.xml
|
examples/webhooks/src/main/java/com/mycompany/app/mailgun/Controller.java
Outdated
Show resolved
Hide resolved
examples/webhooks/src/main/java/com/mycompany/app/mailgun/Controller.java
Outdated
Show resolved
Hide resolved
examples/webhooks/src/main/java/com/mycompany/app/mailgun/ServerBusinessLogic.java
Outdated
Show resolved
Hide resolved
| value = "/SmsEvent", | ||
| consumes = MediaType.APPLICATION_JSON_VALUE, | ||
| produces = MediaType.APPLICATION_JSON_VALUE) | ||
| public ResponseEntity<Void> smsDeliveryEvent( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nit: events are not about delivery reports only.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed
| public void processInboundEvent(InboundMessage event) { | ||
| trace(event); | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As you are splitting the inbounds and the delivery reports, do you think it makes sense to also showcase the different kinds of inbounds and delivery reports?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not sure, I could have think to reduce snippet to simple "SmsEvent"
The current trace will display the type of event (because of part of the toString from classes)
|
|
||
| LOGGER.finest("JSON response: " + serializedResponse); | ||
|
|
||
| return ResponseEntity.ok().body(serializedResponse); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same question for DICE, PIE and notification events: does it make sense to return "null" in the 200 OK JSON response body?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Right.
Replaced with empty bodies too.
|
|
||
| LOGGER.info("Handle event :" + event); | ||
|
|
||
| return SvamlControl.builder().build(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is there a place where you showcase the construction of a SVAML response (action + multiple instructions)?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No, not within this skeleton (this is not the purpose of this skeleton)
But when getting-started and tutorials will be migrated here: yes:
- https://github.com/sinch/sinch-sdk-java-quickstart/blob/main/getting-started/voice/respond-to-incoming-call/server/src/main/java/com/mycompany/app/voice/ServerBusinessLogic.java
- https://github.com/sinch/sinch-sdk-java-quickstart/blob/main/tutorials/voice/capture-leads-app/src/main/java/com/mycompany/app/voice/ServerBusinessLogic.java
| application-api-key: | ||
| application-api-secret: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nit: referred as application key and application secret in the documentation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Snippets are also based onto "xxx-api-xxx"; if going to this direction will be part of dedicated ticket
examples/webhooks/README.md
Outdated
| @@ -0,0 +1,79 @@ | |||
| # Backend application built using Sinch Java SDK to handle incoming webhooks | |||
|
|
|||
| This directory contains a server application based onto [Sinch SDK java](https://github.com/sinch/sinch-sdk-java) | |||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
English: don't use onto but on:
| This directory contains a server application based onto [Sinch SDK java](https://github.com/sinch/sinch-sdk-java) | |
| This directory contains a server application based on the [Sinch SDK java](https://github.com/sinch/sinch-sdk-java) |
Onto implies movement toward a surface or position, this is not the case here
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
| - `project-id`: YOUR_project_id | ||
| - `key-id`: YOUR_access_key_id | ||
| - `key-secret`: YOUR_access_key_secret |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For most of the use cases, they are not needed
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes but here we are delivering a general purpose skeleton
Added a dedicated comment for this section to help users/AIs to consume it properly
examples/webhooks/src/main/java/com/mycompany/app/mailgun/Controller.java
Outdated
Show resolved
Hide resolved
examples/webhooks/src/main/java/com/mycompany/app/numbers/Controller.java
Show resolved
Hide resolved
* feat (examples/getting-started): Report 'getting started' from quickstart repository
…emplate-from-quickstart-repo
…ory (#304) * feat (examples/getting-started): Report 'getting started' from quickstart repository
No description provided.