Summary

Allow commenting on draft (unpublished) items.

Review Request #12949 — Created April 12, 2023 and submitted April 25, 2023, 11:42 a.m.

Information

Owner

david

Repository

Review Board

Branch

release-6.x

Bugs

Depends On

Reviewers

Groups

reviewboard

People

Description

For as long as Review Board has existed, we've disallowed commenting on

items before they're published. The reasoning here was that if you add a

diff comment, replacing the diff will delete the comment (and similar

for file attachments).
This change updates the comment dialog to remove the hasDraft checks.

This is replaced with a new mechanism whereby we add a public state to

diffs and file attachments, and use that to show a warning to the user

if they're commenting on something which has not yet been published. The

bulk of the code changes here are just plumbing that state all the way

down to the comment block models, which use it to decide what to display

to the user.

Testing Done


Ran unit tests.
Created review requests with published and unpublished diffs and file

  attachments. Checked that adding a comment to published items worked

  as normal, and that the draft warnings were correctly shown for draft

  diffs and file attachments, as well as for interdiffs and file

  attachment diffs involving an unpublished tip revision.

Commits

Summary	ID
Allow commenting on draft (unpublished) items. For as long as Review Board has existed, we've disallowed commenting on items before they're published. The reasoning here was that if you add a diff comment, replacing the diff will delete the comment (and similar for file attachments). This change updates the comment dialog to remove the `hasDraft` checks. This is replaced with a new mechanism whereby we add a `public` state to diffs and file attachments, and use that to show a warning to the user if they're commenting on something which has not yet been published. The bulk of the code changes here are just plumbing that state all the way down to the comment block models, which use it to decide what to display to the user. Testing Done: - Ran unit tests. - Created review requests with published and unpublished diffs and file attachments. Checked that adding a comment to published items worked as normal, and that the draft warnings were correctly shown for draft diffs and file attachments, as well as for interdiffs and file attachment diffs involving an unpublished tip revision.	5ae837284a1299d99ebcb65ecd4757837c8de21e

Summary

Allow commenting on draft (unpublished) items.

For as long as Review Board has existed, we've disallowed commenting on items before they're published. The reasoning here was that if you add a diff comment, replacing the diff will delete the comment (and similar for file attachments). This change updates the comment dialog to remove the `hasDraft` checks. This is replaced with a new mechanism whereby we add a `public` state to diffs and file attachments, and use that to show a warning to the user if they're commenting on something which has not yet been published. The bulk of the code changes here are just plumbing that state all the way down to the comment block models, which use it to decide what to display to the user. Testing Done: - Ran unit tests. - Created review requests with published and unpublished diffs and file attachments. Checked that adding a comment to published items worked as normal, and that the draft warnings were correctly shown for draft diffs and file attachments, as well as for interdiffs and file attachment diffs involving an unpublished tip revision.

5ae837284a1299d99ebcb65ecd4757837c8de21e

Issues

Description	From	Last Updated
Do comments get deleted when the draft diff/file is replaced? I think we should update the unit tests to confirm …	maubin	April 19, 2023, 3:23 p.m.
line too long (80 > 79 characters) Column: 80 Error code: E501	reviewbot	April 12, 2023, 9:57 a.m.
If comments get deleted when a draft diff is replaced, I think we should explicitely say delete here instead of …	maubin	April 17, 2023, 12:20 p.m.
If comments get deleted when the file is replaced, I think we should explicitely say delete here instead of invalidate.	maubin	April 17, 2023, 12:21 p.m.

flake8 failed.

JSHint passed.

flake8

reviewboard/diffviewer/tests/test_diffutils.py (Diff revision 1)
Show all issues
```
line too long (80 > 79 characters)

Column: 80
Error code: E501
```

Commits:

	Summary	ID
	Allow commenting on draft (unpublished) items. For as long as Review Board has existed, we've disallowed commenting on items before they're published. The reasoning here was that if you add a diff comment, replacing the diff will delete the comment (and similar for file attachments). This change updates the comment dialog to remove the `hasDraft` checks. This is replaced with a new mechanism whereby we add a `public` state to diffs and file attachments, and use that to show a warning to the user if they're commenting on something which has not yet been published. The bulk of the code changes here are just plumbing that state all the way down to the comment block models, which use it to decide what to display to the user. Testing Done: - Ran unit tests. - Created review requests with published and unpublished diffs and file attachments. Checked that adding a comment to published items worked as normal, and that the draft warnings were correctly shown for draft diffs and file attachments, as well as for interdiffs and file attachment diffs involving an unpublished tip revision.	26fa6a100ab2e62d3731c18c2642d729bdf3b79c
	Allow commenting on draft (unpublished) items. For as long as Review Board has existed, we've disallowed commenting on items before they're published. The reasoning here was that if you add a diff comment, replacing the diff will delete the comment (and similar for file attachments). This change updates the comment dialog to remove the `hasDraft` checks. This is replaced with a new mechanism whereby we add a `public` state to diffs and file attachments, and use that to show a warning to the user if they're commenting on something which has not yet been published. The bulk of the code changes here are just plumbing that state all the way down to the comment block models, which use it to decide what to display to the user. Testing Done: - Ran unit tests. - Created review requests with published and unpublished diffs and file attachments. Checked that adding a comment to published items worked as normal, and that the draft warnings were correctly shown for draft diffs and file attachments, as well as for interdiffs and file attachment diffs involving an unpublished tip revision.	8ae716c43d8bb0a823b76c91936d6b228d6dcb81

