feat: add support for 'feedback --name slack-platform' #70
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -51,8 +51,6 @@ type SlackSurvey struct { | |
| // Info prints additional information about the survey; displayed when the option is selected in `feedback` | ||
| // Info is optional | ||
| Info func(ctx context.Context, clients *shared.ClientFactory) | ||
| // Ask either prints text or prompts the user to complete the survey | ||
| // Potentially displayed after `run`/`deploy`/`doctor` (or other places where ShowSurveyMessages is called) | ||
| Ask func(ctx context.Context, clients *shared.ClientFactory) (bool, error) | ||
|
|
@@ -70,8 +68,9 @@ const ( | |
|
|
||
| // Supported survey names | ||
| const ( | ||
| SlackCLIFeedback = "slack-cli" | ||
| SlackPlatformFeedback = "slack-platform" | ||
| SlackPlatformFeedbackDeprecated = "platform-improvements" // DEPRECATED(semver:major) | ||
| ) | ||
|
|
||
| type SurveyConfigInterface interface { | ||
|
|
@@ -94,11 +93,10 @@ var SurveyStore = map[string]SlackSurvey{ | |
| Info: func(ctx context.Context, clients *shared.ClientFactory) { | ||
| clients.IO.PrintInfo(ctx, false, fmt.Sprintf( | ||
| "%s\n%s\n", | ||
| style.Secondary("Ask questions, submit issues, or suggest features for the Slack CLI:"), | ||
|
Member
Author
There was a problem hiding this comment. note: Writing tests found a typo!
Member
There was a problem hiding this comment. @mwbrooks Whew great catch! It's nice to know this wasn't released, but darn that's an |
||
| style.Secondary(style.Highlight("https://github.com/slackapi/slack-cli/issues")), | ||
| )) | ||
| }, | ||
| Ask: func(ctx context.Context, clients *shared.ClientFactory) (bool, error) { | ||
| clients.IO.PrintInfo(ctx, false, style.Sectionf(style.TextSection{ | ||
| Emoji: "love_letter", | ||
|
|
@@ -127,7 +125,6 @@ var SurveyStore = map[string]SlackSurvey{ | |
| style.Secondary("Or, share your experiences at "+style.Highlight("https://docs.slack.dev/developer-support")), | ||
| )) | ||
| }, | ||
| Ask: func(ctx context.Context, clients *shared.ClientFactory) (bool, error) { | ||
| clients.IO.PrintInfo(ctx, false, style.Sectionf(style.TextSection{ | ||
| Emoji: "love_letter", | ||
|
|
@@ -246,18 +243,21 @@ func runFeedbackCommand(ctx context.Context, clients *shared.ClientFactory, cmd | |
| return nil | ||
| } | ||
|
|
||
| // DEPRECATED(semver:major): Support the deprecated survey name for backwards compatibility | ||
| if surveyNameFlag == SlackPlatformFeedbackDeprecated { | ||
| surveyNameFlag = SlackPlatformFeedback | ||
| clients.IO.PrintDebug(ctx, "DEPRECATED: The '--name %s' flag is deprecated; use '--name %s' instead", SlackPlatformFeedbackDeprecated, SlackPlatformFeedback) | ||
| } | ||
|
|
||
|
Member
Author
There was a problem hiding this comment. note: I print a deprecation message to our
Member
There was a problem hiding this comment. 📣 I'm in favor of annoying and loud deprecation messages to encourage making expected changes as soon as possible. This is a pattern I've found in build tools like
Member
Author
There was a problem hiding this comment. Cool with me! Commit 06e247e now prints this loud and in the open! 
|
||
| surveyNames, surveyPromptOptions := initSurveyOpts(ctx, clients, SurveyStore) | ||
|
|
||
| if _, ok := SurveyStore[surveyNameFlag]; !ok && surveyNameFlag != "" { | ||
| return slackerror.New(slackerror.ErrFeedbackNameInvalid). | ||
| WithMessage("Invalid feedback name provided: %s", surveyNameFlag) | ||
| } | ||
|
|
||
| if surveyNameFlag == "" && noPromptFlag { | ||
| return slackerror.New(slackerror.ErrFeedbackNameRequired) | ||
| } | ||
|
|
||
| clients.IO.PrintInfo(ctx, false, style.Sectionf(style.TextSection{ | ||
|
|
@@ -338,7 +338,8 @@ func executeSurvey(ctx context.Context, clients *shared.ClientFactory, s SlackSu | |
| if s.Info != nil { | ||
| s.Info(ctx, clients) | ||
| } | ||
|
|
||
| clients.IO.PrintTrace(ctx, slacktrace.FeedbackMessage, s.Name) | ||
|
|
||
| var err error | ||
| var ok bool | ||
|
|
||
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -26,15 +26,16 @@ import ( | |
| "github.com/slackapi/slack-cli/internal/shared" | ||
| "github.com/slackapi/slack-cli/internal/slackcontext" | ||
| "github.com/slackapi/slack-cli/internal/slackerror" | ||
| "github.com/slackapi/slack-cli/internal/slacktrace" | ||
| "github.com/slackapi/slack-cli/internal/style" | ||
| "github.com/slackapi/slack-cli/test/testutil" | ||
| "github.com/spf13/cobra" | ||
| "github.com/stretchr/testify/assert" | ||
| "github.com/stretchr/testify/mock" | ||
| ) | ||
|
|
||
| func TestFeedbackCommand(t *testing.T) { | ||
| t.Run("when there is only one survey option", func(t *testing.T) { | ||
| surveys := map[string]SlackSurvey{ | ||
| SlackPlatformFeedback: { | ||
| Name: SlackPlatformFeedback, | ||
|
|
@@ -67,17 +68,21 @@ func TestFeedbackCommand(t *testing.T) { | |
|
|
||
| clients := shared.NewClientFactory(clientsMock.MockClientFactory()) | ||
|
|
||
| _surveys := SurveyStore | ||
| SurveyStore = surveys | ||
| defer func() { SurveyStore = _surveys }() | ||
|
Comment on lines
+71
to
+73
Member
Author
There was a problem hiding this comment. note: After adding some Command Table Tests, I noticed that the tests started failing when ran together. The reason is that the current tests clobber the global 🧪 In a follow-up PR, I'd like to refactor these tests to all use the Command Table Test. |
||
|
|
||
| // Execute test | ||
| cmd := NewFeedbackCommand(clients) | ||
| err := runFeedbackCommand(ctx, clients, cmd) | ||
|
|
||
| // Assertions | ||
| assert.NoError(t, err) | ||
| clientsMock.Browser.AssertCalled(t, "OpenURL", "https://survey.com?project_id=projectID&system_id=systemID&utm_medium=cli&utm_source=cli") | ||
| clientsMock.IO.AssertCalled(t, "PrintTrace", mock.Anything, slacktrace.FeedbackMessage, []string{SlackPlatformFeedback}) | ||
| }) | ||
|
|
||
| t.Run("when there are multiple survey options", func(t *testing.T) { | ||
| surveys := map[string]SlackSurvey{ | ||
| "A_test": { | ||
| Name: "A_test", | ||
|
|
@@ -133,18 +138,22 @@ func TestFeedbackCommand(t *testing.T) { | |
|
|
||
| clients := shared.NewClientFactory(clientsMock.MockClientFactory()) | ||
|
|
||
| _surveys := SurveyStore | ||
| SurveyStore = surveys | ||
| defer func() { SurveyStore = _surveys }() | ||
|
|
||
| // Execute test | ||
| cmd := NewFeedbackCommand(clients) | ||
| err := runFeedbackCommand(ctx, clients, cmd) | ||
| assert.NoError(t, err) | ||
|
|
||
| // Assertions | ||
| clientsMock.Browser.AssertCalled(t, "OpenURL", "https://survey.com?project_id=projectID&system_id=systemID&utm_medium=cli&utm_source=cli") | ||
| clientsMock.IO.AssertCalled(t, "PrintTrace", mock.Anything, slacktrace.FeedbackMessage, []string{SlackPlatformFeedback}) | ||
| }) | ||
| } | ||
|
|
||
| func TestShowSurveyMessages(t *testing.T) { | ||
| t.Run("surveys asked or not asked based on the stored config", func(t *testing.T) { | ||
| surveys := map[string]SlackSurvey{ | ||
| // Should be asked once; already asked | ||
|
|
@@ -240,13 +249,71 @@ func TestShowSurveyMessages(t *testing.T) { | |
|
|
||
| clients := shared.NewClientFactory(clientsMock.MockClientFactory()) | ||
|
|
||
| _surveys := SurveyStore | ||
| SurveyStore = surveys | ||
| defer func() { SurveyStore = _surveys }() | ||
|
|
||
| // Execute test | ||
| err := ShowSurveyMessages(ctx, clients) | ||
| assert.NoError(t, err) | ||
| clientsMock.Browser.AssertCalled(t, "OpenURL", "https://B.com?project_id=projectID&system_id=systemID&utm_medium=cli&utm_source=cli") | ||
| clientsMock.Browser.AssertCalled(t, "OpenURL", "https://C.com?project_id=projectID&system_id=systemID&utm_medium=cli&utm_source=cli") | ||
| }) | ||
| } | ||
|
|
||
| func Test_Feedback_FeedbackCommand(t *testing.T) { | ||
| testutil.TableTestCommand(t, testutil.CommandTests{ | ||
| // DEPRECATED(semver:major): Support the deprecated survey name for backwards compatibility | ||
| "supports deprecated --name platform-improvements": { | ||
| CmdArgs: []string{"--name", "platform-improvements"}, | ||
| Setup: func(t *testing.T, ctx context.Context, cm *shared.ClientsMock, cf *shared.ClientFactory) { | ||
| setupFeedbackCommandMocks(t, ctx, cm, cf) | ||
| }, | ||
| ExpectedOutputs: []string{ | ||
| "[email protected]", | ||
| "https://docs.slack.dev/developer-support", | ||
| }, | ||
| }, | ||
| "supports --name slack-cli": { | ||
| CmdArgs: []string{"--name", "slack-cli"}, | ||
| Setup: func(t *testing.T, ctx context.Context, cm *shared.ClientsMock, cf *shared.ClientFactory) { | ||
| setupFeedbackCommandMocks(t, ctx, cm, cf) | ||
| }, | ||
| ExpectedOutputs: []string{ | ||
| "Ask questions, submit issues, or suggest features for the Slack CLI", | ||
| "https://github.com/slackapi/slack-cli/issues", | ||
| }, | ||
| }, | ||
| "supports --name slack-platform": { | ||
| CmdArgs: []string{"--name", "slack-platform"}, | ||
| Setup: func(t *testing.T, ctx context.Context, cm *shared.ClientsMock, cf *shared.ClientFactory) { | ||
| setupFeedbackCommandMocks(t, ctx, cm, cf) | ||
| }, | ||
| ExpectedOutputs: []string{ | ||
| "[email protected]", | ||
| "https://docs.slack.dev/developer-support", | ||
| }, | ||
| }, | ||
|
Comment on lines
+287
to
+296
|
||
| }, func(cf *shared.ClientFactory) *cobra.Command { | ||
| cmd := NewFeedbackCommand(cf) | ||
| return cmd | ||
| }) | ||
| } | ||
|
|
||
| // setupFeedbackCommandMocks prepares common mocks for these tests | ||
| func setupFeedbackCommandMocks(t *testing.T, ctx context.Context, cm *shared.ClientsMock, cf *shared.ClientFactory) { | ||
| cm.AddDefaultMocks() | ||
|
|
||
| scm := &config.SystemConfigMock{} | ||
| scm.On("GetSurveyConfig", mock.Anything, mock.Anything).Return(config.SurveyConfig{}, nil) | ||
| scm.On("GetSystemID", mock.Anything).Return("systemID", nil) | ||
| scm.On("SetSurveyConfig", mock.Anything, mock.Anything, mock.Anything).Return(nil) | ||
| cm.Config.SystemConfig = scm | ||
|
|
||
| pcm := &config.ProjectConfigMock{} | ||
| pcm.On("GetSurveyConfig", mock.Anything, mock.Anything).Return(config.SurveyConfig{}, nil) | ||
| pcm.On("GetProjectID", mock.Anything).Return("projectID", nil) | ||
| cm.Config.ProjectConfig = pcm | ||
|
|
||
| cm.IO.On("ConfirmPrompt", mock.Anything, "Open in browser?", mock.Anything).Return(false) | ||
| } | ||
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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: Added a
DEPRECATED(semver:X)comment to make this easier to find in the future. We are a little consistent with our deprecation comments -TODO(semver:major)and others also exist. Preference?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.
Thanks for both this comment and calling out the formatting!
IIRC we are using
TODO(semver:major)more often, but this is often for internal breaking changes AFAICT.I think
DEPRECATEDis meaningful here since it captures the ongoing support with an existing alternative option, but IMO thesemver:majorpart is the most important! This is what I would use in searching for these 😉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.
Also I learned in review of #71 that
gohas a convention around making machine-readable comments that I'm interested in exploring:https://go.dev/wiki/Comments#directives
Using a similar pattern might be interesting for these more "conventional" comments, but I don't believe we have explicit notes on how these should be used overall...
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.
One addition to this comment might also note the deprecated suggestions? 📚
Edit: I realize you make a similar comment a few lines down - no blocker here!
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
//go:directive is interesting. It led me down a path to find the use of the// DEPRECATED:comment in Golang. So, perhaps we can start to use that convention to signal deprecated code. I definitely agree that the(semver:major)scope is most important though!