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.

While here, can you add a module docstring?

Can you add a Version Changed and mention the addition of client-name and client-url?

Since this is multi-line, let's do one value per line.

Let's inverse the attribute logic here. Compute a local value, then set it on the attribute.

Looks like we compute the ?client-name=%s&client-url=%s%s three times in this class. Maybe we should precompute this in dispatch as an attribute and then reference it? Along with a flag that can be used instead of pulling out and checking client_name and client_url.

Second sentence should be in the description area.

These are both present in LoginView as well. Maybe we should centralize a list of allowed hosts somewhere?

Small nit: We can get more out of a line if we start the text on the next line, indented 1 level.

This can be const.

Because these are now in a .es6.js (which Babel will compile), you can now leave out the interpolate. I think you'll need to pull out the client name and username into local variables first, but I might be wrong there.
You can just reference local variables in the template literal as usual.
The plugin will take care of wrapping it in interpolate() and passing in those variables.

Can you postfix this instead? Put the -- at the end? Just a small nit, but is more consistent with usual styling.

Let's indent the blocktrans (space inside the {%).

Indentation of tags should be one level higher, since they're inside a {% block %}.

Same here.

All user input should be escaped.
What I recommend is to have a get_js_view_data() function that returns everything that should go in here (aside from el), and then |json_dumps_items that, so we get a safe JSON payload to inject.

Same notes about indentation in this file.

Blank line between blocks.

As above, let's have some central variables we can access for the URLs, so we can control it in one place and don't have to repeat.
That'll also be safer, since we don't have any risk of injection attacks here.

Template tags should be indented relative to {% block %}.

Looking good! There's a few very small issues to fix, and some inconsistencies in variable names in JavaScript. But I think this is just about done :)

Swap these around. Should go newest to oldest.

This feels like it should be an independent block of work. The rest are just simply setting state, but this is generating, updating, and then setting state. It might be more readable as its own block.

These lines are indented too far.

Might be worth mentioning what happens if this is None.

Alternatively, something like:

if port:
    suffix = f':{port}'
else:
    suffix = ''

return {
    f'127.0.0.1{suffix}'
    f'localhost{suffix{'
}


Avoids having to loop over results and build a new set.

Just realized the naming conventions here. These should be _clientName, _clientURL, etc. in JavaScript.

Indentation is off on the description.

We have a RB.navigateTo that should be used here instead.
If _redirect is for unit test purposes, you can remove it and spy on RB.navigateTo instead.

In this case, you can combine these statements:

const redirectCounter = --this._redirectCounter;


(And in this case, -- beforehand makes sense, because we're decrementing and then storing the result.)

Ship It!