-
Notifications
You must be signed in to change notification settings - Fork 24
chore: default to bolt experiment behavior for app selection #158
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
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #158 +/- ##
==========================================
- Coverage 63.37% 62.95% -0.42%
==========================================
Files 212 212
Lines 22333 21643 -690
==========================================
- Hits 14153 13625 -528
+ Misses 7093 6966 -127
+ Partials 1087 1052 -35 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
zimeg
left a comment
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.
📝 Leaving first impressions for the wonderful reviewers!
I apologise for the size of this changeset as well. To keep diffs reasonable I'll open another PR that explores testing 🔍
| @@ -1789,7 +941,7 @@ func TestPrompt_AppSelectPrompt_FlatAppSelectPrompt(t *testing.T) { | |||
| expectedStdout string | |||
| expectedStderr string | |||
| }{ | |||
| "selects a saved applications using prompts": { | |||
| "returns a saved applications using prompts": { | |||
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.
📚 Wording updates to start all of these cases with either "returns" or "errors" follow this comment.
|
|
||
| // TeamAppSelectPrompt prompts the user to select an app from a specified team environment, | ||
| // returning the selected app. This app might require installation before use if `status == ShowAllApps`. | ||
| func TeamAppSelectPrompt(ctx context.Context, clients *shared.ClientFactory, env AppEnvironmentType, status AppInstallStatus) (SelectedApp, error) { |
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.
👁️🗨️ With a flattened app selector we are so close to removing TeamAppSelectPrompt altogether to prefer passing the AppEnvironmentType in calling code as ShowAllEnvironments I hope.
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.
🧪 I am planning to update tests with this in mind, preferring to focus on logic in the flatAppSelectPrompt using various "environments".
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.
🗣️ The removal of TeamAppSelect might be in #159!
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.
💌 For the kind reviewers, notes on matching or later added tests follow!
Some notes I want to call out here, as well as comments, might include:
- Not all tests have a match but an assortment of important cases should remain covered after changes of #164.
- We've removed tests on legacy "dev" team domain handling for apps saved to
apps.dev.json - Enterprise workspace apps are expected to have authentications resolved on the enterprise, not the workspace. Logic hasn't changed for this but tests that handled workspace authentications are removed.
| "error when the token authentication returns an error": { | ||
| tokenFlag: team1Token, | ||
| mockAuthWithTokenError: slackerror.New(slackerror.ErrTokenExpired), | ||
| expectedAuth: types.SlackAuth{}, | ||
| expectedError: slackerror.New(slackerror.ErrTokenExpired), | ||
| }, | ||
| "retrieve apps and authentication associated with a token": { | ||
| tokenFlag: team1Token, | ||
| deployedApps: []types.App{deployedTeam1InstalledApp}, | ||
| localApps: []types.App{localTeam1UninstalledApp}, | ||
| mockAuthWithTokenResponse: fakeAuthsByTeamDomain[team1TeamDomain], | ||
| mockAuthWithTeamIDError: slackerror.New(slackerror.ErrCredentialsNotFound), | ||
| mockGetAppStatusResponse: api.GetAppStatusResult{ | ||
| Apps: []api.AppStatusResultAppInfo{ | ||
| { | ||
| AppID: deployedTeam1InstalledAppID, | ||
| Installed: true, | ||
| }, | ||
| { | ||
| AppID: localTeam1UninstalledAppID, | ||
| Installed: false, | ||
| }, | ||
| }, | ||
| }, | ||
| expectedTeamID: team1TeamID, | ||
| expectedAuth: fakeAuthsByTeamDomain[team1TeamDomain], | ||
| }, |
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.
🗣️ These tests are covered with existing TestGetTokenApp cases:
slack-cli/internal/prompts/app_select_test.go
Line 175 in 8466439
| func TestGetTokenApp(t *testing.T) { |
- "error on an unknown token"
- "return an uninstalled but saved local app"
| func TestFilterAuthsByToken_NoLogin(t *testing.T) { | ||
| test := struct { | ||
| title string | ||
| TokenFlag string | ||
| expectedAuth types.SlackAuth | ||
| }{ | ||
| title: "return the correct mocked api response", | ||
| TokenFlag: team1Token, | ||
| expectedAuth: fakeAuthsByTeamDomain[team1TeamDomain], | ||
| } |
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.
🗣️ This test is covered in expected outputs of tests using token flags:
slack-cli/internal/prompts/app_select_test.go
Lines 1128 to 1143 in 5b9f5ab
| "returns selection for token flag and team id flag if app saved": { | |
| mockAppsLocal: []types.App{ | |
| localTeam1UninstalledApp, | |
| }, | |
| mockAuthWithToken: fakeAuthsByTeamDomain[team1TeamDomain], | |
| mockAuthWithTeamIDError: slackerror.New(slackerror.ErrCredentialsNotFound), | |
| mockAuthWithTeamIDTeamID: team1TeamID, | |
| mockFlagTeam: team1TeamID, | |
| mockFlagToken: fakeAuthsByTeamDomain[team1TeamDomain].Token, | |
| appPromptConfigStatus: ShowAllApps, | |
| appPromptConfigEnvironment: ShowLocalOnly, | |
| expectedSelection: SelectedApp{ | |
| App: localTeam1UninstalledApp, | |
| Auth: fakeAuthsByTeamDomain[team1TeamDomain], | |
| }, | |
| }, |
| "token with mismatched workspace flag": { | ||
| AppFlag: "", | ||
| TokenFlag: team1Token, | ||
| TeamFlag: team2TeamDomain, | ||
| expectedAuth: types.SlackAuth{}, | ||
| expectedErr: *slackerror.New(slackerror.ErrInvalidToken), | ||
| }, |
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.
🗣️ Error cases for token tests are also covered in tests
slack-cli/internal/prompts/app_select_test.go
Lines 1144 to 1151 in 5b9f5ab
| "errors if token flag and team id flag do not match": { | |
| mockFlagTeam: team1TeamID, | |
| mockFlagToken: fakeAuthsByTeamDomain[team2TeamDomain].Token, | |
| mockAuthWithToken: fakeAuthsByTeamDomain[team2TeamDomain], | |
| mockAuthWithTeamIDError: slackerror.New(slackerror.ErrCredentialsNotFound), | |
| mockAuthWithTeamIDTeamID: team2TeamID, | |
| expectedError: slackerror.New(slackerror.ErrTeamNotFound), | |
| }, |
| "token with mismatched app flag": { | ||
| AppFlag: "A1EXAMPLE01", | ||
| TokenFlag: team2Token, | ||
| TeamFlag: "", | ||
| expectedAuth: types.SlackAuth{}, | ||
| expectedErr: *slackerror.New(slackerror.ErrInvalidToken), | ||
| }, |
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.
🗣️ The token and app flag combination is checked in persisting tests!
slack-cli/internal/prompts/app_select_test.go
Lines 294 to 303 in 8466439
| "error if an error occurred while collecting app info": { | |
| tokenFlag: team1Token, | |
| tokenAuth: fakeAuthsByTeamDomain[team1TeamDomain], | |
| appFlag: localTeam1UninstalledApp.AppID, | |
| appStatus: api.GetAppStatusResult{}, | |
| appStatusErr: slackerror.New(slackerror.ErrAppNotFound), | |
| selectStatus: ShowAllApps, | |
| expectedApp: SelectedApp{}, | |
| expectedErr: slackerror.New(slackerror.ErrAppNotFound), | |
| }, |
| func TestPrompt_AppSelectPrompt_AuthsNoApps(t *testing.T) { | ||
|
|
||
| // Set up mocks | ||
| ctx := slackcontext.MockContext(t.Context()) | ||
| clientsMock := shared.NewClientsMock() | ||
| clientsMock.API.On("ValidateSession", mock.Anything, mock.Anything).Return(api.AuthSession{}, nil) | ||
| clients := shared.NewClientFactory(clientsMock.MockClientFactory()) | ||
|
|
||
| clientsMock.AddDefaultMocks() | ||
|
|
||
| // Execute test | ||
| selectedApp, err := AppSelectPrompt(ctx, clients, AppInstallStatus(ShowInstalledAppsOnly)) | ||
| require.Equal(t, selectedApp, SelectedApp{}) | ||
| require.Error(t, err, slackerror.New(slackerror.ErrInstallationRequired)) | ||
| } |
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.
🗣️ An equivalent test exists!
slack-cli/internal/prompts/app_select_test.go
Lines 746 to 754 in 8466439
| "errors if installation required and no apps saved": { | |
| mockAuths: fakeAuthsByTeamDomainSlice, | |
| mockAppsDeployed: []types.App{}, | |
| mockManifestSource: config.ManifestSourceLocal, | |
| appPromptConfigEnvironment: ShowHostedOnly, | |
| appPromptConfigStatus: ShowInstalledAppsOnly, | |
| expectedError: slackerror.New(slackerror.ErrInstallationRequired), | |
| expectedSelection: SelectedApp{}, | |
| }, |
| // | ||
| // TeamAppSelectPrompt tests | ||
| // |
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.
🪓 The TeamAppSelectPrompt is removed altogether in #159! If these tests have a matching AppSelectPrompt tests, that is used.
| clientsMock.API.AssertCalled(t, "ExchangeAuthTicket", mock.Anything, mock.Anything, mock.Anything, mock.Anything) | ||
| } | ||
|
|
||
| func TestPrompt_TeamAppSelectPrompt_NoAuths_UserReAuthenticates(t *testing.T) { |
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.
📝 This is noted as a needed improvement to the app selector!
| // Test to ensure that legacy apps.dev.json entries which have | ||
| // team_domain set as "dev" are overridden with the correct team_domain when the auth | ||
| // context is known |
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.
🗣️ This migration was removed with the 3.0.0 release - the tests are following now.
I'm open to returning this if we want to continue supporting this, but also would be open to removing similar logic in:
slack-cli/internal/pkg/apps/list.go
Lines 46 to 63 in f9b74e0
| // Update local run app names to include domain (when possible) | |
| for i, devApp := range devApps { | |
| if auth, err := clients.Auth().AuthWithTeamID(ctx, devApp.TeamID); err == nil { | |
| // modify for display | |
| devApps[i].TeamDomain = fmt.Sprintf("%s %s", auth.TeamDomain, style.LocalRunNameTag) | |
| // Handle legacy apps.dev.json format | |
| // Legacy dev apps have team_domain as "dev" | |
| // instead of the team domain of the team they | |
| // were created in. Selector UI that relies on the | |
| // app's team domain will incorrectly display "dev" | |
| // So we override the TeamDomain when the auth | |
| // context is known and after we've confirmed that | |
| // auth.TeamID matches the app.TeamID | |
| devApp.TeamDomain = auth.TeamDomain | |
| _ = clients.AppClient().SaveLocal(ctx, devApp) | |
| } | |
| } |
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.
Since it was removed in the major release of v3.0.0, I think we're free to remove this logic whenever we're ready.
| } | ||
| } | ||
|
|
||
| func TestPrompt_TeamAppSelectPrompt_AppSelectPrompt_EnterpriseWorkspaceApps_HasWorkspaceAuth(t *testing.T) { |
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.
🗣️ Tests that resolve authentications for enterprise workspace apps are found here:
slack-cli/internal/prompts/app_select_test.go
Lines 476 to 545 in f9b74e0
| "returns enterprise workspace apps with matching auths": { | |
| mockAuths: []types.SlackAuth{ | |
| { | |
| Token: enterprise1Token, | |
| TeamDomain: enterprise1TeamDomain, | |
| TeamID: enterprise1TeamID, | |
| EnterpriseID: enterprise1TeamID, | |
| }, | |
| }, | |
| mockAppsSavedDeployed: []types.App{ | |
| { | |
| AppID: deployedTeam1InstalledAppID, | |
| EnterpriseID: enterprise1TeamID, | |
| TeamID: team1TeamID, | |
| }, | |
| }, | |
| mockAppsSavedLocal: []types.App{ | |
| { | |
| AppID: localTeam2InstalledAppID, | |
| EnterpriseID: enterprise1TeamID, | |
| TeamID: team2TeamID, | |
| }, | |
| }, | |
| mockTeam1SavedAuthError: slackerror.New(slackerror.ErrCredentialsNotFound), | |
| mockTeam1StatusAppIDs: []string{ | |
| deployedTeam1InstalledAppID, | |
| }, | |
| mockTeam1Status: api.GetAppStatusResult{ | |
| Apps: []api.AppStatusResultAppInfo{ | |
| deployedTeam1InstalledAppStatus, | |
| }, | |
| }, | |
| mockTeam2SavedAuthError: slackerror.New(slackerror.ErrCredentialsNotFound), | |
| mockTeam2StatusAppIDs: []string{ | |
| localTeam2InstalledAppID, | |
| }, | |
| mockTeam2Status: api.GetAppStatusResult{ | |
| Apps: []api.AppStatusResultAppInfo{ | |
| localTeam2InstalledAppStatus, | |
| }, | |
| }, | |
| mockEnterprise1SavedAuth: types.SlackAuth{ | |
| Token: enterprise1Token, | |
| }, | |
| expectedApps: map[string]SelectedApp{ | |
| deployedTeam1InstalledAppID: { | |
| App: types.App{ | |
| AppID: deployedTeam1InstalledAppID, | |
| EnterpriseID: enterprise1TeamID, | |
| TeamID: team1TeamID, | |
| InstallStatus: types.AppStatusInstalled, | |
| }, | |
| Auth: types.SlackAuth{ | |
| Token: enterprise1Token, | |
| }, | |
| }, | |
| localTeam2InstalledAppID: { | |
| App: types.App{ | |
| AppID: localTeam2InstalledAppID, | |
| EnterpriseID: enterprise1TeamID, | |
| TeamID: team2TeamID, | |
| InstallStatus: types.AppStatusInstalled, | |
| IsDev: true, | |
| }, | |
| Auth: types.SlackAuth{ | |
| Token: enterprise1Token, | |
| }, | |
| }, | |
| }, | |
| }, |
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.
🗣️ This particular case might also not need to have a match now since authentication is performed on the enterprise, but please call me out if this doesn't seem right!
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.
I think it's better to keep this test for now, but we're at that point where we can consider removing the migration logic for folks who have auth'd against an Enterprise Workspace and have their auth migrated to the Organization. At this point, I imagine most of those tokens have expired anyhow.
| } | ||
| } | ||
|
|
||
| func TestPrompt_TeamAppSelectPrompt_AppSelectPrompt_EnterpriseWorkspaceApps_MissingWorkspaceAuth_MissingOrgAuth_UserReAuthenticates(t *testing.T) { |
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.
🗣️ Reauthentication tests for related logic are kept in:
slack-cli/internal/prompts/app_select_test.go
Lines 1648 to 1671 in f9b74e0
| "revalidates an expired authentication on a dev instance": { | |
| authProvided: types.SlackAuth{ | |
| APIHost: &apiHostDev, | |
| Token: "xoxb-development", | |
| }, | |
| authExpected: types.SlackAuth{ | |
| APIHost: &apiHostDev, | |
| TeamDomain: team1TeamDomain, | |
| TeamID: team1TeamID, | |
| Token: fakeAuthsByTeamDomain[team1TeamDomain].Token, | |
| UserID: "U1", | |
| }, | |
| apiExchangeAuthTicketResultResponse: api.ExchangeAuthTicketResult{ | |
| TeamDomain: team1TeamDomain, | |
| TeamID: team1TeamID, | |
| Token: fakeAuthsByTeamDomain[team1TeamDomain].Token, | |
| UserID: "U1", | |
| }, | |
| apiValidateSessionError: slackerror.New(slackerror.ErrInvalidAuth), | |
| authFilteredKnownAuthErrorsResponse: true, | |
| authFilteredKnownAuthErrorsError: nil, | |
| authIsAPIHostSlackProdResponse: false, | |
| ioIsTTYResponse: true, | |
| }, |
mwbrooks
left a comment
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.
🤯 Wow, what a monster PR swinging the axe! 💥 🪓
✅ Code removal looks good and manual tests on a Standalone Workspace and Enterprise Organization works well on my side.
🤔 Odd that test coverage dropped - looks like it's the format.go. However, I don't see anything special in the test cases, so I think we're safe to move forward.
|
@mwbrooks Praises to the axe! 🪓 ✨ And thanks for reviewing and testing the changes too! I'll merge this now along with a few follow ups to app selection. Hoping to also take a look at test coverage when handling the "no authentications" case and saved "dev" team domains but for now let's celebrate some lines removed 🎉 |
Summary
This PR defaults to using the
boltexperiment behavior for app selection. No change to behavior.Notes
Changes to app selection perhaps needed to match previous test cases are listed below, but are saved for a follow up:
Requirements