Summary

Fix bugs related to general comments and Ship It indications in review emails.

Review Request #8240 — Created June 12, 2016 and submitted June 21, 2016, 10:21 p.m.

Information

Owner

gmyers

Repository

Review Board

Branch

release-3.0.x

Bugs

Depends On

Commit

a36a3b6...

Reviewers

Groups

reviewboard

People

Description

Address two inconsistencies where general comments were not being handled in
the same fashion as the other classes of comments (e.g. diff, file attachment,
and screenshot comments). These would lead to errant details in review emails
for reviews where a Ship It had been declared.

The existence of general comments was not being considered when determining
the ship_it_only state of Review objects. This resulted in review emails
where the X-ReviewBoard-ShipIt-Only header was set even when general comments
were present.
The existence of general comments with open issues was not being considered
when determining the need for the "Fix it, then Ship it!" designation for review
emails. This produced review emails with the "Ship it!" designation, even when
the review contained general comments with issues.

To resolve both bugs, separate queries that accounted for the other three
classes of comments were centralized into a new function, has_comments(),
and a query for general comments was introduced here as well. Furthermore, with
respect to item 2, the logic for determining the presence of any comments with
issues is now conditioned on review.ship_it, since this is the only state under
which this information is necessary. Previously this was calculated
unconditionally, which was overkill.

To facilitate consistency and testability, a FIX_IT_THEN_SHIP_IT_TEXT
constant has been added to the Review class to hold the
"Fix it, then Ship it!" text. This is analogous to SHIP_IT_TEXT which
already exists. These constants are now utilized in the review email templates
rather than the prior string literals. This introduces one minor delta for
Ship It emails. Previously, the string literal was "Ship it!", whereas the
string in SHIP_IT_TEXT is "Ship It!'. That is, the "I" in "It" is now
capitalized.

Also, added GeneralComment to the list of comment classes discussed in the
documentation for the CommentDetailDisplayHook.

Testing Done


Augmented all existing unit tests related to review emails with Ship It to

verify that "Fix it, then Ship it!" does not appear in the message.
Added a new unit test for review emails with a Ship It and general

comments.  This is analogous to existing tests for the other three classes of 

comments.  This test would fail without the fix.
Added new unit tests for all four classes of comments to further test review

email's with a Ship It and comments with an opened issue.  Prior to the fix,

the specific test for the general comment class would fail.

Confirmed all unit tests pass.  Built docs and reviewed.

Issues

Description	From	Last Updated
Checking review.ship_it_only and then re-doing queries to check for open issues means we have to hit the database up to …	brennie	June 14, 2016, 6:07 p.m.
Can we refactor this into a method like: def has_comments(check_for_issues=False): qs = [ self.comments, self.file_attachment_comments, self.screenshot_comments, self.general_comments, ] if check_for_issues: …	brennie	June 14, 2016, 6:07 p.m.
Single space after a period. "Whether or not" "with open issues only"	brennie	June 21, 2016, 6:38 p.m.
Mind adding a comment next to these that these are explicitly not marked for localization because they're used in e-mails, …	david	June 16, 2016, 5:25 p.m.
I think this would be slightly clearer if the parameter was named only_issues	david	June 16, 2016, 5:25 p.m.
This should be in the imperative mood ("Return whether" rather than "Returns whether")	david	June 16, 2016, 5:25 p.m.
This can easily fit all on one line.	david	June 16, 2016, 5:25 p.m.
The description should be on the line after the type: bool: ``True`` if the review contains any comments/issues and ``False`` …	david	June 18, 2016, 5:43 a.m.

Tool: Pyflakes
Processed Files:
    reviewboard/notifications/email.py
    reviewboard/notifications/tests.py
    reviewboard/reviews/models/review.py

Ignored Files:
    docs/manual/extending/extensions/hooks/comment-detail-display-hook.rst
    reviewboard/templates/notifications/review_email.txt
    reviewboard/templates/notifications/review_email.html



Tool: PEP8 Style Checker
Processed Files:
    reviewboard/notifications/email.py
    reviewboard/notifications/tests.py
    reviewboard/reviews/models/review.py

Ignored Files:
    docs/manual/extending/extensions/hooks/comment-detail-display-hook.rst
    reviewboard/templates/notifications/review_email.txt
    reviewboard/templates/notifications/review_email.html

Thanks for catching this! I have some refactoring suggestions to clean up the code (it was already a bit messy before you got to it).

reviewboard/notifications/email.py (Diff revision 1)

Show all issues

Checking review.ship_it_only and then re-doing queries to check for open issues means we have to hit the database up to 8 times (twice for each comment type).

I also prefer the name has_issues over fix_it_then_ship_it (its more wordy). However, you are right that we do not always need to calculate it!

With my suggested refactor for has_comments, we can do:

