[communication] update new phone numbers client #13560

0rland0Wats0n · 2021-02-03T20:24:17Z

This PR:

updates autorest/typescript extension
adds directive to make areaCode a required member of PhoneNumberSearchRequest
regenerates from the latest phone number swagger
sets up tracing in public client methods
adds tests for public methods

NB
PR does not include test recordings to reduce noise.

Co-authored-by: Dominik <[email protected]>

…Client.ts Co-authored-by: Dominik <[email protected]>

…pt/README.md Co-authored-by: Dominik <[email protected]>

…Client.ts Co-authored-by: Dominik <[email protected]>

Co-authored-by: Dominik <[email protected]>

…Client.ts Co-authored-by: Dominik <[email protected]>

…ign-phone-number-client

…Azure/azure-sdk-for-js into redesign-phone-number-client

0rland0Wats0n · 2021-02-03T20:25:46Z

sdk/communication/communication-phone-numbers/test/update.spec.ts

+      acquiredPhoneNumber,
+      update
+    );
+    const phoneNumber = await updatePoller.pollUntilDone();


I'm still working on the LROs. This actually works in node but fails in the browser.

DominikMe

Great progress!!
Left some comments but feel free to merge and iterate.

DominikMe · 2021-02-11T06:09:37Z

sdk/communication/communication-phone-numbers/package.json

@@ -30,7 +30,7 @@
    "lint": "eslint package.json tsconfig.json api-extractor.json src test --ext .ts -f html -o communication-phone-numbers-lintReport.html || exit 0",
    "pack": "npm pack 2>&1",
    "prebuild": "npm run clean",
-    "test": "npm run build:test && npm run unit-test && npm run integration-test",
+    "test": "rimraf dist-test && npm run build:test && npm run unit-test && npm run integration-test",


Is there a reason why we need to add rimraf here?

DominikMe · 2021-02-11T06:10:52Z

sdk/communication/communication-phone-numbers/review/communication-phone-numbers.api.md

