Summary

Fix file attachment revision info with draft updates.

Review Request #14021 — Created July 10, 2024 and submitted Aug. 8, 2024, 8:59 a.m.

Information

Owner

david

Repository

Review Board

Branch

release-7.x

Bugs

Depends On

Reviewers

Groups

reviewboard

People

Description

This change makes it so we check the last attachment in the history to
see if it's a draft, and only include it in the revision list if that
draft should be visible.

Testing Done


Ran unit tests.
Created a draft with an updated revision to a file. Saw that the owner

  saw two revisions, a regular user saw one, and admins saw either one

  or two depending on if they were choosing to view draft data.

Commits

Summary	ID
Fix file attachment revision info with draft updates. When a file attachment has an updated revision in a pending draft, that revision could leak into the page for users not able to see the draft. If they then moved the revision slider to show the new revision, they'd get a 404. This change makes it so we check the last attachment in the history to see if it's a draft, and only include it in the revision list if that draft should be visible. Testing Done: - Ran unit tests. - Created a draft with an updated revision to a file. Saw that the owner saw two revisions, a regular user saw one, and admins saw either one or two depending on if they were choosing to view draft data.	012b6fe552ad20450fcd500a6d7259c93f6a9e8a

Summary

Fix file attachment revision info with draft updates.

When a file attachment has an updated revision in a pending draft, that revision could leak into the page for users not able to see the draft. If they then moved the revision slider to show the new revision, they'd get a 404. This change makes it so we check the last attachment in the history to see if it's a draft, and only include it in the revision list if that draft should be visible. Testing Done: - Ran unit tests. - Created a draft with an updated revision to a file. Saw that the owner saw two revisions, a regular user saw one, and admins saw either one or two depending on if they were choosing to view draft data.

012b6fe552ad20450fcd500a6d7259c93f6a9e8a

Issues

Description	From	Last Updated
continuation line under-indented for visual indent Column: 21 Error code: E128	reviewbot	July 10, 2024, 10:12 p.m.
line too long (80 > 79 characters) Column: 80 Error code: E501	reviewbot	July 10, 2024, 10:12 p.m.
closing bracket does not match indentation of opening bracket's line Column: 25 Error code: E123	reviewbot	July 10, 2024, 10:12 p.m.
Left-over debug code.	chipx86	July 15, 2024, 9:35 p.m.
Can we use keyword arguments to help self-document this? We should also explicitly compare request against None, to ensure a …	chipx86	July 15, 2024, 9:36 p.m.
Let's protect against an empty list of files. I know we have a FileAttachmentHistory, but I'd feel more comfortable if …	chipx86	July 15, 2024, 9:37 p.m.
I had to re-read this several times. It's late, so that's maybe part of it, but I think this might …	chipx86	July 31, 2024, 10:07 a.m.

flake8 failed.

JSHint passed.

flake8

reviewboard/reviews/ui/base.py (Diff revision 1)
The issue has been dropped. Show all issues
```
continuation line under-indented for visual indent

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

Column: 80
Error code: E501
```

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

The issue has been dropped. Show all issues

closing bracket does not match indentation of opening bracket's line

Column: 25
Error code: E123

Commits:

	Summary	ID
	Fix file attachment revision info with draft updates. When a file attachment has an updated revision in a pending draft, that revision could leak into the page for users not able to see the draft. If they then moved the revision slider to show the new revision, they'd get a 404. This change makes it so we check the last attachment in the history to see if it's a draft, and only include it in the revision list if that draft should be visible. Testing Done: - Ran unit tests. - Created a draft with an updated revision to a file. Saw that the owner saw two revisions, a regular user saw one, and admins saw either one or two depending on if they were choosing to view draft data.	eb3dbcf840e82da0cecd841e042ef01794af5a7e
	Fix file attachment revision info with draft updates. When a file attachment has an updated revision in a pending draft, that revision could leak into the page for users not able to see the draft. If they then moved the revision slider to show the new revision, they'd get a 404. This change makes it so we check the last attachment in the history to see if it's a draft, and only include it in the revision list if that draft should be visible. Testing Done: - Ran unit tests. - Created a draft with an updated revision to a file. Saw that the owner saw two revisions, a regular user saw one, and admins saw either one or two depending on if they were choosing to view draft data.	51e65e7385d78587884327155a016d8741dc4b2a

