Summary

Add connected service account "needs attention" and track GH App state.

Review Request #15197 — Created July 24, 2026 and updated Aug. 16, 2026, 2:33 p.m.

Information

Owner

david

Repository

Review Board

Branch

release-9.x

Bugs

Depends On

Reviewers

Groups

reviewboard

People

Description

GitHub Apps can be uninstalled or suspended from the GitHub side, which
can cause our integration to start failing. This change makes us handle
that in a nice way. This involves several pieces:

Connected Services "needs attention"

Accounts can now indicate that they need attention in some way, and
provide actions to handle this.

This is now implemented for app-based accounts with GitHub in order to
allow the admin to reestablish the correction.

Later work will involve using this for other hosting services (and
PAT-based GitHub connections) to have accounts indicate when their
credentials need to be updated.

GitHub App webhook handler

The stub webhook handler has been fleshed out to verify the signature
header and handle several events:

deleted and suspend mark the installation account with the state
so that they'll be flagged in the connected services list.
created can heal a reinstall by refreshing the stored installation
ID, matched to the existing account. This allows a reinstall performed
entirely from the GitHub side to restore the connection without
requiring anything to happen inside Review Board.
installation_repositories keeps the repository_selection flag up
to date.
installation_target updates the stored account name.

API request error handling

In the case of a missed webhook delivery, we can still end up with the
stored state being wrong (in either direction). API requests have been
updated to handle this in two ways:

Token minting will map a 403 or 404 response to the
suspended/uninstalled state (checking first to ensure that it's not
some other ephemeral failure or rate limit error). If we got the error
because the app had been uninstalled but is now reinstalled, we'll
adopt the new installation ID.
403 or 404 errors for cached installation tokens will drop the token
and try again (limited to once per minute).
If we have a stored non-active status, we'll recheck that state (also
limited to once per minute) before refusing to mint a new token.

GitHub reconnect

The reconnect action for suspended/uninstalled accounts will first try
to verify that state (in case of a missed webhook delivery), and if it's
still an issue, will deep link the user to the relevant page where they
can fix it.

Testing Done


Ran unit tests.
Suspended and deleted app installs and saw that the state was properly

  reflected in the connected services list.
Reconnected suspended and deleted app installs and saw that state

  healed correctly.

Commits

Summary	ID
Add connected service account "needs attention" and track GH App state. GitHub Apps can be uninstalled or suspended from the GitHub side, which can cause our integration to start failing. This change makes us handle that in a nice way. This involves several pieces: # Connected Services "needs attention" Accounts can now indicate that they need attention in some way, and provide actions to handle this. This is now implemented for app-based accounts with GitHub in order to allow the admin to reestablish the correction. Later work will involve using this for other hosting services (and PAT-based GitHub connections) to have accounts indicate when their credentials need to be updated. # GitHub App webhook handler The stub webhook handler has been fleshed out to verify the signature header and handle several events: - `deleted` and `suspend` mark the installation account with the state so that they'll be flagged in the connected services list. - `created` can heal a reinstall by refreshing the stored installation ID, matched to the existing account. This allows a reinstall performed entirely from the GitHub side to restore the connection without requiring anything to happen inside Review Board. - `installation_repositories` keeps the `repository_selection` flag up to date. - `installation_target` updates the stored account name. # API request error handling In the case of a missed webhook delivery, we can still end up with the stored state being wrong (in either direction). API requests have been updated to handle this in two ways: - Token minting will map a 403 or 404 response to the suspended/uninstalled state (checking first to ensure that it's not some other ephemeral failure or rate limit error). If we got the error because the app had been uninstalled but is now reinstalled, we'll adopt the new installation ID. - 403 or 404 errors for cached installation tokens will drop the token and try again (limited to once per minute). - If we have a stored non-active status, we'll recheck that state (also limited to once per minute) before refusing to mint a new token. # GitHub reconnect The reconnect action for suspended/uninstalled accounts will first try to verify that state (in case of a missed webhook delivery), and if it's still an issue, will deep link the user to the relevant page where they can fix it. Testing Done: - Ran unit tests. - Suspended and deleted app installs and saw that the state was properly reflected in the connected services list. - Reconnected suspended and deleted app installs and saw that state healed correctly.	ummslvyqxyoqyzlozxkmksvtrxvnrpwn

Summary

Add connected service account "needs attention" and track GH App state.

