Summary

Fix the order shown for file attachment diffs.

Review Request #7089 — Created March 19, 2015 and submitted March 25, 2015, 4:55 p.m.

Information

Owner

chipx86

Repository

Review Board

Branch

release-2.5.x

Bugs

Depends On

Commit

3e416f8...

Reviewers

Groups

reviewboard

People

Description

When viewing diffs of file attachments, the displayed order between the
two revisions were reversed. For instance, on images, the new image was
being shown as the original version, and the original image was shown a
the new version.

This was due to the order in which the URL arguments were captured. In
file attachment terms, the original file is the one being diffed
against, but the URL was capturing the new file as the one being diffed
against.

Text attachments were correctly shown, but had reversed the IDs and
state. They're now using the correct variables.

This change fixes up the capture order, the template rendering for text
attachments, and the logic for text attachments, fixing all display
issues.

Testing Done

Tested with some revisioned images I had. The order was correct for all revisions.

Issues

Description	From	Last Updated
I don't like this change. We have it in the other order for diffs+interdiffs. I'd rather fix the other code …	david	March 25, 2015, 1:30 p.m.

Tool: PEP8 Style Checker
Processed Files:
    reviewboard/reviews/urls.py



Tool: Pyflakes
Processed Files:
    reviewboard/reviews/urls.py

Can you test that the text review UIs still behave appropriately?

chipx86 March 19, 2015, 3:35 p.m.

Hmm, yeah, they're broken.

So, it seems we have half the codebase with one interpretation of these fields, and half with another.

Change Summary:

Fixed the order of data for text-based file attachments.

Description:

		When viewing diffs of file attachments, the displayed order between the
		two revisions were reversed. For instance, on images, the new image was
		being shown as the original version, and the original image was shown a
		the new version.

		This was due to the order in which the URL arguments were captured. In
		file attachment terms, the original file is the one being diffed
		against, but the URL was capturing the new file as the one being diffed
		against.

~		This change just fixes up the capture order, fixing all display issues.
	~	Text attachments were correctly shown, but had reversed the IDs and
	+	state. They're now using the correct variables.
	+
	+	This change fixes up the capture order, the template rendering for text
	+	attachments, and the logic for text attachments, fixing all display
	+	issues.

Commit:

-	0078b275fe30012fd1d2eb432f491158412b00dd
+	3e416f8ecaeb13ec0e3170469a65c9433cdfd4d0

Diff:

Revision 2 (+17 -16)

Show changes

	reviewboard/reviews/urls.py
	reviewboard/reviews/ui/text.py
	reviewboard/static/rb/js/views/fileAttachmentRevisionLabelView.js
	reviewboard/templates/reviews/ui/text.html

Tool: Pyflakes
Processed Files:
    reviewboard/reviews/ui/text.py
    reviewboard/reviews/urls.py

Ignored Files:
    reviewboard/templates/reviews/ui/text.html
    reviewboard/static/rb/js/views/fileAttachmentRevisionLabelView.js



Tool: PEP8 Style Checker
Processed Files:
    reviewboard/reviews/ui/text.py
    reviewboard/reviews/urls.py

Ignored Files:
    reviewboard/templates/reviews/ui/text.html
    reviewboard/static/rb/js/views/fileAttachmentRevisionLabelView.js

reviewboard/reviews/urls.py (Diff revision 2)

The issue has been dropped. Show all issues

I don't like this change. We have it in the other order for diffs+interdiffs. I'd rather fix the other code that generates these URLs.

chipx86 March 19, 2015, 7:12 p.m.

The problem isn't that the URLs are generated incorrectly, but rather that this view and the text-based review UI diff support were reversed in their interpretation of these IDs. To get a proper diff between IDs 3 and 4, you'd previously have to generate a URL like ..../file/4-3/, which is wrong. That's the case for images, anyway. Text-based attachments were using the right objects, but then swapping the display order in the template.
This change fixes up everything to let you do ..../file/3-4/, as expected, matching the URL format of interdiffs.

Ship it!

```
Ship It!
```

Status: Closed (submitted)

Change Summary:

Pushed to release-2.5.x (34ce1d9)