Summary

Fix ID collision in comment issue banners when resolving issues.

Review Request #12437 — Created July 5, 2022 and submitted July 6, 2022, 6:19 p.m.

Information

Owner

chipx86

Repository

Review Board

Branch

release-5.0.x

Bugs

Depends On

Reviewers

Groups

reviewboard

People

Description

This was due to a bad check in CommentIssueBarView. It listened for
state updates and assumed that if the IDs matched, that was good enough.
It also needed to consider the comment type.

In order to do that, we first needed a way to represent the types. We
had hard-coded strings like 'diff_comments', but nothing formal, and
no way to get to it from the event handler.

There's now a CommentIssueManager.CommentTypes mapping of type
constants to values. The appropriate value is determined and then
provided when triggering issueStatusChanged. This allows the event
handler to perform the proper match.

Unit tests have been updated to check this for each supported comment
type.

In the future, we'll want to consider rethinking some of the way this
works, moving toward a model where we work directly with the comment
objects (ensuring there's only one object per comment) rather than
a more broad listener approach (which requires each handler to
perform these checks).

Testing Done

Unit tests pass.

Reproduced the original problem in a new database, and verified that
this fixes the state and UI collision.

Commits

Summary	ID
Fix ID collision in comment issue banners when resolving issues. If a review request contains multiple comments of different types but with the same IDs (such as a General Comment and a Diff Comment both with ID 2), closing one of the comments would result in the other comment appearing to close as well. In fact, the comment did not close, it merely appeared to. This was due to a bad check in `CommentIssueBarView`. It listened for state updates and assumed that if the IDs matched, that was good enough. It also needed to consider the comment type. In order to do that, we first needed a way to represent the types. We had hard-coded strings like `'diff_comments'`, but nothing formal, and no way to get to it from the event handler. There's now a `CommentIssueManager.CommentTypes` mapping of type constants to values. The appropriate value is determined and then provided when triggering `issueStatusChanged`. This allows the event handler to perform the proper match. Unit tests have been updated to check this for each supported comment type.	9526cf0a189ed99a919c2920e741961e1a6b6558

Summary

Fix ID collision in comment issue banners when resolving issues.

If a review request contains multiple comments of different types but with the same IDs (such as a General Comment and a Diff Comment both with ID 2), closing one of the comments would result in the other comment appearing to close as well. In fact, the comment did not close, it merely appeared to. This was due to a bad check in `CommentIssueBarView`. It listened for state updates and assumed that if the IDs matched, that was good enough. It also needed to consider the comment type. In order to do that, we first needed a way to represent the types. We had hard-coded strings like `'diff_comments'`, but nothing formal, and no way to get to it from the event handler. There's now a `CommentIssueManager.CommentTypes` mapping of type constants to values. The appropriate value is determined and then provided when triggering `issueStatusChanged`. This allows the event handler to perform the proper match. Unit tests have been updated to check this for each supported comment type.

9526cf0a189ed99a919c2920e741961e1a6b6558

Issues

Description	From	Last Updated
Expected '===' and instead saw '=='. Column: 27 Error code: W116	reviewbot	July 5, 2022, 5:45 p.m.
A constructor name should start with an uppercase letter. Column: 41 Error code: W055	reviewbot	July 5, 2022, 5:45 p.m.
A constructor name should start with an uppercase letter. Column: 45 Error code: W055	reviewbot	July 5, 2022, 5:45 p.m.
A constructor name should start with an uppercase letter. Column: 45 Error code: W055	reviewbot	July 5, 2022, 5:45 p.m.
Could fix the typo here, "dispaly" -> "display"	maubin	July 6, 2022, 2:29 p.m.

flake8 passed.

JSHint failed.

JSHint

reviewboard/static/rb/js/views/commentIssueBarView.es6.js (Diff revision 1)
The issue has been resolved. Show all issues
```
Expected '===' and instead saw '=='.

Column: 27
Error code: W116
```
reviewboard/static/rb/js/views/tests/commentIssueBarViewTests.es6.js (Diff revision 1)
The issue has been resolved. Show all issues
```
A constructor name should start with an uppercase letter.

Column: 41
Error code: W055
```
reviewboard/static/rb/js/views/tests/commentIssueBarViewTests.es6.js (Diff revision 1)
The issue has been resolved. Show all issues
```
A constructor name should start with an uppercase letter.

Column: 45
Error code: W055
```
reviewboard/static/rb/js/views/tests/commentIssueBarViewTests.es6.js (Diff revision 1)
The issue has been resolved. Show all issues
```
A constructor name should start with an uppercase letter.

Column: 45
Error code: W055
```

Change Summary:


Fixed a comparison that was using == instead of ===.
Updated variables for comment class types to start with a capital letter.

Commits:

	Summary	ID
	Fix ID collision in comment issue banners when resolving issues. If a review request contains multiple comments of different types but with the same IDs (such as a General Comment and a Diff Comment both with ID 2), closing one of the comments would result in the other comment appearing to close as well. In fact, the comment did not close, it merely appeared to. This was due to a bad check in `CommentIssueBarView`. It listened for state updates and assumed that if the IDs matched, that was good enough. It also needed to consider the comment type. In order to do that, we first needed a way to represent the types. We had hard-coded strings like `'diff_comments'`, but nothing formal, and no way to get to it from the event handler. There's now a `CommentIssueManager.CommentTypes` mapping of type constants to values. The appropriate value is determined and then provided when triggering `issueStatusChanged`. This allows the event handler to perform the proper match. Unit tests have been updated to check this for each supported comment type.	33ca77ef52f1735dbe03bf60baa6d8423295ce46
	Fix ID collision in comment issue banners when resolving issues. If a review request contains multiple comments of different types but with the same IDs (such as a General Comment and a Diff Comment both with ID 2), closing one of the comments would result in the other comment appearing to close as well. In fact, the comment did not close, it merely appeared to. This was due to a bad check in `CommentIssueBarView`. It listened for state updates and assumed that if the IDs matched, that was good enough. It also needed to consider the comment type. In order to do that, we first needed a way to represent the types. We had hard-coded strings like `'diff_comments'`, but nothing formal, and no way to get to it from the event handler. There's now a `CommentIssueManager.CommentTypes` mapping of type constants to values. The appropriate value is determined and then provided when triggering `issueStatusChanged`. This allows the event handler to perform the proper match. Unit tests have been updated to check this for each supported comment type.	5000b65438672b4d2db746ddbf0825723602e456

Diff:

Revision 2 (+448 -78)

Show changes

	reviewboard/static/rb/js/models/commentIssueManagerModel.es6.js
	reviewboard/static/rb/js/views/commentIssueBarView.es6.js
	reviewboard/static/rb/js/views/tests/commentIssueBarViewTests.es6.js

Checks run (2 succeeded)

flake8 passed.

JSHint passed.

reviewboard/static/rb/js/views/commentIssueBarView.es6.js (Diff revision 2)
The issue has been resolved. Show all issues
```
Could fix the typo here, "dispaly" -> "display"
```

Change Summary:

Fixed a typo in the docs: dispaly -> display.

Commits:

	Summary	ID
	Fix ID collision in comment issue banners when resolving issues. If a review request contains multiple comments of different types but with the same IDs (such as a General Comment and a Diff Comment both with ID 2), closing one of the comments would result in the other comment appearing to close as well. In fact, the comment did not close, it merely appeared to. This was due to a bad check in `CommentIssueBarView`. It listened for state updates and assumed that if the IDs matched, that was good enough. It also needed to consider the comment type. In order to do that, we first needed a way to represent the types. We had hard-coded strings like `'diff_comments'`, but nothing formal, and no way to get to it from the event handler. There's now a `CommentIssueManager.CommentTypes` mapping of type constants to values. The appropriate value is determined and then provided when triggering `issueStatusChanged`. This allows the event handler to perform the proper match. Unit tests have been updated to check this for each supported comment type.	5000b65438672b4d2db746ddbf0825723602e456
	Fix ID collision in comment issue banners when resolving issues. If a review request contains multiple comments of different types but with the same IDs (such as a General Comment and a Diff Comment both with ID 2), closing one of the comments would result in the other comment appearing to close as well. In fact, the comment did not close, it merely appeared to. This was due to a bad check in `CommentIssueBarView`. It listened for state updates and assumed that if the IDs matched, that was good enough. It also needed to consider the comment type. In order to do that, we first needed a way to represent the types. We had hard-coded strings like `'diff_comments'`, but nothing formal, and no way to get to it from the event handler. There's now a `CommentIssueManager.CommentTypes` mapping of type constants to values. The appropriate value is determined and then provided when triggering `issueStatusChanged`. This allows the event handler to perform the proper match. Unit tests have been updated to check this for each supported comment type.	9526cf0a189ed99a919c2920e741961e1a6b6558

Diff:

Revision 3 (+450 -80)

Show changes

	reviewboard/static/rb/js/models/commentIssueManagerModel.es6.js
	reviewboard/static/rb/js/views/commentIssueBarView.es6.js
	reviewboard/static/rb/js/views/tests/commentIssueBarViewTests.es6.js

Checks run (2 succeeded)

flake8 passed.

JSHint passed.

Ship it!

```
Ship It!
```

Ship it!

```
Ship It!
```

Status:: Completed
Change Summary:: Pushed to release-5.0.x (4b04007)