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

Fixes anomalies for 'tenant people profilecardproperty' commands #5635

Closed
wants to merge 1 commit into from
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
48 changes: 0 additions & 48 deletions docs/docs/cmd/tenant/people/people-profilecardproperty-add.mdx
Original file line number Diff line number Diff line change
Expand Up @@ -66,54 +66,6 @@ m365 tenant people profilecardproperty add --name customAttribute1 --displayName

## Response

### Standard response

<Tabs>
<TabItem value="JSON">

```json
{
"directoryPropertyName": "UserPrincipalName",
"annotations": []
}
```

</TabItem>
<TabItem value="Text">

```text
annotations : []
directoryPropertyName: UserPrincipalName
```

</TabItem>
<TabItem value="CSV">

```csv
directoryPropertyName
UserPrincipalName
```

</TabItem>
<TabItem value="Markdown">

```md
# tenant people profilecardproperty add --name 'UserPrincipalName'

Date: 11/2/2023

Property | Value
---------|-------
directoryPropertyName | UserPrincipalName
```

</TabItem>
</Tabs>

### Response with a customAttribute

When we make use of one of the customAttributes, the response will differ.

<Tabs>
<TabItem value="JSON">

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -95,3 +95,7 @@ m365 tenant people profilecardproperty get --name customAttribute1

</TabItem>
</Tabs>

## More information

- https://learn.microsoft.com/graph/add-properties-profilecard
20 changes: 6 additions & 14 deletions docs/docs/cmd/tenant/people/people-profilecardproperty-list.mdx
Original file line number Diff line number Diff line change
Expand Up @@ -38,8 +38,7 @@ m365 tenant people profilecardproperty list
<TabItem value="JSON">

```json
{
"value": [
[
{
"directoryPropertyName": "customAttribute1",
"annotations": [
Expand All @@ -53,13 +52,8 @@ m365 tenant people profilecardproperty list
]
}
]
},
{
"directoryPropertyName": "Alias",
Adam-it marked this conversation as resolved.
Show resolved Hide resolved
"annotations": []
}
]
}
```

</TabItem>
Expand All @@ -69,7 +63,6 @@ m365 tenant people profilecardproperty list
directoryPropertyName displayName displayName de
--------------------- -------------- --------------
customAttribute1 Cost center Kostenstelle
Alias
```

</TabItem>
Expand All @@ -78,7 +71,6 @@ m365 tenant people profilecardproperty list
```csv
directoryPropertyName,displayName,displayName de
customAttribute1,Cost center,Kostenstelle
Alias,,
```

</TabItem>
Expand All @@ -96,11 +88,11 @@ m365 tenant people profilecardproperty list
directoryPropertyName | customAttribute1
displayName | Cost center
displayName de | Kostenstelle

Property | Value
---------|-------
directoryPropertyName | Alias
```

</TabItem>
</Tabs>
</Tabs>

## More information

- https://learn.microsoft.com/graph/add-properties-profilecard
Original file line number Diff line number Diff line change
Expand Up @@ -141,7 +141,7 @@ describe(commands.PEOPLE_PROFILECARDPROPERTY_ADD, () => {
throw `Invalid request ${opts.url}`;
});

await assert.doesNotReject(command.action(logger, { options: { name: 'userPrincipalName' } }));
await command.action(logger, { options: { name: 'userPrincipalName' } });
assert(loggerLogSpy.calledOnceWithExactly(propertyResponse));
});

Expand All @@ -154,7 +154,7 @@ describe(commands.PEOPLE_PROFILECARDPROPERTY_ADD, () => {
throw `Invalid request ${opts.url}`;
});

await assert.doesNotReject(command.action(logger, { options: { name: 'userPrincipalName', debug: true } }));
await command.action(logger, { options: { name: 'userPrincipalName', debug: true } });
assert(loggerLogSpy.calledOnceWithExactly(propertyResponse));
});

