Summary

Comment serialization cleanup part 2: Clarify serialization structures.

Review Request #13656 — Created March 21, 2024 and submitted April 10, 2024, 1:20 p.m.

Information

Owner

david

Repository

Review Board

Branch

release-7.x

Bugs

Depends On

Reviewers

Groups

reviewboard

People

Description

This also makes the serialization stage within the Review UIs more
consistent. One of the giant warts we've had is that the basic ReviewUI
implementation would serialize the comments as a list of dicts, and then
our subclasses would serialize them as a dict, where the key was some
logical grouping (such as x/y/width/height for region comments, or
line/span for text comments) and the value was a list of serialized
comments for that region. We now always serialize to the dict form, and
the base ReviewUI will just use indices for the key and single-value
lists for the values.

A new method has been added to the base ReviewUI to facilitate this, and
just filters and yields the comments. Subclasses can then use this and
sort them however they like. Using this new method fixes a bug in the
TextBasedReviewUI where we weren't doing filtering, and draft comments
from other users could show up when they shouldn't.

Testing Done

Ran js-tests.

Commits

Summary	ID
Comment serialization cleanup part 2: Clarify serialization structures. This is part 2 of the serialization cleanup. In this change, we now have matching structures for serialized comments in Python (defined in the Review UI files) and JavaScript (defined in the commentData.ts file). These are fully documented and include cross-references to ensure that we keep them in sync. This also makes the serialization stage within the Review UIs more consistent. One of the giant warts we've had is that the basic ReviewUI implementation would serialize the comments as a list of dicts, and then our subclasses would serialize them as a dict, where the key was some logical grouping (such as x/y/width/height for region comments, or line/span for text comments) and the value was a list of serialized comments for that region. We now always serialize to the dict form, and the base ReviewUI will just use indices for the key and single-value lists for the values. A new method has been added to the base ReviewUI to facilitate this, and just filters and yields the comments. Subclasses can then use this and sort them however they like. Using this new method fixes a bug in the `TextBasedReviewUI` where we weren't doing filtering, and draft comments from other users could show up when they shouldn't. Testing Done: Ran js-tests.	8ed7bcfe26b0f4152dab8462a36ef137db433f6a

Summary

Comment serialization cleanup part 2: Clarify serialization structures.

This is part 2 of the serialization cleanup. In this change, we now have matching structures for serialized comments in Python (defined in the Review UI files) and JavaScript (defined in the commentData.ts file). These are fully documented and include cross-references to ensure that we keep them in sync. This also makes the serialization stage within the Review UIs more consistent. One of the giant warts we've had is that the basic ReviewUI implementation would serialize the comments as a list of dicts, and then our subclasses would serialize them as a dict, where the key was some logical grouping (such as x/y/width/height for region comments, or line/span for text comments) and the value was a list of serialized comments for that region. We now always serialize to the dict form, and the base ReviewUI will just use indices for the key and single-value lists for the values. A new method has been added to the base ReviewUI to facilitate this, and just filters and yields the comments. Subclasses can then use this and sort them however they like. Using this new method fixes a bug in the `TextBasedReviewUI` where we weren't doing filtering, and draft comments from other users could show up when they shouldn't. Testing Done: Ran js-tests.

8ed7bcfe26b0f4152dab8462a36ef137db433f6a

Issues

