Azure Communication Service - Phone Number Administration SDK by waynemo · Pull Request #15497 · Azure/azure-sdk-for-java

waynemo · 2020-09-22T23:39:07Z

This PR adds the initial Phone Number Administration SDK. It includes the following:

AutoRest Generated Client Code
Convenience Layer Sync + Async Clients and Builder
Unit Tests for Convenience Layer Builder
Playback Tests for Convenience Layer Sync + Async Clients

To be completed in separate PRs:

Exception handling / Null Checks
Java Doc Code Snippets
Long Running Operations
README updates

...istration/src/main/java/com/azure/communication/administration/PhoneNumberClientBuilder.java

...n/java/com/azure/communication/administration/implementation/PhoneNumberAdminClientImpl.java

srnagar · 2020-09-24T17:31:18Z

...inistration/src/main/java/com/azure/communication/administration/PhoneNumberAsyncClient.java

+     * @return A {@link Mono} containing a {@link AreaCodes} representing area codes.
+     */
+    @ServiceMethod(returns = ReturnType.SINGLE)
+    public Mono<AreaCodes> getAllAreaCodes(


Please ensure that none of these methods throw exception. Exception should be returned through Mono.error or Flux.error.

Perhaps we can update the error handling in a separate PR.

srnagar · 2020-09-24T17:32:48Z

...inistration/src/main/java/com/azure/communication/administration/PhoneNumberAsyncClient.java

+import java.util.stream.Collectors;
+
+/**
+ * Asynchronous client for Communication service phone number operations


In general, our javadocs should also include codesnippets on how to instantiate the client at the class level and how to use the APIs for each of the public methods.

Let's add this in a separate PR.

srnagar · 2020-09-24T17:38:17Z

...inistration/src/main/java/com/azure/communication/administration/PhoneNumberAsyncClient.java

+    public Mono<UpdateNumberCapabilitiesResponse> updateCapabilities(
+        Map<PhoneNumber, NumberUpdateCapabilities> phoneNumberCapabilitiesUpdate) {
+        Map<String, NumberUpdateCapabilities> capabilitiesMap = new HashMap<>();
+        for (Map.Entry<PhoneNumber, NumberUpdateCapabilities> entry : phoneNumberCapabilitiesUpdate.entrySet()) {


Add null checks before using user-input (phoneNumberCapabilitiesUpdate) and provide helpful error message. Do the same for other methods too.

Objects.requireNonNull(phoneNumberCapabilitiesUpdate, "'phoneNumberCapabilitiesUpdate' cannot be null");

Perhaps we can update the error handling in a separate PR.

srnagar · 2020-09-24T17:41:45Z

...inistration/src/main/java/com/azure/communication/administration/PhoneNumberAsyncClient.java

+    private final PhoneNumberAdminClientImpl phoneNumberAdminClient;
+
+    PhoneNumberAsyncClient(PhoneNumberAdminClientImpl phoneNumberAdminClient) {
+        this.phoneNumberAdminClient = phoneNumberAdminClient;


Looks like all APIs in this class only uses phoneNumberAdminClient.getAdministrations(). So, it might be simpler to have the Administrations type as instance variable.

Good idea. Will update.

srnagar · 2020-09-24T17:45:15Z

...istration/src/main/java/com/azure/communication/administration/PhoneNumberClientBuilder.java

+     * @return The updated {@link PhoneNumberClientBuilder} object.
+     */
+    public PhoneNumberClientBuilder pipeline(HttpPipeline pipeline) {
+        this.pipeline = Objects.requireNonNull(pipeline, "'pipeline' cannot be null.");


This can be null in scenarios where the user initially set the pipeline but then decided to clear it out. These validations can be postponed until the build method is called.

To be addressed in separate PR.

...istration/src/main/java/com/azure/communication/administration/PhoneNumberClientBuilder.java

...src/test/java/com/azure/communication/administration/PhoneNumberAsyncClientPlaybackTest.java

srnagar · 2020-09-24T17:51:15Z

...ation/src/test/java/com/azure/communication/administration/PhoneNumberClientBuilderTest.java

+import static org.junit.jupiter.api.Assertions.*;
+import static org.mockito.Mockito.*;
+
+@Execution(value = ExecutionMode.SAME_THREAD)


Why is this required?

It's needed for mocking in the unit tests. If the tests are run in parallel, the mocks will have conflicts.

…15497) * Add Phone Number functionality to azure-communication-administration * Update checkstyle suppressions

* Azure Communication Service - Phone Number Administration SDK (#15497) * Add Phone Number functionality to azure-communication-administration * Update checkstyle suppressions * Azure Communication Services - Refactor PhoneNumberAsyncClient to Wrap Exceptions (#15794) * Catch exceptions and return error Mono/PagedFlux * Refactor PhoneNumberAsyncClient method overloads to consolidate logic * Azure Communication Service - Update Phone Number Admin Null Checks (#15811) * Add null checks for required method arguments * Remove setter null check to allow clearing of httpPipeline * Updating azure-communication-administration README with a few samples (#15816) * Updating javadoc with a few samples * resolve comments * Fixing spot check errors * fix typo * Add Env Var check to skip PhoneNumber integration tests (#15855) * Samples and Readme update for Administration package (#15853) * More samples and readme changes * PR comments * build failure * fix indentation * azure-communication-administration - Regenerate Code for Updated Swagger (#15861) * Update swagger and regenerate code * Fix tests after code regeneration * Fix the param name * updating the sample code Co-authored-by: waynemo <wamo@microsoft.com>

waynemo added 3 commits September 22, 2020 16:34

Add Phone Number functionality to azure-communication-administration

4b5ee61

Update checkstyle suppressions

7b0eea4

Add missing copyright header

5a92026

waynemo marked this pull request as ready for review September 23, 2020 03:52

waynemo requested review from JianpingChen, JonathanGiles, RezaJooyandeh and mssfang as code owners September 23, 2020 03:52

waynemo requested a review from srnagar September 23, 2020 03:54

JianpingChen approved these changes Sep 23, 2020

View reviewed changes

...istration/src/main/java/com/azure/communication/administration/PhoneNumberClientBuilder.java Show resolved Hide resolved

RezaJooyandeh requested a review from ankitarorabit September 23, 2020 19:51

ankitarorabit reviewed Sep 23, 2020

View reviewed changes

...n/java/com/azure/communication/administration/implementation/PhoneNumberAdminClientImpl.java Show resolved Hide resolved

ankitarorabit approved these changes Sep 23, 2020

View reviewed changes

srnagar reviewed Sep 24, 2020

View reviewed changes

waynemo added 3 commits September 24, 2020 14:48

Updated class naming for integration tests

a97ccae

Simplify calls to autorest generated client

749ae4b

Update async client tests to use StepVerifier

4770006

waynemo merged commit 7a5fa95 into Azure:feature/communication/phonenumber Sep 25, 2020

This was referenced Sep 29, 2020

Azure Communication Services - Refactor PhoneNumberAsyncClient to Wrap Exceptions #15794

Merged

Azure Communication Service - Update Phone Number Admin Null Checks #15811

Merged

Conversation

waynemo commented Sep 22, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

waynemo commented Sep 22, 2020 •

edited

Loading