batches: create GitHub App UX flow by courier-new · Pull Request #52979 · sourcegraph/sourcegraph-public-snapshot

courier-new · 2023-06-06T07:27:16Z

Closes https://github.com/sourcegraph/sourcegraph/issues/52223, https://github.com/sourcegraph/sourcegraph/issues/52220, https://github.com/sourcegraph/sourcegraph/issues/52475.

Walkthrough

Code walkthrough time, because it's a fairly chunky diff! Let's start at the front of the front and loop our way around.

CreateGitHubAppPage - [code]

This component is now configurable to different GH App contexts via a series of props, everything from which permissions to send for the new GH App manifest to the page headers to a URL validation function.
The "Create" form now gives better clientside validation, such as when the GitHub instance URL is invalid, or your app name is too long (GitHub requires it be 34 characters or less).
Some of the copy has been updated to read more clearly and better reflect the language used on the GitHub side of things.

BatchChangesCreateGitHubAppPage - [code]

This component is a wrapper around CreateGitHubAppPage for specifically the BC App context. It provides a custom URL validation function to ensure the provided URL is from a unique host as well as sets the manifest details, page header, "batches" App domain, and base URL.
You can review how this page differs from the initial "Repos" one in the first set of demo videos below.

CommitSigningIntegrationNode - [code]

Hooked up the "Create" and "Remove" buttons.

batches/resolvers/code_host.go - [code]

Surfaced the GitHub App id and base_url properties to the GraphQL type. The former is needed for the mutation to delete the App, and the latter is needed to validate that the URL an admin inputs when creating a new App is unique.

githubappauth/middleware.go - [code]

Brought over Bolaji's PR to support different redirects for different App domains.
Cleaned up some error handling.
I changed how the redirect in the final stages of the setup works slightly. Before, if anything were to error out in this stage of the process, it would just return a bare, plain response with the text error message, stranding the user at an internal URL like https://sourcegraph.test:3443/.auth/githubapp/setup... and requiring them to manually get themselves back to Sourcegraph and figure out what to do next. Now if an error occurs, we will attempt to redirect the user back to the appropriate GitHub Apps site admin page and surface the exact error message in the UI. You can see this in action in the 2nd and 3rd failure path demo videos below.

GitHubFailureAlert - [code]

This is the common component used to render one of those middleware errors from the UI.

Demo videos

Success path, creating a GitHub App. I recorded creating for repos here, too, just to confirm nothing broke:

Batch Changes	Repos
Screen.Recording.2023-06-07.at.12.16.48.AM.mov	Screen.Recording.2023-06-07.at.12.14.54.AM.mov

Failure path, where we failed to create the initial state to use for initiating the GitHub App creation process:

Batch Changes	Repos
Screen.Recording.2023-06-06.at.11.51.07.PM.mov	Screen.Recording.2023-06-06.at.11.53.45.PM.mov

Failure path, where we got all the way through to validating the setup and recording the new installation, but failed before we could ascertain the domain that the new GitHub App was intended for:

Batch Changes	Repos
Screen.Recording.2023-06-06.at.11.40.13.PM.mov	Screen.Recording.2023-06-06.at.11.57.54.PM.mov

Failure path, where we got all the way through to validating the setup and recording the new installation, but something went wrong at the very last step:

Batch Changes	Repos
Screen.Recording.2023-06-06.at.11.44.09.PM.mov	Screen.Recording.2023-06-07.at.12.01.16.AM.mov

Test plan

Extensive manual testing with a variety of forced failure breakpoints. Middleware unit tests updated.

…ovided, clean up text

commit c3b078c Author: BolajiOlajide <cooproton@gmail.com> Date: Sat Jun 3 22:29:17 2023 +0100 update batches redirect url commit 30f8985 Author: BolajiOlajide <cooproton@gmail.com> Date: Sat Jun 3 22:27:12 2023 +0100 bazel it commit e4e734c Author: BolajiOlajide <cooproton@gmail.com> Date: Sat Jun 3 21:57:39 2023 +0100 add tests for middleware commit 80855ed Author: BolajiOlajide <cooproton@gmail.com> Date: Sat Jun 3 16:18:46 2023 +0100 add tests commit 5b3c0f8 Author: BolajiOlajide <cooproton@gmail.com> Date: Tue May 30 17:54:42 2023 +0100 handle redirect for batches app

courier-new · 2023-06-07T17:26:06Z

+            if (!response.ok) {
+                if (response.body instanceof ReadableStream) {
+                    const error = await response.text()
+                    throw new Error(error)
+                }
+            }
+            const state = (await response.json()) as StateResponse


