I'm not sure of how I should add unit tests for this request handler. I'm not sure how to mock an HTTPServer in order to test things, or if I should just run a server on localhost:8000 or something during the tests.

There's one design consideration we should figure out. This all works great for the case of rbt commands, but RBTools can also be consumed via services that may need to talk to multiple Review Board servers (in theory) and may or may not be multi-threaded.
Right now, the web server and its state are global, so the first login to a server wins. There's also only one server that can run at a time. And we have to block on response.
I think an alternative approach could provide wider compatibility with different use cases:


Have a class that manages all this state and the server. WebLoginManager or something. This would contain the functionality for creating a server, starting it, and querying successful login.


Allow this to take a callback function for when login is successful, which is called immediately after setting self.login_successful.


Add a wait_successful() method or something that can do the blocking loop waiting for success/fail (you'll probably want a time.sleep(0.1) to avoid high-CPU usage), making it easy to use for the rbt case.


Introduce timeouts. This should probably be on WebLoginManager. After a certain amount of time, the server shuts down and any blocking operations give up. This avoids the situation where a background process outputs a URL and just stalls for ever, and covers the blocking loop and async results usages.


This would allow for multiple (even concurrent) web login processes, and avoid global state to avoid threading issues. It'd also give async processes a way to wait for successful login/timeout (important for UI processes, such as possible IDE integrations)

): should be on the next line.
Can you update this to specify types while here?

Let's require keyword arguments here. Helps with letting us add/rearrange arguments as we go without breaking API.

These are missing documentation.

, optional

I think we should use the same user agent we use for API requests (defined in api/request.py), so we can more easily trace user agent activity in logs.

Let's make these keyword-only. Same with ones below.

Version Added will show as part of the description, so this will need to be above Args.

For logging statements, we should avoid f-strings and instead pass parameters to the logging function. These get tracked separately, allowing the message and the arguments to be pulled out separately by logging implementations.
Same elsewhere.

Blank line between statements and blocks.

Since we're repeating this block of code, we should have a standard function somewhere for opening pages in a browser.

This should go before Returns.

Can we use constants to define this range? Even if just as arguments to this function. That'll help document them a bit better.

Hmm. Instead of "traditional", maybe "console" or "terminal"?

I think I'd prefer to revert this change.

Type here doesn't match the type hint.

Ship It!

Typo: loggin -> logging

Let's plumb the capabilities object in the command through to here, so we can use capabilities.has_capability('authentication', 'client_web_login')

Too many logins.
Let's say "The login command now directs the user to log in via the Review Board web page, instead of ..."

And here.

Could do:

if capabilities is None:
    capabilities = Capabilities(server_info.capabilities)

return capabilities.has_capability(...)

Ship It!

This is technically not a Python Standard Library, so it should go in a 3rd-party group.

"shut down"

Types should go on the next line, indented 4.
Same below.

Summary must be a single line.
Same with ones below.

Let's call this timeout_time_secs, so it's always clear what the unit is.

Let's add a unit of time here as well.
Does it need to be a float?

We should just use the local variables here.

Both of these should be , optional.

I'd rather we avoid a SystemExit here, since potentially this is being used in a third-party codebase. It'd be better to have a more relevant exception to raise.

Same comment as above re: the type being on the next line.
This applies throughout the file.

Missing docs for this.

There's an extra blank line here.

This string should use single quotes.
Also noticed that the casing is different than the other headers. Pretty sure these get normalized, but for consistency, maybe we want to use Content-Type here.

This function doesn't have a return type, so no need for the return None.

Ship It!

Ship It!