Added Show Links for Interdiffs

Review Request #2810 — Created Jan. 21, 2012 and discarded

Information

Review Board

Reviewers

Added Show Links for Interdiffs.

When a user uploads a diff, we show a link to the uploaded diff revision. We should also provide a linke to the interdiff, showing what's changed.
User Testing:        Local testing done on Ubuntu 11.10 running Firefox 9.0.1
Integration Testing: Merge branch 'master' into interdiff
Automated Testing:   Tests Passed 470 (18 Skipped)
                     PEP8
Description From Last Updated

Excellent Work! One thing: this could be "Show changes" rather than "See what's changed." Opinions?

ME medanat

Wont work with multiple reviews

CI cim1

Can you break this into multiple lines to enhance readability? Also, I see a loose comma there, not sure what …

ME medanat

These template tags are a little hard to read. Notice above how we do indentation within the tags, like so: …

mike_conleymike_conley

I'm not a template expert here, but looking above, I think we prefer: {% ifequal fieldinfo.title == "Diff" %} Also, …

mike_conleymike_conley

Same comment as above for this section, regarding indentation.

mike_conleymike_conley

You have trailing whitespace

daviddavid

This line looks like more than 80 characters

daviddavid

You don't need the parentheses.

daviddavid

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

daviddavid

Trailing whitespace

daviddavid

I know this isn't your code, but: This needs to be indented.

ME medanat

Again, I know this isn't your code, but: This needs to be indented.

ME medanat

Please use this form: {% if item.1 %} {% else %} {{item.0}} {% endif %} Makes it easier to read.

chipx86chipx86

Same here, and below.

chipx86chipx86

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

chipx86chipx86

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

mike_conleymike_conley

The {% of this endif should be at the beginning of the line, and the "e" of "endif" should be …

mike_conleymike_conley

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

mike_conleymike_conley

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

mike_conleymike_conley

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

mike_conleymike_conley

Comma at the end of this.

chipx86chipx86

No need for parens unless it spans multiple lines.

chipx86chipx86

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

chipx86chipx86

Mind fixing this line while you're in here?

chipx86chipx86

This one too.

chipx86chipx86

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

chipx86chipx86

In Django, you never want to hard-code a URL. So, what you should do is use the {% url %} …

chipx86chipx86

Alignment is off.

chipx86chipx86

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

chipx86chipx86

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

chipx86chipx86

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

mike_conleymike_conley

This must be aligned with the other parameters.

chipx86chipx86

No need for the \

chipx86chipx86

Must be aligned.

chipx86chipx86

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

chipx86chipx86
ME
  1. 
      
  2. Excellent Work!
    
    One thing: this could be "Show changes" rather than "See what's changed."
    
    Opinions?
    1. I tride to mimic http://reviews.reviewboard.org/r/2810/s/304/
  3. 
      
CI
mike_conley
  1. 
      
  2. reviewboard/templates/reviews/review_detail.html (Diff revision 1)
     
     
     
     
    What happens if we've updated the review request, but we didn't upload a new diff?  So, for example, we changed the Description?
    1. Just tested.
      
      It doesn't increment, so it doesn't appear to be a problem. But it did lead me to discover another small problem while investigating further.
  3. 
      
CI
CI
  1. 
      
  2. Wont work with multiple reviews
    1. I need to find where the "2" integer comes from in this screenshot: http://reviews.reviewboard.org/r/2810/s/307/
      
      Somewhere in the database it is hiding, however I have only been able to find that integer included in the entire string as either a name or the url.
  3. 
      
CI
CI
CI
  1. 
      
  2. Using the diff's title and cutting out "Diff r" is not the most elegant solution.
  3. 
      
ME
  1. 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
  2. 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.
    1. I belive it is when their are multiple changes made at the same time, they will be comma seperated, excluding the last change.
    2. I see it now. Thanks.
  3. Break into multiple lines please.
    
    Comma issue again.
  4. 
      
CI
CI
mike_conley
  1. 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
  2. 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?
  3. 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.
  4. Same comment as above for this section, regarding indentation.
  5. 
      
david
  1. 
      
  2. reviewboard/templates/reviews/review_detail.html (Diff revision 4)
     
     
     
     
     
     
    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.
  3. 
      
CI
CI
david
  1. Something is pretty weird with your change: your diff seems to include several recently-committed changes.
    1. I was following the Development | Getting Started guide. I merged with the upstream changes on master before I posted my review, to ensure there were no conflicts. It now seems to be showing a diff for every file changed on master, along with my code changes.
      
      The two files I made changes in are:
      reviewboard/reviews/templatetags/reviewtags.py
      reviewboard/templates/reviews/review_detail.html
      
      How should I approach this now?
  2. 
      
CI
david
  1. 
      
  2. You have trailing whitespace
  3. This line looks like more than 80 characters
  4. You don't need the parentheses.
  5. You can just use item[2] inline, instead of assigning it to diffset_id
  6. Trailing whitespace
  7. 
      
CI
ME
  1. No show stoppers, just some comments.
    
    Well done!
  2. reviewboard/reviews/templatetags/reviewtags.py (Diff revision 9)
     
     
     
     
     
    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
  3. I know this isn't your code, but:
    
    This needs to be indented.
  4. Again, I know this isn't your code, but:
    
    This needs to be indented.
  5. 
      
CI
chipx86
  1. 
      
  2. In general, it's best not to touch more lines than the change requires.
  3. Please use this form:
    
    {% if item.1 %}
     <a href="...">
    {% else %}
     {{item.0}}
    {% endif %}
    
    Makes it easier to read.
  4. Same here, and below.
  5. reviewboard/templates/reviews/review_detail.html (Diff revision 10)
     
     
     
     
     
     
    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.
    1. Ach, sorry for leading you down the wrong path, Curtis. :/
  6. Not this part, though. That's fine, because there's no else or multi-line stuff involved.
  7. 
      
CI
mike_conley
  1. This is getting super-close, Curtis.  Great work so far,
    
    -Mike
  2. reviewboard/reviews/views.py (Diff revision 11)
     
     
    Just for consistencies sake, lets use double quotes for 'added'.
  3. reviewboard/reviews/views.py (Diff revision 11)
     
     
    > 80 chars
    1. How can I make this less than 80 chars?
  4. 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 %}
  5. We'll want to wrap "Show Changes" inside a trans.
    1. Thanks! This used to be in a blocktrans, hard one to catch.
  6. Same as above, re the alignment of {% endif %}
  7. 
      