The errors we respond with from the server middleware are plain text, not JSON. Prior to this change, middleware errors would surface in the UI as confusing JSON parsing errors:

This ensures a middleware response error is properly handled. Check out the demo videos to see it!

courier-new · 2023-06-07T17:26:39Z

+            if (state.webhookUUID?.length) {
+                webhookURL = new URL(`/.api/webhooks/${state.webhookUUID}`, originURL).href
+            }
+            if (!state.state?.length) {
+                throw new Error('Response from server missing state parameter')
+            }


Covering our grounds here rather than blindly assuming what we got back from the server was a StateResponse.

courier-new · 2023-06-07T17:30:09Z

+            try {
+                new URL(trimmedURL)
+            } catch {
+                return setUrlError('URL is not valid.')
+            }


This ensures the GitHub URL an admin input is actually a valid URL. Before if you made a typo it would just... completely blow up 😂

Screen.Recording.2023-06-07.at.10.29.11.AM.mov

Omgosh. Good fix!

courier-new · 2023-06-07T17:58:52Z

Tests from Bolaji's PR, thank you Bolaji!! 🙌 The only part that's new since my PR is the addition of several test cases for GenerateRedirectURL to ensure a redirect URL is still returned with an error message encoded as a URL parameter now.

courier-new · 2023-06-07T17:59:14Z

+		{
+			name:        "invalid domain",
+			domain:      &invalidDomain,
+			expectedURL: "/site-admin/github-apps?success=false&error=invalid+domain%3A+invalid",
+		},
+		{
+			name:        "repos creation error",
+			domain:      &reposDomain,
+			creationErr: creationErr,
+			expectedURL: "/site-admin/github-apps?success=false&error=uh+oh%21",
+		},
+		{
+			name:        "batches creation error",
+			domain:      &batchesDomain,
+			creationErr: creationErr,
+			expectedURL: "/site-admin/batch-changes?success=false&error=uh+oh%21",
+		},


The new test cases.

courier-new · 2023-06-07T18:09:16Z

            />
-            {error && <ErrorAlert className="mt-4 mb-0 text-left" error={error} />}
+            {!success && setupError && <GitHubAppFailureAlert error={setupError} />}
            <ConnectionContainer>
                {error && <ErrorAlert error={error} />}


The existing error here from the connection was actually redundant with the one on line 84/90.

courier-new · 2023-06-07T18:11:31Z

-                    <Button
-                        variant="primary"
-                        onClick={createState}
-                        disabled={name.trim().length < 3 || url.trim().length < 10}
-                    >
+                    <Button variant="primary" onClick={createState} disabled={!!nameError || !!urlError}>


The condition to disable the button here was arbitrary and didn't provide any feedback to the user as to why it was disabled. I've replaced it with actual per-input validation using <Input status="error" error={...} /> and updated this condition to disable the button corresponding with either of those input errors being present.

courier-new · 2023-06-07T18:13:59Z

@pjlast @kopancek tagging you just in case you want to review the middleware changes. Don't feel like you need to give an exhaustive review of the batches stuff. 🙂

sourcegraph-bot · 2023-06-07T18:14:39Z

Codenotify: Notifying subscribers in CODENOTIFY files for diff fc8071b...262bc44.

Notify	File(s)
@BolajiOlajide	client/web/src/enterprise/batches/settings/BatchChangesCreateGitHubAppPage.tsx client/web/src/enterprise/batches/settings/BatchChangesSettingsArea.story.tsx client/web/src/enterprise/batches/settings/BatchChangesSiteConfigSettingsPage.story.tsx client/web/src/enterprise/batches/settings/BatchChangesSiteConfigSettingsPage.tsx client/web/src/enterprise/batches/settings/CommitSigningIntegrationNode.tsx client/web/src/enterprise/batches/settings/CommitSigningIntegrations.tsx client/web/src/enterprise/batches/settings/backend.ts
@eseliger	client/web/src/enterprise/batches/settings/BatchChangesCreateGitHubAppPage.tsx client/web/src/enterprise/batches/settings/BatchChangesSettingsArea.story.tsx client/web/src/enterprise/batches/settings/BatchChangesSiteConfigSettingsPage.story.tsx client/web/src/enterprise/batches/settings/BatchChangesSiteConfigSettingsPage.tsx client/web/src/enterprise/batches/settings/CommitSigningIntegrationNode.tsx client/web/src/enterprise/batches/settings/CommitSigningIntegrations.tsx client/web/src/enterprise/batches/settings/backend.ts cmd/frontend/graphqlbackend/batches.go
@unknwon	enterprise/cmd/frontend/internal/auth/githubappauth/BUILD.bazel enterprise/cmd/frontend/internal/auth/githubappauth/middleware.go enterprise/cmd/frontend/internal/auth/githubappauth/middleware_test.go enterprise/cmd/frontend/internal/auth/githubappauth/resolver_test.go

