Excellent Work!

One thing: this could be "Show changes" rather than "See what's changed."

Opinions?

What happens if we've updated the review request, but we didn't upload a new diff?  So, for example, we changed the Description?

Wont work with multiple reviews

Using the diff's title and cutting out "Diff r" is not the most elegant solution.

Well done Curtis. I personally love this feature. I can see you're your own worst critic, haha.

As I mentioned earlier, I'm not a huge fan of "See what's changed", but that just may be me.

Thanks!

-Yazan

Can you break this into multiple lines to enhance readability?

Also, I see a loose comma there, not sure what the purpose of it is.

Break into multiple lines please.

Comma issue again.

Hey Curtis:

This is good work!  I'm a little concerned that we're getting a bit too logic-y in the template, and that templatetags might be of some better use... let's see what the others think.

Review is below.  Thanks!

-Mike

These template tags are a little hard to read.

Notice above how we do indentation within the tags, like so:

{% for item in fieldinfo.info.removed %}
{%   if item.1 %}...
{%   else %}...
{%   endif %}
{% endfor %}

Can you make yours the same way, for readability?

I'm not a template expert here, but looking above, I think we prefer:

{% ifequal fieldinfo.title == "Diff" %}

Also, I'm a little worried that this conditional will break if/when we do localization...

Is there no other, less localization-sensitive way of ensuring that this fieldinfo is about a diff?  If not, perhaps a templatetag would be useful here.

Same comment as above for this section, regarding indentation.

Based on the code at reviewboard/reviews/models.py:1240, I believe that if {{item.0}} is 'diff', {{item.2}} should be the diffset ID. You can then use this and look through review_request.diffset_history.diffsets to find the matching diffset and get its revision.

Something is pretty weird with your change: your diff seems to include several recently-committed changes.

You have trailing whitespace

This line looks like more than 80 characters

You don't need the parentheses.

You can just use item[2] inline, instead of assigning it to diffset_id

Trailing whitespace

No show stoppers, just some comments.

Well done!

One style recommendation: (This is one less line of code)

revision_number = ""

if fieldtitle == "Diff":
    revision_number = context['review_request'].diffset_history.diffsets.get(id=item[2]).revision
	
return revision_number

I know this isn't your code, but:

This needs to be indented.

Again, I know this isn't your code, but:

This needs to be indented.

In general, it's best not to touch more lines than the change requires.

Please use this form:

{% if item.1 %}
 <a href="...">
{% else %}
 {{item.0}}
{% endif %}

Makes it easier to read.

Same here, and below.

So, I'm actually not wild about using template tags for this. It's the wrong way to compute the logic.

Instead, this data should just be part of the fields_changed list generated in reviews/views.py in review_detail(). It already does a bunch of processing on things. It should add an additional URL for the interdiff (since it can easily calculate it there), and then the template just simply displays that URL.

Not this part, though. That's fine, because there's no else or multi-line stuff involved.

This is getting super-close, Curtis.  Great work so far,

-Mike

Just for consistencies sake, lets use double quotes for 'added'.

> 80 chars

The {% of this endif should be at the beginning of the line, and the "e" of "endif" should be aligned with the "i" of the associated "if".

So something like:

{%     endif %}

We'll want to wrap "Show Changes" inside a trans.

Same as above, re the alignment of {% endif %}

Just one last nit from me.  Once this is fixed, I'll happily give my ship-it. :)

This {% endif %} needs to be aligned like:

{%    endif %}

Comma at the end of this.

No need for parens unless it spans multiple lines.

Should be pk= instead of id=. pk is an alias, but it's better to use it instead of hard-coding when it comes to IDs.

Mind fixing this line while you're in here?

This one too.

The HTML tags' indentation should be relative to the other HTML tags, not the templatetags.

In Django, you never want to hard-code a URL. So, what you should do is use the {% url %} tag. This takes a name and parameters.

But, first you'll have to give the URL for what you want a name. This lives in reviews/urls.py. Look at the "# Review request interdiffs" section, at the first URL. You want to make this use the url() stuff, like elsewhere in the file. The name should be "view-interdiff".

Then you'd use {% url view-interdiff review_request.display_id past_revision current_revision %}

This looks good to me - great job, Curtis - thanks for your work!

Alignment is off.

This should be inside the elif, and no blank line before the elif.

This may trigger an additional query, so I'd recommend adding 'diffset_history__diffsets' to a review_request.select_related() query above.

Curtis - can you (or ChipX86) confirm this does what ChipX86 asked?

For the select_related stuff, you'll need to first modify _find_review_request to take an optional extra parameter to specify what should be selected.

The changes would end up looking something like:

def _find_review_request(..., select_related=None):
    q = ReviewRequest.objects.all()

    if select_related:
        q = q.select_related(*select_related)
    
    if local_site_name:
        ...
        q = q.filter(local_site=local_site,
                     local_id=review_request_id)
    else:
        q = q.filter(pk=review_request_id)
    
    try:
        review_request = q.get()
    except ReviewRequest.DoesNotExist:
        raise Http404

    ...

(You may have to make modifications to this)

Then your call to _find_review_request at the top of review_detail would pass in a list of fields we want to query. The one you care about right now is "diffset_history__diffsets".

Almost there!

This must be aligned with the other parameters.

No need for the \

Must be aligned.

No need for the \.

The reason being that it's in parens, so Python knows the line isn't ending.