Summary

Return binary flag when with filediffs and allow querying by the binary flag.

Review Request #10729 — Created Sept. 26, 2019 and updated Nov. 30, 2019, 4:06 p.m.

Information

Owner

perplex

Repository

Review Board

Branch

master

Bugs

Depends On

Reviewers

Groups

reviewboard, students

People

Description

RB Tools required getting the binary status of each filediff so it
could determine which files need an accompanying file upload. This
allows RB Tools to query the backend for the filediffs for a diff. The
filediffs returned will contain the binart flag. An optional flag is
also provide for querying only for filediffs that have the binary flag
set to either true or false.

Testing Done

Included unit test for these changes.

Commits

Summary	ID
Address David's comments	2cb39d374b5799e821a07e734ed6a614904bf64c
Fix grammar mistake	4b1e111fed1df7c8cd8ae27b89c8f7b1f9dec0b5
Revert "Fix grammar mistake" This reverts commit 4b1e111fed1df7c8cd8ae27b89c8f7b1f9dec0b5.	9ae87d7ca6a8c5bad33d32df78baae77f8abde4b
Fix grammar	2371a6a041e2ca4c3c9147b85eac56039872fea2
Add binary filter test	46a4cabcc5cf318a5fefe46ebb5b98b5789628b2
replace double quotes with single quotes	9da99cd37e3d89ecd9ea02bd65a4ff646ae5a9c2
Use djblets to map query value to parameter	90b4f57a8285cd9be9d847410144002529972775
Add binary parameter to function description	3047b4be526f39ded08bf118b14930d5252f3b79
drop get_queryset parameters done one line	bd8db61615a5ce70d82438900eb6dff63a7c105d
format get_queryset description to fit line length limit	0cea0271d77078b30ee22e45bbb1356d111b7dbb

Issues

Description	From	Last Updated
Typo in summary: "amd"	brennie	Oct. 26, 2019, 1:03 p.m.
This is including some strange delete/re-add of some changes. Can you look into what happened, try to revert those?	chipx86	Oct. 26, 2019, 8:46 p.m.
Can you flesh out the description and testing done? See https://www.notion.so/reviewboard/Writing-Good-Change-Descriptions-10529e7c207743fa8ca90153d4b21fea	david	Oct. 27, 2019, 2:03 p.m.
Can you add unit tests for both the query parameter and the new field in the output?	david	Oct. 27, 2019, 2:03 p.m.
Let's keep these in alphabetical order	david	Oct. 12, 2019, 1:18 a.m.
This needs a period at the end.	david	Oct. 12, 2019, 1:18 a.m.
Please use single quotes rather than double.	david	Oct. 12, 2019, 1:18 a.m.
This can be simplified down to: qs = qs.filter(binary=(param == 'true'))	david	Oct. 12, 2019, 1:18 a.m.
"is binary" or "is a binary file".	brennie	Oct. 26, 2019, 1:03 p.m.
This should be updated to talk about the new binary query parameter.	david	Oct. 27, 2019, 4:23 p.m.
Instead of doing this, you can add a binary=False keyword arugment to get_queryset() and the API field will do the …	brennie	Oct. 27, 2019, 4:21 p.m.
E501 line too long (80 > 79 characters)	reviewbot	Oct. 26, 2019, 5:02 p.m.
E501 line too long (80 > 79 characters)	reviewbot	Oct. 26, 2019, 8:45 p.m.
E501 line too long (80 > 79 characters)	reviewbot	Oct. 27, 2019, 2:03 p.m.
E501 line too long (80 > 79 characters)	reviewbot	Oct. 27, 2019, 4:25 p.m.
E501 line too long (86 > 79 characters)	reviewbot	Oct. 27, 2019, 4:25 p.m.
E501 line too long (80 > 79 characters)	reviewbot	Oct. 27, 2019, 4:27 p.m.
E501 line too long (83 > 79 characters)	reviewbot	Oct. 27, 2019, 4:27 p.m.
E501 line too long (80 > 79 characters)	reviewbot	Oct. 27, 2019, 4:27 p.m.