sourcegraph-bot · 2023-06-07T18:36:42Z

📖 Storybook live preview

courier-new · 2023-06-07T18:57:58Z

Hmm, not sure why buildkite is showing test failures. Those tests are passing locally:

And the buildkite output doesn't show why the test failed. 😞

…_test.go

courier-new · 2023-06-08T03:56:18Z

Okay build is finally all green again!! 😌

st0nebreaker

Looks really good!

I tested through the happy path and all is gucci. For the sad paths you featured in your description, is there a clear way to trigger those or did you have to manually change parts of the code/setup to get that? I don't need to test the failure path flow, just curious, and the added tests/flows you tested give me confidence.

st0nebreaker · 2023-06-08T18:01:45Z

+            try {
+                new URL(trimmedURL)
+            } catch {
+                return setUrlError('URL is not valid.')
+            }


Omgosh. Good fix!

st0nebreaker · 2023-06-08T18:36:06Z

+	switch *parsedDomain {
+	case types.ReposGitHubAppDomain:
+		if creationErr != nil {
+			return fmt.Sprintf("/site-admin/github-apps?success=false&error=%s", url.QueryEscape(creationErr.Error()))
+		}
+		if installationID == nil || appID == nil {
+			return fmt.Sprintf("/site-admin/github-apps?success=false&error=%s", url.QueryEscape("missing installation ID or app ID"))
+		}
+
+		return fmt.Sprintf("/site-admin/github-apps/%s?installation_id=%d", MarshalGitHubAppID(int64(*appID)), *installationID)
+	case types.BatchesGitHubAppDomain:
+		if creationErr != nil {
+			return fmt.Sprintf("/site-admin/batch-changes?success=false&error=%s", url.QueryEscape(creationErr.Error()))
+		}
+
+		// This shouldn't really happen unless we also had an error, but we handle it just
+		// in case
+		if appName == nil {
+			return "/site-admin/batch-changes?success=true"
+		}
+		return fmt.Sprintf("/site-admin/batch-changes?success=true&app_name=%s", *appName)
+	default:
+		return fmt.Sprintf("/site-admin/github-apps?success=false&error=%s", url.QueryEscape(fmt.Sprintf("unsupported github apps domain: %v", parsedDomain)))
+	}
+}


Love this redirect logic & passing the success & error info through query params!

courier-new · 2023-06-09T02:34:51Z

For the sad paths you featured in your description, is there a clear way to trigger those or did you have to manually change parts of the code/setup to get that? I don't need to test the failure path flow, just curious, and the added tests/flows you tested give me confidence.

Yeah I was manually forcing failures to happen by changing the middleware code at various points. It's not too annoying if you're interested in playing around with it. If you find the handler for the /setup endpoint:

r.HandleFunc(prefix+"/setup", ...

Anywhere there's an error code path that's normally behind a condition, e.g.

if err != nil {
	redirectURL := generateRedirectURL(nil, nil, nil, nil, errors.New("Bad request, invalid state"))
	http.Redirect(w, req, redirectURL, http.StatusFound)
	return
}

I'd just comment out the condition to test what it would be like to fail at that point:

// if err != nil {
redirectURL := generateRedirectURL(nil, nil, nil, nil, errors.New("Bad request, invalid state"))
http.Redirect(w, req, redirectURL, http.StatusFound)
return
// }

BolajiOlajide

Code-wise, this looks good to me. Thanks for the detailed explanation.

Just need to figure out why I get a nil response from createCommitFromPatch

pjlast

Amazing PR, thanks for all the tweaks and polish and the super detailed code walk through 🙏

courier-new · 2023-06-12T15:28:12Z

~~@st0nebraker I'm going to go ahead and merge this and start rebasing the other ones, but still feel free to leave any additional feedback and I can gladly address it in a follow-up!! 😄~~ NEVERMIND I spoke a second too late, thanks for the review haha!!

courier-new self-assigned this Jun 6, 2023

cla-bot Bot added the cla-signed label Jun 6, 2023

courier-new added batch-changes Issues related to Batch Changes batches-signed-commit labels Jun 6, 2023

sourcegraph-bot-devx mentioned this pull request Jun 6, 2023

Sign commits created by Batch Changes with GitHub App (Tracking) #48611

Closed

31 tasks

courier-new changed the title ~~Make GH App manifest events/permissions prop-configurable~~ batches: create GitHub App UX flow Jun 6, 2023

courier-new force-pushed the kr/ux-create-app-2 branch from 3a0de1d to bb1df55 Compare June 6, 2023 23:21

Base automatically changed from kr/ux-create-app to batches/commit-signing June 7, 2023 01:41

courier-new added 19 commits June 6, 2023 21:37

Make GH App manifest events/permissions prop-configurable

43698a0

BatchChangesSiteConfigSettingsArea -> BatchChangesSiteConfigSettingsPage

2a37f5d

Add more configurable properties to Create page, hide URL field if pr…

677113b

…ovided, clean up text

Clean up validation/error handling,

6cdf35c

Add and customize BC CreateGitHubAppPage route

e98e7fe

Link to new Create page from node

f58a298

Provide GH App domain

bb28d8b

Handle when no webhook URL in state response

855bab0

Show success alert from list page on return

250f717

Surface node ID on GitHubAppConfiguration type

63cad59

Hook up delete button

1b44db4

Update storybook data

f23373c

Bazel configure

4e87515

Cleanup

c198f0a

Update test

bd7ef8b

Surface base URL on GitHubAppConfiguration type

6700991

Query configured GH Apps' base urls, validate that it's unique

a65ecfa

appName param -> app_name, update tests

ec71e29

courier-new force-pushed the kr/ux-create-app-2 branch from f7c74ed to ec71e29 Compare June 7, 2023 04:37

courier-new added 2 commits June 7, 2023 00:20

Improve clientside error handling, add prop comments

c23faec

More graceful middleware error handling

8d7d7aa

courier-new force-pushed the kr/ux-create-app-2 branch from 4c480a6 to 28a3f70 Compare June 7, 2023 18:11

courier-new commented Jun 7, 2023

View reviewed changes

courier-new marked this pull request as ready for review June 7, 2023 18:12

courier-new requested review from a team, kopancek and pjlast June 7, 2023 18:13

BolajiOlajide reviewed Jun 7, 2023

View reviewed changes

Comment thread enterprise/cmd/frontend/internal/auth/githubappauth/middleware_test.go Outdated

BolajiOlajide and others added 4 commits June 7, 2023 21:32

Update enterprise/cmd/frontend/internal/auth/githubappauth/middleware…

44d1cc3

…_test.go

Try indicating test requires network in bazel build file

a9cd7df

AGAIN WITH THE BAZEL

911f2d7

EVEN MORE BAZEL

6e830cf

st0nebreaker reviewed Jun 8, 2023

View reviewed changes

courier-new mentioned this pull request Jun 9, 2023

github app: better "delete App" UX #53203

Merged

courier-new added 3 commits June 8, 2023 19:55

Merge branch 'batches/commit-signing' into kr/ux-create-app-2

12fadf2

Merge branch 'batches/commit-signing' into kr/ux-create-app-2

02c79d5

github app: better "delete App" UX (#53203)

262bc44

This was referenced Jun 9, 2023

batches: support distinct GH App installs for commit signing across namespaces #53212

Merged

github app: toggle app default visibility #53286

Merged

BolajiOlajide approved these changes Jun 10, 2023

View reviewed changes

Comment thread enterprise/cmd/frontend/internal/auth/githubappauth/middleware.go

pjlast approved these changes Jun 12, 2023

View reviewed changes

st0nebreaker approved these changes Jun 12, 2023

View reviewed changes

courier-new merged commit 09b07ba into batches/commit-signing Jun 12, 2023

courier-new deleted the kr/ux-create-app-2 branch June 12, 2023 15:29

Conversation

courier-new commented Jun 6, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Walkthrough

Demo videos

Test plan

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

courier-new commented Jun 7, 2023

Uh oh!

sourcegraph-bot commented Jun 7, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

sourcegraph-bot commented Jun 7, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

courier-new commented Jun 7, 2023

Uh oh!

Uh oh!

courier-new commented Jun 8, 2023

Uh oh!

st0nebreaker left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

courier-new commented Jun 9, 2023

Uh oh!

BolajiOlajide left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

pjlast left a comment

Choose a reason for hiding this comment

Uh oh!

courier-new commented Jun 12, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

courier-new commented Jun 6, 2023 •

edited

Loading

sourcegraph-bot commented Jun 7, 2023 •

edited

Loading

sourcegraph-bot commented Jun 7, 2023 •

edited

Loading

courier-new commented Jun 12, 2023 •

edited

Loading