Summary:

Build GeneralComment Model

Review Request #6375 — Created Sept. 26, 2014 and submitted Nov. 30, 2014, 3:21 p.m.

Information
Owner:	nicolexin
Repository:	Review Board
Branch:	master
Bugs:
Depends On:
Commit:	cd5ccbf...
Reviewers
Groups:	reviewboard, students
People:

Description

Reviewboard currently support 3 types of comments: Diff commment, File attachment comments, Screenshot comments.
This is a new type of comments: General comments.

A general comment on a review request is used when a comment is not tied to specific lines of code or a special file attachment, and an issue is opened. Examples include suggestions for testing or pointing out errors in the change description.

This review request is for GeneralComment backend model.

Testing Done

Five test added in reviews/tests.py:
1. test_review_detail_general_comment_ordering
2. test_init_with_general_comments
3. test_init_with_mix
4. test_init_general_comment_with_replies
5. test_save_reply_comment_to_general_comment

All test passed.

Previously test 1 failed due to use diff viewer 'get' method on general comments.
Figured out general comments behave the same as screenshot/file attachment comments.

Issues

Description	From	Last Updated
'models' imported but unused	reviewbot	Oct. 3, 2014, 8:13 a.m.
'six' imported but unused	reviewbot	Oct. 3, 2014, 8:13 a.m.
'_' imported but unused	reviewbot	Oct. 3, 2014, 8:13 a.m.
'BaseComment' imported but unused	reviewbot	Oct. 3, 2014, 8:13 a.m.
Col: 1 E265 block comment should start with '# '	reviewbot	Oct. 3, 2014, 8:13 a.m.
Col: 11 W292 no newline at end of file	reviewbot	Oct. 3, 2014, 8:13 a.m.
Col: 1 E265 block comment should start with '# '	reviewbot	Oct. 3, 2014, 8:13 a.m.
Is this supposed to match raw_id_fields? If so, it's missing an 'e' in "general"	AD adrw.hong	Oct. 5, 2014, 8:17 a.m.
Col: 1 E302 expected 2 blank lines, found 1	reviewbot	Oct. 3, 2014, 8:13 a.m.
'models' imported but unused	reviewbot	Oct. 3, 2014, 8:13 a.m.
'_' imported but unused	reviewbot	Oct. 3, 2014, 8:13 a.m.
Something you might want to consider is encapsulating this notion of a comment's context within each type of comment. For …	ML mloyzer	Oct. 14, 2014, 2:47 p.m.
list comprehension redefines 'file_attachment' from line 585	reviewbot	Oct. 3, 2014, 8:13 a.m.
undefined name 'issue_opened'	reviewbot	Oct. 3, 2014, 8:13 a.m.
undefined name 'issue_opened'	reviewbot	Oct. 3, 2014, 8:13 a.m.
Col: 1 E302 expected 2 blank lines, found 1	reviewbot	Oct. 5, 2014, 8:31 a.m.
'models' imported but unused	reviewbot	Oct. 5, 2014, 8:31 a.m.
'_' imported but unused	reviewbot	Oct. 5, 2014, 8:31 a.m.
list comprehension redefines 'file_attachment' from line 585	reviewbot	Oct. 5, 2014, 8:33 a.m.
undefined name 'issue_opened'	reviewbot	Oct. 5, 2014, 8:31 a.m.
undefined name 'issue_opened'	reviewbot	Oct. 5, 2014, 8:31 a.m.
list comprehension redefines 'file_attachment' from line 585	reviewbot	Oct. 5, 2014, 8:34 a.m.
Shouldn't this be in the API change? (/r/6431)	david	Oct. 16, 2014, 12:01 p.m.
Col: 57 W292 no newline at end of file	reviewbot	Oct. 16, 2014, 12:16 p.m.
Col: 29 W292 no newline at end of file	reviewbot	Oct. 16, 2014, 12:16 p.m.
Col: 80 E501 line too long (80 > 79 characters)	reviewbot	Oct. 16, 2014, 12:16 p.m.
Should this be "gcomment"?	brennie	Oct. 25, 2014, 6:15 p.m.
Generally, we like to keep these in alphabetical order. FileAttachmentComment clearly isn't, but can you reorder these so they'll be …	chipx86	Oct. 29, 2014, 12:03 p.m.
Should be text. (We fixed the above cases recently.)	chipx86	Oct. 29, 2014, 12:03 p.m.
Same comment about order.	chipx86	Oct. 29, 2014, 12:03 p.m.
This should be fleshed out more to describe the purpose of these comments. Also, you should always use complete sentences …	chipx86	Oct. 29, 2014, 12:03 p.m.
Should use single quotes.	chipx86	Oct. 29, 2014, 12:03 p.m.
Single quotes.	chipx86	Oct. 29, 2014, 12:03 p.m.
Alphabetical order.	chipx86	Oct. 29, 2014, 12:03 p.m.
These should be single quotes as well.	ML mloyzer	Nov. 2, 2014, 3:28 p.m.
Alphabetical order.	chipx86	Oct. 29, 2014, 12:03 p.m.
This is unrelated to your change. Can you revert it?	chipx86	Oct. 29, 2014, 12:03 p.m.
Why is there an apostrophe(,) at the end of this function call?	justy777	Nov. 1, 2014, 3:37 p.m.
and here.	justy777	Nov. 1, 2014, 3:37 p.m.
This should probably be more specific, like: "A comment on a review request not tied to code or a file."	chipx86	Nov. 8, 2014, 2:35 p.m.
"Examples" and "testing".	chipx86	Nov. 8, 2014, 2:35 p.m.
Why not use a list instead of 3 variables?	justy777	Nov. 1, 2014, 3:40 p.m.
There should be a blank line between these lines.	justy777	Nov. 8, 2014, 2:35 p.m.
Should use a loop instead of repeating a function call. for _ in range(3): <function here>	justy777	Nov. 1, 2014, 3:46 p.m.
There should be a trailing apostrophe(,) after 'general_comments': []	justy777	Nov. 8, 2014, 2:35 p.m.
Minor grammar fix, it should read: "...review request that is not tied..."	AD adrw.hong	Nov. 28, 2014, 12:39 p.m.