CI
CI
mike_conley
  1. Just one last nit from me.  Once this is fixed, I'll happily give my ship-it. :)
  2. This {% endif %} needs to be aligned like:
    
    {%    endif %}
  3. 
      
CI
CI
chipx86
  1. 
      
  2. reviewboard/reviews/views.py (Diff revision 15)
     
     
    Comma at the end of this.
  3. reviewboard/reviews/views.py (Diff revision 15)
     
     
    No need for parens unless it spans multiple lines.
  4. reviewboard/reviews/views.py (Diff revision 15)
     
     
    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.
  5. Mind fixing this line while you're in here?
  6. This one too.
  7. The HTML tags' indentation should be relative to the other HTML tags, not the templatetags.
  8. 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 %}
    
  9. 
      
CI
mike_conley
  1. This looks good to me - great job, Curtis - thanks for your work!
  2. 
      
chipx86
  1. 
      
  2. reviewboard/reviews/urls.py (Diff revision 16)
     
     
    Alignment is off.
  3. reviewboard/reviews/views.py (Diff revision 16)
     
     
     
    This should be inside the elif, and no blank line before the elif.
  4. reviewboard/reviews/views.py (Diff revision 16)
     
     
     
    This may trigger an additional query, so I'd recommend adding 'diffset_history__diffsets' to a review_request.select_related() query above.
    1. This still needs to be done.
    2. I have been reading the docs, and looking at examples of select_related(), spending way too much time on this.
      
      Is this the proper way to use select_related() in this instance?
          diffs = review_request.diffset_history.diffsets.select_related().all()
          diff_revision = diffs.get(pk=info['added'][0][2]).revision
  5. 
      
CI
mike_conley
  1. 
      
  2. reviewboard/reviews/views.py (Diff revision 17)
     
     
     
    Curtis - can you (or ChipX86) confirm this does what ChipX86 asked?
  3. 
      
chipx86
  1. 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".
  2. 
      
CI
chipx86
  1. Almost there!
  2. reviewboard/reviews/views.py (Diff revision 18)
     
     
    This must be aligned with the other parameters.
  3. reviewboard/reviews/views.py (Diff revision 18)
     
     
    No need for the \
  4. reviewboard/reviews/views.py (Diff revision 18)
     
     
    Must be aligned.
  5. reviewboard/reviews/views.py (Diff revision 18)
     
     
    No need for the \.
    
    The reason being that it's in parens, so Python knows the line isn't ending.
  6. 
      
CI
Review request changed

Status: Discarded

Loading...