has_issues = (review.ship_it and
              review.has_comments(check_for_issues=True))

This is a lot cleaner, IMO.

reviewboard/reviews/models/review.py (Diff revision 1)

Show all issues

Can we refactor this into a method like:

def has_comments(check_for_issues=False):
    qs = [
        self.comments,
        self.file_attachment_comments,
        self.screenshot_comments,
        self.general_comments,
    ]

    if check_for_issues:
        qs = [
            q.filter(issue_opened=True)
            for q in qs
        ]

    return any(
        q.exists()
        for q in qs
    )


Then we can refactor ship_it_only to use has_comments(check_for_issues=False) and mail_review can be refactored as well.

Change Summary:

Implement Barret's refactoring suggestions.

Description:

		Address two inconsistencies where general comments were not being handled in
		the same fashion as the other classes of comments (e.g. diff, file attachment,
		and screenshot comments). These would lead to errant details in review emails
		for reviews where a Ship It had been declared.

		The existence of general comments was not being considered when determining the `ship_it_only` state of `Review` objects. This resulted in review emails where the `X-ReviewBoard-ShipIt-Only` header was set even when general comments were present.
		The existence of general comments with open issues was not being considered when determining the need for the "Fix it, then Ship it!" designation for review emails. This produced review emails with the "Ship it!" designation, even when the review contained general comments with issues.

~		To resolve both bugs, queries to test general comment existence were added to
~		existing logic that already accounted for the other three classes of comments.
~		Furthermore, with respect to item 2, the logic for determining the presence of
~		any comments with issues is now conditioned on
~		`review.ship_it and not review.ship_it_only`, since this is the only state under
	~	To resolve both bugs, separate queries that accounted for the other three
	~	classes of comments were centralized into a new function, `has_comments()`,
	~	and a query for general comments was introduced here as well. Furthermore, with
	~	respect to item 2, the logic for determining the presence of any comments with
	~	issues is now conditioned on `review.ship_it`, since this is the only state under
		which this information is necessary. Previously this was calculated
		unconditionally, which was overkill.

		To facilitate consistency and testability, a `FIX_IT_THEN_SHIP_IT_TEXT`
		constant has been added to the `Review` class to hold the
		"Fix it, then Ship it!" text. This is analogous to `SHIP_IT_TEXT` which
		already exists. These constants are now utilized in the review email templates
		rather than the prior string literals. This introduces one minor delta for
		Ship It emails. Previously, the string literal was "Ship it!", whereas the
		string in `SHIP_IT_TEXT` is "Ship It!'. That is, the "I" in "It" is now
		capitalized.

		Also, added `GeneralComment` to the list of comment classes discussed in the
		documentation for the `CommentDetailDisplayHook`.

Commit:

-	bf451e852a4f8d8db5b8abe68f9b1c61fcdf0b3e
+	76d0d999538e01e250d0855b6dfae96ad08aab62

Diff:

Revision 2 (+188 -13)

Show changes

	docs/manual/extending/extensions/hooks/comment-detail-display-hook.rst
	reviewboard/notifications/email.py
	reviewboard/notifications/tests.py
	reviewboard/reviews/models/review.py
	reviewboard/templates/notifications/review_email.txt
	reviewboard/templates/notifications/review_email.html

Tool: Pyflakes
Processed Files:
    reviewboard/notifications/email.py
    reviewboard/notifications/tests.py
    reviewboard/reviews/models/review.py

Ignored Files:
    docs/manual/extending/extensions/hooks/comment-detail-display-hook.rst
    reviewboard/templates/notifications/review_email.txt
    reviewboard/templates/notifications/review_email.html



Tool: PEP8 Style Checker
Processed Files:
    reviewboard/notifications/email.py
    reviewboard/notifications/tests.py
    reviewboard/reviews/models/review.py

Ignored Files:
    docs/manual/extending/extensions/hooks/comment-detail-display-hook.rst
    reviewboard/templates/notifications/review_email.txt
    reviewboard/templates/notifications/review_email.html

reviewboard/reviews/models/review.py (Diff revision 2)

Show all issues

Mind adding a comment next to these that these are explicitly not marked for localization because they're used in e-mails, where we don't want to take the submitting user's locale into effect?

reviewboard/reviews/models/review.py (Diff revision 2)

Show all issues

I think this would be slightly clearer if the parameter was named only_issues

reviewboard/reviews/models/review.py (Diff revision 2)

Show all issues

This should be in the imperative mood ("Return whether" rather than "Returns whether")

reviewboard/reviews/models/review.py (Diff revision 2)
Show all issues
```
This can easily fit all on one line.
```

Change Summary:

Address David's feedback.

Commit:

-	76d0d999538e01e250d0855b6dfae96ad08aab62
+	db73a2d5573110955321d9e9fb03fb9949cb9a44