Tool: PEP8 Style Checker
Processed Files:
    reviewboard/reviews/models/review.py
    reviewboard/reviews/models/general_comment.py



Tool: Pyflakes
Processed Files:
    reviewboard/reviews/models/review.py
    reviewboard/reviews/models/general_comment.py

reviewboard/reviews/models/general_comment.py (Diff revision 1)
Show all issues
```
 'models' imported but unused
```
reviewboard/reviews/models/general_comment.py (Diff revision 1)
Show all issues
```
 'six' imported but unused
```
reviewboard/reviews/models/general_comment.py (Diff revision 1)
Show all issues
```
 '_' imported but unused
```
reviewboard/reviews/models/general_comment.py (Diff revision 1)
Show all issues
```
 'BaseComment' imported but unused
```
reviewboard/reviews/models/general_comment.py (Diff revision 1)
Show all issues
```
Col: 1
 E265 block comment should start with '# '
```
reviewboard/reviews/models/general_comment.py (Diff revision 1)
Show all issues
```
Col: 11
 W292 no newline at end of file
```
reviewboard/reviews/models/review.py (Diff revision 1)
Show all issues
```
Col: 1
 E265 block comment should start with '# '
```

Groups:

+	students

Description:

		Building the general comment model.
		Create a new class called GeneralComment using base class BaseComment.
		get_absolute_url() to be determined.
~		Need to integrated with review.py(review model) and view.py(entire page view model).
	~	Need to integrated with review.py(review model) and view.py(entire page view model).
	+
	+	Modified lines are commennted since I still want to test with the server.
		Work in Progress.

Summary:

-	[WIP]Build GeneralComment Model
+	Build GeneralComment Model

Testing Done:

	+	Server can be launched without error.
	+	No additional test is written.

Diff:

Revision 2 (+99 -24)

Show changes

	reviewboard/reviews/admin.py
	reviewboard/reviews/views.py
	reviewboard/reviews/models/__init__.py
	reviewboard/reviews/models/general_comment.py
	reviewboard/reviews/models/review.py
	reviewboard/reviews/templatetags/reviewtags.py
	reviewboard/testing/testcase.py
	reviewboard/webapi/resources/__init__.py

Tool: Pyflakes
Processed Files:
    reviewboard/reviews/views.py
    reviewboard/testing/testcase.py
    reviewboard/webapi/resources/__init__.py
    reviewboard/reviews/templatetags/reviewtags.py
    reviewboard/reviews/admin.py
    reviewboard/reviews/models/__init__.py
    reviewboard/reviews/models/review.py
    reviewboard/reviews/models/general_comment.py



Tool: PEP8 Style Checker
Processed Files:
    reviewboard/reviews/views.py
    reviewboard/testing/testcase.py
    reviewboard/webapi/resources/__init__.py
    reviewboard/reviews/templatetags/reviewtags.py
    reviewboard/reviews/admin.py
    reviewboard/reviews/models/__init__.py
    reviewboard/reviews/models/review.py
    reviewboard/reviews/models/general_comment.py

reviewboard/reviews/admin.py (Diff revision 2)
Show all issues
```
Col: 1
 E302 expected 2 blank lines, found 1
```
reviewboard/reviews/models/general_comment.py (Diff revision 2)
Show all issues
```
 'models' imported but unused
```
reviewboard/reviews/models/general_comment.py (Diff revision 2)
Show all issues
```
 '_' imported but unused
```

reviewboard/reviews/views.py (Diff revision 2)

Show all issues

 list comprehension redefines 'file_attachment' from line 585

reviewboard/testing/testcase.py (Diff revision 2)
Show all issues
```
 undefined name 'issue_opened'
```
reviewboard/testing/testcase.py (Diff revision 2)
Show all issues
```
 undefined name 'issue_opened'
```

Description:

~		Building the general comment model.
~		Create a new class called GeneralComment using base class BaseComment.
	~	The general comment model is built.
	~	Created a new class called GeneralComment using base class BaseComment.
