Repository: mattermost/mattermost-plugin-github
Pull request #679 (2023-05-21 18:53:54)

[MM-42918] Add debug information for setup wizard
#### Summary
I'm unable to reproduce the issue linked in the ticket.

To debug the issue if it occurs again, I've added debug information on why exactly the wizard didn't run.

If the PR gets approved, I'm going to copy the change into https://github.com/mattermost/mattermost-plugin-gitlab.

#### Ticket Link
https://mattermost.atlassian.net/browse/MM-42918


codecov-commenter: ## [Codecov](https://app.codecov.io/gh/mattermost/mattermost-plugin-github/pull/679?src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=mattermost) Report
Patch coverage: **`59.09`**% and project coverage change: **`+0.17`** :tada:
> Comparison is base [(`64a725b`)](https://app.codecov.io/gh/mattermost/mattermost-plugin-github/commit/64a725b9dfaed2158e24621f414ab6251feae7ff?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=mattermost) 15.46% compared to head [(`25ec692`)](https://app.codecov.io/gh/mattermost/mattermost-plugin-github/pull/679?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=mattermost) 15.63%.

<details><summary>Additional details and impacted files</summary>


```diff
@@            Coverage Diff             @@
##           master     #679      +/-   ##
==========================================
+ Coverage   15.46%   15.63%   +0.17%     
==========================================
  Files          15       15              
  Lines        5497     5518      +21     
==========================================
+ Hits          850      863      +13     
- Misses       4605     4613       +8     
  Partials       42       42              
```


| [Impacted Files](https://app.codecov.io/gh/mattermost/mattermost-plugin-github/pull/679?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=mattermost) | Coverage Δ | |
|---|---|---|
| [server/plugin/flows.go](https://app.codecov.io/gh/mattermost/mattermost-plugin-github/pull/679?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=mattermost#diff-c2VydmVyL3BsdWdpbi9mbG93cy5nbw==) | `0.00% <0.00%> (ø)` | |
| [server/plugin/plugin.go](https://app.codecov.io/gh/mattermost/mattermost-plugin-github/pull/679?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=mattermost#diff-c2VydmVyL3BsdWdpbi9wbHVnaW4uZ28=) | `5.92% <0.00%> (-0.06%)` | :arrow_down: |
| [server/plugin/utils.go](https://app.codecov.io/gh/mattermost/mattermost-plugin-github/pull/679?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=mattermost#diff-c2VydmVyL3BsdWdpbi91dGlscy5nbw==) | `60.16% <100.00%> (+2.22%)` | :arrow_up: |


</details>

[:umbrella: View full report in Codecov by Sentry](https://app.codecov.io/gh/mattermost/mattermost-plugin-github/pull/679?src=pr&el=continue&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=mattermost).   
:loudspeaker: Do you have feedback about the report comment? [Let us know in this issue](https://about.codecov.io/codecov-pr-comment-feedback/?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=mattermost).

mickmister: @hanzei What are the requirements to get this merged?
hanzei: @mickmister Good question. In order to test this PR, one has to jump through a couple of hoops, namely removing the signature check for the plugin so this build can be installed as part of the onboarding flow.

Once that is done, the PR can be tested. It produces output like this one:
`debug [2023-05-22 23:19:30.590 +02:00] OAuth is configured, skipping setup wizard    caller="app/plugin_api.go:970" plugin_id=github GitHubOAuthClientID="***8cb3" GitHubOAuthClientSecret="***3d78" UsePreregisteredApplication=false
`

To me that is enough testing given the complex steps to get there. Are you fine with emerging the PR as it is?


hanzei: GitLab PR: https://github.com/mattermost/mattermost-plugin-gitlab/pull/379
mickmister: > To me that is enough testing given the complex steps to get there. Are you fine with emerging the PR as it is?

LGTM :+1:

Patch:
diff --git a/server/plugin/flows.go b/server/plugin/flows.go
index c480401..b8e2674 100644
--- a/server/plugin/flows.go
+++ b/server/plugin/flows.go
@@ -221,6 +221,8 @@ func (fm *FlowManager) StartSetupWizard(userID string, delegatedFrom string) err
 		return err
 	}
 
+	fm.client.Log.Debug("Started setup wizard", "userID", userID, "delegatedFrom", delegatedFrom)
+
 	fm.trackStartSetupWizard(userID, delegatedFrom != "")
 
 	return nil
diff --git a/server/plugin/plugin.go b/server/plugin/plugin.go
index ec71101..74748ff 100644
--- a/server/plugin/plugin.go
+++ b/server/plugin/plugin.go
@@ -436,8 +436,14 @@ func (p *Plugin) ReactionHasBeenRemoved(c *plugin.Context, reaction *model.React
 }
 
 func (p *Plugin) OnInstall(c *plugin.Context, event model.OnInstallEvent) error {
+	conf := p.getConfiguration()
+
 	// Don't start wizard if OAuth is configured
-	if p.getConfiguration().IsOAuthConfigured() {
+	if conf.IsOAuthConfigured() {
+		p.client.Log.Debug("OAuth is configured, skipping setup wizard",
+			"GitHubOAuthClientID", lastN(conf.GitHubOAuthClientID, 4),
+			"GitHubOAuthClientSecret", lastN(conf.GitHubOAuthClientSecret, 4),
+			"UsePreregisteredApplication", conf.UsePreregisteredApplication)
 		return nil
 	}
 
diff --git a/server/plugin/utils.go b/server/plugin/utils.go
index 91ccc94..5c5ae93 100644
--- a/server/plugin/utils.go
+++ b/server/plugin/utils.go
@@ -351,3 +351,23 @@ func isValidURL(rawURL string) error {
 
 	return nil
 }
+
+// lastN returns the last n characters of a string, with the rest replaced by *.
+// At most 3 characters are replaced. The rest is cut off.
+func lastN(s string, n int) string {
+	if n < 0 {
+		return ""
+	}
+
+	out := []byte(s)
+	if len(out) > n+3 {
+		out = out[len(out)-n-3:]
+	}
+	for i := range out {
+		if i < len(out)-n {
+			out[i] = '*'
+		}
+	}
+
+	return string(out)
+}


Test patch:
diff --git a/server/plugin/utils_test.go b/server/plugin/utils_test.go
index 46ca183..65ae625 100644
--- a/server/plugin/utils_test.go
+++ b/server/plugin/utils_test.go
@@ -254,3 +254,25 @@ func TestGetToDoDisplayText(t *testing.T) {
 		})
 	}
 }
+
+func TestLastN(t *testing.T) {
+	tcs := []struct {
+		Text     string
+		N        int
+		Expected string
+	}{
+		{Text: "", N: -99, Expected: ""},
+		{Text: "", N: -1, Expected: ""},
+		{Text: "", N: 0, Expected: ""},
+		{Text: "", N: 1, Expected: ""},
+		{Text: "", N: 99, Expected: ""},
+		{Text: "abcdef", N: 4, Expected: "**cdef"},
+		{Text: "abcdefghi", N: 2, Expected: "***hi"},
+		{Text: "abcdefghi", N: 0, Expected: "***"},
+		{Text: "abcdefghi", N: 99, Expected: "abcdefghi"},
+	}
+
+	for _, tc := range tcs {
+		assert.Equal(t, tc.Expected, lastN(tc.Text, tc.N))
+	}
+}