GitHub Apps can be uninstalled or suspended from the GitHub side, which can cause our integration to start failing. This change makes us handle that in a nice way. This involves several pieces: # Connected Services "needs attention" Accounts can now indicate that they need attention in some way, and provide actions to handle this. This is now implemented for app-based accounts with GitHub in order to allow the admin to reestablish the correction. Later work will involve using this for other hosting services (and PAT-based GitHub connections) to have accounts indicate when their credentials need to be updated. # GitHub App webhook handler The stub webhook handler has been fleshed out to verify the signature header and handle several events: - `deleted` and `suspend` mark the installation account with the state so that they'll be flagged in the connected services list. - `created` can heal a reinstall by refreshing the stored installation ID, matched to the existing account. This allows a reinstall performed entirely from the GitHub side to restore the connection without requiring anything to happen inside Review Board. - `installation_repositories` keeps the `repository_selection` flag up to date. - `installation_target` updates the stored account name. # API request error handling In the case of a missed webhook delivery, we can still end up with the stored state being wrong (in either direction). API requests have been updated to handle this in two ways: - Token minting will map a 403 or 404 response to the suspended/uninstalled state (checking first to ensure that it's not some other ephemeral failure or rate limit error). If we got the error because the app had been uninstalled but is now reinstalled, we'll adopt the new installation ID. - 403 or 404 errors for cached installation tokens will drop the token and try again (limited to once per minute). - If we have a stored non-active status, we'll recheck that state (also limited to once per minute) before refusing to mint a new token. # GitHub reconnect The reconnect action for suspended/uninstalled accounts will first try to verify that state (in case of a missed webhook delivery), and if it's still an issue, will deep link the user to the relevant page where they can fix it. Testing Done: - Ran unit tests. - Suspended and deleted app installs and saw that the state was properly reflected in the connected services list. - Reconnected suspended and deleted app installs and saw that state healed correctly.

ummslvyqxyoqyzlozxkmksvtrxvnrpwn

Files

Issues

Description	From	Last Updated
line too long (80 > 79 characters) Column: 80 Error code: E501	reviewbot	July 24, 2026, 4:02 p.m.
line too long (83 > 79 characters) Column: 80 Error code: E501	reviewbot	July 24, 2026, 4:02 p.m.
Can probably get rid of this.	maubin	July 31, 2026, 8:25 a.m.
Would it be better to change the return type to Literal['active', 'suspended', 'removed', '']?	maubin	July 31, 2026, 8:29 a.m.
Would it be better to change the return type to Literal['active', 'suspended', 'removed', '']?	maubin	July 31, 2026, 8:29 a.m.
Can we use _get_github_urls()['app_base'] here instead?	maubin	July 31, 2026, 9:18 a.m.
We may want to do local_site=request.local_site here.	maubin	July 31, 2026, 8:43 a.m.
Let's pull out account.login into a variable since we use it a lot below.	maubin	July 31, 2026, 8:46 a.m.
redefinition of unused 'GitHubAppInstallationData' from line 36 Column: 5 Error code: F811	reviewbot	July 31, 2026, 9:21 a.m.
We generally keep TypedDict fields in alphabetical order (in required/not-required groups).	chipx86	Aug. 11, 2026, 8:10 a.m.
Small nit, but can you move the example to a new paragraph so it stands out better from the description?	chipx86	Aug. 11, 2026, 8:10 a.m.
These are missing "Version Added". Also to check, what's the benefit to using strings here over an enum?	chipx86	Aug. 11, 2026, 8:22 a.m.
Can we alphabetize these?	chipx86	Aug. 11, 2026, 8:22 a.m.
Can this be InstallationStatus?	chipx86	Aug. 11, 2026, 8:22 a.m.
No parens here.	chipx86	Aug. 11, 2026, 8:23 a.m.
AI smell. We never use this.	chipx86	Aug. 11, 2026, 8:23 a.m.
This should be pulled out into a variable.	chipx86	Aug. 11, 2026, 8:23 a.m.
Blank line required between these.	chipx86	Aug. 11, 2026, 8:24 a.m.
We should stick with the standard grouping of required, optional. And alphabetical within that. Same as a TypedDict.	chipx86	Aug. 11, 2026, 8:24 a.m.
Summary must be one line.	chipx86	Aug. 11, 2026, 8:25 a.m.
These should be in alphabetical order.	chipx86	Aug. 11, 2026, 8:25 a.m.
Missing parens.	chipx86	Aug. 11, 2026, 8:25 a.m.
Missing parens, and should be in alphabetical order.	chipx86	Aug. 11, 2026, 8:25 a.m.
The )) for each string should be on its own line, so the string is self-contained.	chipx86	Aug. 11, 2026, 8:26 a.m.
No parens needed here.	chipx86	Aug. 11, 2026, 8:26 a.m.
Looks like we can just do a simple if and return [reconnect_item] or [].	chipx86	Aug. 11, 2026, 8:37 a.m.
Can this be a Sequence?	chipx86	Aug. 11, 2026, 8:38 a.m.
This would fit on one line.	chipx86	Aug. 11, 2026, 8:38 a.m.
Can you update this for alphabetical order?	chipx86	Aug. 11, 2026, 8:38 a.m.
Should we check explicitly for the removed state? If we add a new state for any reason in the future, …	chipx86	Aug. 11, 2026, 8:39 a.m.
To be safe, we should escape arguments going into the URL.	chipx86	Aug. 11, 2026, 8:40 a.m.
To be safe, we should escape values going into URLs.	chipx86	Aug. 11, 2026, 8:41 a.m.
If this can't be imported top-level, can we import at the top of the function like we typically do?	chipx86	Aug. 16, 2026, 2:25 p.m.
Can you pull this out into a variable?	chipx86	Aug. 11, 2026, 8:41 a.m.
These are missing docs.	chipx86	Aug. 11, 2026, 8:43 a.m.
Can you pull this out into a variable? Helps with readability/maintainability.	chipx86	Aug. 11, 2026, 8:43 a.m.
SyntaxError: invalid decimal literal Column: 20 Error code: E999	reviewbot	Aug. 16, 2026, 2:33 p.m.
SyntaxError: invalid decimal literal Column: 20 Error code: E999	reviewbot	Aug. 16, 2026, 2:33 p.m.

flake8 failed.

JSHint passed.

flake8

reviewboard/admin/views.py (Diff revision 1)
The issue has been dropped. Show all issues
```
line too long (80 > 79 characters)

Column: 80
Error code: E501
```
reviewboard/admin/views.py (Diff revision 1)
The issue has been dropped. Show all issues
```
line too long (83 > 79 characters)

Column: 80
Error code: E501
```

Commits:

	Summary	ID
	Add connected service account "needs attention" and track GH App state. GitHub Apps can be uninstalled or suspended from the GitHub side, which can cause our integration to start failing. This change makes us handle that in a nice way. This involves several pieces: # Connected Services "needs attention" Accounts can now indicate that they need attention in some way, and provide actions to handle this. This is now implemented for app-based accounts with GitHub in order to allow the admin to reestablish the correction. Later work will involve using this for other hosting services (and PAT-based GitHub connections) to have accounts indicate when their credentials need to be updated. # GitHub App webhook handler The stub webhook handler has been fleshed out to verify the signature header and handle several events: - `deleted` and `suspend` mark the installation account with the state so that they'll be flagged in the connected services list. - `created` can heal a reinstall by refreshing the stored installation ID, matched to the existing account. This allows a reinstall performed entirely from the GitHub side to restore the connection without requiring anything to happen inside Review Board. - `installation_repositories` keeps the `repository_selection` flag up to date. - `installation_target` updates the stored account name. # API request error handling In the case of a missed webhook delivery, we can still end up with the stored state being wrong (in either direction). API requests have been updated to handle this in two ways: - Token minting will map a 403 or 404 response to the suspended/uninstalled state (checking first to ensure that it's not some other ephemeral failure or rate limit error). If we got the error because the app had been uninstalled but is now reinstalled, we'll adopt the new installation ID. - 403 or 404 errors for cached installation tokens will drop the token and try again (limited to once per minute). - If we have a stored non-active status, we'll recheck that state (also limited to once per minute) before refusing to mint a new token. # GitHub reconnect The reconnect action for suspended/uninstalled accounts will first try to verify that state (in case of a missed webhook delivery), and if it's still an issue, will deep link the user to the relevant page where they can fix it. Testing Done: - Ran unit tests. - Suspended and deleted app installs and saw that the state was properly reflected in the connected services list. - Reconnected suspended and deleted app installs and saw that state healed correctly.	ummslvyqxyoqyzlozxkmksvtrxvnrpwn
	Add connected service account "needs attention" and track GH App state. GitHub Apps can be uninstalled or suspended from the GitHub side, which can cause our integration to start failing. This change makes us handle that in a nice way. This involves several pieces: # Connected Services "needs attention" Accounts can now indicate that they need attention in some way, and provide actions to handle this. This is now implemented for app-based accounts with GitHub in order to allow the admin to reestablish the correction. Later work will involve using this for other hosting services (and PAT-based GitHub connections) to have accounts indicate when their credentials need to be updated. # GitHub App webhook handler The stub webhook handler has been fleshed out to verify the signature header and handle several events: - `deleted` and `suspend` mark the installation account with the state so that they'll be flagged in the connected services list. - `created` can heal a reinstall by refreshing the stored installation ID, matched to the existing account. This allows a reinstall performed entirely from the GitHub side to restore the connection without requiring anything to happen inside Review Board. - `installation_repositories` keeps the `repository_selection` flag up to date. - `installation_target` updates the stored account name. # API request error handling In the case of a missed webhook delivery, we can still end up with the stored state being wrong (in either direction). API requests have been updated to handle this in two ways: - Token minting will map a 403 or 404 response to the suspended/uninstalled state (checking first to ensure that it's not some other ephemeral failure or rate limit error). If we got the error because the app had been uninstalled but is now reinstalled, we'll adopt the new installation ID. - 403 or 404 errors for cached installation tokens will drop the token and try again (limited to once per minute). - If we have a stored non-active status, we'll recheck that state (also limited to once per minute) before refusing to mint a new token. # GitHub reconnect The reconnect action for suspended/uninstalled accounts will first try to verify that state (in case of a missed webhook delivery), and if it's still an issue, will deep link the user to the relevant page where they can fix it. Testing Done: - Ran unit tests. - Suspended and deleted app installs and saw that the state was properly reflected in the connected services list. - Reconnected suspended and deleted app installs and saw that state healed correctly.	ummslvyqxyoqyzlozxkmksvtrxvnrpwn

Diff:

Revision 2 (+5448 -340)

Show changes

	reviewboard/admin/views.py
	reviewboard/admin/tests/test_connected_services_list_view.py
	reviewboard/hostingsvcs/base/hosting_service.py
	reviewboard/hostingsvcs/github/accounts.py
	reviewboard/hostingsvcs/github/api.py
	reviewboard/hostingsvcs/github/client.py
	reviewboard/hostingsvcs/github/service.py
	reviewboard/hostingsvcs/github/views.py
	8 more

Checks run (2 succeeded)

flake8 passed.

JSHint passed.

reviewboard/admin/views.py (Diff revision 2)

The issue has been dropped. Show all issues

Can probably get rid of this.

david

July 31, 2026, 9:19 a.m.

I'm not sure what you mean? This is the format that we need for tuples in return values to parse out.

maubin

Aug. 3, 2026, 1:10 a.m.
```
Oh oops.
```

reviewboard/hostingsvcs/github/client.py (Diff revision 2)

The issue has been resolved. Show all issues

Would it be better to change the return type to Literal['active', 'suspended', 'removed', '']?

reviewboard/hostingsvcs/github/client.py (Diff revision 2)

The issue has been resolved. Show all issues

Would it be better to change the return type to Literal['active', 'suspended', 'removed', '']?

reviewboard/hostingsvcs/github/service.py (Diff revision 2)

The issue has been resolved. Show all issues

Can we use _get_github_urls()['app_base'] here instead?

david

July 31, 2026, 9:19 a.m.

I'll have to move it to a shared place (it's currently a staticmethod on the view base) but sure.

reviewboard/hostingsvcs/github/views.py (Diff revision 2)
The issue has been resolved. Show all issues
```
We may want to do local_site=request.local_site here.
```
reviewboard/hostingsvcs/github/views.py (Diff revision 2)
The issue has been resolved. Show all issues
```
Let's pull out account.login into a variable since we use it a lot below.
```

Commits:

	Summary	ID
	Add connected service account "needs attention" and track GH App state. GitHub Apps can be uninstalled or suspended from the GitHub side, which can cause our integration to start failing. This change makes us handle that in a nice way. This involves several pieces: # Connected Services "needs attention" Accounts can now indicate that they need attention in some way, and provide actions to handle this. This is now implemented for app-based accounts with GitHub in order to allow the admin to reestablish the correction. Later work will involve using this for other hosting services (and PAT-based GitHub connections) to have accounts indicate when their credentials need to be updated. # GitHub App webhook handler The stub webhook handler has been fleshed out to verify the signature header and handle several events: - `deleted` and `suspend` mark the installation account with the state so that they'll be flagged in the connected services list. - `created` can heal a reinstall by refreshing the stored installation ID, matched to the existing account. This allows a reinstall performed entirely from the GitHub side to restore the connection without requiring anything to happen inside Review Board. - `installation_repositories` keeps the `repository_selection` flag up to date. - `installation_target` updates the stored account name. # API request error handling In the case of a missed webhook delivery, we can still end up with the stored state being wrong (in either direction). API requests have been updated to handle this in two ways: - Token minting will map a 403 or 404 response to the suspended/uninstalled state (checking first to ensure that it's not some other ephemeral failure or rate limit error). If we got the error because the app had been uninstalled but is now reinstalled, we'll adopt the new installation ID. - 403 or 404 errors for cached installation tokens will drop the token and try again (limited to once per minute). - If we have a stored non-active status, we'll recheck that state (also limited to once per minute) before refusing to mint a new token. # GitHub reconnect The reconnect action for suspended/uninstalled accounts will first try to verify that state (in case of a missed webhook delivery), and if it's still an issue, will deep link the user to the relevant page where they can fix it. Testing Done: - Ran unit tests. - Suspended and deleted app installs and saw that the state was properly reflected in the connected services list. - Reconnected suspended and deleted app installs and saw that state healed correctly.	ummslvyqxyoqyzlozxkmksvtrxvnrpwn
	Add connected service account "needs attention" and track GH App state. GitHub Apps can be uninstalled or suspended from the GitHub side, which can cause our integration to start failing. This change makes us handle that in a nice way. This involves several pieces: # Connected Services "needs attention" Accounts can now indicate that they need attention in some way, and provide actions to handle this. This is now implemented for app-based accounts with GitHub in order to allow the admin to reestablish the correction. Later work will involve using this for other hosting services (and PAT-based GitHub connections) to have accounts indicate when their credentials need to be updated. # GitHub App webhook handler The stub webhook handler has been fleshed out to verify the signature header and handle several events: - `deleted` and `suspend` mark the installation account with the state so that they'll be flagged in the connected services list. - `created` can heal a reinstall by refreshing the stored installation ID, matched to the existing account. This allows a reinstall performed entirely from the GitHub side to restore the connection without requiring anything to happen inside Review Board. - `installation_repositories` keeps the `repository_selection` flag up to date. - `installation_target` updates the stored account name. # API request error handling In the case of a missed webhook delivery, we can still end up with the stored state being wrong (in either direction). API requests have been updated to handle this in two ways: - Token minting will map a 403 or 404 response to the suspended/uninstalled state (checking first to ensure that it's not some other ephemeral failure or rate limit error). If we got the error because the app had been uninstalled but is now reinstalled, we'll adopt the new installation ID. - 403 or 404 errors for cached installation tokens will drop the token and try again (limited to once per minute). - If we have a stored non-active status, we'll recheck that state (also limited to once per minute) before refusing to mint a new token. # GitHub reconnect The reconnect action for suspended/uninstalled accounts will first try to verify that state (in case of a missed webhook delivery), and if it's still an issue, will deep link the user to the relevant page where they can fix it. Testing Done: - Ran unit tests. - Suspended and deleted app installs and saw that the state was properly reflected in the connected services list. - Reconnected suspended and deleted app installs and saw that state healed correctly.	ummslvyqxyoqyzlozxkmksvtrxvnrpwn

Diff:

Revision 3 (+5546 -412)

Show changes

	reviewboard/admin/views.py
	reviewboard/admin/tests/test_connected_services_list_view.py
	reviewboard/hostingsvcs/base/hosting_service.py
	reviewboard/hostingsvcs/github/accounts.py
	reviewboard/hostingsvcs/github/api.py
	reviewboard/hostingsvcs/github/client.py
	reviewboard/hostingsvcs/github/service.py
	reviewboard/hostingsvcs/github/views.py
	8 more

Checks run (1 failed, 1 succeeded)

flake8 failed.

JSHint passed.

flake8

reviewboard/hostingsvcs/github/client.py (Diff revision 3)

The issue has been dropped. Show all issues

redefinition of unused 'GitHubAppInstallationData' from line 36

Column: 5
Error code: F811

Commits:

	Summary	ID
	Add connected service account "needs attention" and track GH App state. GitHub Apps can be uninstalled or suspended from the GitHub side, which can cause our integration to start failing. This change makes us handle that in a nice way. This involves several pieces: # Connected Services "needs attention" Accounts can now indicate that they need attention in some way, and provide actions to handle this. This is now implemented for app-based accounts with GitHub in order to allow the admin to reestablish the correction. Later work will involve using this for other hosting services (and PAT-based GitHub connections) to have accounts indicate when their credentials need to be updated. # GitHub App webhook handler The stub webhook handler has been fleshed out to verify the signature header and handle several events: - `deleted` and `suspend` mark the installation account with the state so that they'll be flagged in the connected services list. - `created` can heal a reinstall by refreshing the stored installation ID, matched to the existing account. This allows a reinstall performed entirely from the GitHub side to restore the connection without requiring anything to happen inside Review Board. - `installation_repositories` keeps the `repository_selection` flag up to date. - `installation_target` updates the stored account name. # API request error handling In the case of a missed webhook delivery, we can still end up with the stored state being wrong (in either direction). API requests have been updated to handle this in two ways: - Token minting will map a 403 or 404 response to the suspended/uninstalled state (checking first to ensure that it's not some other ephemeral failure or rate limit error). If we got the error because the app had been uninstalled but is now reinstalled, we'll adopt the new installation ID. - 403 or 404 errors for cached installation tokens will drop the token and try again (limited to once per minute). - If we have a stored non-active status, we'll recheck that state (also limited to once per minute) before refusing to mint a new token. # GitHub reconnect The reconnect action for suspended/uninstalled accounts will first try to verify that state (in case of a missed webhook delivery), and if it's still an issue, will deep link the user to the relevant page where they can fix it. Testing Done: - Ran unit tests. - Suspended and deleted app installs and saw that the state was properly reflected in the connected services list. - Reconnected suspended and deleted app installs and saw that state healed correctly.	ummslvyqxyoqyzlozxkmksvtrxvnrpwn
	Add connected service account "needs attention" and track GH App state. GitHub Apps can be uninstalled or suspended from the GitHub side, which can cause our integration to start failing. This change makes us handle that in a nice way. This involves several pieces: # Connected Services "needs attention" Accounts can now indicate that they need attention in some way, and provide actions to handle this. This is now implemented for app-based accounts with GitHub in order to allow the admin to reestablish the correction. Later work will involve using this for other hosting services (and PAT-based GitHub connections) to have accounts indicate when their credentials need to be updated. # GitHub App webhook handler The stub webhook handler has been fleshed out to verify the signature header and handle several events: - `deleted` and `suspend` mark the installation account with the state so that they'll be flagged in the connected services list. - `created` can heal a reinstall by refreshing the stored installation ID, matched to the existing account. This allows a reinstall performed entirely from the GitHub side to restore the connection without requiring anything to happen inside Review Board. - `installation_repositories` keeps the `repository_selection` flag up to date. - `installation_target` updates the stored account name. # API request error handling In the case of a missed webhook delivery, we can still end up with the stored state being wrong (in either direction). API requests have been updated to handle this in two ways: - Token minting will map a 403 or 404 response to the suspended/uninstalled state (checking first to ensure that it's not some other ephemeral failure or rate limit error). If we got the error because the app had been uninstalled but is now reinstalled, we'll adopt the new installation ID. - 403 or 404 errors for cached installation tokens will drop the token and try again (limited to once per minute). - If we have a stored non-active status, we'll recheck that state (also limited to once per minute) before refusing to mint a new token. # GitHub reconnect The reconnect action for suspended/uninstalled accounts will first try to verify that state (in case of a missed webhook delivery), and if it's still an issue, will deep link the user to the relevant page where they can fix it. Testing Done: - Ran unit tests. - Suspended and deleted app installs and saw that the state was properly reflected in the connected services list. - Reconnected suspended and deleted app installs and saw that state healed correctly.	ummslvyqxyoqyzlozxkmksvtrxvnrpwn

Diff:

Revision 4 (+5544 -412)

Show changes

	reviewboard/admin/views.py
	reviewboard/admin/tests/test_connected_services_list_view.py
	reviewboard/hostingsvcs/base/hosting_service.py
	reviewboard/hostingsvcs/github/accounts.py
	reviewboard/hostingsvcs/github/api.py
	reviewboard/hostingsvcs/github/client.py
	reviewboard/hostingsvcs/github/service.py
	reviewboard/hostingsvcs/github/views.py
	8 more

Checks run (2 succeeded)

flake8 passed.

JSHint passed.

Ship it!

reviewboard/hostingsvcs/base/hosting_service.py (Diff revision 4)

The issue has been resolved. Show all issues

We generally keep TypedDict fields in alphabetical order (in required/not-required groups).

reviewboard/hostingsvcs/base/hosting_service.py (Diff revision 4)

The issue has been resolved. Show all issues

Small nit, but can you move the example to a new paragraph so it stands out better from the description?

reviewboard/hostingsvcs/github/accounts.py (Diff revision 4)

The issue has been resolved. Show all issues

These are missing "Version Added".

Also to check, what's the benefit to using strings here over an enum?

reviewboard/hostingsvcs/github/accounts.py (Diff revision 4)
The issue has been dropped. Show all issues
```
Can we alphabetize these?
```
reviewboard/hostingsvcs/github/accounts.py (Diff revision 4)
The issue has been resolved. Show all issues
```
Can this be InstallationStatus?
```
reviewboard/hostingsvcs/github/accounts.py (Diff revision 4)
The issue has been resolved. Show all issues
```
No parens here.
```
reviewboard/hostingsvcs/github/accounts.py (Diff revision 4)
The issue has been resolved. Show all issues
```
AI smell. We never use this.
```
reviewboard/hostingsvcs/github/accounts.py (Diff revision 4)
The issue has been resolved. Show all issues
```
This should be pulled out into a variable.
```
reviewboard/hostingsvcs/github/api.py (Diff revision 4)
The issue has been resolved. Show all issues
```
Blank line required between these.
```

reviewboard/hostingsvcs/github/api.py (Diff revision 4)

The issue has been resolved. Show all issues

We should stick with the standard grouping of required, optional. And alphabetical within that. Same as a TypedDict.

reviewboard/hostingsvcs/github/api.py (Diff revision 4)
The issue has been resolved. Show all issues
```
Summary must be one line.
```
reviewboard/hostingsvcs/github/api.py (Diff revision 4)
The issue has been resolved. Show all issues
```
These should be in alphabetical order.
```
reviewboard/hostingsvcs/github/api.py (Diff revision 4)
The issue has been resolved. Show all issues
```
Missing parens.
```

reviewboard/hostingsvcs/github/api.py (Diff revision 4)

The issue has been resolved. Show all issues

Missing parens, and should be in alphabetical order.

reviewboard/hostingsvcs/github/client.py (Diff revision 4)

The issue has been resolved. Show all issues

The )) for each string should be on its own line, so the string is self-contained.

reviewboard/hostingsvcs/github/client.py (Diff revision 4)
The issue has been resolved. Show all issues
```
No parens needed here.
```

reviewboard/hostingsvcs/github/service.py (Diff revision 4)

The issue has been resolved. Show all issues

Looks like we can just do a simple if and return [reconnect_item] or [].

reviewboard/hostingsvcs/github/service.py (Diff revision 4)
The issue has been resolved. Show all issues
```
Can this be a Sequence?
```
reviewboard/hostingsvcs/github/service.py (Diff revision 4)
The issue has been resolved. Show all issues
```
This would fit on one line.
```
reviewboard/hostingsvcs/github/service.py (Diff revision 4)
The issue has been resolved. Show all issues
```
Can you update this for alphabetical order?
```

reviewboard/hostingsvcs/github/service.py (Diff revision 4)

The issue has been resolved. Show all issues

Should we check explicitly for the removed state? If we add a new state for any reason in the future, this may produce the wrong result.

reviewboard/hostingsvcs/github/service.py (Diff revision 4)
The issue has been resolved. Show all issues
```
To be safe, we should escape arguments going into the URL.
```
reviewboard/hostingsvcs/github/views.py (Diff revision 4)
The issue has been resolved. Show all issues
```
To be safe, we should escape values going into URLs.
```

reviewboard/hostingsvcs/github/views.py (Diff revision 4)

The issue has been resolved. Show all issues

If this can't be imported top-level, can we import at the top of the function like we typically do?

reviewboard/hostingsvcs/github/views.py (Diff revision 4)
The issue has been resolved. Show all issues
```
Can you pull this out into a variable?
```

reviewboard/static/rb/css/pages/admin/connected-services.less (Diff revision 4)

The issue has been resolved. Show all issues

These are missing docs.

reviewboard/static/rb/js/admin/views/connectedServicesView.ts (Diff revision 4)
The issue has been resolved. Show all issues
```
Can you pull this out into a variable? Helps with readability/maintainability.
```

Commits:

	Summary	ID
	Add connected service account "needs attention" and track GH App state. GitHub Apps can be uninstalled or suspended from the GitHub side, which can cause our integration to start failing. This change makes us handle that in a nice way. This involves several pieces: # Connected Services "needs attention" Accounts can now indicate that they need attention in some way, and provide actions to handle this. This is now implemented for app-based accounts with GitHub in order to allow the admin to reestablish the correction. Later work will involve using this for other hosting services (and PAT-based GitHub connections) to have accounts indicate when their credentials need to be updated. # GitHub App webhook handler The stub webhook handler has been fleshed out to verify the signature header and handle several events: - `deleted` and `suspend` mark the installation account with the state so that they'll be flagged in the connected services list. - `created` can heal a reinstall by refreshing the stored installation ID, matched to the existing account. This allows a reinstall performed entirely from the GitHub side to restore the connection without requiring anything to happen inside Review Board. - `installation_repositories` keeps the `repository_selection` flag up to date. - `installation_target` updates the stored account name. # API request error handling In the case of a missed webhook delivery, we can still end up with the stored state being wrong (in either direction). API requests have been updated to handle this in two ways: - Token minting will map a 403 or 404 response to the suspended/uninstalled state (checking first to ensure that it's not some other ephemeral failure or rate limit error). If we got the error because the app had been uninstalled but is now reinstalled, we'll adopt the new installation ID. - 403 or 404 errors for cached installation tokens will drop the token and try again (limited to once per minute). - If we have a stored non-active status, we'll recheck that state (also limited to once per minute) before refusing to mint a new token. # GitHub reconnect The reconnect action for suspended/uninstalled accounts will first try to verify that state (in case of a missed webhook delivery), and if it's still an issue, will deep link the user to the relevant page where they can fix it. Testing Done: - Ran unit tests. - Suspended and deleted app installs and saw that the state was properly reflected in the connected services list. - Reconnected suspended and deleted app installs and saw that state healed correctly.	ummslvyqxyoqyzlozxkmksvtrxvnrpwn
	Add connected service account "needs attention" and track GH App state. GitHub Apps can be uninstalled or suspended from the GitHub side, which can cause our integration to start failing. This change makes us handle that in a nice way. This involves several pieces: # Connected Services "needs attention" Accounts can now indicate that they need attention in some way, and provide actions to handle this. This is now implemented for app-based accounts with GitHub in order to allow the admin to reestablish the correction. Later work will involve using this for other hosting services (and PAT-based GitHub connections) to have accounts indicate when their credentials need to be updated. # GitHub App webhook handler The stub webhook handler has been fleshed out to verify the signature header and handle several events: - `deleted` and `suspend` mark the installation account with the state so that they'll be flagged in the connected services list. - `created` can heal a reinstall by refreshing the stored installation ID, matched to the existing account. This allows a reinstall performed entirely from the GitHub side to restore the connection without requiring anything to happen inside Review Board. - `installation_repositories` keeps the `repository_selection` flag up to date. - `installation_target` updates the stored account name. # API request error handling In the case of a missed webhook delivery, we can still end up with the stored state being wrong (in either direction). API requests have been updated to handle this in two ways: - Token minting will map a 403 or 404 response to the suspended/uninstalled state (checking first to ensure that it's not some other ephemeral failure or rate limit error). If we got the error because the app had been uninstalled but is now reinstalled, we'll adopt the new installation ID. - 403 or 404 errors for cached installation tokens will drop the token and try again (limited to once per minute). - If we have a stored non-active status, we'll recheck that state (also limited to once per minute) before refusing to mint a new token. # GitHub reconnect The reconnect action for suspended/uninstalled accounts will first try to verify that state (in case of a missed webhook delivery), and if it's still an issue, will deep link the user to the relevant page where they can fix it. Testing Done: - Ran unit tests. - Suspended and deleted app installs and saw that the state was properly reflected in the connected services list. - Reconnected suspended and deleted app installs and saw that state healed correctly.	ummslvyqxyoqyzlozxkmksvtrxvnrpwn

Diff:

Revision 5 (+5640 -420)

Show changes

	reviewboard/admin/views.py
	reviewboard/admin/tests/test_connected_services_list_view.py
	reviewboard/hostingsvcs/base/connect_ui.py
	reviewboard/hostingsvcs/github/accounts.py
	reviewboard/hostingsvcs/github/api.py
	reviewboard/hostingsvcs/github/client.py
	reviewboard/hostingsvcs/github/service.py
	reviewboard/hostingsvcs/github/views.py
	8 more

Checks run (1 failed, 1 succeeded)

