chore: default to bolt experiment behavior for app selection#158
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.
📝 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.
📚 Wording updates to start all of these cases with either "returns" or "errors" follow this comment.
| @@ -1497,7 +1090,6 @@ func flatTeamSelectPrompt( | |||
| // 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.
👁️🗨️ 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.
🧪 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.
🗣️ The removal of TeamAppSelect might be in #159!
There was a problem hiding this comment.
💌 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.
🗣️ These tests are covered with existing TestGetTokenApp cases:
slack-cli/internal/prompts/app_select_test.go
Line 175 in 8466439
- "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.
🗣️ 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
| "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.
🗣️ Error cases for token tests are also covered in tests
slack-cli/internal/prompts/app_select_test.go
Lines 1144 to 1151 in 5b9f5ab
| "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.
🗣️ 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
| 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.
🗣️ An equivalent test exists!
slack-cli/internal/prompts/app_select_test.go
Lines 746 to 754 in 8466439
| // | ||
| // TeamAppSelectPrompt tests | ||
| // |
There was a problem hiding this comment.
🪓 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.
📝 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.
🗣️ 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
There was a problem hiding this comment.
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.
🗣️ 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
There was a problem hiding this comment.
🗣️ 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.
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.
🗣️ Reauthentication tests for related logic are kept in:
slack-cli/internal/prompts/app_select_test.go
Lines 1648 to 1671 in f9b74e0
mwbrooks
left a comment
There was a problem hiding this comment.
🤯 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