-
Notifications
You must be signed in to change notification settings - Fork 24
feat: add 'app settings' command to open app settings webpage for a selected app #92
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
Changes from all commits
34443a2
6239a1c
4a3e18e
452a9e3
0937dad
a178dd7
2d78fb8
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change | ||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
| @@ -0,0 +1,122 @@ | ||||||||||||||||
| // Copyright 2022-2025 Salesforce, Inc. | ||||||||||||||||
| // | ||||||||||||||||
| // Licensed under the Apache License, Version 2.0 (the "License"); | ||||||||||||||||
| // you may not use this file except in compliance with the License. | ||||||||||||||||
| // You may obtain a copy of the License at | ||||||||||||||||
| // | ||||||||||||||||
| // http://www.apache.org/licenses/LICENSE-2.0 | ||||||||||||||||
| // | ||||||||||||||||
| // Unless required by applicable law or agreed to in writing, software | ||||||||||||||||
| // distributed under the License is distributed on an "AS IS" BASIS, | ||||||||||||||||
| // WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. | ||||||||||||||||
| // See the License for the specific language governing permissions and | ||||||||||||||||
| // limitations under the License. | ||||||||||||||||
|
|
||||||||||||||||
| package app | ||||||||||||||||
|
|
||||||||||||||||
| import ( | ||||||||||||||||
| "fmt" | ||||||||||||||||
| "net/url" | ||||||||||||||||
| "strings" | ||||||||||||||||
|
|
||||||||||||||||
| "github.com/slackapi/slack-cli/internal/cmdutil" | ||||||||||||||||
| "github.com/slackapi/slack-cli/internal/config" | ||||||||||||||||
| "github.com/slackapi/slack-cli/internal/prompts" | ||||||||||||||||
| "github.com/slackapi/slack-cli/internal/shared" | ||||||||||||||||
| "github.com/slackapi/slack-cli/internal/slackerror" | ||||||||||||||||
| "github.com/slackapi/slack-cli/internal/slacktrace" | ||||||||||||||||
| "github.com/slackapi/slack-cli/internal/style" | ||||||||||||||||
| "github.com/spf13/cobra" | ||||||||||||||||
| ) | ||||||||||||||||
|
|
||||||||||||||||
| var settingsAppSelectPromptFunc = prompts.AppSelectPrompt | ||||||||||||||||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. thought: [Non-Blocker] I'd like us to use this new command as a way to nudge out best practices for maintainability and testability. While we use the I think the better approach would be dependency injection: But, how do we design this to scale for all commands and Maybe we use I think this is out-of-scope of this PR, but perhaps we can follow-up with some approach in a future PR? We can take some time to see if it helps address other
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @mwbrooks I'm so interested in this as well 🧠 ✨ I considered mocking the calls used in Using RunE: func(cmd *cobra.Command, args []string) error {
return appSettingsCommandRunE(clients, cmd, args)
},But we've discussed avoiding I'll hold off on making changes here so we can keep these patterns consistent for now, but I want to know what you think and I'm happy to follow up with changes we do decide on 🤖
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Totally agree, good call on not adding more complexity in this PR. Let's discuss it offline to figure out what we want to try out! |
||||||||||||||||
|
|
||||||||||||||||
| func NewSettingsCommand(clients *shared.ClientFactory) *cobra.Command { | ||||||||||||||||
| cmd := &cobra.Command{ | ||||||||||||||||
| Use: "settings [flags]", | ||||||||||||||||
| Short: "Open app settings for configurations", | ||||||||||||||||
| Long: strings.Join([]string{ | ||||||||||||||||
| "Open app settings to configure an application in a web browser.", | ||||||||||||||||
| "", | ||||||||||||||||
| "Discovering new features and customizing an app manifest can be done from this", | ||||||||||||||||
| fmt.Sprintf("web interface for apps with a \"%s\" manifest source.", config.ManifestSourceRemote.String()), | ||||||||||||||||
| "", | ||||||||||||||||
| "This command does not support apps deployed to Run on Slack infrastructure.", | ||||||||||||||||
| }, "\n"), | ||||||||||||||||
| Example: style.ExampleCommandsf([]style.ExampleCommand{ | ||||||||||||||||
| { | ||||||||||||||||
| Meaning: "Open app settings for a prompted app", | ||||||||||||||||
| Command: "app settings", | ||||||||||||||||
| }, | ||||||||||||||||
| { | ||||||||||||||||
| Meaning: "Open app settings for a specific app", | ||||||||||||||||
| Command: "app settings --app A0123456789", | ||||||||||||||||
| }, | ||||||||||||||||
| }), | ||||||||||||||||
| Args: cobra.MaximumNArgs(0), | ||||||||||||||||
| PreRunE: func(cmd *cobra.Command, args []string) error { | ||||||||||||||||
| return appSettingsCommandPreRunE(clients, cmd, args) | ||||||||||||||||
| }, | ||||||||||||||||
| RunE: func(cmd *cobra.Command, args []string) error { | ||||||||||||||||
| return appSettingsCommandRunE(clients, cmd, args) | ||||||||||||||||
| }, | ||||||||||||||||
| } | ||||||||||||||||
| return cmd | ||||||||||||||||
| } | ||||||||||||||||
|
|
||||||||||||||||
| // appSettingsCommandPreRunE determines if the command can be run in a project | ||||||||||||||||
| func appSettingsCommandPreRunE(clients *shared.ClientFactory, cmd *cobra.Command, args []string) error { | ||||||||||||||||
| ctx := cmd.Context() | ||||||||||||||||
| err := cmdutil.IsValidProjectDirectory(clients) | ||||||||||||||||
| if err != nil { | ||||||||||||||||
| return err | ||||||||||||||||
| } | ||||||||||||||||
| // Allow the force flag to ignore hosted apps and try to open app settings | ||||||||||||||||
| if clients.Config.ForceFlag { | ||||||||||||||||
| return nil | ||||||||||||||||
| } | ||||||||||||||||
zimeg marked this conversation as resolved.
Show resolved
Hide resolved
|
||||||||||||||||
| err = cmdutil.IsSlackHostedProject(ctx, clients) | ||||||||||||||||
| if err != nil { | ||||||||||||||||
| if slackerror.Is(err, slackerror.ErrAppNotHosted) { | ||||||||||||||||
| return nil | ||||||||||||||||
| } else { | ||||||||||||||||
| return err | ||||||||||||||||
| } | ||||||||||||||||
| } | ||||||||||||||||
| return slackerror.New(slackerror.ErrAppHosted). | ||||||||||||||||
| WithDetails(slackerror.ErrorDetails{ | ||||||||||||||||
| { | ||||||||||||||||
| Message: "App settings is not supported with Run on Slack infrastructure", | ||||||||||||||||
| }, | ||||||||||||||||
| }) | ||||||||||||||||
| } | ||||||||||||||||
|
|
||||||||||||||||
| // appSettingsCommandRunE opens app settings in a browser for the selected app | ||||||||||||||||
| func appSettingsCommandRunE(clients *shared.ClientFactory, cmd *cobra.Command, args []string) error { | ||||||||||||||||
| ctx := cmd.Context() | ||||||||||||||||
| clients.IO.PrintTrace(ctx, slacktrace.AppSettingsStart) | ||||||||||||||||
|
|
||||||||||||||||
| app, err := settingsAppSelectPromptFunc(ctx, clients, prompts.ShowInstalledAndUninstalledApps) | ||||||||||||||||
| if err != nil { | ||||||||||||||||
| return err | ||||||||||||||||
| } | ||||||||||||||||
| host := clients.API().Host() | ||||||||||||||||
| parsed, err := url.Parse(host) | ||||||||||||||||
| if err != nil { | ||||||||||||||||
| return err | ||||||||||||||||
| } | ||||||||||||||||
| parsed.Host = "api." + parsed.Host | ||||||||||||||||
| settingsURL := fmt.Sprintf("%s/apps/%s", parsed.String(), app.App.AppID) | ||||||||||||||||
|
Comment on lines
+104
to
+109
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. praise: Very nice work 👏🏻 |
||||||||||||||||
|
|
||||||||||||||||
| clients.IO.PrintInfo(ctx, false, "\n%s", style.Sectionf(style.TextSection{ | ||||||||||||||||
| Emoji: "house", | ||||||||||||||||
| Text: "App Settings", | ||||||||||||||||
| Secondary: []string{ | ||||||||||||||||
| settingsURL, | ||||||||||||||||
| }, | ||||||||||||||||
|
Comment on lines
+114
to
+116
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. suggestion: What do you think about adding output explaining the action being taken (opening the web browser)?
Suggested change
☝🏻 You might have better wording than that.
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🤔 I might have preference against this since erroring cases might cause confusion that the standalone link saves? The balance of text also feels lopsided but I don't think this is significant reason against this:
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 📝 It does make me think
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Sounds good to me. Also |
||||||||||||||||
| })) | ||||||||||||||||
| clients.Browser().OpenURL(settingsURL) | ||||||||||||||||
|
|
||||||||||||||||
| clients.IO.PrintTrace(ctx, slacktrace.AppSettingsSuccess, settingsURL) | ||||||||||||||||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. praise: heh, you read my mind to add
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The setups of I'm hoping soon we can equip all commands with at least a simple "start" and "success" with follow ups in
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It seems like a reasonable PR to add a "start" and "success" to each command and land the equivalent definitions in |
||||||||||||||||
| return nil | ||||||||||||||||
| } | ||||||||||||||||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,199 @@ | ||
| // Copyright 2022-2025 Salesforce, Inc. | ||
| // | ||
| // Licensed under the Apache License, Version 2.0 (the "License"); | ||
| // you may not use this file except in compliance with the License. | ||
| // You may obtain a copy of the License at | ||
| // | ||
| // http://www.apache.org/licenses/LICENSE-2.0 | ||
| // | ||
| // Unless required by applicable law or agreed to in writing, software | ||
| // distributed under the License is distributed on an "AS IS" BASIS, | ||
| // WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. | ||
| // See the License for the specific language governing permissions and | ||
| // limitations under the License. | ||
|
|
||
| package app | ||
|
|
||
| import ( | ||
| "context" | ||
| "testing" | ||
|
|
||
| "github.com/slackapi/slack-cli/internal/app" | ||
| "github.com/slackapi/slack-cli/internal/config" | ||
| "github.com/slackapi/slack-cli/internal/prompts" | ||
| "github.com/slackapi/slack-cli/internal/shared" | ||
| "github.com/slackapi/slack-cli/internal/shared/types" | ||
| "github.com/slackapi/slack-cli/internal/slackerror" | ||
| "github.com/slackapi/slack-cli/internal/slacktrace" | ||
| "github.com/slackapi/slack-cli/test/testutil" | ||
| "github.com/spf13/cobra" | ||
| "github.com/stretchr/testify/mock" | ||
| ) | ||
|
|
||
| func Test_App_SettingsCommand(t *testing.T) { | ||
| testutil.TableTestCommand(t, testutil.CommandTests{ | ||
| "requires a valid project directory": { | ||
| ExpectedError: slackerror.New(slackerror.ErrInvalidAppDirectory), | ||
| }, | ||
| "errors for rosi applications": { | ||
| Setup: func(t *testing.T, ctx context.Context, cm *shared.ClientsMock, cf *shared.ClientFactory) { | ||
| cf.SDKConfig.WorkingDirectory = "." | ||
| projectConfigMock := config.NewProjectConfigMock() | ||
| projectConfigMock.On( | ||
| "GetManifestSource", | ||
| mock.Anything, | ||
| ).Return( | ||
| config.ManifestSourceLocal, | ||
| nil, | ||
| ) | ||
| cm.Config.ProjectConfig = projectConfigMock | ||
| manifestMock := &app.ManifestMockObject{} | ||
| manifestMock.On( | ||
| "GetManifestLocal", | ||
| mock.Anything, | ||
| mock.Anything, | ||
| mock.Anything, | ||
| ).Return( | ||
| types.SlackYaml{ | ||
| AppManifest: types.AppManifest{ | ||
| Settings: &types.AppSettings{FunctionRuntime: types.SlackHosted}, | ||
| }, | ||
| }, | ||
| nil, | ||
| ) | ||
| cm.AppClient.Manifest = manifestMock | ||
| }, | ||
| ExpectedError: slackerror.New(slackerror.ErrAppHosted), | ||
| }, | ||
| "opens a rosi application with the force flag": { | ||
| CmdArgs: []string{"--force"}, | ||
| Setup: func(t *testing.T, ctx context.Context, cm *shared.ClientsMock, cf *shared.ClientFactory) { | ||
| cf.SDKConfig.WorkingDirectory = "." | ||
| projectConfigMock := config.NewProjectConfigMock() | ||
| projectConfigMock.On( | ||
| "GetManifestSource", | ||
| mock.Anything, | ||
| ).Return( | ||
| config.ManifestSourceLocal, | ||
| nil, | ||
| ) | ||
| cm.Config.ProjectConfig = projectConfigMock | ||
| manifestMock := &app.ManifestMockObject{} | ||
| manifestMock.On( | ||
| "GetManifestLocal", | ||
| mock.Anything, | ||
| mock.Anything, | ||
| mock.Anything, | ||
| ).Return( | ||
| types.SlackYaml{ | ||
| AppManifest: types.AppManifest{ | ||
| Settings: &types.AppSettings{FunctionRuntime: types.SlackHosted}, | ||
| }, | ||
| }, | ||
| nil, | ||
| ) | ||
| cm.AppClient.Manifest = manifestMock | ||
| appSelectMock := prompts.NewAppSelectMock() | ||
| appSelectMock.On( | ||
| "AppSelectPrompt", | ||
| ).Return( | ||
| prompts.SelectedApp{App: types.App{AppID: "A0101010101"}}, | ||
| nil, | ||
| ) | ||
| settingsAppSelectPromptFunc = appSelectMock.AppSelectPrompt | ||
| }, | ||
| ExpectedAsserts: func(t *testing.T, ctx context.Context, cm *shared.ClientsMock) { | ||
| expectedURL := "https://api.slack.com/apps/A0101010101" | ||
| cm.Browser.AssertCalled(t, "OpenURL", expectedURL) | ||
| cm.IO.AssertCalled(t, "PrintTrace", mock.Anything, slacktrace.AppSettingsStart, mock.Anything) | ||
| cm.IO.AssertCalled(t, "PrintTrace", mock.Anything, slacktrace.AppSettingsSuccess, []string{expectedURL}) | ||
| }, | ||
| }, | ||
| "requires an existing application": { | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. note: Nice, I tested this manually and I'm happy to see you already added a test! |
||
| Setup: func(t *testing.T, ctx context.Context, cm *shared.ClientsMock, cf *shared.ClientFactory) { | ||
| cf.SDKConfig.WorkingDirectory = "." | ||
| projectConfigMock := config.NewProjectConfigMock() | ||
| projectConfigMock.On( | ||
| "GetManifestSource", | ||
| mock.Anything, | ||
| ).Return( | ||
| config.ManifestSourceRemote, | ||
| nil, | ||
| ) | ||
| cm.Config.ProjectConfig = projectConfigMock | ||
| appSelectMock := prompts.NewAppSelectMock() | ||
| appSelectMock.On( | ||
| "AppSelectPrompt", | ||
| ).Return( | ||
| prompts.SelectedApp{}, | ||
| slackerror.New(slackerror.ErrInstallationRequired), | ||
| ) | ||
| settingsAppSelectPromptFunc = appSelectMock.AppSelectPrompt | ||
| }, | ||
| ExpectedError: slackerror.New(slackerror.ErrInstallationRequired), | ||
| }, | ||
| "opens the url to app settings of an app in production": { | ||
| Setup: func(t *testing.T, ctx context.Context, cm *shared.ClientsMock, cf *shared.ClientFactory) { | ||
| cf.SDKConfig.WorkingDirectory = "." | ||
| projectConfigMock := config.NewProjectConfigMock() | ||
| projectConfigMock.On( | ||
| "GetManifestSource", | ||
| mock.Anything, | ||
| ).Return( | ||
| config.ManifestSourceRemote, | ||
| nil, | ||
| ) | ||
| cm.Config.ProjectConfig = projectConfigMock | ||
| appSelectMock := prompts.NewAppSelectMock() | ||
| appSelectMock.On( | ||
| "AppSelectPrompt", | ||
| ).Return( | ||
| prompts.SelectedApp{App: types.App{AppID: "A0123456789"}}, | ||
| nil, | ||
| ) | ||
| settingsAppSelectPromptFunc = appSelectMock.AppSelectPrompt | ||
| }, | ||
| ExpectedAsserts: func(t *testing.T, ctx context.Context, cm *shared.ClientsMock) { | ||
| expectedURL := "https://api.slack.com/apps/A0123456789" | ||
| cm.Browser.AssertCalled(t, "OpenURL", expectedURL) | ||
| cm.IO.AssertCalled(t, "PrintTrace", mock.Anything, slacktrace.AppSettingsStart, mock.Anything) | ||
| cm.IO.AssertCalled(t, "PrintTrace", mock.Anything, slacktrace.AppSettingsSuccess, []string{expectedURL}) | ||
| }, | ||
| }, | ||
| "opens the url to app settings of an app in development": { | ||
| Setup: func(t *testing.T, ctx context.Context, cm *shared.ClientsMock, cf *shared.ClientFactory) { | ||
| host := "https://dev1234.slack.com" | ||
| cf.SDKConfig.WorkingDirectory = "." | ||
| projectConfigMock := config.NewProjectConfigMock() | ||
| projectConfigMock.On( | ||
| "GetManifestSource", | ||
| mock.Anything, | ||
| ).Return( | ||
| config.ManifestSourceRemote, | ||
| nil, | ||
| ) | ||
| cm.Config.ProjectConfig = projectConfigMock | ||
| appSelectMock := prompts.NewAppSelectMock() | ||
| appSelectMock.On( | ||
| "AppSelectPrompt", | ||
| ).Return( | ||
| prompts.SelectedApp{ | ||
| App: types.App{AppID: "A0123456789"}, | ||
| Auth: types.SlackAuth{APIHost: &host}, | ||
| }, | ||
| nil, | ||
| ) | ||
| settingsAppSelectPromptFunc = appSelectMock.AppSelectPrompt | ||
| cm.API.On("Host").Return(host) // SetHost is implemented in AppSelectPrompt | ||
| }, | ||
| ExpectedAsserts: func(t *testing.T, ctx context.Context, cm *shared.ClientsMock) { | ||
| expectedURL := "https://api.dev1234.slack.com/apps/A0123456789" | ||
| cm.Browser.AssertCalled(t, "OpenURL", expectedURL) | ||
| cm.IO.AssertCalled(t, "PrintTrace", mock.Anything, slacktrace.AppSettingsStart, mock.Anything) | ||
| cm.IO.AssertCalled(t, "PrintTrace", mock.Anything, slacktrace.AppSettingsSuccess, []string{expectedURL}) | ||
| }, | ||
| }, | ||
| }, func(cf *shared.ClientFactory) *cobra.Command { | ||
| return NewSettingsCommand(cf) | ||
| }) | ||
| } | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -42,6 +42,7 @@ const ( | |
| ErrAppExists = "app_add_exists" | ||
| ErrAppFlagRequired = "app_flag_required" | ||
| ErrAppFound = "app_found" | ||
| ErrAppHosted = "app_hosted" | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. note: I can see this being a useful error for us in the future! |
||
| ErrAppInstall = "app_install_error" | ||
| ErrAppManifestAccess = "app_manifest_access_error" | ||
| ErrAppManifestCreate = "app_manifest_create_error" | ||
|
|
@@ -381,6 +382,11 @@ Otherwise start your app for local development with: %s`, | |
| Message: "An app was found", | ||
| }, | ||
|
|
||
| ErrAppHosted: { | ||
| Code: ErrAppHosted, | ||
| Message: "App is configured for Run on Slack infrastructure", | ||
| }, | ||
|
|
||
| ErrAppInstall: { | ||
| Code: ErrAppInstall, | ||
| Message: "Couldn't install your app to a workspace", | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
note: Foreseeing a future of alphabetically ordered examples prefixed with
app🔮There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As much as I appreicate alphabetics I was thinking these examples were ordered with expected steps in mind?
To me this tells an interesting story:
installlinklistsettingsuninstalldeleteWhich is almost in alphabetics I realize now! So perhaps following "install" with "delete" isn't so wild 👾
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ahhhh, that's alright as well. For now, the order flows well and it's easy to see where
unlink&updatewould land.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
When
unlinklands we should revisit this. Background thoughts are now nudging me towards preferring the more standard order 😳