-		get_absolute_url() to be determined.
-		Need to integrated with review.py(review model) and view.py(entire page view model).

~		Modified lines are commennted since I still want to test with the server.
~		Work in Progress.
	~	TODO:
	~	1. integration with web-api
	+	2. javascript front-end

reviewboard/reviews/templatetags/reviewtags.py (Diff revision 2)

Show all issues

Something you might want to consider is encapsulating this notion of a comment's context within each type of comment.  For example you can then just to:
context_id = comment.get_context_id() if bool(comment)
or use the original condition comment != "" instead of converting it to a boolean value.
It reduces to having this context logic outside the class it represents.  This is just my opinion though, it would be interesting for one of the mentors to give their input but both ways are valid.

nicolexin Oct. 4, 2014, 2:35 p.m.

Well I personally perfer the original one since this context-type is only used in the html templates, to communicate with front-end. So the context logic info is not quite useful in my backend class since we already know its type, we don't need to store extra data.

adrw.hong Oct. 5, 2014, 8:13 a.m.

Just one minor thing, otherwise it looks great so far!

reviewboard/reviews/admin.py (Diff revision 2)

Show all issues

Is this supposed to match raw_id_fields?
If so, it's missing an 'e' in "general"

nicolexin Oct. 5, 2014, 8:14 a.m.

Yeah, it was a stupid typo, gonna fix it right away.

Commit:

-	3e6f373c76a55bb9b8045a59f93a16c9ce8c209a
+	63c3fdfc449557cd3b1ead33adcd97596871f953

Diff:

Revision 3 (+99 -24)

Show changes

	reviewboard/reviews/admin.py
	reviewboard/reviews/views.py
	reviewboard/reviews/models/__init__.py
	reviewboard/reviews/models/general_comment.py
	reviewboard/reviews/models/review.py
	reviewboard/reviews/templatetags/reviewtags.py
	reviewboard/testing/testcase.py
	reviewboard/webapi/resources/__init__.py

Tool: Pyflakes
Processed Files:
    reviewboard/reviews/views.py
    reviewboard/testing/testcase.py
    reviewboard/webapi/resources/__init__.py
    reviewboard/reviews/templatetags/reviewtags.py
    reviewboard/reviews/admin.py
    reviewboard/reviews/models/__init__.py
    reviewboard/reviews/models/review.py
    reviewboard/reviews/models/general_comment.py



Tool: PEP8 Style Checker
Processed Files:
    reviewboard/reviews/views.py
    reviewboard/testing/testcase.py
    reviewboard/webapi/resources/__init__.py
    reviewboard/reviews/templatetags/reviewtags.py
    reviewboard/reviews/admin.py
    reviewboard/reviews/models/__init__.py
    reviewboard/reviews/models/review.py
    reviewboard/reviews/models/general_comment.py

reviewboard/reviews/admin.py (Diff revision 3)
Show all issues
```
Col: 1
 E302 expected 2 blank lines, found 1
```
reviewboard/reviews/models/general_comment.py (Diff revision 3)
Show all issues
```
 'models' imported but unused
```
reviewboard/reviews/models/general_comment.py (Diff revision 3)
Show all issues
```
 '_' imported but unused
```

reviewboard/reviews/views.py (Diff revision 3)

Show all issues

 list comprehension redefines 'file_attachment' from line 585

reviewboard/testing/testcase.py (Diff revision 3)
Show all issues
```
 undefined name 'issue_opened'
```
reviewboard/testing/testcase.py (Diff revision 3)
Show all issues
```
 undefined name 'issue_opened'
```

Commit:

-	63c3fdfc449557cd3b1ead33adcd97596871f953
+	b43614d00d300338a9f66809b3622170de127edd

Diff:

Revision 4 (+97 -24)

Show changes

	reviewboard/reviews/admin.py
	reviewboard/reviews/views.py
	reviewboard/reviews/models/__init__.py
	reviewboard/reviews/models/general_comment.py
	reviewboard/reviews/models/review.py
	reviewboard/reviews/templatetags/reviewtags.py
	reviewboard/testing/testcase.py
	reviewboard/webapi/resources/__init__.py

Tool: Pyflakes
Processed Files:
    reviewboard/reviews/views.py
    reviewboard/testing/testcase.py
    reviewboard/webapi/resources/__init__.py
    reviewboard/reviews/templatetags/reviewtags.py
    reviewboard/reviews/admin.py
    reviewboard/reviews/models/__init__.py
    reviewboard/reviews/models/review.py
    reviewboard/reviews/models/general_comment.py



Tool: PEP8 Style Checker
Processed Files:
    reviewboard/reviews/views.py
    reviewboard/testing/testcase.py
    reviewboard/webapi/resources/__init__.py
    reviewboard/reviews/templatetags/reviewtags.py
    reviewboard/reviews/admin.py
    reviewboard/reviews/models/__init__.py
    reviewboard/reviews/models/review.py
    reviewboard/reviews/models/general_comment.py

reviewboard/reviews/views.py (Diff revision 4)

