Summary

Use FileAttachment.added_in_filediff for modified files as well as new.

Review Request #13548 — Created Feb. 17, 2024 and submitted Feb. 27, 2024, 8:24 a.m.

Information

Owner

david

Repository

Review Board

Branch

master

Bugs

Depends On

Reviewers

Groups

reviewboard

People

Description

In the initial design of the FileAttachment in diffs feature, we had
four possible fields:
- added_in_filediff
- repository
- repo_path
- repo_revision

For newly-added files, we'd set the added_in_filediff relation. For
all others, we'd set the other three. There are two major problems with
this.

First, the "revision" for a modified file is unique when talking about
git (a blob SHA), but isn't in many other systems. For example, the
FileDiff.dest_detail for an SVN change might just be "(working copy)".
This would result in attachments corresponding to multiple revisions of
the diff stomping on each other.

Second, once we had the file, any FileAttachmentComments would not be
able to link back to the diff in question, because that information was
not available.

This change modifies things so we use the added_in_filediff relation
for both newly-added and the modified version of any binary files. We
still use the repository/path/revision fields for the original version
of files.

Testing Done


Ran unit tests.
Uploaded a diff with binary files and checked the relations.

Commits

Summary	ID
Use FileAttachment.added_in_filediff for modified files as well as new. In the initial design of the FileAttachment in diffs feature, we had four possible fields: - `added_in_filediff` - `repository` - `repo_path` - `repo_revision` For newly-added files, we'd set the `added_in_filediff` relation. For all others, we'd set the other three. There are two major problems with this. First, the "revision" for a modified file is unique when talking about git (a blob SHA), but isn't in many other systems. For example, the `FileDiff.dest_detail` for an SVN change might just be "(working copy)". This would result in attachments corresponding to multiple revisions of the diff stomping on each other. Second, once we had the file, any FileAttachmentComments would not be able to link back to the diff in question, because that information was not available. This change modifies things so we use the `added_in_filediff` relation for both newly-added and the modified version of any binary files. We still use the repository/path/revision fields for the original version of files. Testing Done: - Ran unit tests. - Uploaded a diff with binary files and checked the relations.	f8729a6fb2439576d6bccdc52dae37c9fe5a787f

Summary

Use FileAttachment.added_in_filediff for modified files as well as new.

In the initial design of the FileAttachment in diffs feature, we had four possible fields: - `added_in_filediff` - `repository` - `repo_path` - `repo_revision` For newly-added files, we'd set the `added_in_filediff` relation. For all others, we'd set the other three. There are two major problems with this. First, the "revision" for a modified file is unique when talking about git (a blob SHA), but isn't in many other systems. For example, the `FileDiff.dest_detail` for an SVN change might just be "(working copy)". This would result in attachments corresponding to multiple revisions of the diff stomping on each other. Second, once we had the file, any FileAttachmentComments would not be able to link back to the diff in question, because that information was not available. This change modifies things so we use the `added_in_filediff` relation for both newly-added and the modified version of any binary files. We still use the repository/path/revision fields for the original version of files. Testing Done: - Ran unit tests. - Uploaded a diff with binary files and checked the relations.

f8729a6fb2439576d6bccdc52dae37c9fe5a787f

Issues

Description	From	Last Updated
These should be assertIsNone.	chipx86	Feb. 19, 2024, 9:33 p.m.

flake8 passed.

JSHint passed.

reviewboard/attachments/tests.py (Diff revision 1)
The issue has been resolved. Show all issues
```
These should be assertIsNone.
```

Commits:

	Summary	ID
	Use FileAttachment.added_in_filediff for modified files as well as new. In the initial design of the FileAttachment in diffs feature, we had four possible fields: - `added_in_filediff` - `repository` - `repo_path` - `repo_revision` For newly-added files, we'd set the `added_in_filediff` relation. For all others, we'd set the other three. There are two major problems with this. First, the "revision" for a modified file is unique when talking about git (a blob SHA), but isn't in many other systems. For example, the `FileDiff.dest_detail` for an SVN change might just be "(working copy)". This would result in attachments corresponding to multiple revisions of the diff stomping on each other. Second, once we had the file, any FileAttachmentComments would not be able to link back to the diff in question, because that information was not available. This change modifies things so we use the `added_in_filediff` relation for both newly-added and the modified version of any binary files. We still use the repository/path/revision fields for the original version of files. Testing Done: - Ran unit tests. - Uploaded a diff with binary files and checked the relations.	809889a6d64d7af3a75f87088f56174fea726cb8
	Use FileAttachment.added_in_filediff for modified files as well as new. In the initial design of the FileAttachment in diffs feature, we had four possible fields: - `added_in_filediff` - `repository` - `repo_path` - `repo_revision` For newly-added files, we'd set the `added_in_filediff` relation. For all others, we'd set the other three. There are two major problems with this. First, the "revision" for a modified file is unique when talking about git (a blob SHA), but isn't in many other systems. For example, the `FileDiff.dest_detail` for an SVN change might just be "(working copy)". This would result in attachments corresponding to multiple revisions of the diff stomping on each other. Second, once we had the file, any FileAttachmentComments would not be able to link back to the diff in question, because that information was not available. This change modifies things so we use the `added_in_filediff` relation for both newly-added and the modified version of any binary files. We still use the repository/path/revision fields for the original version of files. Testing Done: - Ran unit tests. - Uploaded a diff with binary files and checked the relations.	f8729a6fb2439576d6bccdc52dae37c9fe5a787f

Diff:

Revision 2 (+78 -82)

Show changes

	reviewboard/attachments/managers.py
	reviewboard/attachments/tests.py
	reviewboard/webapi/tests/test_diff_file_attachment.py
	reviewboard/webapi/tests/test_draft_filediff.py
	reviewboard/webapi/tests/test_filediff.py

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 (9a15d76)