Added Show Links for Interdiffs
Review Request #2810 — Created Jan. 21, 2012 and discarded
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
Screenshots
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: … |
|
|
I'm not a template expert here, but looking above, I think we prefer: {% ifequal fieldinfo.title == "Diff" %} Also, … |
|
|
Same comment as above for this section, regarding indentation. |
|
|
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 |
|
|
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. |
|
|
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, … |
|
|
Just for consistencies sake, lets use double quotes for 'added'. |
|
|
The {% of this endif should be at the beginning of the line, and the "e" of "endif" should be … |
|
|
We'll want to wrap "Show Changes" inside a trans. |
|
|
Same as above, re the alignment of {% endif %} |
|
|
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 … |
|
|
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 %} … |
|
|
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? |
|
|
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. |
|
-
-
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?
Change Summary:
Fixed issue, it now shows the incremental changes from a prior posted diff, and the version before it.
Diff: |
Revision 2 (+7 -2) |
---|
-
-
reviewboard/templates/reviews/review_detail.html (Diff revision 4) 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
-
reviewboard/templates/reviews/review_detail.html (Diff revision 4) 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.
-
reviewboard/templates/reviews/review_detail.html (Diff revision 4) Break into multiple lines please. Comma issue again.
Change Summary:
Removed interdiff showing for files added/removed. Changed wording to Show Changes
Diff: |
Revision 5 (+9 -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
-
reviewboard/templates/reviews/review_detail.html (Diff revision 6) 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?
-
reviewboard/templates/reviews/review_detail.html (Diff revision 6) 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.
-
reviewboard/templates/reviews/review_detail.html (Diff revision 6) Same comment as above for this section, regarding indentation.
-
-
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.
Change Summary:
Used a better approach for interdiffs thanks to Davids and Mikes code reviews, involving custom template tags. Cleaned up syntax. Merged with master. Ran automated tests.
Description: |
|
||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Testing Done: |
|
||||||||||||
Diff: |
Revision 7 (+137 -37) |
Description: |
|
---|
-
Something is pretty weird with your change: your diff seems to include several recently-committed changes.
-
-
-
reviewboard/reviews/templatetags/reviewtags.py (Diff revision 8) This line looks like more than 80 characters
-
-
reviewboard/reviews/templatetags/reviewtags.py (Diff revision 8) You can just use item[2] inline, instead of assigning it to diffset_id
-
-
No show stoppers, just some comments. Well done!
-
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
-
reviewboard/templates/reviews/review_detail.html (Diff revision 9) I know this isn't your code, but: This needs to be indented.
-
reviewboard/templates/reviews/review_detail.html (Diff revision 9) Again, I know this isn't your code, but: This needs to be indented.
-
-
reviewboard/reviews/templatetags/reviewtags.py (Diff revision 10) In general, it's best not to touch more lines than the change requires.
-
reviewboard/templates/reviews/review_detail.html (Diff revision 10) Please use this form: {% if item.1 %} <a href="..."> {% else %} {{item.0}} {% endif %} Makes it easier to read.
-
-
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.
-
reviewboard/templates/reviews/review_detail.html (Diff revision 10) Not this part, though. That's fine, because there's no else or multi-line stuff involved.
Change Summary:
Implemented Christian's changes. Moved logic from tmeplatetags to views.
Diff: |
Revision 11 (+34 -6) |
---|
-
This is getting super-close, Curtis. Great work so far, -Mike
-
reviewboard/reviews/views.py (Diff revision 11) Just for consistencies sake, lets use double quotes for 'added'.
-
-
reviewboard/templates/reviews/review_detail.html (Diff revision 11) 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 %}
-
reviewboard/templates/reviews/review_detail.html (Diff revision 11) We'll want to wrap "Show Changes" inside a trans.
-
reviewboard/templates/reviews/review_detail.html (Diff revision 11) 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. :)
-
reviewboard/templates/reviews/review_detail.html (Diff revision 13) This {% endif %} needs to be aligned like: {% endif %}
-
-
-
-
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.
-
reviewboard/templates/reviews/review_detail.html (Diff revision 15) Mind fixing this line while you're in here?
-
-
reviewboard/templates/reviews/review_detail.html (Diff revision 15) The HTML tags' indentation should be relative to the other HTML tags, not the templatetags.
-
reviewboard/templates/reviews/review_detail.html (Diff revision 15) 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 %}
-
-
-
reviewboard/reviews/views.py (Diff revision 16) This should be inside the elif, and no blank line before the elif.
-
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.
-
-
reviewboard/reviews/views.py (Diff revision 17) 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".
Change Summary:
Implemented select_related() with the aid of Christian. Modified _find_review_request so that it would take an optional select_related paramater to perform when fetching the review request. Ran though PEP8. Automated Testing. Tests Passed 470 (18 Skipped)
Testing Done: |
|
|||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Diff: |
Revision 18 (+58 -18) |
-
Almost there!
-
-
-
-
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.