Summary

Improve construction of ReviewUIs and show error for diff type mismatches.

Review Request #13533 — Created Feb. 12, 2024 and submitted Feb. 27, 2024, 8:24 a.m.

Information

Owner

david

Repository

Review Board

Branch

master

Bugs

Depends On

Reviewers

Groups

reviewboard

People

Description

When instantiating review UIs for attachments with diffs, we would
create two instances, and then call set_diff_against and discard one
of them. We'd then reconstruct the second instance when rendering to set
the diffTypeMismatch field in the JS model. This also had a bug where
if the two revisions did not match in file types, we'd still always
instantiate the review UI for the "modified" version, which would then
fail in various ways on the client side depending on which particular
ReviewUI was in charge.

This change makes it so we find the ReviewUI classes for both the
original and modified files first, and compare them. If they match, we
can instantiate a single instance. If they don't, we instantiate a
generic review UI that just sets that the type doesn't match.

On the frontend, the DummyReviewableView (used for any ReviewUI that
doesn't provide their own) has been fixed up and enhanced in a couple
ways. First, the way that captions were displayed was backwards (showing
the original file caption on the right). The caption table was also not
properly styled so everything jammed up on the left. Finally, I've also
added a new error message that shows up when diffTypeMismatch is set,
to tell the user that we can't show a diff.

The particular styling rules for the DummyReviewableView are a little
ugly and just inserted into the reviews.less file. I plan to redo all
the styling for all the review UIs in a later change.

Testing Done


Ran python tests.
Ran JS tests.
Created an attachment with multiple revisions that were all images.

  Saw that individual display and diffs still worked properly.
Created an attachment with multiple revisions where some revisions

  were images and some were text. Saw that when I selected a diff

  between incompatible file types, I got a review UI page that showed

  the correct file revision slider and captions, and the body had the

  new error about file types not matching.

Commits

Summary	ID
Improve construction of ReviewUIs and show error for diff type mismatches. When instantiating review UIs for attachments with diffs, we would create two instances, and then call `set_diff_against` and discard one of them. We'd then reconstruct the second instance when rendering to set the `diffTypeMismatch` field in the JS model. This also had a bug where if the two revisions did not match in file types, we'd still always instantiate the review UI for the "modified" version, which would then fail in various ways on the client side depending on which particular ReviewUI was in charge. This change makes it so we find the ReviewUI classes for both the original and modified files first, and compare them. If they match, we can instantiate a single instance. If they don't, we instantiate a generic review UI that just sets that the type doesn't match. On the frontend, the `DummyReviewableView` (used for any ReviewUI that doesn't provide their own) has been fixed up and enhanced in a couple ways. First, the way that captions were displayed was backwards (showing the original file caption on the right). The caption table was also not properly styled so everything jammed up on the left. Finally, I've also added a new error message that shows up when `diffTypeMismatch` is set, to tell the user that we can't show a diff. The particular styling rules for the `DummyReviewableView` are a little ugly and just inserted into the reviews.less file. I plan to redo all the styling for all the review UIs in a later change. Testing Done: - Ran python tests. - Ran JS tests. - Created an attachment with multiple revisions that were all images. Saw that individual display and diffs still worked properly. - Created an attachment with multiple revisions where some revisions were images and some were text. Saw that when I selected a diff between incompatible file types, I got a review UI page that showed the correct file revision slider and captions, and the body had the new error about file types not matching.	7fd8745e5b4e20742ed5f85f091a45566f88c296

Summary

Improve construction of ReviewUIs and show error for diff type mismatches.

When instantiating review UIs for attachments with diffs, we would create two instances, and then call `set_diff_against` and discard one of them. We'd then reconstruct the second instance when rendering to set the `diffTypeMismatch` field in the JS model. This also had a bug where if the two revisions did not match in file types, we'd still always instantiate the review UI for the "modified" version, which would then fail in various ways on the client side depending on which particular ReviewUI was in charge. This change makes it so we find the ReviewUI classes for both the original and modified files first, and compare them. If they match, we can instantiate a single instance. If they don't, we instantiate a generic review UI that just sets that the type doesn't match. On the frontend, the `DummyReviewableView` (used for any ReviewUI that doesn't provide their own) has been fixed up and enhanced in a couple ways. First, the way that captions were displayed was backwards (showing the original file caption on the right). The caption table was also not properly styled so everything jammed up on the left. Finally, I've also added a new error message that shows up when `diffTypeMismatch` is set, to tell the user that we can't show a diff. The particular styling rules for the `DummyReviewableView` are a little ugly and just inserted into the reviews.less file. I plan to redo all the styling for all the review UIs in a later change. Testing Done: - Ran python tests. - Ran JS tests. - Created an attachment with multiple revisions that were all images. Saw that individual display and diffs still worked properly. - Created an attachment with multiple revisions where some revisions were images and some were text. Saw that when I selected a diff between incompatible file types, I got a review UI page that showed the correct file revision slider and captions, and the body had the new error about file types not matching.

7fd8745e5b4e20742ed5f85f091a45566f88c296

Issues

Description	From	Last Updated
'housekeeping.functions.deprecate_non_keyword_only_args' imported but unused Column: 1 Error code: F401	reviewbot	Feb. 12, 2024, 10:09 a.m.
line too long (80 > 79 characters) Column: 80 Error code: E501	reviewbot	Feb. 12, 2024, 10:09 a.m.
It looks like you didn't actually add this argument.	maubin	Feb. 15, 2024, 9:09 a.m.
Can we type this while here, so it's not interpreted as a literal False?	chipx86	Feb. 15, 2024, 10:43 p.m.
Can we type this?	chipx86	Feb. 15, 2024, 10:44 p.m.
Let's pull this.model out into a local variable. We do an attribute lookup 10 times in this function.	chipx86	Feb. 15, 2024, 10:44 p.m.
Should the quotes just be around the captions? It feels like the revision information should be outside of that. Another …	chipx86	Feb. 15, 2024, 10:45 p.m.

flake8 failed.

JSHint passed.

flake8

reviewboard/reviews/ui/base.py (Diff revision 1)

The issue has been dropped. Show all issues

'housekeeping.functions.deprecate_non_keyword_only_args' imported but unused

Column: 1
Error code: F401

reviewboard/reviews/views/attachments.py (Diff revision 1)
The issue has been dropped. Show all issues
```
line too long (80 > 79 characters)

Column: 80
Error code: E501
```

Commits:

	Summary	ID
	Improve construction of ReviewUIs and show error for diff type mismatches. When instantiating review UIs for attachments with diffs, we would create two instances, and then call `set_diff_against` and discard one of them. We'd then reconstruct the second instance when rendering to set the `diffTypeMismatch` field in the JS model. This also had a bug where if the two revisions did not match in file types, we'd still always instantiate the review UI for the "modified" version, which would then fail in various ways on the client side depending on which particular ReviewUI was in charge. This change makes it so we find the ReviewUI classes for both the original and modified files first, and compare them. If they match, we can instantiate a single instance. If they don't, we instantiate a generic review UI that just sets that the type doesn't match. On the frontend, the `DummyReviewableView` (used for any ReviewUI that doesn't provide their own) has been fixed up and enhanced in a couple ways. First, the way that captions were displayed was backwards (showing the original file caption on the right). The caption table was also not properly styled so everything jammed up on the left. Finally, I've also added a new error message that shows up when `diffTypeMismatch` is set, to tell the user that we can't show a diff. The particular styling rules for the `DummyReviewableView` are a little ugly and just inserted into the reviews.less file. I plan to redo all the styling for all the review UIs in a later change. Testing Done: - Ran python tests. - Ran JS tests. - Created an attachment with multiple revisions that were all images. Saw that individual display and diffs still worked properly. - Created an attachment with multiple revisions where some revisions were images and some were text. Saw that when I selected a diff between incompatible file types, I got a review UI page that showed the correct file revision slider and captions, and the body had the new error about file types not matching.	1e93e0d53955939ce05e6e55cf3b618e5b2842f4
	Improve construction of ReviewUIs and show error for diff type mismatches. When instantiating review UIs for attachments with diffs, we would create two instances, and then call `set_diff_against` and discard one of them. We'd then reconstruct the second instance when rendering to set the `diffTypeMismatch` field in the JS model. This also had a bug where if the two revisions did not match in file types, we'd still always instantiate the review UI for the "modified" version, which would then fail in various ways on the client side depending on which particular ReviewUI was in charge. This change makes it so we find the ReviewUI classes for both the original and modified files first, and compare them. If they match, we can instantiate a single instance. If they don't, we instantiate a generic review UI that just sets that the type doesn't match. On the frontend, the `DummyReviewableView` (used for any ReviewUI that doesn't provide their own) has been fixed up and enhanced in a couple ways. First, the way that captions were displayed was backwards (showing the original file caption on the right). The caption table was also not properly styled so everything jammed up on the left. Finally, I've also added a new error message that shows up when `diffTypeMismatch` is set, to tell the user that we can't show a diff. The particular styling rules for the `DummyReviewableView` are a little ugly and just inserted into the reviews.less file. I plan to redo all the styling for all the review UIs in a later change. Testing Done: - Ran python tests. - Ran JS tests. - Created an attachment with multiple revisions that were all images. Saw that individual display and diffs still worked properly. - Created an attachment with multiple revisions where some revisions were images and some were text. Saw that when I selected a diff between incompatible file types, I got a review UI page that showed the correct file revision slider and captions, and the body had the new error about file types not matching.	37520aa9fd464337ac5659127acf0e3362087fa0

Diff:

Revision 2 (+280 -76)

Show changes

	reviewboard/reviews/tests/test_file_attachment_review_ui.py
	reviewboard/reviews/ui/base.py
	reviewboard/reviews/views/attachments.py
	reviewboard/static/rb/css/pages/reviews.less
	reviewboard/static/rb/js/reviews/models/fileAttachmentReviewableModel.ts
	reviewboard/static/rb/js/reviews/views/dummyReviewableView.ts

Checks run (2 succeeded)

flake8 passed.

JSHint passed.

reviewboard/reviews/ui/base.py (Diff revision 2)
The issue has been resolved. Show all issues
```
It looks like you didn't actually add this argument.
```

Commits:

	Summary	ID
	Improve construction of ReviewUIs and show error for diff type mismatches. When instantiating review UIs for attachments with diffs, we would create two instances, and then call `set_diff_against` and discard one of them. We'd then reconstruct the second instance when rendering to set the `diffTypeMismatch` field in the JS model. This also had a bug where if the two revisions did not match in file types, we'd still always instantiate the review UI for the "modified" version, which would then fail in various ways on the client side depending on which particular ReviewUI was in charge. This change makes it so we find the ReviewUI classes for both the original and modified files first, and compare them. If they match, we can instantiate a single instance. If they don't, we instantiate a generic review UI that just sets that the type doesn't match. On the frontend, the `DummyReviewableView` (used for any ReviewUI that doesn't provide their own) has been fixed up and enhanced in a couple ways. First, the way that captions were displayed was backwards (showing the original file caption on the right). The caption table was also not properly styled so everything jammed up on the left. Finally, I've also added a new error message that shows up when `diffTypeMismatch` is set, to tell the user that we can't show a diff. The particular styling rules for the `DummyReviewableView` are a little ugly and just inserted into the reviews.less file. I plan to redo all the styling for all the review UIs in a later change. Testing Done: - Ran python tests. - Ran JS tests. - Created an attachment with multiple revisions that were all images. Saw that individual display and diffs still worked properly. - Created an attachment with multiple revisions where some revisions were images and some were text. Saw that when I selected a diff between incompatible file types, I got a review UI page that showed the correct file revision slider and captions, and the body had the new error about file types not matching.	37520aa9fd464337ac5659127acf0e3362087fa0
	Improve construction of ReviewUIs and show error for diff type mismatches. When instantiating review UIs for attachments with diffs, we would create two instances, and then call `set_diff_against` and discard one of them. We'd then reconstruct the second instance when rendering to set the `diffTypeMismatch` field in the JS model. This also had a bug where if the two revisions did not match in file types, we'd still always instantiate the review UI for the "modified" version, which would then fail in various ways on the client side depending on which particular ReviewUI was in charge. This change makes it so we find the ReviewUI classes for both the original and modified files first, and compare them. If they match, we can instantiate a single instance. If they don't, we instantiate a generic review UI that just sets that the type doesn't match. On the frontend, the `DummyReviewableView` (used for any ReviewUI that doesn't provide their own) has been fixed up and enhanced in a couple ways. First, the way that captions were displayed was backwards (showing the original file caption on the right). The caption table was also not properly styled so everything jammed up on the left. Finally, I've also added a new error message that shows up when `diffTypeMismatch` is set, to tell the user that we can't show a diff. The particular styling rules for the `DummyReviewableView` are a little ugly and just inserted into the reviews.less file. I plan to redo all the styling for all the review UIs in a later change. Testing Done: - Ran python tests. - Ran JS tests. - Created an attachment with multiple revisions that were all images. Saw that individual display and diffs still worked properly. - Created an attachment with multiple revisions where some revisions were images and some were text. Saw that when I selected a diff between incompatible file types, I got a review UI page that showed the correct file revision slider and captions, and the body had the new error about file types not matching.	ff5f9cea55893de3188ced416f61a20722b2edfb

Diff:

Revision 3 (+272 -76)

Show changes

	reviewboard/reviews/tests/test_file_attachment_review_ui.py
	reviewboard/reviews/ui/base.py
	reviewboard/reviews/views/attachments.py
	reviewboard/static/rb/css/pages/reviews.less
	reviewboard/static/rb/js/reviews/models/fileAttachmentReviewableModel.ts
	reviewboard/static/rb/js/reviews/views/dummyReviewableView.ts

Checks run (2 succeeded)

flake8 passed.

JSHint passed.

Ship it!

```
Ship It!
```

reviewboard/reviews/ui/base.py (Diff revision 3)
The issue has been resolved. Show all issues
```
Can we type this while here, so it's not interpreted as a literal False?
```
reviewboard/reviews/views/attachments.py (Diff revision 3)
The issue has been resolved. Show all issues
```
Can we type this?
```

reviewboard/static/rb/js/reviews/views/dummyReviewableView.ts (Diff revision 3)

The issue has been resolved. Show all issues

Let's pull this.model out into a local variable. We do an attribute lookup 10 times in this function.

reviewboard/static/rb/js/reviews/views/dummyReviewableView.ts (Diff revision 3)

The issue has been resolved. Show all issues

Should the quotes just be around the captions? It feels like the revision information should be outside of that.

Another approach would be to mark up that information in bold or something, so it's more readily identifiable as something other than just text in this string.

Commits:

	Summary	ID
	Improve construction of ReviewUIs and show error for diff type mismatches. When instantiating review UIs for attachments with diffs, we would create two instances, and then call `set_diff_against` and discard one of them. We'd then reconstruct the second instance when rendering to set the `diffTypeMismatch` field in the JS model. This also had a bug where if the two revisions did not match in file types, we'd still always instantiate the review UI for the "modified" version, which would then fail in various ways on the client side depending on which particular ReviewUI was in charge. This change makes it so we find the ReviewUI classes for both the original and modified files first, and compare them. If they match, we can instantiate a single instance. If they don't, we instantiate a generic review UI that just sets that the type doesn't match. On the frontend, the `DummyReviewableView` (used for any ReviewUI that doesn't provide their own) has been fixed up and enhanced in a couple ways. First, the way that captions were displayed was backwards (showing the original file caption on the right). The caption table was also not properly styled so everything jammed up on the left. Finally, I've also added a new error message that shows up when `diffTypeMismatch` is set, to tell the user that we can't show a diff. The particular styling rules for the `DummyReviewableView` are a little ugly and just inserted into the reviews.less file. I plan to redo all the styling for all the review UIs in a later change. Testing Done: - Ran python tests. - Ran JS tests. - Created an attachment with multiple revisions that were all images. Saw that individual display and diffs still worked properly. - Created an attachment with multiple revisions where some revisions were images and some were text. Saw that when I selected a diff between incompatible file types, I got a review UI page that showed the correct file revision slider and captions, and the body had the new error about file types not matching.	ff5f9cea55893de3188ced416f61a20722b2edfb
	Improve construction of ReviewUIs and show error for diff type mismatches. When instantiating review UIs for attachments with diffs, we would create two instances, and then call `set_diff_against` and discard one of them. We'd then reconstruct the second instance when rendering to set the `diffTypeMismatch` field in the JS model. This also had a bug where if the two revisions did not match in file types, we'd still always instantiate the review UI for the "modified" version, which would then fail in various ways on the client side depending on which particular ReviewUI was in charge. This change makes it so we find the ReviewUI classes for both the original and modified files first, and compare them. If they match, we can instantiate a single instance. If they don't, we instantiate a generic review UI that just sets that the type doesn't match. On the frontend, the `DummyReviewableView` (used for any ReviewUI that doesn't provide their own) has been fixed up and enhanced in a couple ways. First, the way that captions were displayed was backwards (showing the original file caption on the right). The caption table was also not properly styled so everything jammed up on the left. Finally, I've also added a new error message that shows up when `diffTypeMismatch` is set, to tell the user that we can't show a diff. The particular styling rules for the `DummyReviewableView` are a little ugly and just inserted into the reviews.less file. I plan to redo all the styling for all the review UIs in a later change. Testing Done: - Ran python tests. - Ran JS tests. - Created an attachment with multiple revisions that were all images. Saw that individual display and diffs still worked properly. - Created an attachment with multiple revisions where some revisions were images and some were text. Saw that when I selected a diff between incompatible file types, I got a review UI page that showed the correct file revision slider and captions, and the body had the new error about file types not matching.	7fd8745e5b4e20742ed5f85f091a45566f88c296

Diff:

Revision 4 (+286 -86)

Show changes

	reviewboard/reviews/tests/test_file_attachment_review_ui.py
	reviewboard/reviews/ui/base.py
	reviewboard/reviews/views/attachments.py
	reviewboard/static/rb/css/pages/reviews.less
	reviewboard/static/rb/js/reviews/models/fileAttachmentReviewableModel.ts
	reviewboard/static/rb/js/reviews/views/dummyReviewableView.ts

Checks run (2 succeeded)

flake8 passed.

JSHint passed.

Ship it!

```
Ship It!
```

Status: Closed (submitted)

Change Summary:

Pushed to release-7.x (d81af44)