Would it make sense to build this payload differently according to which external client we're dealing with? For example maybe one client expects some extra data in the payload, or some different authentication details other than a session cookie. If so, then for building the payload should I have a registry of external clients? Then choose which external client to use from the registry based on the external_client param in the request, and then have that build the payload?

Blank line between these.

Can you add -> None to each of these?

We're a bit all over the place with the formatting of view docstrings, but we mostly settle on Testing ViewName GET [...] for the docstrings.

Blank line between this block and statement.

Can you add -> None to each of these?
Same with other test files.

The docstring's indented too far.

Blank line here.

Blank line here.

Blank line here.

Blank line here.

Blank line here.

While here, can you add a __future__ ... annotations import?

Can you add typing to these?
In fact, we should move these to the class body as instance variables, document them, add typing, and set them in here (I have a comment about this below, but I've gone back in time to edit this one).

No need for the outer parens.

Can we name this client-name? (I made some comments about why on /r/13032).

request is local, so we don't need to access self.request here.

To better convey intent here (this is only for GET requests), let's check the method first.

We should probably preserve ?next=, in case the caller wants to eventually redirect back somewhere.
In fact, I wonder if we'd want to provide some client-state= parameter we'd pass back to the client?
Same applies below.

I think dispatch should be responsible for pulling these out for other methods to use.
State set on self within a View subclass will only live for the lifetime of the request.

Let's call this client_name.

No need for parens here.

We can ditch the else, since it's implied.

We access self.request a lot, so let's pull it out so we don't have to perform attribute lookups more than once.

This is a bit tricky to read. Let's pull this out into a variable and then reference that in the if.

I think we'll want to include 127.0.0.1:{client_url_port} in this list.
For get_host(), do we want to include the port as well?

I just made a comment on /r/13032/ about this, but generate_token() can raise a WebAPITokenGenerationError, which would bubble up to here. We should handle this gracefully.

Missing a trailing comma.

To keep this consistent, how about prefixing this with client? And maybe flip it, so it's client_allowed = [True,False]?

For logging-related statements, pass arguments as positional arguments, rather than using %.

Same comments as above.

Same comments re: pulling out request.

Maybe support a client-state here as well?

No space before the {% blocktrans %}. Indentation happens within the {% ... %}.
Same with template tags below and in other files.

Let's put a blank line between these blocks, so they don't blur together.

Instead of "Blocking log in" (which I feel may be misunderstood by some users), let's try to help them out a bit. How about something along the lines of:



For security reasons, clients logging in to the Review Board API must pass a <tt>client-url</tt> that points to the local host. This may be a bug in your software.

I think we'll only hit that under this case, right?

We should put this in scripts-post, so it doesn't block anything on page load.

We know the value of this variable at template time, so let's use it to wire off the entire <script>, rather than injecting it and bailing within the code.
I sorta wonder if all this JavaScript code should just live in the main JavaScript codebase instead of within this page, due to the limitations outlined below.

Missing a trailing comma.

We can't safely use these template literals in here. We'd have to call interpolate(gettext('...', [params...])) directly.
We also can't localize text that hard-codes a dynamic value coming from the Django template. The localization string scanner will see the literal {{username}}, and will never match at runtime. So any values like this must be provided as parameters.

No need for the />. That's a legacy holdover that we don't need to continue with.
Also, no space after value.