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
	Address David's comments
	Fix grammar mistake
	Revert "Fix grammar mistake"
	Fix grammar
	Add binary filter test
	replace double quotes with single quotes
	Use djblets to map query value to parameter
	Add binary parameter to function description
	drop get_queryset parameters done one line
	format get_queryset description to fit line length limit

Screenshots

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.

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)
Show all issues
```
Let's keep these in alphabetical order
```
reviewboard/webapi/resources/filediff.py (Diff revision 1)
Show all issues
```
This needs a period at the end.
```
reviewboard/webapi/resources/filediff.py (Diff revision 1)
Show all issues
```
Please use single quotes rather than double.
```

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

Show all issues

This can be simplified down to:

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

Commits:

	Summary
-	Return binary flag when with filediffs amd allow querying by the binary flag.
+	Address David's comments

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.

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

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

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.

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)

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
-	Address David's comments
+	Address David's comments
+	Fix grammar mistake

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)
Show all issues
```
E501 line too long (80 > 79 characters)
```

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
-		Address David's comments
-		Fix grammar mistake
+		Address David's comments
+		Fix grammar mistake
+		Revert "Fix grammar mistake"
+		Fix grammar

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)
Show all issues
```
E501 line too long (80 > 79 characters)
```

Commits:

	Summary
-		Address David's comments
-		Fix grammar mistake
-		Revert "Fix grammar mistake"
-		Fix grammar
+		Address David's comments
+		Fix grammar mistake
+		Revert "Fix grammar mistake"
+		Fix grammar
+		Add binary filter test
+		replace double quotes with single quotes

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)
Show all issues
```
E501 line too long (80 > 79 characters)
```

Commits:

	Summary
-		Address David's comments
-		Fix grammar mistake
-		Revert "Fix grammar mistake"
-		Fix grammar
-		Add binary filter test
-		replace double quotes with single quotes
+		Address David's comments
+		Fix grammar mistake
+		Revert "Fix grammar mistake"
+		Fix grammar
+		Add binary filter test
+		replace double quotes with single quotes
+		Use djblets to map query value to parameter

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)
Show all issues
```
E501 line too long (80 > 79 characters)
```
reviewboard/webapi/resources/filediff.py (Diff revision 6)
Show all issues
```
E501 line too long (86 > 79 characters)
```

Commits:

	Summary
-		Address David's comments
-		Fix grammar mistake
-		Revert "Fix grammar mistake"
-		Fix grammar
-		Add binary filter test
-		replace double quotes with single quotes
-		Use djblets to map query value to parameter
+		Address David's comments
+		Fix grammar mistake
+		Revert "Fix grammar mistake"
+		Fix grammar
+		Add binary filter test
+		replace double quotes with single quotes
+		Use djblets to map query value to parameter
+		Add binary parameter to function description
+		drop get_queryset parameters done one line

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)
Show all issues
```
E501 line too long (80 > 79 characters)
```
reviewboard/webapi/resources/filediff.py (Diff revision 7)
Show all issues
```
E501 line too long (83 > 79 characters)
```

Commits:

	Summary
-		Address David's comments
-		Fix grammar mistake
-		Revert "Fix grammar mistake"
-		Fix grammar
-		Add binary filter test
-		replace double quotes with single quotes
-		Use djblets to map query value to parameter
-		Add binary parameter to function description
-		drop get_queryset parameters done one line
+		Address David's comments
+		Fix grammar mistake
+		Revert "Fix grammar mistake"
+		Fix grammar
+		Add binary filter test
+		replace double quotes with single quotes
+		Use djblets to map query value to parameter
+		Add binary parameter to function description
+		drop get_queryset parameters done one line
+		format get_queryset description to fit line length limit

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)
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.