Diff:

Revision 2 (+454 -44)

Show changes

	.eslintrc.yaml
	reviewboard/diffviewer/diffutils.py
	reviewboard/diffviewer/tests/test_diffutils.py
	reviewboard/reviews/tests/test_file_attachment_review_ui.py
	reviewboard/reviews/ui/base.py
	reviewboard/reviews/views/diffviewer.py
	reviewboard/static/rb/js/diffviewer/collections/diffReviewableCollection.es6.js
	reviewboard/static/rb/js/diffviewer/models/diffCommentBlockModel.es6.js
	10 more

Checks run (2 succeeded)

flake8 passed.

JSHint passed.

Show all issues

Do comments get deleted when the draft diff/file is replaced? I think we should update the unit tests to confirm what happens after draft diffs/files get deleted while they have comments.

reviewboard/static/rb/js/diffviewer/models/diffCommentBlockModel.es6.js (Diff revision 2)

Show all issues

If comments get deleted when a draft diff is replaced, I think we should explicitely say delete here instead of invalidate.

reviewboard/static/rb/js/models/fileAttachmentCommentBlockModel.es6.js (Diff revision 2)

Show all issues

If comments get deleted when the file is replaced, I think we should explicitely say delete here instead of invalidate.

Commits:

	Summary	ID
	Allow commenting on draft (unpublished) items. For as long as Review Board has existed, we've disallowed commenting on items before they're published. The reasoning here was that if you add a diff comment, replacing the diff will delete the comment (and similar for file attachments). This change updates the comment dialog to remove the `hasDraft` checks. This is replaced with a new mechanism whereby we add a `public` state to diffs and file attachments, and use that to show a warning to the user if they're commenting on something which has not yet been published. The bulk of the code changes here are just plumbing that state all the way down to the comment block models, which use it to decide what to display to the user. Testing Done: - Ran unit tests. - Created review requests with published and unpublished diffs and file attachments. Checked that adding a comment to published items worked as normal, and that the draft warnings were correctly shown for draft diffs and file attachments, as well as for interdiffs and file attachment diffs involving an unpublished tip revision.	8ae716c43d8bb0a823b76c91936d6b228d6dcb81
	Allow commenting on draft (unpublished) items. For as long as Review Board has existed, we've disallowed commenting on items before they're published. The reasoning here was that if you add a diff comment, replacing the diff will delete the comment (and similar for file attachments). This change updates the comment dialog to remove the `hasDraft` checks. This is replaced with a new mechanism whereby we add a `public` state to diffs and file attachments, and use that to show a warning to the user if they're commenting on something which has not yet been published. The bulk of the code changes here are just plumbing that state all the way down to the comment block models, which use it to decide what to display to the user. Testing Done: - Ran unit tests. - Created review requests with published and unpublished diffs and file attachments. Checked that adding a comment to published items worked as normal, and that the draft warnings were correctly shown for draft diffs and file attachments, as well as for interdiffs and file attachment diffs involving an unpublished tip revision.	5ae837284a1299d99ebcb65ecd4757837c8de21e

Diff:

Revision 3 (+538 -48)

Show changes

	.eslintrc.yaml
	reviewboard/diffviewer/diffutils.py
	reviewboard/diffviewer/tests/test_diffutils.py
	reviewboard/reviews/tests/test_file_attachment_review_ui.py
	reviewboard/reviews/tests/test_review.py
	reviewboard/reviews/ui/base.py
	reviewboard/reviews/views/diffviewer.py
	reviewboard/static/rb/js/diffviewer/collections/diffReviewableCollection.es6.js
	11 more

Checks run (2 succeeded)

flake8 passed.

JSHint passed.

Ship it!

```
Ship It!
```

Status: Closed (submitted)

Change Summary:

Pushed to release-6.x (cb8c50c)