Show all issues

 list comprehension redefines 'file_attachment' from line 585

You'll also need to update fetch_issue_counts in reviewboard/reviews/models/review_request.py

reviewboard/webapi/resources/__init__.py (Diff revision 4)

Show all issues

Shouldn't this be in the API change? (/r/6431)

Summary:

-	Build GeneralComment Model
+	[WIP]Build GeneralComment Model

Description:

		The general comment model is built.
		Created a new class called GeneralComment using base class BaseComment.

~		TODO:
~		1. integration with web-api
	~	Todo:
	~	Debuging till all tests passed.
-		2. javascript front-end

Testing Done:

~		Server can be launched without error.
~		No additional test is written.
	~	Five test added in reviews/tests.py:
	~	1. test_review_detail_general_comment_ordering
	+	2. test_init_with_general_comments
	+	3. test_init_with_mix
	+	4. test_init_general_comment_with_replies
	+	5. test_save_reply_comment_to_general_comment
	+
	+	Testing Result:
	+	Failing Test 1 & 3
	+	Test 1 fails at: self.assertEqual(len(comments), 3)
	+	Test 3 fails at: self.assertEqual(self.review_request.issue_open_count, 10)

Commit:

-	b43614d00d300338a9f66809b3622170de127edd
+	646687a21aacdf5a341dff99f60d2e719e882038

Diff:

Revision 5 (+224 -23)

Show changes

	reviewboard/notifications/tests.py
	reviewboard/reviews/admin.py
	reviewboard/reviews/tests.py
	reviewboard/reviews/views.py
	reviewboard/reviews/models/__init__.py
	reviewboard/reviews/models/general_comment.py
	reviewboard/reviews/models/review.py
	reviewboard/reviews/models/review_request.py
	2 more

Tool: Pyflakes
Processed Files:
    reviewboard/reviews/views.py
    reviewboard/testing/testcase.py
    reviewboard/notifications/tests.py
    reviewboard/reviews/tests.py
    reviewboard/reviews/admin.py
    reviewboard/reviews/models/review_request.py
    reviewboard/reviews/models/__init__.py
    reviewboard/reviews/models/review.py
    reviewboard/reviews/models/general_comment.py
    reviewboard/reviews/templatetags/reviewtags.py



Tool: PEP8 Style Checker
Processed Files:
    reviewboard/reviews/views.py
    reviewboard/testing/testcase.py
    reviewboard/notifications/tests.py
    reviewboard/reviews/tests.py
    reviewboard/reviews/admin.py
    reviewboard/reviews/models/review_request.py
    reviewboard/reviews/models/__init__.py
    reviewboard/reviews/models/review.py
    reviewboard/reviews/models/general_comment.py
    reviewboard/reviews/templatetags/reviewtags.py

reviewboard/reviews/admin.py (Diff revision 5)
Show all issues
```
Col: 57
 W292 no newline at end of file
```
reviewboard/reviews/models/general_comment.py (Diff revision 5)
Show all issues
```
Col: 29
 W292 no newline at end of file
```
reviewboard/reviews/tests.py (Diff revision 5)
Show all issues
```
Col: 80
 E501 line too long (80 > 79 characters)
```

Commit:

-	646687a21aacdf5a341dff99f60d2e719e882038
+	1916ed9cc4a89d79044bf95880ed70c9abcd6dd2

Diff:

Revision 6 (+226 -23)

Show changes

	reviewboard/notifications/tests.py
	reviewboard/reviews/admin.py
	reviewboard/reviews/tests.py
	reviewboard/reviews/views.py
	reviewboard/reviews/models/__init__.py
	reviewboard/reviews/models/general_comment.py
	reviewboard/reviews/models/review.py
	reviewboard/reviews/models/review_request.py
	2 more

Tool: Pyflakes
Processed Files:
    reviewboard/reviews/views.py
    reviewboard/testing/testcase.py
    reviewboard/notifications/tests.py
    reviewboard/reviews/tests.py
    reviewboard/reviews/admin.py
    reviewboard/reviews/models/review_request.py
    reviewboard/reviews/models/__init__.py
    reviewboard/reviews/models/review.py
    reviewboard/reviews/models/general_comment.py
    reviewboard/reviews/templatetags/reviewtags.py



Tool: PEP8 Style Checker
Processed Files:
    reviewboard/reviews/views.py
    reviewboard/testing/testcase.py
    reviewboard/notifications/tests.py
    reviewboard/reviews/tests.py
    reviewboard/reviews/admin.py
    reviewboard/reviews/models/review_request.py
    reviewboard/reviews/models/__init__.py
    reviewboard/reviews/models/review.py
    reviewboard/reviews/models/general_comment.py
    reviewboard/reviews/templatetags/reviewtags.py

Description:

		The general comment model is built.
		Created a new class called GeneralComment using base class BaseComment.

		Todo:
~		Debuging till all tests passed.
	~	Debuging till test 1 passed.

