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

Enhancement: Run "pa app remove" as admin. Closes #5519 #5610

Closed

Conversation

MartinM85
Copy link
Contributor

Closes #5519

@MartinM85
Copy link
Contributor Author

Maybe related to this issue...app-remove.spec.ts has two tests which have the same implementation

  • removes the specified Microsoft Power App when prompt confirmed (debug)
  • removes the specified Microsoft Power App from other user when prompt confirmed (debug)

It seems to me that removes the specified Microsoft Power App from other user when prompt confirmed (debug) can be replaced with the new test removes the specified Microsoft Power App from other user as admin when prompt confirmed (debug).

@milanholemans
Copy link
Contributor

Thank you @MartinM85! We'll try to review it ASAP!

@milanholemans
Copy link
Contributor

Maybe related to this issue...app-remove.spec.ts has two tests which have the same implementation

* `removes the specified Microsoft Power App when prompt confirmed (debug)`

* `removes the specified Microsoft Power App from other user when prompt confirmed (debug)`

It seems to me that removes the specified Microsoft Power App from other user when prompt confirmed (debug) can be replaced with the new test removes the specified Microsoft Power App from other user as admin when prompt confirmed (debug).

Great catch @MartinM85, looks like two identical tests indeed. I think we can remove test removes the specified Microsoft Power App from other user when prompt confirmed (debug) here.

@milanholemans milanholemans self-assigned this Oct 31, 2023
Copy link
Contributor

@milanholemans milanholemans left a comment

Choose a reason for hiding this comment

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

Nice work so far @MartinM85! Let's change a few things before we proceed.

docs/docs/cmd/pa/app/app-remove.mdx Show resolved Hide resolved
docs/docs/cmd/pa/app/app-remove.mdx Outdated Show resolved Hide resolved
docs/docs/cmd/pa/app/app-remove.mdx Outdated Show resolved Hide resolved
docs/docs/cmd/pa/app/app-remove.mdx Outdated Show resolved Hide resolved
docs/docs/cmd/pa/app/app-remove.mdx Outdated Show resolved Hide resolved
src/m365/pa/commands/app/app-remove.ts Outdated Show resolved Hide resolved
src/m365/pa/commands/app/app-remove.ts Outdated Show resolved Hide resolved
src/m365/pa/commands/app/app-remove.ts Outdated Show resolved Hide resolved
src/m365/pa/commands/app/app-remove.ts Outdated Show resolved Hide resolved
src/m365/pa/commands/app/app-remove.spec.ts Outdated Show resolved Hide resolved
@milanholemans milanholemans marked this pull request as draft December 15, 2023 22:11
@MartinM85 MartinM85 marked this pull request as ready for review December 16, 2023 13:43
Copy link
Contributor

@milanholemans milanholemans left a comment

Choose a reason for hiding this comment

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

Command works great, let's change a few tiny things before we merge this.

docs/docs/cmd/pa/app/app-remove.mdx Outdated Show resolved Hide resolved
docs/docs/cmd/pa/app/app-remove.mdx Outdated Show resolved Hide resolved
Comment on lines +93 to +98
if (args.options.asAdmin) {
endpoint = `${this.resource}/providers/Microsoft.PowerApps/scopes/admin/environments/${formatting.encodeQueryParameter(args.options.environmentName!)}/apps/${formatting.encodeQueryParameter(args.options.name)}?api-version=2017-08-01`;
}
else {
endpoint = `${this.resource}/providers/Microsoft.PowerApps/apps/${formatting.encodeQueryParameter(args.options.name)}?api-version=2017-08-01`;
}
Copy link
Contributor

Choose a reason for hiding this comment

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

These endpoint URLs have a lot of common parts. Let's just use a single inline condition to add the admin scope and environment when needed (just like we do with pa app get).

Copy link
Contributor Author

@MartinM85 MartinM85 Dec 26, 2023

Choose a reason for hiding this comment

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

I think that single inline condition was there and it was changed according to the previous review. In pa app get I've removed the single inline condition

@milanholemans milanholemans marked this pull request as draft December 23, 2023 23:25
@MartinM85 MartinM85 marked this pull request as ready for review December 26, 2023 18:11
Copy link
Contributor

@milanholemans milanholemans left a comment

Choose a reason for hiding this comment

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

Looks good! Made a few small changes while merging.

Comment on lines +92 to +98
let endpoint;
if (args.options.asAdmin) {
endpoint = `${this.resource}/providers/Microsoft.PowerApps/scopes/admin/environments/${formatting.encodeQueryParameter(args.options.environmentName!)}/apps/${formatting.encodeQueryParameter(args.options.name)}?api-version=2017-08-01`;
}
else {
endpoint = `${this.resource}/providers/Microsoft.PowerApps/apps/${formatting.encodeQueryParameter(args.options.name)}?api-version=2017-08-01`;
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Let's isolate the common parts.

@milanholemans
Copy link
Contributor

Merged manually, thank you for this addition!

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: Run pa app remove as admin
2 participants