Skip to content
This repository was archived by the owner on Sep 30, 2024. It is now read-only.

github app: no editing support for "batches" app#53130

Merged
courier-new merged 2 commits into
batches/commit-signingfrom
kr/batches-app-edit-page
Jun 8, 2023
Merged

github app: no editing support for "batches" app#53130
courier-new merged 2 commits into
batches/commit-signingfrom
kr/batches-app-edit-page

Conversation

@courier-new

@courier-new courier-new commented Jun 7, 2023

Copy link
Copy Markdown
Contributor

Closes https://github.com/sourcegraph/sourcegraph/issues/52341.

Tip: Make sure to view the PR diff with whitespace changes hidden. 🙂

There should not be any direct links to this page for Batches domain GitHub Apps, but in case a user were to manually plug in this URL, we want the page contents to reflect that at present, we don't support additional configuration or install management of Batches domain GH Apps. We may decide to revisit this in the future.

image

Test plan

Manually verified that visiting a route for a Batches app appeared as in the screenshot above, and that a Repos app appeared as before.

@courier-new courier-new requested a review from a team June 7, 2023 22:28
@courier-new courier-new self-assigned this Jun 7, 2023
@cla-bot cla-bot Bot added the cla-signed label Jun 7, 2023
@courier-new courier-new force-pushed the kr/batches-app-edit-page branch from bb05281 to f5446cc Compare June 7, 2023 22:33
@courier-new courier-new force-pushed the kr/batches-app-edit-page branch from f5446cc to 9a35ccb Compare June 7, 2023 22:39
@sourcegraph sourcegraph deleted a comment from sourcegraph-bot Jun 7, 2023
@sourcegraph-bot

Copy link
Copy Markdown
Contributor

📖 Storybook live preview

@st0nebreaker st0nebreaker left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM!

@BolajiOlajide BolajiOlajide left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to me.
Added a comment about code readability but I'm not sure my suggestion is even better, feel free to ignore.

<AuthProviderMessage app={app} id={appID} />
</>
)}
{!app ? null : app.domain !== GitHubAppDomain.REPOS ? (

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This was a bit hard for me to understand at first glance. Is there a way we can simplify it?

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I was thinking something like what I have below:

{app && app.domain !== GitHubAppDomain.REPOS && ()}
{app && app.domain !== GitHubAppDomain.BATCHES && ()}

But at another glance, I don't even know if it's better. Feel free to ignore this.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Haha I actually had it this way at first, too, but I couldn't decide what felt better. 😂 I'll leave it for now but I think we'll be able to clean this up a bit when we built multi-org installations support.

@courier-new courier-new merged commit f6e06be into batches/commit-signing Jun 8, 2023
@courier-new courier-new deleted the kr/batches-app-edit-page branch June 8, 2023 21:04
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants