Can we make a pydantic model for the GitHub app data (the github_app data stored on the installation account)?

Do we nicely handle things if there's a lot of repositories here? Like having a vertical scrollbar if there's a bunch of repositories listed.

Can we rename to AppManifestResponse.

Can we rename to InstallationAccountResponse.

Can you add a Version Added to this.

In this change the App views were put into reviewboard.hostingsvcs.github.views and I don't see any other changes where you split them out into an app_views module.

This needs a Version Added.

Should we pass local_site=request.get('local_site') here?

I think there will always be a request.local_site attribute because of our LocalSiteMiddleware.

Same note about request.local_site always existing.

Ok so thinking about local sites again (this is on my mind from neglecting to really consider local sites when implementing document review and then having to go in and add it later), is the idea for GitHub app-record accounts to be local-site dependent? This seems to be the current design because we pass local_site for both the app-record accounts and installation accounts (e.g. in make_connect_ui_context() we do HostingServiceAccount.objects.filter(service_name=..., local_site=request.local_site).
But I know you mentioned possibly having a "GitHub App for all of RBCommons" in a reply to one of my other reviews. And we have staff_member_required decorators on the views, which means only server admins (not local site team admins) will be able to access the views.
So even though RBCommons will require its own development effort, I think we need to make some design decisions at this stage since we are currently passing around local sites in this code.

Missing a docstring.

Could this be Literal['user', 'organization', '']?

Same comment as above.

Seems like this could be Literal['all', 'selected'].

Can you add a docstring for this.

This can be removed, we do this below in the if condition.

We do these isinstance checks on the app data a lot more than we do is_installation_account() or is_app_record_account(). Might be worth making some is_installation_data() is_app_record_data() functions?

We can remove local_site here because we do a local_site = app_account.local_site in the view instead of using this value that was passed in.

Same comment as above.

I'm getting a bit mixed up with the namings. We have "app-record accounts" and "installation accounts". A hosting service account can be either of those.
Can we rename _create_app_record() to _create_app_record_account() to clarify that. And then rename _create_app_account() to _create_app_installation_account() to specify that its an installation account. Because "app account" could really mean either a record account or installation account, you even name the record account variable app_account.

This can be an elif or add back in the blank line before this.

This change is far too big to reason about. I had to stop part-way through views.py after 3 review sessions, and I'm not even half-way through
I have a lot of higher-level questions and lots of code style issues. It's all blurring together and I can't fit this into my head. What I'd suggest is breaking this up into:

The base shared account state stuff (and I really need to spend time just going over all that)
Management for that state
Flow
Templates
UI

Something like that.
A lot of the comments I have left are applicable across the change as well.

To check, is there a situation where we'd want to establish multiple GitHub hidden app accounts? What's the story on DB import/export there?

Can we pull the URL out before this? Helps keep this readable.

We access self.client 4 times in this test. Can we pull it out? Same with other tests.

Can we move the encryption method to the description? We'll ultimately want to move away from encrypt_password() entirely and may need to describe that here.

Same here.

Should this be a TypeAlias?

These are missing a "Version Added".

Final[str]?

We have a standard symbol for this.

What happens if we need to change this data in the future? Will we hit validation problems?
We probably should version the state as well.

This is missing a description.

This is missing "Version Added".

All others have a "False if..." Can we add one for consistency?

This is missing a "Version Added".

This is missing a "Version Added".

This is missing a "Version Added".

This is missing a "Version Added".

Can we pull this out? It's easier to read function calls when they're not nested in f-strings.
Also would help keep this header on one line.

APIError sorts first.

This doesn't provide a lot of guidance as to what this means. Can we elaborate?

Can you move the )) to its own line, so the string is isolated?

Instead of cast, can we assert isinstance?

Is this something we want to encrypt?

These shouldn't be doc comments. Unless these should be on the class.

Should we log this?

Can you use the standard Tuple: form here?

Should this (and the one below) be Sequence?

We access self.account twice. Let's pull it out.

Can you use the standard Tuple: form?

We're mixing up locations for classmethods. These should be higher up.
But I have a question about the design here. Are these part of the hosting service interface now, these methods?
I think rather than putting connect-related state here, we should do like we do with the forms, have a class that implements the interface and point to it. That keeps things manageable as hosting services grow.

This is missing the full class path.

This is hard to read. Can you pull this out into a variable and then iterate over that?

This is missing a blank line. But this block would all be better as:

if is_app_record(...):
    app_account = account
else:
    ...

    connected_accounts.append(...)

This should probably be Mapping.

We should be escaping variables going into this.

These are missing Version Added, and should be Final.

Can you sort these alphabetically?

Can you document what this is protecting against (the TypeError and ValueError)? It's not clear.

Can you document this fallback? It's not clear why we'd need it or why this value would work.

Can you sort all these alphabetically?

From the above, this could end up being "Review Board (Review Board)".

Blank line before the comment.

Alphabetical here too.

Alphabetical here too.

And here.

And here.

Should we be doing this above if we return an error and this state is set?

Blank line before the comment.

Can you pull these out into separate statements? Upcoming work on stronger encryption's going to require more complex statements here.

Can you alphabetize this?

We should escape values going into the URL.

This can be one statement.

Or validation failed, I'm guessing?

We should escape the code going into this.

This should be in alphabetical order.

We should escape values going into the URL.

This can be one statement.

Same question about re: stale state if we hit an error above.

We should catch an error here.

Do we need the import to be inline here?
If inline, it should be at the top of the function.

Can you pull this out into a variable so this is more readable/maintainable?

This needs to be the full class path.

This needs to be the full class path.

If this has to be an inline import, can we move it to the top of the function?

This needs to be the full class path.

No need for parens.

Can we move to the top of the function if needed here?

No need for parens.

Missing a Version Added.

Missing a Version Added.

Missing a Version Added.

Let's pull this out, because the newer cryptography stuff will involve some multi-line statements here eventually.
Same below.

Let's pull out the encrypt_password calls, because the newer cryptography stuff will involve some multi-line statements here eventually and it won't be able to be inlined.

Missing a Version Added.

Missing a Version Added.

Missing a Version Added.

We never type this. That should belong to the parent anyway, since we'd only retype in this case to aid subclasses to avoid typing as a literal.

Missing a Version Added.

This is missing docs.