flake8 failed.

JSHint passed.

flake8

reviewboard/admin/views.py (Diff revision 5)
The issue has been dropped. Show all issues
```
SyntaxError: invalid decimal literal

Column: 20
Error code: E999
```
reviewboard/hostingsvcs/base/connect_ui.py (Diff revision 5)
The issue has been dropped. Show all issues
```
SyntaxError: invalid decimal literal

Column: 20
Error code: E999
```

Commits:

	Summary	ID
	Add connected service account "needs attention" and track GH App state. GitHub Apps can be uninstalled or suspended from the GitHub side, which can cause our integration to start failing. This change makes us handle that in a nice way. This involves several pieces: # Connected Services "needs attention" Accounts can now indicate that they need attention in some way, and provide actions to handle this. This is now implemented for app-based accounts with GitHub in order to allow the admin to reestablish the correction. Later work will involve using this for other hosting services (and PAT-based GitHub connections) to have accounts indicate when their credentials need to be updated. # GitHub App webhook handler The stub webhook handler has been fleshed out to verify the signature header and handle several events: - `deleted` and `suspend` mark the installation account with the state so that they'll be flagged in the connected services list. - `created` can heal a reinstall by refreshing the stored installation ID, matched to the existing account. This allows a reinstall performed entirely from the GitHub side to restore the connection without requiring anything to happen inside Review Board. - `installation_repositories` keeps the `repository_selection` flag up to date. - `installation_target` updates the stored account name. # API request error handling In the case of a missed webhook delivery, we can still end up with the stored state being wrong (in either direction). API requests have been updated to handle this in two ways: - Token minting will map a 403 or 404 response to the suspended/uninstalled state (checking first to ensure that it's not some other ephemeral failure or rate limit error). If we got the error because the app had been uninstalled but is now reinstalled, we'll adopt the new installation ID. - 403 or 404 errors for cached installation tokens will drop the token and try again (limited to once per minute). - If we have a stored non-active status, we'll recheck that state (also limited to once per minute) before refusing to mint a new token. # GitHub reconnect The reconnect action for suspended/uninstalled accounts will first try to verify that state (in case of a missed webhook delivery), and if it's still an issue, will deep link the user to the relevant page where they can fix it. Testing Done: - Ran unit tests. - Suspended and deleted app installs and saw that the state was properly reflected in the connected services list. - Reconnected suspended and deleted app installs and saw that state healed correctly.	ummslvyqxyoqyzlozxkmksvtrxvnrpwn
	Add connected service account "needs attention" and track GH App state. GitHub Apps can be uninstalled or suspended from the GitHub side, which can cause our integration to start failing. This change makes us handle that in a nice way. This involves several pieces: # Connected Services "needs attention" Accounts can now indicate that they need attention in some way, and provide actions to handle this. This is now implemented for app-based accounts with GitHub in order to allow the admin to reestablish the correction. Later work will involve using this for other hosting services (and PAT-based GitHub connections) to have accounts indicate when their credentials need to be updated. # GitHub App webhook handler The stub webhook handler has been fleshed out to verify the signature header and handle several events: - `deleted` and `suspend` mark the installation account with the state so that they'll be flagged in the connected services list. - `created` can heal a reinstall by refreshing the stored installation ID, matched to the existing account. This allows a reinstall performed entirely from the GitHub side to restore the connection without requiring anything to happen inside Review Board. - `installation_repositories` keeps the `repository_selection` flag up to date. - `installation_target` updates the stored account name. # API request error handling In the case of a missed webhook delivery, we can still end up with the stored state being wrong (in either direction). API requests have been updated to handle this in two ways: - Token minting will map a 403 or 404 response to the suspended/uninstalled state (checking first to ensure that it's not some other ephemeral failure or rate limit error). If we got the error because the app had been uninstalled but is now reinstalled, we'll adopt the new installation ID. - 403 or 404 errors for cached installation tokens will drop the token and try again (limited to once per minute). - If we have a stored non-active status, we'll recheck that state (also limited to once per minute) before refusing to mint a new token. # GitHub reconnect The reconnect action for suspended/uninstalled accounts will first try to verify that state (in case of a missed webhook delivery), and if it's still an issue, will deep link the user to the relevant page where they can fix it. Testing Done: - Ran unit tests. - Suspended and deleted app installs and saw that the state was properly reflected in the connected services list. - Reconnected suspended and deleted app installs and saw that state healed correctly.	ummslvyqxyoqyzlozxkmksvtrxvnrpwn

Diff:

Revision 6 (+5624 -428)

Show changes

	reviewboard/admin/views.py
	reviewboard/admin/tests/test_connected_services_list_view.py
	reviewboard/hostingsvcs/base/connect_ui.py
	reviewboard/hostingsvcs/github/accounts.py
	reviewboard/hostingsvcs/github/api.py
	reviewboard/hostingsvcs/github/client.py
	reviewboard/hostingsvcs/github/service.py
	reviewboard/hostingsvcs/github/views.py
	8 more

Checks run (2 succeeded)

flake8 passed.

JSHint passed.