Summary

Move functionality from FileAttachmentReviewUI into base ReviewUI.

Review Request #13525 — Created Feb. 6, 2024 and submitted Feb. 15, 2024, 9:06 a.m.

Information

Owner

david

Repository

Review Board

Branch

master

Bugs

Depends On

Reviewers

Groups

reviewboard

People

Description

In order to make Review UI classes more flexible, we're going to

consolidate all the functionality down into the base class. We currently

had the ReviewUI class which has an opaque obj member, and the

FileAttachmentReviewUI class which assumes that self.obj is a

FileAttachment. This change takes the functionality from

FileAttachmentReviewUI and moves it into the base class,

conditionalizing it based on the type of self.obj. This will allow us

to add additional conditions for other types of objects, such as

allowing it to be a FileDiff.
This also adds a new flag to the ReviewUI classes for whether they

support operating on a FileAttachment. This is used when selecting the

best ReviewUI for a given object.

Testing Done


Ran unit tests.
Tested to make sure Review UIs for common file types (images,

  markdown) still worked correctly.

Commits

Summary	ID
Move functionality from FileAttachmentReviewUI into base ReviewUI. In order to make Review UI classes more flexible, we're going to consolidate all the functionality down into the base class. We currently had the `ReviewUI` class which has an opaque `obj` member, and the `FileAttachmentReviewUI` class which assumes that `self.obj` is a `FileAttachment`. This change takes the functionality from `FileAttachmentReviewUI` and moves it into the base class, conditionalizing it based on the type of `self.obj`. This will allow us to add additional conditions for other types of objects, such as allowing it to be a `FileDiff`. This also adds a new flag to the ReviewUI classes for whether they support operating on a `FileAttachment`. This is used when selecting the best ReviewUI for a given object. Testing Done: - Ran unit tests. - Tested to make sure Review UIs for common file types (images, markdown) still worked correctly.	39f014dad0dde611d23886aaa48467f89482e558

Summary

Move functionality from FileAttachmentReviewUI into base ReviewUI.

In order to make Review UI classes more flexible, we're going to consolidate all the functionality down into the base class. We currently had the `ReviewUI` class which has an opaque `obj` member, and the `FileAttachmentReviewUI` class which assumes that `self.obj` is a `FileAttachment`. This change takes the functionality from `FileAttachmentReviewUI` and moves it into the base class, conditionalizing it based on the type of `self.obj`. This will allow us to add additional conditions for other types of objects, such as allowing it to be a `FileDiff`. This also adds a new flag to the ReviewUI classes for whether they support operating on a `FileAttachment`. This is used when selecting the best ReviewUI for a given object. Testing Done: - Ran unit tests. - Tested to make sure Review UIs for common file types (images, markdown) still worked correctly.

39f014dad0dde611d23886aaa48467f89482e558

Issues

Description	From	Last Updated
line too long (93 > 79 characters) Column: 80 Error code: E501	reviewbot	Feb. 6, 2024, 3:55 p.m.
line too long (82 > 79 characters) Column: 80 Error code: E501	reviewbot	Feb. 6, 2024, 3:55 p.m.
do not compare types, for exact checks use `is` / `is not`, for instance checks use `isinstance()` Column: 20 Error …	reviewbot	Feb. 6, 2024, 3:55 p.m.
line too long (80 > 79 characters) Column: 80 Error code: E501	reviewbot	Feb. 6, 2024, 3:56 p.m.
Typo "RevieUII" -> "ReviewUI"	maubin	Feb. 15, 2024, 9:05 a.m.
Shouldn't this be typed as Optional[object]?	maubin	Feb. 15, 2024, 9:05 a.m.
Add , optional to these args.	maubin	Feb. 15, 2024, 9:06 a.m.
do not compare types, for exact checks use `is` / `is not`, for instance checks use `isinstance()` Column: 20 Error …	reviewbot	Feb. 6, 2024, 3:57 p.m.
Wrap ReviewUI with :py:class:	maubin	Feb. 15, 2024, 9:06 a.m.

flake8 failed.