flake8 passed.

JSHint passed.

The issue has been resolved. Show all issues

Can you flesh out the description and testing done? See https://www.notion.so/reviewboard/Writing-Good-Change-Descriptions-10529e7c207743fa8ca90153d4b21fea

reviewboard/webapi/resources/filediff.py (Diff revision 1)
The issue has been resolved. Show all issues
```
Let's keep these in alphabetical order
```
reviewboard/webapi/resources/filediff.py (Diff revision 1)
The issue has been resolved. Show all issues
```
This needs a period at the end.
```
reviewboard/webapi/resources/filediff.py (Diff revision 1)
The issue has been resolved. Show all issues
```
Please use single quotes rather than double.
```

reviewboard/webapi/resources/filediff.py (Diff revision 1)

The issue has been resolved. Show all issues

This can be simplified down to:

qs = qs.filter(binary=(param == 'true'))

Commits:

	Summary	ID
	Return binary flag when with filediffs amd allow querying by the binary flag.	d97e179568dbb30a12452ce21a990905c0bcd742
	Address David's comments	8d2cdb32bc41a59bff8948bc547c5eba15c34e94

Diff:

Revision 2 (+20 -2)

Show changes

reviewboard/webapi/resources/filediff.py

Checks run (2 succeeded)

flake8 passed.

JSHint passed.

Description:: ~
Backend support for RB Tools to know which binary files it needs to upload with a diff.
~
RB Tools required getting the binary status of each filediff so it could determine which files need an accompanying file upload. This allows RB Tools to query the backend for the filediffs for a diff. The filediffs returned will contain the binart flag. An optional flag is also provide for querying only for filediffs that have the binary flag set to either true or false.

The issue has been resolved. Show all issues
```
Typo in summary: "amd"
```
reviewboard/webapi/resources/filediff.py (Diff revision 2)
The issue has been resolved. Show all issues
```
"is binary" or "is a binary file".
```

reviewboard/webapi/resources/filediff.py (Diff revision 2)

The issue has been resolved. Show all issues

Instead of doing this, you can add a binary=False keyword arugment to get_queryset() and the API field will do the rest.

The issue has been resolved. Show all issues

Can you add unit tests for both the query parameter and the new field in the output?

reviewboard/webapi/resources/filediff.py (Diff revision 2)
The issue has been resolved. Show all issues
```
This should be updated to talk about the new binary query parameter.
```

Summary:

Return binary flag when with filediffs amd allow querying by the binary flag.

Return binary flag when with filediffs and allow querying by the binary flag.

Commits:

	Summary	ID
	Address David's comments	8d2cdb32bc41a59bff8948bc547c5eba15c34e94
	Address David's comments	2cb39d374b5799e821a07e734ed6a614904bf64c
	Fix grammar mistake	4b1e111fed1df7c8cd8ae27b89c8f7b1f9dec0b5

Diff:

Revision 3 (+5911 -5893)

Show changes

	reviewboard/attachments/forms.py
	reviewboard/attachments/managers.py
	reviewboard/reviews/views.py
	reviewboard/webapi/resources/draft_filediff.py
	reviewboard/webapi/resources/filediff.py

Checks run (1 failed, 1 succeeded)

flake8 failed.

JSHint passed.

flake8

reviewboard/webapi/resources/filediff.py (Diff revision 3)
The issue has been dropped. Show all issues
```
E501 line too long (80 > 79 characters)
```

The issue has been resolved. Show all issues

This is including some strange delete/re-add of some changes. Can you look into what happened, try to revert those?

Commits:

	Summary	ID
	Address David's comments	2cb39d374b5799e821a07e734ed6a614904bf64c
	Fix grammar mistake	4b1e111fed1df7c8cd8ae27b89c8f7b1f9dec0b5
	Address David's comments	2cb39d374b5799e821a07e734ed6a614904bf64c
	Fix grammar mistake	4b1e111fed1df7c8cd8ae27b89c8f7b1f9dec0b5
	Revert "Fix grammar mistake" This reverts commit 4b1e111fed1df7c8cd8ae27b89c8f7b1f9dec0b5.	9ae87d7ca6a8c5bad33d32df78baae77f8abde4b
	Fix grammar	2371a6a041e2ca4c3c9147b85eac56039872fea2

