Skip to content

Conversation

@JPPortier
Copy link
Contributor

No description provided.

.setType(numberType)
.setRegionCode(regionCode)
.setSmsConfiguration(
SmsConfiguration.builder().setServicePlanId(servicePlanId).build())
Copy link
Contributor

Choose a reason for hiding this comment

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

Although the other getting started insists and the fact the number will be rented and configured, in this one, the outcome will be the same, but we don't explain the SMS configuration part.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Right (this is coming from current snippet implementation https://developers.sinch.com/docs/numbers/getting-started/java-sdk/rentany/#rent-the-first-available-number-using-the-java-sdk)

@alex-sberna Regarding this getting started title and content should highlight
What is your opinion ?

  • remove the configuration part ?
  • improve the comment to highlight the configuration ?
  • ?

Copy link
Contributor

Choose a reason for hiding this comment

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

For the rent any, I think it is important to highlight that you are renting and configuring the number in the same call, so improving the comment would be good.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done : b1e7ca9

Copy link
Contributor

@asein-sinch asein-sinch left a comment

Choose a reason for hiding this comment

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

Approved, waiting for Alex's feedback

@samwil-sinch
Copy link
Contributor

Successfully sent SMS using SMS API and Conversation API. However, I needed to manually setConversationRegion to ConversationRegion.EU; we may want to include a function that allows us to define the region in the config.properties file (in the same way we can set SMS_REGION).

Haven't tested/can't receive SMS because of ngrok restriction.

@alex-sberna
Copy link
Contributor

I just tested handling an incoming message with SMS, which I now realize already existed and isn't part of this PR, but one thing I ran into was the validation/authorization of the callback URL, with the web secret. I think for a getting started/quickstart that that should be commented out by default, rather than having a user needing to comment it out. If someone is just starting and testing things out, they very likely won't bother contacting their account manager to get a secret added in order for it to work.

@JPPortier
Copy link
Contributor Author

Successfully sent SMS using SMS API and Conversation API. However, I needed to manually setConversationRegion to ConversationRegion.EU; we may want to include a function that allows us to define the region in the config.properties file (in the same way we can set SMS_REGION).

Done

@JPPortier
Copy link
Contributor Author

I just tested handling an incoming message with SMS, which I now realize already existed and isn't part of this PR, but one thing I ran into was the validation/authorization of the callback URL, with the web secret. I think for a getting started/quickstart that that should be commented out by default, rather than having a user needing to comment it out. If someone is just starting and testing things out, they very likely won't bother contacting their account manager to get a secret added in order for it to work.

Done.
By default sources are not validating the webhooks but code still present (and not executed)

Copy link
Contributor

@alex-sberna alex-sberna left a comment

Choose a reason for hiding this comment

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

LGTM

@JPPortier JPPortier merged commit edb7d27 into main May 28, 2025
3 checks passed
@JPPortier JPPortier deleted the DEVEXP-912-snippets-migration branch May 28, 2025 14:18
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.

5 participants