-export interface AcquiredPhoneNumberUpdate {
-    applicationId?: string;
-    callbackUri?: string;
+    purchaseDate?: Date;


Heads up: These will change again in a future swagger, applicationId and callbackUri are going away, cost and purchaseDate will become required.

DominikMe · 2021-02-11T06:12:51Z

sdk/communication/communication-phone-numbers/review/communication-phone-numbers.api.md

-// @public (undocumented)
-export type ListPhoneNumbersOptions = OperationOptions;
+// @public
+export const enum KnownBillingFrequency {


Where does this Known prefix come from?

DominikMe · 2021-02-11T06:14:01Z

sdk/communication/communication-phone-numbers/review/communication-phone-numbers.api.md

+}
+
+// @public
+export const enum KnownPhoneNumberType {


Did autorest change how it models extensible enums? I would have expected string unions (and prefer them from a TS language perspective :-( )

Yea this changed with the version of autorest/ts I got to fix the LRO problem.

DominikMe · 2021-02-11T06:20:46Z

sdk/communication/communication-phone-numbers/src/phoneNumbersClient.ts

@@ -243,23 +158,14 @@ export class PhoneNumbersClient {
   */
  public listPhoneNumbers(
    options: ListPhoneNumbersOptions = {}
-  ): PagedAsyncIterableIterator<AcquiredPhoneNumber> {
+  ): PagedAsyncIterableIterator<AcquiredPhoneNumber, AcquiredPhoneNumber[], PageSettings> {


When I look at PagedAsyncIterableIterator, the second and third value are T[] and PageSettings by default, so this should still be the same as PagedAsyncIterableIterator<AcquiredPhoneNumber>

DominikMe · 2021-02-11T07:25:02Z

sdk/communication/communication-phone-numbers/test/lro.search.spec.ts

+      sms: "inbound+outbound",
+      calling: "none"
+    },
+    areaCode: "844"


Similar comment. If the service keeps giving out of stock errors, we should not specify an area code and let the service pick one.

Service requires an area code to be sent though ¯_(ツ)_/¯

DominikMe · 2021-02-11T07:26:55Z

sdk/communication/communication-phone-numbers/test/lro.search.spec.ts

+    }
+
+    const quantity = 2;
+    searchRequest.quantity = quantity;


nit: instead of modifying the searchRequest that's shared across tests I'd spread it and modify quantity, so on line 61
{...searchRequest, quantity: 2}

DominikMe · 2021-02-11T07:30:25Z

sdk/communication/communication-phone-numbers/test/utils/index.ts

+  };
+};
+
+// Fisher-Yates Shuffle algo implementation


Haha, overkill but okay. Used to be one of the easier interview questions I asked.

DominikMe · 2021-02-11T07:31:41Z

sdk/communication/communication-phone-numbers/test/utils/mockHttpClients.ts

 export const getPhoneNumberHttpClient: HttpClient = createMockHttpClient<AcquiredPhoneNumber>(200, {
  id: "+18005550100",
  phoneNumber: "+18005550100",
  countryCode: "US",
  phoneNumberType: "geographic",
-  assignmentType: "person",
+  assignmentType: "user",


Oooh, this should still be "person". Need to follow up with service folks

DominikMe · 2021-02-11T07:33:46Z

sdk/communication/communication-phone-numbers/test/utils/recordedClient.ts

-      recording.replace(/\/identities\/[^\/'",]*/, "/identities/sanitized"),
-    (recording: string): string => recording.replace(/\+\d{1}\d{3}\d{3}\d{4}/g, "+18005551234"),
+    (recording: string): string => recording.replace(/(https:\/\/)([^\/'",}]*)/, "$1endpoint"),
+    (recording: string): string => recording.replace(/\d{1}\d{3}\d{3}\d{4}/g, "14155550100"),


Thanks for cleaning this up and removing the unused bits 👍

* [communication] new phone numbers package (#13349) * [communication] update new phone numbers client (#13560) * [communication] disable extensible enums (#13806) * [communication] new phone numbers package (#13349) * [communication] update new phone numbers client (#13560) * [communication] disable extensible enums (#13806) * [communication] add samples to readme (#13957) * [communication] regenerate client from latest swagger (#13976) * regenerate client from latest swagger * update autorest/ts version also validates that change to lro code from regen does not break lros that worked before * add space * get lock file from master and build * Update package.json * rebuild

Merge Dev-containerservice-microsoft.containerservice-2021-03-01 API to master (Azure#13737) * Adds base for updating Microsoft.ContainerService from version stable/2021-02-01 to version 2021-03-01 * Updates readme * Updates API version in new specs and examples * fix the top swagger offenses caused by privateLinkResources, enableCSIProxy and OSDiskSize range (Azure#13349) Co-authored-by: Charlie Li <[email protected]> * Add enableFIPS property to 2021-03-01 API in dev 2021-03-01 branch (Azure#13359) * fix the top swagger offenses caused by privateLinkResources, enableCSIProxy and OSDiskSize range * Add enableFIPS property to the 2021-03-01 API * Add enableFIPS property to the 2021-03-01 API Co-authored-by: Charlie Li <[email protected]> * adding node-image upgrade channel enum (Azure#13375) Co-authored-by: Charlie McBride <[email protected]> * Add httpProxyConfig specs to 2021-03-01 API - dev (Azure#13410) * Add httpProxyConfig specs to 2021-03-01 API - dev * Fix prettier * aks: add `bindingSelector` to managed cluster pod identity profile (Azure#13399) * AKS runCommand new feature (Azure#13420) * runCommand target to 03-01 * fix api-version in examples * Add property disableLocalAccounts to 2021-03-01 API version (Azure#13385) * Add property disableLocalAccounts * Fix indentation * Add OSSKU and GPUInstanceProfile to containerservice/microsoft.container API 2021-03-01 (Azure#13532) * Add OSSKU and GPUInstanceProfile * Add sample for OSSKU and GPUInstanceProfile * Fix custom words * Reference sample files * update custom-words * Update readme for sdks (Azure#13515) * update readme for sdks * update readme for sdks Co-authored-by: Charlie Li <[email protected]> * Add example to create agent pool with FIPS enabled (Azure#13557) * add example to create agent pool with FIPS enabled * change a property name in the example file Co-authored-by: Charlie Li <[email protected]> * Added extended location parameter to managed cluster which will target all agent pools to that extended location. (Azure#13560) * Add data model for get OS options. (Azure#13619) * Add data model for get OS options. * Add default resource name in API path. * Fix container service tags. (Azure#13739) Co-authored-by: Tongyao Si <[email protected]> Co-authored-by: Charlie Li <[email protected]> Co-authored-by: Charlie McBride <[email protected]> Co-authored-by: Charlie McBride <[email protected]> Co-authored-by: Bo Wang <[email protected]> Co-authored-by: hbc <[email protected]> Co-authored-by: Haitao Chen <[email protected]> Co-authored-by: Tony Xu <[email protected]> Co-authored-by: Mirza Sikander <[email protected]> Co-authored-by: Jun Sun <[email protected]>

0rland0Wats0n and others added 30 commits January 20, 2021 09:19

ctrl+s

101b8b9

.

edc2db0

ctrl+s

771cf85

ctrl+s

d9eb0d9

.

d12ea5f

create new phone nnumbers package

c1a0262

.

b3b0fbc

.

c8dc1a0

add managed identity changes

87bbbae

use PhoneNumbersClient in samples to fix build

160eebc

add relevant env vars to tests.yml

cfdd24e

Update sdk/communication/communication-phone-numbers/test/README.md

b7aceab

Co-authored-by: Dominik <[email protected]>

Update sdk/communication/communication-phone-numbers/src/utils/logger.ts

d201cf9

Co-authored-by: Dominik <[email protected]>

Update sdk/communication/communication-phone-numbers/src/phoneNumbers…

fefc6e7

…Client.ts Co-authored-by: Dominik <[email protected]>

Update sdk/communication/communication-phone-numbers/src/phoneNumbers…

12e2567

…Client.ts Co-authored-by: Dominik <[email protected]>

Update sdk/communication/communication-phone-numbers/src/phoneNumbers…

70d7c38

…Client.ts Co-authored-by: Dominik <[email protected]>

Update sdk/communication/communication-phone-numbers/samples/typescri…

884ab05

…pt/README.md Co-authored-by: Dominik <[email protected]>

Update sdk/communication/communication-phone-numbers/samples/typescri…

a54bbe6

…pt/README.md Co-authored-by: Dominik <[email protected]>

Update sdk/communication/communication-phone-numbers/samples/javascri…

177b6e5

…pt/README.md Co-authored-by: Dominik <[email protected]>

Update sdk/communication/communication-phone-numbers/samples/javascri…

4e806b0

…pt/README.md Co-authored-by: Dominik <[email protected]>

Update sdk/communication/communication-phone-numbers/src/phoneNumbers…

7a011eb

…Client.ts Co-authored-by: Dominik <[email protected]>

Update sdk/communication/communication-phone-numbers/README.md

263a76d

Co-authored-by: Dominik <[email protected]>

Update sdk/communication/communication-phone-numbers/src/phoneNumbers…

f8a8300

…Client.ts Co-authored-by: Dominik <[email protected]>

rebuild to update review

58a459b

comment out links reported as broken

358c3e8

update test

9a0c714

.

66edf4c

regenerate

75a8fb2

add header tests

41e5212

formatted

66b94f3

0rland0Wats0n added 5 commits February 3, 2021 10:08

add getPhoneNumber tests

7dc19d7

add update tests

f5224cb

Merge branch 'master' of github.com:Azure/azure-sdk-for-js into redes…

45abc50

…ign-phone-number-client

Merge branch 'feature/communication/new-phone-numbers' of github.com:…

1162146

…Azure/azure-sdk-for-js into redesign-phone-number-client

Merge branch 'feature/communication/new-phone-numbers' of github.com:…

b0d5d8e

…Azure/azure-sdk-for-js into redesign-phone-number-client

ghost added the Communication label Feb 3, 2021

0rland0Wats0n requested a review from DominikMe February 3, 2021 20:24

0rland0Wats0n commented Feb 3, 2021

View reviewed changes

0rland0Wats0n added 8 commits February 4, 2021 08:42

update autorest.typescript version and regen

f9715df

update list method

2f56a28

ctrl+s

0e646f5

.

040471a

add remaining lro tests

0ad1e2d

.

05ada55

Delete .gitKeep

cc15101

.

d134c20

DominikMe reviewed Feb 11, 2021

View reviewed changes

0rland0Wats0n added 2 commits February 16, 2021 08:21

.

0948ebd

.

9ae961a

0rland0Wats0n marked this pull request as ready for review February 16, 2021 16:56

0rland0Wats0n requested review from ankitarorabit, bterlson, chradek, xirzec and a team as code owners February 16, 2021 16:56

.

f362fd6

0rland0Wats0n merged commit 91e5dc8 into Azure:feature/communication/new-phone-numbers Feb 16, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[communication] update new phone numbers client #13560

[communication] update new phone numbers client #13560

0rland0Wats0n commented Feb 3, 2021 •

edited

Loading

0rland0Wats0n Feb 3, 2021

DominikMe left a comment

DominikMe Feb 11, 2021

DominikMe Feb 11, 2021

DominikMe Feb 11, 2021

DominikMe Feb 11, 2021

0rland0Wats0n Feb 11, 2021

DominikMe Feb 11, 2021

DominikMe Feb 11, 2021

0rland0Wats0n Feb 11, 2021

DominikMe Feb 11, 2021

DominikMe Feb 11, 2021

DominikMe Feb 11, 2021

DominikMe Feb 11, 2021

[communication] update new phone numbers client #13560

[communication] update new phone numbers client #13560

Conversation

0rland0Wats0n commented Feb 3, 2021 • edited Loading

Choose a reason for hiding this comment

DominikMe left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

0rland0Wats0n commented Feb 3, 2021 •

edited

Loading