Diff:

Revision 2 (+140 -14)

Show changes

	reviewboard/reviews/tests/test_file_attachment_review_ui.py
	reviewboard/reviews/ui/base.py

Checks run (2 succeeded)

flake8 passed.

JSHint passed.

reviewboard/reviews/ui/base.py (Diff revision 2)
The issue has been resolved. Show all issues
```
Left-over debug code.
```

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

The issue has been resolved. Show all issues

Can we use keyword arguments to help self-document this?

We should also explicitly compare request against None, to ensure a bool result.

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

The issue has been resolved. Show all issues

Let's protect against an empty list of files. I know we have a FileAttachmentHistory, but I'd feel more comfortable if we safeguard against something having gone wrong with entries here.

Commits:

	Summary	ID
	Fix file attachment revision info with draft updates. When a file attachment has an updated revision in a pending draft, that revision could leak into the page for users not able to see the draft. If they then moved the revision slider to show the new revision, they'd get a 404. This change makes it so we check the last attachment in the history to see if it's a draft, and only include it in the revision list if that draft should be visible. Testing Done: - Ran unit tests. - Created a draft with an updated revision to a file. Saw that the owner saw two revisions, a regular user saw one, and admins saw either one or two depending on if they were choosing to view draft data.	51e65e7385d78587884327155a016d8741dc4b2a
	Fix file attachment revision info with draft updates. When a file attachment has an updated revision in a pending draft, that revision could leak into the page for users not able to see the draft. If they then moved the revision slider to show the new revision, they'd get a 404. This change makes it so we check the last attachment in the history to see if it's a draft, and only include it in the revision list if that draft should be visible. Testing Done: - Ran unit tests. - Created a draft with an updated revision to a file. Saw that the owner saw two revisions, a regular user saw one, and admins saw either one or two depending on if they were choosing to view draft data.	84257474feab38c4516d67c1b7609a4baf1dfb1a

Diff:

Revision 3 (+144 -14)

Show changes

	reviewboard/reviews/tests/test_file_attachment_review_ui.py
	reviewboard/reviews/ui/base.py

Checks run (2 succeeded)

flake8 passed.

JSHint passed.

Ship it!

```
Ship It!
```

reviewboard/reviews/ui/base.py (Diff revisions 2 - 3)

The issue has been resolved. Show all issues

I had to re-read this several times. It's late, so that's maybe part of it, but I think this might be more clear as:

if (include_draft or
    review_request.get_file_attachment_state(
        last_attachment
    ) not in (
         FileAttachmentState.NEW,
         FileAttachmentState.NEW_REVISION,
    )):

Still kind of weird, but keeps the call and the list from running together.

Commits:

	Summary	ID
	Fix file attachment revision info with draft updates. When a file attachment has an updated revision in a pending draft, that revision could leak into the page for users not able to see the draft. If they then moved the revision slider to show the new revision, they'd get a 404. This change makes it so we check the last attachment in the history to see if it's a draft, and only include it in the revision list if that draft should be visible. Testing Done: - Ran unit tests. - Created a draft with an updated revision to a file. Saw that the owner saw two revisions, a regular user saw one, and admins saw either one or two depending on if they were choosing to view draft data.	84257474feab38c4516d67c1b7609a4baf1dfb1a
	Fix file attachment revision info with draft updates. When a file attachment has an updated revision in a pending draft, that revision could leak into the page for users not able to see the draft. If they then moved the revision slider to show the new revision, they'd get a 404. This change makes it so we check the last attachment in the history to see if it's a draft, and only include it in the revision list if that draft should be visible. Testing Done: - Ran unit tests. - Created a draft with an updated revision to a file. Saw that the owner saw two revisions, a regular user saw one, and admins saw either one or two depending on if they were choosing to view draft data.	012b6fe552ad20450fcd500a6d7259c93f6a9e8a

Diff:

Revision 4 (+146 -14)

Show changes

	reviewboard/reviews/tests/test_file_attachment_review_ui.py
	reviewboard/reviews/ui/base.py

Checks run (2 succeeded)

flake8 passed.

JSHint passed.

Ship it!

```
Ship It!
```

Status:: Completed