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

Extends 'teams app remove' command with support for name option. Closes #5445 #5454

Closed
wants to merge 4 commits into from

Conversation

nanddeepn
Copy link
Contributor

@nanddeepn nanddeepn commented Sep 4, 2023

Extends teams app remove command with support for specifying the name option. Closes #5445

@milanholemans
Copy link
Contributor

Thank you @nanddeepn, we'll try to review it ASAP!

@nanddeepn nanddeepn changed the title Extends 'teams app user remove' command with support for name option. Closes #5447 Extends 'teams app user remove' command with support for name option. Closes #5445 Sep 5, 2023
@nanddeepn nanddeepn changed the title Extends 'teams app user remove' command with support for name option. Closes #5445 Extends 'teams app remove' command with support for name option. Closes #5445 Sep 5, 2023
Copy link
Contributor

@martinlingstuyl martinlingstuyl left a comment

Choose a reason for hiding this comment

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

Hi @nanddeepn, great work! I've got a few small comments before we merge this 👍

src/m365/teams/commands/app/app-remove.ts Outdated Show resolved Hide resolved
src/m365/teams/commands/app/app-remove.ts Outdated Show resolved Hide resolved
src/m365/teams/commands/app/app-remove.ts Outdated Show resolved Hide resolved
src/m365/teams/commands/app/app-remove.ts Outdated Show resolved Hide resolved
@martinlingstuyl martinlingstuyl marked this pull request as draft September 14, 2023 18:48
@martinlingstuyl martinlingstuyl self-assigned this Sep 14, 2023
@nanddeepn nanddeepn marked this pull request as ready for review September 15, 2023 06:44
Copy link
Contributor

@martinlingstuyl martinlingstuyl left a comment

Choose a reason for hiding this comment

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

Looks great @nanddeepn, I'll merge it and fix a few last things while we're at it. 👏

headers: {
accept: 'application/json;odata.metadata=none'
if (this.verbose) {
await logger.logToStderr(`Removing app with ID ${args.options.id}`);
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
await logger.logToStderr(`Removing app with ID ${args.options.id}`);
await logger.logToStderr(`Removing app with ID ${appId}`);

resultAsKeyValuePair[obj.id] = obj;
});

return Cli.handleMultipleResultsFound(`Multiple Teams apps with name '${options.name}' found.`, resultAsKeyValuePair);
Copy link
Contributor

Choose a reason for hiding this comment

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

You should await this and type the result.

Suggested change
return Cli.handleMultipleResultsFound(`Multiple Teams apps with name '${options.name}' found.`, resultAsKeyValuePair);
const result = await Cli.handleMultipleResultsFound<{ id: string; }>(`Multiple Teams apps with name '${options.name}' found.`, resultAsKeyValuePair);
return result.id;

Copy link
Contributor

Choose a reason for hiding this comment

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

oh, and we also missed a test case for this :-)

if (options.id) {
return options.id;
}

Copy link
Contributor

Choose a reason for hiding this comment

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

We should add a verbose logging line here

@martinlingstuyl
Copy link
Contributor

Merged manually, thank you for the awesome work! 👏🎉

@nanddeepn nanddeepn deleted the issue-5445 branch September 22, 2023 06:47
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Enhancement]: Extend teams app remove command with --name option
3 participants