Diff:

Revision 4 (+5913 -5895)

Show changes

reviewboard/webapi/resources/filediff.py

Checks run (1 failed, 1 succeeded)

flake8 failed.

JSHint passed.

flake8

reviewboard/webapi/resources/filediff.py (Diff revision 4)
The issue has been dropped. Show all issues
```
E501 line too long (80 > 79 characters)
```

Commits:

	Summary	ID
	Address David's comments	2cb39d374b5799e821a07e734ed6a614904bf64c
	Fix grammar mistake	4b1e111fed1df7c8cd8ae27b89c8f7b1f9dec0b5
	Revert "Fix grammar mistake" This reverts commit 4b1e111fed1df7c8cd8ae27b89c8f7b1f9dec0b5.	9ae87d7ca6a8c5bad33d32df78baae77f8abde4b
	Fix grammar	2371a6a041e2ca4c3c9147b85eac56039872fea2
	Address David's comments	2cb39d374b5799e821a07e734ed6a614904bf64c
	Fix grammar mistake	4b1e111fed1df7c8cd8ae27b89c8f7b1f9dec0b5
	Revert "Fix grammar mistake" This reverts commit 4b1e111fed1df7c8cd8ae27b89c8f7b1f9dec0b5.	9ae87d7ca6a8c5bad33d32df78baae77f8abde4b
	Fix grammar	2371a6a041e2ca4c3c9147b85eac56039872fea2
	Add binary filter test	46a4cabcc5cf318a5fefe46ebb5b98b5789628b2
	replace double quotes with single quotes	9da99cd37e3d89ecd9ea02bd65a4ff646ae5a9c2

Diff:

Revision 5 (+13587 -13451)

Show changes

	reviewboard/attachments/forms.py
	reviewboard/attachments/managers.py
	reviewboard/reviews/views.py
	reviewboard/testing/testcase.py
	reviewboard/webapi/resources/draft_filediff.py
	reviewboard/webapi/resources/filediff.py
	reviewboard/webapi/tests/test_filediff.py

Checks run (1 failed, 1 succeeded)

flake8 failed.

JSHint passed.

flake8

reviewboard/webapi/resources/filediff.py (Diff revision 5)
The issue has been dropped. Show all issues
```
E501 line too long (80 > 79 characters)
```

Commits:

	Summary	ID
	Address David's comments	2cb39d374b5799e821a07e734ed6a614904bf64c
	Fix grammar mistake	4b1e111fed1df7c8cd8ae27b89c8f7b1f9dec0b5
	Revert "Fix grammar mistake" This reverts commit 4b1e111fed1df7c8cd8ae27b89c8f7b1f9dec0b5.	9ae87d7ca6a8c5bad33d32df78baae77f8abde4b
	Fix grammar	2371a6a041e2ca4c3c9147b85eac56039872fea2
	Add binary filter test	46a4cabcc5cf318a5fefe46ebb5b98b5789628b2
	replace double quotes with single quotes	9da99cd37e3d89ecd9ea02bd65a4ff646ae5a9c2
	Address David's comments	2cb39d374b5799e821a07e734ed6a614904bf64c
	Fix grammar mistake	4b1e111fed1df7c8cd8ae27b89c8f7b1f9dec0b5
	Revert "Fix grammar mistake" This reverts commit 4b1e111fed1df7c8cd8ae27b89c8f7b1f9dec0b5.	9ae87d7ca6a8c5bad33d32df78baae77f8abde4b
	Fix grammar	2371a6a041e2ca4c3c9147b85eac56039872fea2
	Add binary filter test	46a4cabcc5cf318a5fefe46ebb5b98b5789628b2
	replace double quotes with single quotes	9da99cd37e3d89ecd9ea02bd65a4ff646ae5a9c2
	Use djblets to map query value to parameter	90b4f57a8285cd9be9d847410144002529972775