Testing Done:

		Five test added in reviews/tests.py:
		1. test_review_detail_general_comment_ordering
		2. test_init_with_general_comments
		3. test_init_with_mix
		4. test_init_general_comment_with_replies
		5. test_save_reply_comment_to_general_comment

		Testing Result:
~		Failing Test 1 & 3
~		Test 1 fails at: self.assertEqual(len(comments), 3)
~		Test 3 fails at: self.assertEqual(self.review_request.issue_open_count, 10)
	~	Failing Test 1: at line 874, 875
	~	The ordering on the page is wrong.
	~	should be 3->2, given 2->3
	+
	+	checked timestamp: timestamp(2) > timestamp(3)
	+	haven't figure out why the ordering is wrong

Commit:

-	1916ed9cc4a89d79044bf95880ed70c9abcd6dd2
+	cf7e34ba63330d877d8f7f1979d38388851eb005

Diff:

Revision 7 (+228 -24)

Show changes

	reviewboard/notifications/tests.py
	reviewboard/reviews/admin.py
	reviewboard/reviews/tests.py
	reviewboard/reviews/views.py
	reviewboard/reviews/models/__init__.py
	reviewboard/reviews/models/general_comment.py
	reviewboard/reviews/models/review.py
	reviewboard/reviews/models/review_request.py
	2 more

Tool: Pyflakes
Processed Files:
    reviewboard/reviews/views.py
    reviewboard/testing/testcase.py
    reviewboard/notifications/tests.py
    reviewboard/reviews/tests.py
    reviewboard/reviews/admin.py
    reviewboard/reviews/models/review_request.py
    reviewboard/reviews/models/__init__.py
    reviewboard/reviews/models/review.py
    reviewboard/reviews/models/general_comment.py
    reviewboard/reviews/templatetags/reviewtags.py



Tool: PEP8 Style Checker
Processed Files:
    reviewboard/reviews/views.py
    reviewboard/testing/testcase.py
    reviewboard/notifications/tests.py
    reviewboard/reviews/tests.py
    reviewboard/reviews/admin.py
    reviewboard/reviews/models/review_request.py
    reviewboard/reviews/models/__init__.py
    reviewboard/reviews/models/review.py
    reviewboard/reviews/models/general_comment.py
    reviewboard/reviews/templatetags/reviewtags.py

Summary:

-	[WIP]Build GeneralComment Model
+	Build GeneralComment Model

Description:

		The general comment model is built.
		Created a new class called GeneralComment using base class BaseComment.
-
-		Todo:
-		Debuging till test 1 passed.

Testing Done:

		Five test added in reviews/tests.py:
		1. test_review_detail_general_comment_ordering
		2. test_init_with_general_comments
		3. test_init_with_mix
		4. test_init_general_comment_with_replies
		5. test_save_reply_comment_to_general_comment

~		Testing Result:
	~	All test passed.
-		Failing Test 1: at line 874, 875
-		The ordering on the page is wrong.
-		should be 3->2, given 2->3

~		checked timestamp: timestamp(2) > timestamp(3)
~		haven't figure out why the ordering is wrong
	~	Previously test 1 failed due to use diff viewer 'get' method on general comments.
	~	Figured out general comments behave the same as screenshot/file attachment comments.

Diff:

Revision 8 (+228 -24)

Show changes

	reviewboard/notifications/tests.py
	reviewboard/reviews/admin.py
	reviewboard/reviews/tests.py
	reviewboard/reviews/views.py
	reviewboard/reviews/models/__init__.py
	reviewboard/reviews/models/general_comment.py
	reviewboard/reviews/models/review.py
	reviewboard/reviews/models/review_request.py
	2 more

Tool: Pyflakes
Processed Files:
    reviewboard/reviews/views.py
    reviewboard/testing/testcase.py
    reviewboard/notifications/tests.py
    reviewboard/reviews/tests.py
    reviewboard/reviews/admin.py
    reviewboard/reviews/models/review_request.py
    reviewboard/reviews/models/__init__.py
    reviewboard/reviews/models/review.py
    reviewboard/reviews/models/general_comment.py
    reviewboard/reviews/templatetags/reviewtags.py



Tool: PEP8 Style Checker
Processed Files:
    reviewboard/reviews/views.py
    reviewboard/testing/testcase.py
    reviewboard/notifications/tests.py
    reviewboard/reviews/tests.py
    reviewboard/reviews/admin.py
    reviewboard/reviews/models/review_request.py
    reviewboard/reviews/models/__init__.py
    reviewboard/reviews/models/review.py
    reviewboard/reviews/models/general_comment.py
    reviewboard/reviews/templatetags/reviewtags.py

Commit:

-	cf7e34ba63330d877d8f7f1979d38388851eb005
+	15323ae10ef71324d8effd5e64c39f6a3f2a8816

Diff:

Revision 9 (+210 -24)

Show changes

	reviewboard/reviews/admin.py
	reviewboard/reviews/tests.py
	reviewboard/reviews/views.py
	reviewboard/reviews/models/__init__.py
	reviewboard/reviews/models/general_comment.py
	reviewboard/reviews/models/review.py
	reviewboard/reviews/models/review_request.py
	reviewboard/reviews/templatetags/reviewtags.py
	1 more

