Can you make this available as a branch on your github fork? I applied the patch but it's missing all the binary files.

This is a pretty great start. After playing with it a bit, I have a few requests:
- For the "Server Cache" link, if there's no cache backend configured (or no cache stats), it should link to the page to turn on caching.
- It would be nice to have some kind of blank-slate state for when there are no repositories or groups set up.
- Some of the links on the left-hand side are broken.

These changes should be pulled out into a separate review request.

django and djblets are both 3rd-party modules and should be grouped together.

These should be prefixed with reviewboard.

{{MEDIA_URL}} and {{MEDIA_SERIAL}} are already injected into template contexts via middleware. You shouldn't need to pass them in yourself.

There's some alignment issues here. It looks like several of these are indented only 3 spaces instead of 4.

Same here re: MEDIA_URL and MEDIA_SERIAL

Remove this line.

Remove this line.

These should be prefixed with the reviewboard. module.

This should be a docstring.

This can be made a lot more pythonic:

for i in range(day_total):
    counter_date = start_date + timedelta(days=i)
    dates_in_days.append(counter_date)
    req_array.append([
        request_objects.filter(time_added__lte=counter_date).count(),
        counter_date,
    ])

It might also be nice to fill req_array with dicts instead of with lists, so that you can address the data by name instead of [0] and [1].

Please include "TODO" in this comment.

You can initialize these directly:

requests = {
    'all_requests': review_requests,
    'requests_by_day': req_array,
    'change_descriptions': change_desc_unique,
}

...

You can initialize this all at once.

The items in this list would be better as tuples instead of lists:
('admin/db/auth/user/add/' _('Add New')),


also, add a space after the comma and / at the end of the URLs.

This can be initialized in place.

I see more below. Can you go through and change everywhere that you're creating a dict like this?

Space between // and the comment text. Same below.

Indentation is kind of funky here.

Here's where it might be nice to use a dict:

<a href="{{action.url}" class="{{action.class}}">{{action.text}}</a>

Is this always going to be seconds? That seems like a kind of crummy unit to use for uptime.

These should all have ?{{MEDIA_SERIAL}} at the end.

Please change this to be "Review Request Status"

Can you include an explanation for why this is commented out?

This is weirdly yellow. What happened?

Vlad:

Thanks for your work!  A few notes before - mostly stylistic.

All the best,

-Mike

These need to be placed in alphabetical ordering.  django.contrib... should be before django.core...

Should be in alphabetical ordering - ie, at the top of the from imports.

Straight imports like this should be put before from imports.  Also, are you sure you need to import all of reviewboard?  Or can you from import the individual functions that you need?

I believe this needs to be indented four spaces.

I believe this should go with the group of "from" imports below this block.

one space after =

No linebreak here

A linebreak before this if block

This might look better with the line break after "(activity_data),", as opposed to after "HttpResponse("

Like above, I think this from import should join the group of from imports below.

I believe each import should be on it's own and in alphabetical, like:

import datetime
import time

Need to be put in alphabetical ordering.

I believe we tend to format imports so that the imported objects are tabbed to the same point - see https://github.com/reviewboard/reviewboard/blob/94e5004b28d4e10d4d774822026b1756ec5e067d/reviewboard/webapi/resources.py#L44 for example.

Another linebreak before the start of this function.

This is a little difficult to read.  Is it possible to indent in a way that makes it easier to tell that "(now -datetime.timedelta(days=7)..." belongs within users.filter( for "now"?

Should be 4 spaces indentation, and spaces after the commas.

space before the +

Another linebreak before this function definition

Space before the +

Another linebreak before this function definition.

No linebreak required here.

A space before the +

Another linebreak before this function definition.

No linebreak required here.

Another linebreak before this function definition.

Space after the commas.

Another linebreak before this function definition.  Same applies to the rest of this review request, so I won't mention it anymore.

Space before + sign

Should this be a constant declared somewhere?

no linebreak here.

Only need a single linebreak here

No linebreak here

Space after #

Is this line <= 80 chars?

Only one space before "in", and there should be a linebreak before this for block.

Hm, instead of all this "idx" work, I'd prefer:

for unique_desc in change_desc_unique:
  unique_desc_array = []
  unique_desc_array.append(...)
  unique_desc_array.append(...)
  change_desc_array.append(unique_desc_array).

Or, even better, that unique_desc_array maybe should be a tuple.

Same applies below.

Space after #

Only one space before in, and a linebreak before the for block.  Same for below.

Space after #

Spaces after commas.

The CSS params should be in alphabetical order, so:

display: inline;
padding: 0;
position: relative;
vertical-align: middle;

Same applies below.

Two space indentation here.

Two space indentation.

Two space indentation

Two space indentation.  Other instances below.

Four space indentation.

Javascript variables should always be declared with "var", so:

var centerWidth...
var docWidth...

Space after ,

var before widgetBoxId

Maybe we can move the {% if widget_icon %} just above <img src...>, and the {% endif %} right below, just to make things a little clearer.

No space before this <link> tag.

Needs to be in alphabetical order.

No space just before this <script> tag.

No linebreak required here.

Alphabetical order, please

Proper indentation for the <h1> and <div> tags please.  More instances below.

Can these if / else / endifs in this section get formatted to make it clearer what's within the code blocks?

Like
{% if something %}
  [Block]
{% else %}
  [Block]
{% endif %}

Is that four spaces for indenting?  Not sure, but it seems like it's a bit more.

This line could probably be made clearer:

<p class="no-result">
  {% trans... %}
  <br />
  <a href...
</p>

Four spaces for indentation.

No spaces on either side of 'reload'

No linebreak between these.

Can this be split up into more than 1 line for clarity?

No space before the {%

I think this block of HTML can be un-indented a few spaces...

Two spaces, not one before these tags.

Needs to be properly indented.

No space before the {

Should be "var y..."

Should be "var input_range_end", and the code in this function needs to be properly indented.

"Comments", "Reviews", etc, should be in translate functions.

So there's a fair bit to fix up still. I'm focusing mostly on the styling, but we're going to have to get some webapi work in for the toggles and stuff, because doing an HTTP GET is unsafe, and we want a standard way of doing all this.

There's a lot of style stuff. It boils down to:

* Indentation fixes (it's really inconsistent in most files)
* Import order/grouping
* Variable/function naming (in Python, always use foo_bar instead of fooBar -- note that JavaScript is going to be in fooBar style)
* Other various PEP8 style issues.

I'll do another pass once this is updated.

Imports must be alphabetical.

This must be inside the function, in the form of:

def foo():
    """One-line summary

    Further multi-line description.
    """
    code

delete_widget_cache. We don't use CamelCase for functions.

This should probably be a constant defined outside the function. In this case, you'd call it CACHED_WIDGETS.

Space between parameters, after the comma.

Space between parameters, after the comma.

Overriding the function is bad. It would need a new name or it'll be overwritten.

If we're really just tying delete_widget_cache to a signal, let's just do that directly.

request is already defined above.

Blank line between these.

request is already defined above.

if ('_popup' not in request.REQUEST or
    'pop' not in request.REQUEST):

Indentation is off, and you don't need the \

Why not just provide current_site_config in there? You shouldn't need to pull these out specifically.

request is already defined above.

This is sort of weird. Maybe pull this out above and pass in as a var.

request is used above.

Alphabetical order.

Can you go through the rest of your change and make sure imports are in alphabetical order?

Two blank lines.

So I'm not sure we should have this here. Instead, these should be webapi resources.

Follow the doc format outlined above.

Same with all other instances in your change.

I'd suggest:

state = request.GET.get('collapse', None)
widget = ...

if state and widget:

widget_sets.

Can you go through your diff and make sure all variables and functions are in this_style instead?

Two blank lines.

Can you go through and make sure that's the case in other files? This applies to anything at indentation level 0.

Must be reviewboard.attachments.models, and be grouped with the rest of the imports.

Go through the rest of your change and make sure all imports are correct.

They should also be grouped into three groups: Python built-in modules, 3rd-party models, and project models. Each with a blank line between.

No need for the \ on these. It's covered by being in the {}

4 space indentation.

Should tack onto the same var list, instead of adding a new "var" statement.

Maybe just "width"?

This can be:

$(".admin-extras")
    .width(widSpace)
    .masonry("reload");

Can you explicitly do $(document).ready(..) ?

 space indentation.

No spaces inside the ()

GETs that modify state are dangerous. Should always be a POST.

Ideally, this should use a proper webapi resource class, and go through datastore.js.

The API work I talk about can be done in another change. Let's get the other fixes taken care of.

Indentation is off.

No need for that empty function.

There's an extra space at the beginning.