Diff:

Revision 6 (+13609 -13455)

Show changes

	reviewboard/attachments/forms.py
	reviewboard/attachments/managers.py
	reviewboard/reviews/views.py
	reviewboard/testing/testcase.py
	reviewboard/webapi/resources/draft_filediff.py
	reviewboard/webapi/resources/filediff.py
	reviewboard/webapi/tests/test_filediff.py

Checks run (1 failed, 1 succeeded)

flake8 failed.

JSHint passed.

flake8

reviewboard/webapi/resources/filediff.py (Diff revision 6)
The issue has been dropped. Show all issues
```
E501 line too long (80 > 79 characters)
```
reviewboard/webapi/resources/filediff.py (Diff revision 6)
The issue has been resolved. Show all issues
```
E501 line too long (86 > 79 characters)
```

Commits:

	Summary	ID
	Address David's comments	2cb39d374b5799e821a07e734ed6a614904bf64c
	Fix grammar mistake	4b1e111fed1df7c8cd8ae27b89c8f7b1f9dec0b5
	Revert "Fix grammar mistake" This reverts commit 4b1e111fed1df7c8cd8ae27b89c8f7b1f9dec0b5.	9ae87d7ca6a8c5bad33d32df78baae77f8abde4b
	Fix grammar	2371a6a041e2ca4c3c9147b85eac56039872fea2
	Add binary filter test	46a4cabcc5cf318a5fefe46ebb5b98b5789628b2
	replace double quotes with single quotes	9da99cd37e3d89ecd9ea02bd65a4ff646ae5a9c2
	Use djblets to map query value to parameter	90b4f57a8285cd9be9d847410144002529972775
	Address David's comments	2cb39d374b5799e821a07e734ed6a614904bf64c
	Fix grammar mistake	4b1e111fed1df7c8cd8ae27b89c8f7b1f9dec0b5
	Revert "Fix grammar mistake" This reverts commit 4b1e111fed1df7c8cd8ae27b89c8f7b1f9dec0b5.	9ae87d7ca6a8c5bad33d32df78baae77f8abde4b
	Fix grammar	2371a6a041e2ca4c3c9147b85eac56039872fea2
	Add binary filter test	46a4cabcc5cf318a5fefe46ebb5b98b5789628b2
	replace double quotes with single quotes	9da99cd37e3d89ecd9ea02bd65a4ff646ae5a9c2
	Use djblets to map query value to parameter	90b4f57a8285cd9be9d847410144002529972775
	Add binary parameter to function description	3047b4be526f39ded08bf118b14930d5252f3b79
	drop get_queryset parameters done one line	bd8db61615a5ce70d82438900eb6dff63a7c105d

Diff:

Revision 7 (+13614 -13458)

Show changes

	reviewboard/attachments/forms.py
	reviewboard/attachments/managers.py
	reviewboard/reviews/views.py
	reviewboard/testing/testcase.py
	reviewboard/webapi/resources/draft_filediff.py
	reviewboard/webapi/resources/filediff.py
	reviewboard/webapi/tests/test_filediff.py

Checks run (1 failed, 1 succeeded)

flake8 failed.

JSHint passed.

flake8

reviewboard/webapi/resources/filediff.py (Diff revision 7)
The issue has been dropped. Show all issues
```
E501 line too long (80 > 79 characters)
```
reviewboard/webapi/resources/filediff.py (Diff revision 7)
The issue has been resolved. Show all issues
```
E501 line too long (83 > 79 characters)
```