Tool: Pyflakes
Processed Files:
    reviewboard/reviews/views.py
    reviewboard/testing/testcase.py
    reviewboard/reviews/admin.py
    reviewboard/reviews/tests.py
    reviewboard/reviews/templatetags/reviewtags.py
    reviewboard/reviews/models/review_request.py
    reviewboard/reviews/models/__init__.py
    reviewboard/reviews/models/review.py
    reviewboard/reviews/models/general_comment.py



Tool: PEP8 Style Checker
Processed Files:
    reviewboard/reviews/views.py
    reviewboard/testing/testcase.py
    reviewboard/reviews/admin.py
    reviewboard/reviews/tests.py
    reviewboard/reviews/templatetags/reviewtags.py
    reviewboard/reviews/models/review_request.py
    reviewboard/reviews/models/__init__.py
    reviewboard/reviews/models/review.py
    reviewboard/reviews/models/general_comment.py

Commit:

-	15323ae10ef71324d8effd5e64c39f6a3f2a8816
+	db01216b35eecd70fa68885dc9582cc5c79936f4

Diff:

Revision 10 (+213 -24)

Show changes

	reviewboard/reviews/admin.py
	reviewboard/reviews/tests.py
	reviewboard/reviews/views.py
	reviewboard/reviews/models/__init__.py
	reviewboard/reviews/models/general_comment.py
	reviewboard/reviews/models/review.py
	reviewboard/reviews/models/review_request.py
	reviewboard/reviews/templatetags/reviewtags.py
	1 more

Tool: Pyflakes
Processed Files:
    reviewboard/reviews/views.py
    reviewboard/testing/testcase.py
    reviewboard/reviews/admin.py
    reviewboard/reviews/tests.py
    reviewboard/reviews/templatetags/reviewtags.py
    reviewboard/reviews/models/review_request.py
    reviewboard/reviews/models/__init__.py
    reviewboard/reviews/models/review.py
    reviewboard/reviews/models/general_comment.py



Tool: PEP8 Style Checker
Processed Files:
    reviewboard/reviews/views.py
    reviewboard/testing/testcase.py
    reviewboard/reviews/admin.py
    reviewboard/reviews/tests.py
    reviewboard/reviews/templatetags/reviewtags.py
    reviewboard/reviews/models/review_request.py
    reviewboard/reviews/models/__init__.py
    reviewboard/reviews/models/review.py
    reviewboard/reviews/models/general_comment.py

reviewboard/reviews/models/general_comment.py (Diff revision 10)
Show all issues
```
Should this be "gcomment"?
```

Commit:

-	db01216b35eecd70fa68885dc9582cc5c79936f4
+	2bc2e2f320aa9e98088cdaf9fc3299798a24aa55

Diff:

Revision 11 (+213 -24)

Show changes

	reviewboard/reviews/admin.py
	reviewboard/reviews/tests.py
	reviewboard/reviews/views.py
	reviewboard/reviews/models/__init__.py
	reviewboard/reviews/models/general_comment.py
	reviewboard/reviews/models/review.py
	reviewboard/reviews/models/review_request.py
	reviewboard/reviews/templatetags/reviewtags.py
	1 more

Tool: Pyflakes
Processed Files:
    reviewboard/reviews/views.py
    reviewboard/testing/testcase.py
    reviewboard/reviews/admin.py
    reviewboard/reviews/tests.py
    reviewboard/reviews/templatetags/reviewtags.py
    reviewboard/reviews/models/review_request.py
    reviewboard/reviews/models/__init__.py
    reviewboard/reviews/models/review.py
    reviewboard/reviews/models/general_comment.py



Tool: PEP8 Style Checker
Processed Files:
    reviewboard/reviews/views.py
    reviewboard/testing/testcase.py
    reviewboard/reviews/admin.py
    reviewboard/reviews/tests.py
    reviewboard/reviews/templatetags/reviewtags.py
    reviewboard/reviews/models/review_request.py
    reviewboard/reviews/models/__init__.py
    reviewboard/reviews/models/review.py
    reviewboard/reviews/models/general_comment.py

```
Looking great! A few small comments.
```

reviewboard/reviews/admin.py (Diff revision 11)

Show all issues

Generally, we like to keep these in alphabetical order. FileAttachmentComment clearly isn't, but can you reorder these so they'll be in the right order?

reviewboard/reviews/admin.py (Diff revision 11)

Show all issues

Should be text. (We fixed the above cases recently.)

reviewboard/reviews/admin.py (Diff revision 11)
Show all issues
```
Same comment about order.
```

reviewboard/reviews/models/general_comment.py (Diff revision 11)

Show all issues

This should be fleshed out more to describe the purpose of these comments.

Also, you should always use complete sentences with trailing periods.

For reference, multi-line docstrings are in the form of:

"""Single-line summary.

Multi-line docstring.
"""