Diff:

Revision 3 (+188 -13)

Show changes

	docs/manual/extending/extensions/hooks/comment-detail-display-hook.rst
	reviewboard/notifications/email.py
	reviewboard/notifications/tests.py
	reviewboard/reviews/models/review.py
	reviewboard/templates/notifications/review_email.txt
	reviewboard/templates/notifications/review_email.html

Tool: Pyflakes
Processed Files:
    reviewboard/notifications/email.py
    reviewboard/notifications/tests.py
    reviewboard/reviews/models/review.py

Ignored Files:
    docs/manual/extending/extensions/hooks/comment-detail-display-hook.rst
    reviewboard/templates/notifications/review_email.txt
    reviewboard/templates/notifications/review_email.html



Tool: PEP8 Style Checker
Processed Files:
    reviewboard/notifications/email.py
    reviewboard/notifications/tests.py
    reviewboard/reviews/models/review.py

Ignored Files:
    docs/manual/extending/extensions/hooks/comment-detail-display-hook.rst
    reviewboard/templates/notifications/review_email.txt
    reviewboard/templates/notifications/review_email.html

```
Just one tiny nit:
```

reviewboard/reviews/models/review.py (Diff revision 3)

Show all issues

The description should be on the line after the type:

bool:
``True`` if the review contains any comments/issues and
``False`` otherwise.

Change Summary:

Fix formatting in docstring for return type.

Commit:

-	db73a2d5573110955321d9e9fb03fb9949cb9a44
+	c21c85d8322a05539bdec7f906475268ec5a1951

Diff:

Revision 4 (+189 -13)

Show changes

	docs/manual/extending/extensions/hooks/comment-detail-display-hook.rst
	reviewboard/notifications/email.py
	reviewboard/notifications/tests.py
	reviewboard/reviews/models/review.py
	reviewboard/templates/notifications/review_email.txt
	reviewboard/templates/notifications/review_email.html

Tool: Pyflakes
Processed Files:
    reviewboard/notifications/email.py
    reviewboard/notifications/tests.py
    reviewboard/reviews/models/review.py

Ignored Files:
    docs/manual/extending/extensions/hooks/comment-detail-display-hook.rst
    reviewboard/templates/notifications/review_email.txt
    reviewboard/templates/notifications/review_email.html



Tool: PEP8 Style Checker
Processed Files:
    reviewboard/notifications/email.py
    reviewboard/notifications/tests.py
    reviewboard/reviews/models/review.py

Ignored Files:
    docs/manual/extending/extensions/hooks/comment-detail-display-hook.rst
    reviewboard/templates/notifications/review_email.txt
    reviewboard/templates/notifications/review_email.html

Ship it!

reviewboard/reviews/models/review.py (Diff revisions 1 - 4)

Show all issues

Single space after a period.

"Whether or not"

"with open issues only"

gmyers June 20, 2016, 6:35 p.m.

I'll take care of the first two. With respect to the third, any concern that this could cause confusion about what is being checked here, between a comment with issue_opened = True and one with issue_status = open (I admittedly don't have the greatest sense of the distinction between these fields)?

david June 21, 2016, 9:46 a.m.

The issue_status field only applies if issue_opened is True. Probably we should be specific and say "check for comments where ``issue_opened`` is ``True``"

Change Summary:

Clean up description of only_issues argument.

Commit:

-	c21c85d8322a05539bdec7f906475268ec5a1951
+	a36a3b62446e37a2d786a84b3f6106fb3dd94a8e

Diff:

Revision 5 (+189 -13)

Show changes

	docs/manual/extending/extensions/hooks/comment-detail-display-hook.rst
	reviewboard/notifications/email.py
	reviewboard/notifications/tests.py
	reviewboard/reviews/models/review.py
	reviewboard/templates/notifications/review_email.txt
	reviewboard/templates/notifications/review_email.html

Tool: Pyflakes
Processed Files:
    reviewboard/notifications/email.py
    reviewboard/notifications/tests.py
    reviewboard/reviews/models/review.py

Ignored Files:
    docs/manual/extending/extensions/hooks/comment-detail-display-hook.rst
    reviewboard/templates/notifications/review_email.txt
    reviewboard/templates/notifications/review_email.html



Tool: PEP8 Style Checker
Processed Files:
    reviewboard/notifications/email.py
    reviewboard/notifications/tests.py
    reviewboard/reviews/models/review.py

Ignored Files:
    docs/manual/extending/extensions/hooks/comment-detail-display-hook.rst
    reviewboard/templates/notifications/review_email.txt
    reviewboard/templates/notifications/review_email.html

Status: Closed (submitted)

Change Summary:

Pushed to release-3.0.x (36d91a6)