Commits:

	Summary	ID
	Address David's comments	2cb39d374b5799e821a07e734ed6a614904bf64c
	Fix grammar mistake	4b1e111fed1df7c8cd8ae27b89c8f7b1f9dec0b5
	Revert "Fix grammar mistake" This reverts commit 4b1e111fed1df7c8cd8ae27b89c8f7b1f9dec0b5.	9ae87d7ca6a8c5bad33d32df78baae77f8abde4b
	Fix grammar	2371a6a041e2ca4c3c9147b85eac56039872fea2
	Add binary filter test	46a4cabcc5cf318a5fefe46ebb5b98b5789628b2
	replace double quotes with single quotes	9da99cd37e3d89ecd9ea02bd65a4ff646ae5a9c2
	Use djblets to map query value to parameter	90b4f57a8285cd9be9d847410144002529972775
	Add binary parameter to function description	3047b4be526f39ded08bf118b14930d5252f3b79
	drop get_queryset parameters done one line	bd8db61615a5ce70d82438900eb6dff63a7c105d
	Address David's comments	2cb39d374b5799e821a07e734ed6a614904bf64c
	Fix grammar mistake	4b1e111fed1df7c8cd8ae27b89c8f7b1f9dec0b5
	Revert "Fix grammar mistake" This reverts commit 4b1e111fed1df7c8cd8ae27b89c8f7b1f9dec0b5.	9ae87d7ca6a8c5bad33d32df78baae77f8abde4b
	Fix grammar	2371a6a041e2ca4c3c9147b85eac56039872fea2
	Add binary filter test	46a4cabcc5cf318a5fefe46ebb5b98b5789628b2
	replace double quotes with single quotes	9da99cd37e3d89ecd9ea02bd65a4ff646ae5a9c2
	Use djblets to map query value to parameter	90b4f57a8285cd9be9d847410144002529972775
	Add binary parameter to function description	3047b4be526f39ded08bf118b14930d5252f3b79
	drop get_queryset parameters done one line	bd8db61615a5ce70d82438900eb6dff63a7c105d
	format get_queryset description to fit line length limit	0cea0271d77078b30ee22e45bbb1356d111b7dbb

Diff:

Revision 8 (+13618 -13460)

Show changes

	reviewboard/attachments/forms.py
	reviewboard/attachments/managers.py
	reviewboard/reviews/views.py
	reviewboard/testing/testcase.py
	reviewboard/webapi/resources/draft_filediff.py
	reviewboard/webapi/resources/filediff.py
	reviewboard/webapi/tests/test_filediff.py

Checks run (1 failed, 1 succeeded)

flake8 failed.

JSHint passed.

flake8

reviewboard/webapi/resources/filediff.py (Diff revision 8)
The issue has been dropped. Show all issues
```
E501 line too long (80 > 79 characters)
```

Description:: ~
RB Tools required getting the binary status of each filediff so it could determine which files need an accompanying file upload. This allows RB Tools to query the backend for the filediffs for a diff. The filediffs returned will contain the binart flag. An optional flag is also provide for querying only for filediffs that have the binary flag set to either true or false.
~
RB Tools required getting the binary status of each filediff so it
+ could determine which files need an accompanying file upload. This
+ allows RB Tools to query the backend for the filediffs for a diff. The
+ filediffs returned will contain the binart flag. An optional flag is
+ also provide for querying only for filediffs that have the binary flag
+ set to either true or false.
Testing Done:: +
Included unit test for these changes.

~		Backend support for RB Tools to know which binary files it needs to upload with a diff.
	~	RB Tools required getting the binary status of each filediff so it could determine which files need an accompanying file upload. This allows RB Tools to query the backend for the filediffs for a diff. The filediffs returned will contain the binart flag. An optional flag is also provide for querying only for filediffs that have the binary flag set to either true or false.

~		RB Tools required getting the binary status of each filediff so it could determine which files need an accompanying file upload. This allows RB Tools to query the backend for the filediffs for a diff. The filediffs returned will contain the binart flag. An optional flag is also provide for querying only for filediffs that have the binary flag set to either true or false.
	~	RB Tools required getting the binary status of each filediff so it
	+	could determine which files need an accompanying file upload. This
	+	allows RB Tools to query the backend for the filediffs for a diff. The
	+	filediffs returned will contain the binart flag. An optional flag is
	+	also provide for querying only for filediffs that have the binary flag
	+	set to either true or false.