Expand All @@ -167,7 +167,7 @@ describe(commands.PEOPLE_PROFILECARDPROPERTY_ADD, () => {
throw `Invalid request ${opts.url}`;
});

await assert.doesNotReject(command.action(logger, { options: { name: 'fax' } }));
await command.action(logger, { options: { name: 'fax' } });
assert(loggerLogSpy.calledOnceWithExactly(propertyResponse));
});

Expand Down Expand Up @@ -249,6 +249,19 @@ describe(commands.PEOPLE_PROFILECARDPROPERTY_ADD, () => {
assert(loggerLogSpy.calledOnceWithExactly(customAttributePropertyTextResponse));
});

it('uses correct casing for name when incorrect casing is used', async () => {
const postStub = sinon.stub(request, 'post').callsFake(async (opts) => {
if (opts.url === `https://graph.microsoft.com/v1.0/admin/people/profileCardProperties`) {
return customAttributePropertyResponse;
}

throw 'Invalid Request';
});

await command.action(logger, { options: { name: 'ALIAS', output: 'json' } });
assert.strictEqual(postStub.lastCall.args[0].data.directoryPropertyName, 'Alias');
});

it('fails when the addition conflicts with an existing property', async () => {
sinon.stub(request, 'post').callsFake(async (opts) => {
if (opts.url === `https://graph.microsoft.com/v1.0/admin/people/profileCardProperties`) {
Expand Down
17 changes: 10 additions & 7 deletions src/m365/tenant/commands/people/people-profilecardproperty-add.ts
Original file line number Diff line number Diff line change
Expand Up @@ -15,8 +15,6 @@ interface Options extends GlobalOptions {
}

class TenantPeopleProfileCardPropertyAddCommand extends GraphCommand {
private excludeOptions: string[] = ['name', 'displayName', 'debug', 'verbose', 'output'];

public get name(): string {
return commands.PEOPLE_PROFILECARDPROPERTY_ADD;
}
Expand All @@ -35,8 +33,13 @@ class TenantPeopleProfileCardPropertyAddCommand extends GraphCommand {

#initTelemetry(): void {
this.telemetry.push((args: CommandArgs) => {
// Add unknown options to telemetry
const unknownOptions = Object.keys(this.getUnknownOptions(args.options));
const unknownOptionsObj = unknownOptions.reduce((obj, key) => ({ ...obj, [key]: true }), {});

Object.assign(this.telemetryProperties, {
displayName: typeof args.options.displayName !== 'undefined'
displayName: typeof args.options.displayName !== 'undefined',
...unknownOptionsObj
});
});
}
Expand All @@ -62,15 +65,15 @@ class TenantPeopleProfileCardPropertyAddCommand extends GraphCommand {
return `${args.options.name} is not a valid value for name. Allowed values are ${profileCardPropertyNames.join(', ')}`;
}

if (propertyName.startsWith("customattribute") && args.options.displayName === undefined) {
if (propertyName.startsWith('customattribute') && args.options.displayName === undefined) {
return `The option 'displayName' is required when adding customAttributes as profile card properties`;
}

if (!propertyName.startsWith("customattribute") && args.options.displayName !== undefined) {
if (!propertyName.startsWith('customattribute') && args.options.displayName !== undefined) {
return `The option 'displayName' can only be used when adding customAttributes as profile card properties`;
}

const unknownOptions = Object.keys(args.options).filter(key => !this.excludeOptions.includes(key));
const unknownOptions = Object.keys(this.getUnknownOptions(args.options));

if (!propertyName.startsWith('customattribute') && unknownOptions.length > 0) {
return `Unknown options like ${unknownOptions.join(', ')} are only supported with customAttributes`;
Expand Down Expand Up @@ -148,7 +151,7 @@ class TenantPeopleProfileCardPropertyAddCommand extends GraphCommand {
}

private getLocalizations(options: Options): { languageTag: string, displayName: string }[] {
const unknownOptions = Object.keys(options).filter(key => !this.excludeOptions.includes(key));
const unknownOptions = Object.keys(this.getUnknownOptions(options));

if (unknownOptions.length === 0) {
return [];
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -129,6 +129,19 @@ describe(commands.PEOPLE_PROFILECARDPROPERTY_GET, () => {
assert(loggerLogSpy.calledOnceWith(textOutput));
});

it('uses correct casing for name when incorrect casing is used', async () => {
const getStub = sinon.stub(request, 'get').callsFake(async (opts) => {
if (opts.url === `https://graph.microsoft.com/v1.0/admin/people/profileCardProperties/${profileCardPropertyName}`) {
return response;
}

throw 'Invalid Request';
});

await command.action(logger, { options: { name: profileCardPropertyName.toUpperCase() } });
assert(getStub.called);
});

it('handles error when profile card property does not exist', async () => {
sinon.stub(request, 'get').rejects({
response: {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -56,8 +56,11 @@ class TenantPeopleProfileCardPropertyGetCommand extends GraphCommand {
await logger.logToStderr(`Retrieving information about profile card property '${args.options.name}'...`);
}

// Get the right casing for the profile card property name
const profileCardProperty = profileCardPropertyNames.find(p => p.toLowerCase() === args.options.name.toLowerCase());

const requestOptions: CliRequestOptions = {
url: `${this.resource}/v1.0/admin/people/profileCardProperties/${args.options.name}`,
url: `${this.resource}/v1.0/admin/people/profileCardProperties/${profileCardProperty}`,
Comment on lines +59 to +63
Copy link
Contributor

Choose a reason for hiding this comment

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

Should we really enforce the casing for the get command? Folks could always add properties straight to the graph API with their own casing. If they then want to get the property through the CLI they'll get an empty result while it exists with different casing.

The same goes for the remove and set commands.

Copy link
Contributor Author

@milanholemans milanholemans Nov 5, 2023

Choose a reason for hiding this comment

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

Yes, but on the other hand if they specify for example FAX in the add command and use the same value for the remove or get command, they will receive an error that the property doesn't exist, which is weird.

In my opinion we do it everywhere or nowhere at all, the commands should work seamlessly together. It's a mess that we have to deal with this indeed.

Copy link
Member

Choose a reason for hiding this comment

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

I agree to keep consistent behavior across the commands.

headers: {
accept: 'application/json;odata.metadata=none'
},
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -130,7 +130,7 @@ describe(commands.PEOPLE_PROFILECARDPROPERTY_LIST, () => {
});

await command.action(logger, { options: { verbose: true } });
assert(loggerLogSpy.calledOnceWith(response));
assert(loggerLogSpy.calledOnceWith(response.value));
});

it('lists profile card properties information for other than json output', async () => {
Expand Down
20 changes: 4 additions & 16 deletions src/m365/tenant/commands/people/people-profilecardproperty-list.ts
Original file line number Diff line number Diff line change
@@ -1,9 +1,9 @@
import { Logger } from '../../../../cli/Logger.js';
import GlobalOptions from '../../../../GlobalOptions.js';
import GraphCommand from '../../../base/GraphCommand.js';
import request, { CliRequestOptions } from '../../../../request.js';
import { ProfileCardProperty } from './profileCardProperties.js';
import commands from '../../commands.js';
import { odata } from '../../../../utils/odata.js';

interface CommandArgs {
options: GlobalOptions;
Expand All @@ -18,29 +18,17 @@ class TenantPeopleProfileCardPropertyListCommand extends GraphCommand {
return 'Lists all profile card properties';
}

constructor() {
super();
}

public async commandAction(logger: Logger, args: CommandArgs): Promise<void> {
try {
if (this.verbose) {
await logger.logToStderr(`Lists all profile card properties...`);
await logger.logToStderr(`Listing all profile card properties...`);
}

const requestOptions: CliRequestOptions = {
url: `${this.resource}/v1.0/admin/people/profileCardProperties`,
headers: {
accept: 'application/json;odata.metadata=none'
},
responseType: 'json'
};

const result = await request.get<ProfileCardProperty[]>(requestOptions);
const result = await odata.getAllItems<ProfileCardProperty>(`${this.resource}/v1.0/admin/people/profileCardProperties`);
milanholemans marked this conversation as resolved.
Show resolved Hide resolved
let output: any = result;

if (args.options.output && args.options.output !== 'json') {
output = output.value.sort((n1: ProfileCardProperty, n2: ProfileCardProperty) => {
output = output.sort((n1: ProfileCardProperty, n2: ProfileCardProperty) => {
const localizations1 = n1.annotations[0]?.localizations?.length ?? 0;
const localizations2 = n2.annotations[0]?.localizations?.length ?? 0;
if (localizations1 > localizations2) {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -76,51 +76,68 @@ describe(commands.PEOPLE_PROFILECARDPROPERTY_REMOVE, () => {
});

it('correctly removes profile card property for userPrincipalName', async () => {
sinon.stub(request, 'delete').callsFake(async (opts) => {
const removeStub = sinon.stub(request, 'delete').callsFake(async (opts) => {
if (opts.url === `https://graph.microsoft.com/v1.0/admin/people/profileCardProperties/UserPrincipalName`) {
return;
}

throw `Invalid request ${opts.url}`;
});

await assert.doesNotReject(command.action(logger, { options: { name: 'userPrincipalName' } }));
await command.action(logger, { options: { name: 'userPrincipalName' } });
assert(removeStub.called);
});

it('correctly removes profile card property for userPrincipalName (debug)', async () => {
sinon.stub(request, 'delete').callsFake(async (opts) => {
const removeStub = sinon.stub(request, 'delete').callsFake(async (opts) => {
if (opts.url === `https://graph.microsoft.com/v1.0/admin/people/profileCardProperties/UserPrincipalName`) {
return;
}

throw `Invalid request ${opts.url}`;
});

await assert.doesNotReject(command.action(logger, { options: { name: 'userPrincipalName', debug: true } }));
await command.action(logger, { options: { name: 'userPrincipalName', debug: true } });
assert(removeStub.called);
});

it('correctly removes profile card property for fax', async () => {
sinon.stub(request, 'delete').callsFake(async (opts) => {
const removeStub = sinon.stub(request, 'delete').callsFake(async (opts) => {
if (opts.url === `https://graph.microsoft.com/v1.0/admin/people/profileCardProperties/Fax`) {
return;
}

throw `Invalid request ${opts.url}`;
});

await assert.doesNotReject(command.action(logger, { options: { name: 'fax' } }));
await command.action(logger, { options: { name: 'fax' } });
assert(removeStub.called);
});

it('correctly removes profile card property for state with force', async () => {
sinon.stub(request, 'delete').callsFake(async (opts) => {
const removeStub = sinon.stub(request, 'delete').callsFake(async (opts) => {
if (opts.url === `https://graph.microsoft.com/v1.0/admin/people/profileCardProperties/StateOrProvince`) {
return;
}

throw `Invalid request ${opts.url}`;
});

await assert.doesNotReject(command.action(logger, { options: { name: 'StateOrProvince', force: true } }));
await command.action(logger, { options: { name: 'StateOrProvince', force: true } });
milanholemans marked this conversation as resolved.
Show resolved Hide resolved
assert(removeStub.called);
});

it('uses correct casing for name when incorrect casing is used', async () => {
const deleteStub = sinon.stub(request, 'delete').callsFake(async (opts) => {
if (opts.url === `https://graph.microsoft.com/v1.0/admin/people/profileCardProperties/StateOrProvince`) {
return;
}

throw 'Invalid Request';
});

await command.action(logger, { options: { name: 'STATEORPROVINCE', force: true } });
assert(deleteStub.called);
});

it('fails when the removal runs into a property that is not found', async () => {
Expand Down