JSHint passed.

flake8

reviewboard/reviews/ui/base.py (Diff revision 1)
The issue has been resolved. Show all issues
```
line too long (93 > 79 characters)

Column: 80
Error code: E501
```
reviewboard/reviews/ui/base.py (Diff revision 1)
The issue has been dropped. Show all issues
```
line too long (82 > 79 characters)

Column: 80
Error code: E501
```

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

The issue has been dropped. Show all issues

do not compare types, for exact checks use `is` / `is not`, for instance checks use `isinstance()`

Column: 20
Error code: E721

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

Column: 80
Error code: E501
```

Commits:

	Summary	ID
	Move functionality from FileAttachmentReviewUI into base ReviewUI. In order to make Review UI classes more flexible, we're going to consolidate all the functionality down into the base class. We currently had the `ReviewUI` class which has an opaque `obj` member, and the `FileAttachmentReviewUI` class which assumes that `self.obj` is a `FileAttachment`. This change takes the functionality from `FileAttachmentReviewUI` and moves it into the base class, conditionalizing it based on the type of `self.obj`. This will allow us to add additional conditions for other types of objects, such as allowing it to be a `FileDiff`. This also adds a new flag to the ReviewUI classes for whether they support operating on a `FileAttachment`. This is used when selecting the best ReviewUI for a given object. Testing Done: - Ran unit tests. - Tested to make sure Review UIs for common file types (images, markdown) still worked correctly.	c7ea3fa6c5c100211972ed31d9813eacdf2cbc7c
	Move functionality from FileAttachmentReviewUI into base ReviewUI. In order to make Review UI classes more flexible, we're going to consolidate all the functionality down into the base class. We currently had the `ReviewUI` class which has an opaque `obj` member, and the `FileAttachmentReviewUI` class which assumes that `self.obj` is a `FileAttachment`. This change takes the functionality from `FileAttachmentReviewUI` and moves it into the base class, conditionalizing it based on the type of `self.obj`. This will allow us to add additional conditions for other types of objects, such as allowing it to be a `FileDiff`. This also adds a new flag to the ReviewUI classes for whether they support operating on a `FileAttachment`. This is used when selecting the best ReviewUI for a given object. Testing Done: - Ran unit tests. - Tested to make sure Review UIs for common file types (images, markdown) still worked correctly.	39f014dad0dde611d23886aaa48467f89482e558

Diff:

Revision 2 (+944 -872)

Show changes

	docs/manual/extending/extensions/review-ui.rst
	reviewboard/attachments/models.py
	reviewboard/extensions/hooks/review_ui.py
	reviewboard/reviews/builtin_fields.py
	reviewboard/reviews/models/file_attachment_comment.py
	reviewboard/reviews/tests/test_file_attachment_review_ui.py
	reviewboard/reviews/tests/test_review_ui.py
	reviewboard/reviews/ui/base.py
	4 more

Checks run (1 failed, 1 succeeded)

flake8 failed.

JSHint passed.

flake8

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

The issue has been dropped. Show all issues

do not compare types, for exact checks use `is` / `is not`, for instance checks use `isinstance()`

Column: 20
Error code: E721

reviewboard/reviews/tests/test_review_ui.py (Diff revision 2)
The issue has been resolved. Show all issues
```
Typo "RevieUII" -> "ReviewUI"
```
reviewboard/reviews/ui/base.py (Diff revision 2)
The issue has been resolved. Show all issues
```
Shouldn't this be typed as Optional[object]?
```
reviewboard/reviews/ui/base.py (Diff revision 2)
The issue has been resolved. Show all issues
```
Add  , optional to these args.
```
reviewboard/reviews/ui/base.py (Diff revision 2)
The issue has been resolved. Show all issues
```
Wrap ReviewUI with :py:class:
```

Review Board 7.0 alpha 0 (dev)

Move functionality from FileAttachmentReviewUI into base ReviewUI.

Information

Reviewers

flake8

Commits:

Diff:

Checks run (1 failed, 1 succeeded)

flake8

Status: Closed (submitted)