reviewboard/reviews/models/general_comment.py (Diff revision 11)
Show all issues
```
Should use single quotes.
```
reviewboard/reviews/models/review.py (Diff revision 11)
Show all issues
```
Single quotes.
```
reviewboard/reviews/tests.py (Diff revision 11)
Show all issues
```
Alphabetical order.
```
reviewboard/reviews/views.py (Diff revision 11)
Show all issues
```
Alphabetical order.
```

reviewboard/testing/testcase.py (Diff revision 11)

Show all issues

This is unrelated to your change. Can you revert it?

reviewboard/reviews/tests.py (Diff revision 11)

Show all issues

These should be single quotes as well.

nicolexin Oct. 29, 2014, 12:06 p.m.

I seem to get the idea that string works like an identifier should be in single quotes, not sure if these should be changed. Maybe let's wait for a mentor to comment on this.

mloyzer Oct. 30, 2014, 6:28 p.m.

That's fine but later on in the same file:

def test_review_detail_file_attachment_visibility(self):

        caption_1 = 'File Attachment 1'

        caption_2 = 'File Attachment 2'

        ...

uses single quotes so it should at least be consistent within the file.

Commit:

-	2bc2e2f320aa9e98088cdaf9fc3299798a24aa55
+	13ac8eb929eae303aba325e15f6464441235f9c2

Diff:

Revision 12 (+223 -27)

Show changes

	reviewboard/reviews/admin.py
	reviewboard/reviews/tests.py
	reviewboard/reviews/views.py
	reviewboard/reviews/models/__init__.py
	reviewboard/reviews/models/general_comment.py
	reviewboard/reviews/models/review.py
	reviewboard/reviews/models/review_request.py
	reviewboard/reviews/templatetags/reviewtags.py
	1 more

Tool: Pyflakes
Processed Files:
    reviewboard/reviews/views.py
    reviewboard/testing/testcase.py
    reviewboard/reviews/admin.py
    reviewboard/reviews/tests.py
    reviewboard/reviews/templatetags/reviewtags.py
    reviewboard/reviews/models/review_request.py
    reviewboard/reviews/models/__init__.py
    reviewboard/reviews/models/review.py
    reviewboard/reviews/models/general_comment.py



Tool: PEP8 Style Checker
Processed Files:
    reviewboard/reviews/views.py
    reviewboard/testing/testcase.py
    reviewboard/reviews/admin.py
    reviewboard/reviews/tests.py
    reviewboard/reviews/templatetags/reviewtags.py
    reviewboard/reviews/models/review_request.py
    reviewboard/reviews/models/__init__.py
    reviewboard/reviews/models/review.py
    reviewboard/reviews/models/general_comment.py

reviewboard/reviews/admin.py (Diff revision 12)

Show all issues

Why is there an apostrophe(,) at the end of this function call?

justy777 Oct. 29, 2014, 2:06 p.m.
```
Sorry, I mean comma, not apostrophe.
```

david Oct. 29, 2014, 2:07 p.m.

This isn't a function call, it's a tuple with a length of 1

reviewboard/reviews/admin.py (Diff revision 12)
Show all issues
```
and here.
```

reviewboard/reviews/tests.py (Diff revision 12)

Show all issues

Why not use a list instead of 3 variables?

nicolexin Nov. 1, 2014, 3:43 p.m.

Just to be consistent, and using a list is not gonna saving me some space anyway.

reviewboard/reviews/tests.py (Diff revision 12)

Show all issues

There should be a blank line between these lines.

nicolexin Nov. 1, 2014, 3:43 p.m.

Can I ask why? coz previous code does not include a blank line here

reviewboard/reviews/tests.py (Diff revision 12)

Show all issues

Should use a loop instead of repeating a function call.
for _ in range(3):

   <function here>

nicolexin Nov. 1, 2014, 3:43 p.m.

Again, I'm being consistent with previous code, I think it's fine coz 3 is not too much?

reviewboard/reviews/views.py (Diff revision 12)

Show all issues

There should be a trailing apostrophe(,) after 'general_comments': []

justy777 Oct. 29, 2014, 2:06 p.m.

I meant, comma intsead of apostrophe here too.

chipx86 Nov. 1, 2014, 4:47 p.m.

Yeah, previous code didn't have it, but it's good to add.

The reason is that, if we have to add another item to this list in the future, it's a single insert, rather than a change to a line and an insert (which keeps patches smaller and reduces what they have to touch).

Commit:

-	13ac8eb929eae303aba325e15f6464441235f9c2
+	0c9a220772d3669ec86418acbae4dea4aea26972

Diff:

Revision 13 (+225 -28)

Show changes

	reviewboard/reviews/admin.py
	reviewboard/reviews/managers.py
	reviewboard/reviews/tests.py
	reviewboard/reviews/views.py
	reviewboard/reviews/models/__init__.py
	reviewboard/reviews/models/general_comment.py
	reviewboard/reviews/models/review.py
	reviewboard/reviews/models/review_request.py
	2 more