Description	From	Last Updated
The second argument can be deserialized data to compare. If it's a string, it gets loaded, but we don't need …	chipx86	April 1, 2024, 9:48 p.m.
Maybe worth just comparing the entire payloads going forward?	chipx86	April 1, 2024, 9:54 p.m.
Let's use :file: for this.	chipx86	April 1, 2024, 9:49 p.m.
Let's use :file: for this.	chipx86	April 1, 2024, 9:49 p.m.
This is missing docs.	chipx86	April 1, 2024, 9:49 p.m.
We don't normally use the outer parens here.	chipx86	April 1, 2024, 9:50 p.m.
Should use :file:.	chipx86	April 1, 2024, 9:50 p.m.
We have the nice new embedding in dictionaries now, so we could do: return { super().serialize_comment(comment), 'x': comment.x, 'y': …	chipx86	April 1, 2024, 9:51 p.m.
:file:	chipx86	April 1, 2024, 9:51 p.m.
This should be before any classes/functions.	chipx86	April 1, 2024, 9:51 p.m.
Version Added?	chipx86	April 1, 2024, 9:52 p.m.
This should use the standard multi-line comment format. Maybe add a TODO or TODO [some-tag] here?	chipx86	April 1, 2024, 9:53 p.m.
:file:	chipx86	April 1, 2024, 9:54 p.m.
:file:	chipx86	April 1, 2024, 9:54 p.m.
:file:	chipx86	April 1, 2024, 9:54 p.m.

flake8 passed.

JSHint passed.

reviewboard/reviews/tests/test_review_ui.py (Diff revision 1)

The issue has been resolved. Show all issues

The second argument can be deserialized data to compare. If it's a string, it gets loaded, but we don't need to provide it as a string.

reviewboard/reviews/tests/test_review_ui.py (Diff revision 1)

The issue has been dropped. Show all issues

Maybe worth just comparing the entire payloads going forward?

david April 1, 2024, 10:03 p.m.

Perhaps, but there's a lot of data there. I think for now I'd rather just keep this as-is.

reviewboard/reviews/ui/base.py (Diff revision 1)
The issue has been resolved. Show all issues
```
Let's use :file: for this.
```
reviewboard/reviews/ui/base.py (Diff revision 1)
The issue has been resolved. Show all issues
```
Let's use :file: for this.
```
reviewboard/reviews/ui/base.py (Diff revision 1)
The issue has been resolved. Show all issues
```
This is missing docs.
```
reviewboard/reviews/ui/base.py (Diff revision 1)
The issue has been resolved. Show all issues
```
We don't normally use the outer parens here.
```
reviewboard/reviews/ui/image.py (Diff revision 1)
The issue has been resolved. Show all issues
```
Should use :file:.
```

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

The issue has been resolved. Show all issues

We have the nice new ** embedding in dictionaries now, so we could do:

return {
    **super().serialize_comment(comment),
    'x': comment.x,
    'y': comment.y,
    'width': comment.w,
    'height': comment.h,
}

reviewboard/reviews/ui/text.py (Diff revision 1)
The issue has been resolved. Show all issues
```
:file:
```
reviewboard/reviews/ui/text.py (Diff revision 1)
The issue has been resolved. Show all issues
```
This should be before any classes/functions.
```
reviewboard/static/rb/js/reviews/models/abstractReviewableModel.ts (Diff revision 1)
The issue has been resolved. Show all issues
```
Version Added?
```

reviewboard/static/rb/js/reviews/models/abstractReviewableModel.ts (Diff revision 1)

The issue has been dropped. Show all issues

This should use the standard multi-line comment format.

Maybe add a TODO or TODO [some-tag] here?

david April 1, 2024, 10:03 p.m.

I'm going to drop this because this goes away in a later change in the series.

reviewboard/static/rb/js/reviews/models/commentData.ts (Diff revision 1)
The issue has been resolved. Show all issues
```
:file:
```
reviewboard/static/rb/js/reviews/models/commentData.ts (Diff revision 1)
The issue has been resolved. Show all issues
```
:file:
```
reviewboard/static/rb/js/reviews/models/commentData.ts (Diff revision 1)
The issue has been resolved. Show all issues
```
:file:
```

Commits:

	Summary	ID
	Comment serialization cleanup part 2: Clarify serialization structures. This is part 2 of the serialization cleanup. In this change, we now have matching structures for serialized comments in Python (defined in the Review UI files) and JavaScript (defined in the commentData.ts file). These are fully documented and include cross-references to ensure that we keep them in sync. This also makes the serialization stage within the Review UIs more consistent. One of the giant warts we've had is that the basic ReviewUI implementation would serialize the comments as a list of dicts, and then our subclasses would serialize them as a dict, where the key was some logical grouping (such as x/y/width/height for region comments, or line/span for text comments) and the value was a list of serialized comments for that region. We now always serialize to the dict form, and the base ReviewUI will just use indices for the key and single-value lists for the values. A new method has been added to the base ReviewUI to facilitate this, and just filters and yields the comments. Subclasses can then use this and sort them however they like. Using this new method fixes a bug in the TextBasedReviewUI where we weren't doing filtering, and draft comments from other users could show up when they shouldn't. Testing Done: Ran js-tests.	6c61183d0737a4bdc0e68a2c90917aaebd35db14
	Comment serialization cleanup part 2: Clarify serialization structures. This is part 2 of the serialization cleanup. In this change, we now have matching structures for serialized comments in Python (defined in the Review UI files) and JavaScript (defined in the commentData.ts file). These are fully documented and include cross-references to ensure that we keep them in sync. This also makes the serialization stage within the Review UIs more consistent. One of the giant warts we've had is that the basic ReviewUI implementation would serialize the comments as a list of dicts, and then our subclasses would serialize them as a dict, where the key was some logical grouping (such as x/y/width/height for region comments, or line/span for text comments) and the value was a list of serialized comments for that region. We now always serialize to the dict form, and the base ReviewUI will just use indices for the key and single-value lists for the values. A new method has been added to the base ReviewUI to facilitate this, and just filters and yields the comments. Subclasses can then use this and sort them however they like. Using this new method fixes a bug in the `TextBasedReviewUI` where we weren't doing filtering, and draft comments from other users could show up when they shouldn't. Testing Done: Ran js-tests.	8ed7bcfe26b0f4152dab8462a36ef137db433f6a

Diff:

Revision 2 (+776 -288)

Show changes

	reviewboard/reviews/tests/test_review_ui.py
	reviewboard/reviews/ui/base.py
	reviewboard/reviews/ui/image.py
	reviewboard/reviews/ui/screenshot.py
	reviewboard/reviews/ui/text.py
	reviewboard/static/rb/js/reviews/models/abstractReviewableModel.ts
	reviewboard/static/rb/js/reviews/models/commentData.ts
	reviewboard/static/rb/js/reviews/models/fileAttachmentReviewableModel.ts

Checks run (2 succeeded)

flake8 passed.

JSHint passed.

Ship it!

```
Ship It!
```

Ship it!

```
Ship It!
```

Status: Closed (submitted)

Change Summary:

Pushed to release-7.x (9d4b4cb)