feat: check requests to install an app - #646
Conversation
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #646 +/- ##
==========================================
+ Coverage 72.38% 72.53% +0.15%
==========================================
Files 238 239 +1
Lines 20072 20228 +156
==========================================
+ Hits 14529 14673 +144
- Misses 4275 4279 +4
- Partials 1268 1276 +8 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
Add a hidden `slack app requests` command behind the app-approval-status experiment that reports the most recent install approval request for the selected app on each team in the token's scope. Co-authored-by: Cursor <cursoragent@cursor.com>
Exercise the interrupted app selection and missing app ID branches of the command, plus the unknown timestamp, status, and cancellation actor fallbacks of the output. Co-authored-by: Cursor <cursoragent@cursor.com>
fc808a6 to
1e4be76
Compare
The app select prompt only offers apps saved to a project, so apps created elsewhere could not be checked. An app ID provided with the --app flag now skips both the project requirement and the project app list, gathering a token from the authenticated accounts instead. Co-authored-by: Cursor <cursoragent@cursor.com>
zimeg
added
enhancement
zimeg
left a comment
zimeg
left a comment
There was a problem hiding this comment.
🏁 @AmyScript Thanks for sending this in! It's LGTM per usual and I leave a few comments that might be nice before merge but some are rambles... 🗣️
Overall I lean toward returning API responses or errors instead of expecting certain values and some flag options and error outputs. Nothing blocking! These are exciting experiment to bring to release 🤖⚡
mwbrooks
left a comment
mwbrooks
left a comment
There was a problem hiding this comment.
@AmyScript Please don't merge this yet - I'd like a moment to review it. Just dropping the change request, since @zimeg already approved it which enables merging.
| func NewRequestsCommand(clients *shared.ClientFactory) *cobra.Command { | ||
| cmd := &cobra.Command{ | ||
| Use: "requests [flags]", | ||
| Aliases: []string{"approval-requests", "approvals"}, |
There was a problem hiding this comment.
suggestion: Assuming we go with app approval, alias approvals and remove the others. It's hard to remove an alias once we add it, but it's easy to add one in the future.
| Aliases: []string{"approval-requests", "approvals"}, | |
| Aliases: []string{"approvals"}, |
zimeg
left a comment
zimeg
left a comment
There was a problem hiding this comment.
@AmyScript Super appreciate the fast changes! I'll toss another approval on but defer to @mwbrooks for ongoing review. Nothing is blocking experimental releases for me!
🧪 I do notice the E2E tests aren't running and I think that's an intentional setting to avoid passing secrets to forked branches... It catches us often and I'm unsure a workaround remains but I share this for possible updates:
🔗 https://github.com/slackapi/slack-cli/blob/main/.github/MAINTAINERS_GUIDE.md#fork
Should I have just branched off main? I read about the fork so I forked 😆. Is there a way for me to run the e2e tests locally with a command? |
There was a problem hiding this comment.
🙌🏻 Thanks for the PR @AmyScript! Admin approval management has been a painfully missed feature in the CLI, so I'm happy to see you giving it some TLC.
🙇🏻 Some praise goes to you on code quality. You rocked our latest conventions, patterns, and test coverage goals. Thanks for that!
💡 Small tip for next time - please create a branch on this repo instead of forking. External contributors fork, but as a maintainer you can work on a branch allowing our CI/CD to safely run E2E tests. Before merging, we should push this to a branch on the origin to ensure the E2E tests pass alright.
🧠 My main ask before approving this PR is to consider app approval (alias: approvals) instead of app requests. I think it reads better from the user/developer perspective, aligns with our documentation, and aligns with our API. It also opens space for 3rd-tier of commands such as app approval cancel or app approval list (alias app approval - what you're landing here).
🧪 I hear you're aiming to land this into production end-of-week or early next week. I'd suggestion you delete the experiment entirely, because we won't be doing a release between now-and-then. All production features should not be behind an experiment.
🚦 When you remove the experiment, please switch this to a semver:minor.
📝 The rest of my suggestions are minor. Nice to haves but not blockers if you are short on time.
Once we settle on the name, happy to hit ✅ approve!
| // NewRequestsCommand returns a new Cobra command | ||
| func NewRequestsCommand(clients *shared.ClientFactory) *cobra.Command { | ||
| cmd := &cobra.Command{ | ||
| Use: "requests [flags]", |
There was a problem hiding this comment.
suggestion: After some thought, I'd like to suggest renaming this to approval (singular)
The reason is that app approval matches the API (apps.approvals.requests.list) and leaves room for slack app approval [create|cancel|list] later.
We've had requests for a cancel feature, so this opens the namespace for it.
The bare approval also matches the bare app → app list, trigger → trigger list, auth → auth list, and manifest → manifest list convention. So, if we expand the command with create | cancel | list subcommands, we've made decisions that allow for a smooth forward rollout.
The approvals command also matches the terminology used in our documentation, where as requests is more from the admin-perspective:
- https://docs.slack.dev/admins/managing-app-approvals/
- https://slack.com/help/articles/222386767-Manage-app-approval-for-your-workspace
We should alias the plural approvals. The CLI usually has the canonical command as singular while aliasing the plural. Long ago, we mixed-and-matched singular/plural depending on the command and it was confusing for developers. So, we try to use singular when possible.
There was a problem hiding this comment.
Hey @mwbrooks, thanks for the thought you put into this, and for surfacing that there's been a request for a cancel feature.
I should have explained the design considerations for using app requests in the PR description. My thinking is that Slack's vocabulary splits by role rather than by feature: the person who wants the app requests it, and the admin approves it. This command runs as the requester, reading their own request status, so it sits on the request half.
That split shows up in the sources you linked, too. Both are written for the approver, and even there, the object is a request: the admin listing method is admin.apps.requests.list, the event is app_requested, the companion help article is "Manage app requests for your workspace".
On the requester side, request also shows up in our CLI and UI:
- The CLI install flow: "Your request to install the app is pending", "Please submit a request to install or update your app" (
internal/pkg/apps/install.go), and "Cancel the current request to install this app?" (internal/api/app.go). - On the app settings page under Install App, when approval is on, the button is Request to Install, and Slackbot follows up with "Your request to install [APP] on [WORKSPACE] has been sent for review."
- The member-facing help article has a "Request app approval" section: https://slack.com/help/articles/202035138-Add-apps-to-your-Slack-workspace
Good point about leaving room for subcommands. The requester side verbs are create, cancel, and list, and those read naturally as app request cancel, our own enum comment describes that action as "the requester withdrawing their own request". Whereas app approval cancel sounds like revoking an approval someone granted. That's also why I left approval out of the aliases. If app request claims that name now, we can't give it to an admin side app approval later if we need it.
You're right that we should use the singular, so I've renamed it to app request with requests as the only alias. The command is hidden behind the app-approval-status experiment right now, so if you still feel strongly about it, I am happy to switch it if you'd rather go with approval. Let me know what you think!
| }, | ||
| } | ||
|
|
||
| cmd.Flags().StringSliceVar(&requestsFlags.workspaceIDs, "workspace-ids", nil, "also check these workspaces of an organization,\nwith a maximum of 50 workspaces") |
There was a problem hiding this comment.
thought: Is the 50 workspace limit enforced by the API? A brief look and it appears the API returns invalid_arguments with no remediation. If so, we may want to validate the length here and include it in the error message since it's easy to overlook the help documentation.
There was a problem hiding this comment.
@zimeg @mwbrooks I need a tiebreak on this one, since the two pieces of feedback point you both provided points in opposite directions and both risks are real.
Eden asked to drop the client-side check so a future change to the cap can't leave a stale CLI rejecting calls the API would accept, and I removed it. Michael then pointed out that the API answers with a bare invalid_arguments and no remediation, so the failure doesn't explain itself, and the developer has to go find the limit in the flag help. I think both concerns are valid. I've left the code as is for now, but if there's agreement to go with @mwbrooks' suggestion, I can change it back.
| func NewRequestsCommand(clients *shared.ClientFactory) *cobra.Command { | ||
| cmd := &cobra.Command{ | ||
| Use: "requests [flags]", | ||
| Aliases: []string{"approval-requests", "approvals"}, |
There was a problem hiding this comment.
suggestion: Assuming we go with app approval, alias approvals and remove the others. It's hard to remove an alias once we add it, but it's easy to add one in the future.
| Aliases: []string{"approval-requests", "approvals"}, | |
| Aliases: []string{"approvals"}, |
| {Command: "app requests", Meaning: "Check requests to install an app"}, | ||
| {Command: "app requests --app A0123456789", Meaning: "Check requests for an app outside a project"}, | ||
| {Command: "app requests --workspace-ids T0123456789,T9876543210", Meaning: "Check requests on certain workspaces of an organization"}, |
There was a problem hiding this comment.
suggestion: Assuming we go with app approval:
| {Command: "app requests", Meaning: "Check requests to install an app"}, | |
| {Command: "app requests --app A0123456789", Meaning: "Check requests for an app outside a project"}, | |
| {Command: "app requests --workspace-ids T0123456789,T9876543210", Meaning: "Check requests on certain workspaces of an organization"}, | |
| {Command: "app approval", Meaning: "Check requests to install an app"}, | |
| {Command: "app approval --app A0123456789", Meaning: "Check requests for an app outside a project"}, | |
| {Command: "app approval --workspace-ids T0123456789,T9876543210", Meaning: "Check requests on certain workspaces of an organization"}, |
| // runRequestsCommand will execute the requests command | ||
| func runRequestsCommand(cmd *cobra.Command, clients *shared.ClientFactory) error { | ||
| ctx := cmd.Context() | ||
| span, ctx := opentracing.StartSpanFromContext(ctx, "cmd.app.requests") |
There was a problem hiding this comment.
suggestion: If we accept app approval then we should adjust all of our tracing.
| span, ctx := opentracing.StartSpanFromContext(ctx, "cmd.app.requests") | |
| span, ctx := opentracing.StartSpanFromContext(ctx, "cmd.app.approval") |
Rename the command to the singular "app request" with "requests" as the only alias, matching the CLI convention of a singular canonical name. Rename the API error codes to ErrAPIFeatureNotEnabled and ErrAPIRestrictedAction so it is clear they mirror responses of the API rather than errors raised by the CLI. Sort a copy of the requests while formatting so the slice of the caller keeps its order, and title the section "App Install Approval Requests" to spell out what is being listed. Co-authored-by: Cursor <cursoragent@cursor.com>
@mwbrooks Thanks for your review on my first PR working with this codebase, lots of great suggestions. The code has been updated for another review. I've left the naming of the new command for now. I'm leaving on PTO next week, so that's why I was hoping to launch this by the end of the week, but if we think there are still discussions that need to happen, I'm happy to leave the experiment flag on and merge it for now and pick it back up when I'm back from PTO. I'll create a branch off the repo once i get a ✅ from you so we can merge the branch instead. |
3 participants
Changelog
app-approval-statusexperiment with aslack app requestcommand that checks the status of your most recent request to have an app approved for install.Summary
Installing an app on a team with admin approval required creates an approval request, but there was no way to check what happened to it from the CLI. This adds a hidden
slack app requestcommand (aliased asrequests), gated behind the newapp-approval-statusexperiment, that reports the most recent request for the selected app on each team in the token's scope.Requests are searched on the team of the authenticated account. An account of a workspace belonging to an organization also searches that organization, while an account of an organization searches the organization alone. Other workspaces of an organization can be searched with
--workspace-ids, which the API accepts up to 50 of.Apps saved to a project are chosen with the usual prompt. Since that prompt only offers apps recorded in the project, an app can also be named by ID with
--app, which skips both the project requirement and the project app list and gathers a token from the authenticated accounts instead.Output includes the request ID, status, and timestamps, plus the actor that cancelled a request and a hint when the account can install the app without approval. Only team IDs are returned by the endpoint, so a team is titled by name when it happens to be the team of the authenticated account:
A denied request adds the moment it was reviewed:
A cancelled request names the kind of actor that cancelled it, which is an admin, the system, or you:
A pending request where the user can install without approval says so when the account turns out to be able to install the app without approval, because users who can install without approval are still able to create requests:
Design considerations
slack app requestwas chosen as the command because Slack's vocabulary splits by role rather than by feature: the person who wants the app requests it, and the admin approves it. This command runs as the requester, reading their own request status, so it sits on the request half.On the requester side, request also shows up in our CLI and UI:
Preview
No recording yet — the command is unreleased and hidden, and the output above is taken from the unit tests rather than a live run.
Testing
--appapp ID path, including a failed team selection and a team without a token.--experiment app-approval-status, that a project is still required unless an app ID is given, and that the API error for more than 50 workspaces surfaces to the developer.Notes
Hidden, matching howmanifest syncgates itself; the experiment check lives inPreRunE.feature_not_enabledandrestricted_actionerror codes returned by the endpoint, namedErrAPIFeatureNotEnabledandErrAPIRestrictedActionso it is clear they mirror API responses rather than errors raised by the CLI.slack app --helplists this command despiteHidden. It is a one line change that can be reverted on its own if it ever needs to be.PromptTeamSlackAuthnow callsSetSelectedAuthwhen only one account is authenticated, which the two multi-account paths already did. This sends API calls to the host that account belongs to, and also applies toapp linkandsandbox, which use the same prompt.app_idis required by the endpoint, so requests are reported one app at a time rather than for every app the account owns.Requirements