Tool: Pyflakes
Processed Files:
    reviewboard/reviews/views.py
    reviewboard/testing/testcase.py
    reviewboard/reviews/admin.py
    reviewboard/reviews/templatetags/reviewtags.py
    reviewboard/reviews/tests.py
    reviewboard/reviews/models/__init__.py
    reviewboard/reviews/models/review_request.py
    reviewboard/reviews/models/review.py
    reviewboard/reviews/models/general_comment.py
    reviewboard/reviews/managers.py



Tool: PEP8 Style Checker
Processed Files:
    reviewboard/reviews/views.py
    reviewboard/testing/testcase.py
    reviewboard/reviews/admin.py
    reviewboard/reviews/templatetags/reviewtags.py
    reviewboard/reviews/tests.py
    reviewboard/reviews/models/__init__.py
    reviewboard/reviews/models/review_request.py
    reviewboard/reviews/models/review.py
    reviewboard/reviews/models/general_comment.py
    reviewboard/reviews/managers.py

Looks good! I only have a couple comments left.

reviewboard/reviews/models/general_comment.py (Diff revision 12)

Show all issues

This should probably be more specific, like:

"A comment on a review request not tied to code or a file."

reviewboard/reviews/models/general_comment.py (Diff revision 12)
Show all issues
```
"Examples" and "testing".
```

Commit:

-	0c9a220772d3669ec86418acbae4dea4aea26972
+	09664a6c4070138942079007b173052b3ce09e2f

Diff:

Revision 14 (+225 -28)

Show changes

	reviewboard/reviews/admin.py
	reviewboard/reviews/managers.py
	reviewboard/reviews/tests.py
	reviewboard/reviews/views.py
	reviewboard/reviews/models/__init__.py
	reviewboard/reviews/models/general_comment.py
	reviewboard/reviews/models/review.py
	reviewboard/reviews/models/review_request.py
	2 more

Tool: Pyflakes
Processed Files:
    reviewboard/reviews/views.py
    reviewboard/testing/testcase.py
    reviewboard/reviews/admin.py
    reviewboard/reviews/templatetags/reviewtags.py
    reviewboard/reviews/tests.py
    reviewboard/reviews/models/__init__.py
    reviewboard/reviews/models/review_request.py
    reviewboard/reviews/models/review.py
    reviewboard/reviews/models/general_comment.py
    reviewboard/reviews/managers.py



Tool: PEP8 Style Checker
Processed Files:
    reviewboard/reviews/views.py
    reviewboard/testing/testcase.py
    reviewboard/reviews/admin.py
    reviewboard/reviews/templatetags/reviewtags.py
    reviewboard/reviews/tests.py
    reviewboard/reviews/models/__init__.py
    reviewboard/reviews/models/review_request.py
    reviewboard/reviews/models/review.py
    reviewboard/reviews/models/general_comment.py
    reviewboard/reviews/managers.py

Description:

~		The general comment model is built.
~		Created a new class called GeneralComment using base class BaseComment.
	~	Reviewboard currently support 3 types of comments: Diff commment, File attachment comments, Screenshot comments.
	~	This is a new type of comments: General comments.
	+
	+	A general comment on a review request is used when a comment is not tied to specific lines of code or a special file attachment, and an issue is opened. Examples include suggestions for testing or pointing out errors in the change description.
	+
	+	This review request is for GeneralComment backend model.

reviewboard/reviews/models/general_comment.py (Diff revisions 13 - 14)
Show all issues
```
Minor grammar fix, it should read:

"...review request that is not tied..."
```

Commit:

-	09664a6c4070138942079007b173052b3ce09e2f
+	cd5ccbf9a9e5e07d1f6cae8e41671e43b8f8baa9

Diff:

Revision 15 (+225 -28)

Show changes

	reviewboard/reviews/admin.py
	reviewboard/reviews/managers.py
	reviewboard/reviews/tests.py
	reviewboard/reviews/views.py
	reviewboard/reviews/models/__init__.py
	reviewboard/reviews/models/general_comment.py
	reviewboard/reviews/models/review.py
	reviewboard/reviews/models/review_request.py
	2 more

Tool: Pyflakes
Processed Files:
    reviewboard/reviews/views.py
    reviewboard/testing/testcase.py
    reviewboard/reviews/admin.py
    reviewboard/reviews/templatetags/reviewtags.py
    reviewboard/reviews/tests.py
    reviewboard/reviews/models/__init__.py
    reviewboard/reviews/models/review_request.py
    reviewboard/reviews/models/review.py
    reviewboard/reviews/models/general_comment.py
    reviewboard/reviews/managers.py



Tool: PEP8 Style Checker
Processed Files:
    reviewboard/reviews/views.py
    reviewboard/testing/testcase.py
    reviewboard/reviews/admin.py
    reviewboard/reviews/templatetags/reviewtags.py
    reviewboard/reviews/tests.py
    reviewboard/reviews/models/__init__.py
    reviewboard/reviews/models/review_request.py
    reviewboard/reviews/models/review.py
    reviewboard/reviews/models/general_comment.py
    reviewboard/reviews/managers.py

Ship it!

```
Ship It!
```

Status: Closed (submitted)

Change Summary:

Pushed to ucosp/nicole_xin/general-comments (117bd9f)