Fix bugs related to general comments and Ship It indications in review emails.
Review Request #8240 — Created June 12, 2016 and submitted
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
theship_it_only
state ofReview
objects. This resulted in review emails
where theX-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 onreview.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 theReview
class to hold the
"Fix it, then Ship it!" text. This is analogous toSHIP_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 inSHIP_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 theCommentDetailDisplayHook
.
- 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.
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 | |
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 | |
Single space after a period. "Whether or not" "with open issues only" |
brennie | |
Mind adding a comment next to these that these are explicitly not marked for localization because they're used in e-mails, … |
david | |
I think this would be slightly clearer if the parameter was named only_issues |
david | |
This should be in the imperative mood ("Return whether" rather than "Returns whether") |
david | |
This can easily fit all on one line. |
david | |
The description should be on the line after the type: bool: ``True`` if the review contains any comments/issues and ``False`` … |
david |
-
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).
-
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
overfix_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.
-
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 usehas_comments(check_for_issues=False)
andmail_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
theship_it_only
state ofReview
objects. This resulted in review emails
where theX-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 underwhich 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
whichalready 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 nowcapitalized. Also, added
GeneralComment
to the list of comment classes discussed in thedocumentation for the CommentDetailDisplayHook
. - The existence of general comments was not being considered when determining
- Commit:
-
bf451e852a4f8d8db5b8abe68f9b1c61fcdf0b3e76d0d999538e01e250d0855b6dfae96ad08aab62
- Diff:
-
Revision 2 (+188 -13)
-
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
- Change Summary:
-
Address David's feedback.
- Commit:
-
76d0d999538e01e250d0855b6dfae96ad08aab62db73a2d5573110955321d9e9fb03fb9949cb9a44
- Diff:
-
Revision 3 (+188 -13)
-
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
- Change Summary:
-
Fix formatting in docstring for return type.
- Commit:
-
db73a2d5573110955321d9e9fb03fb9949cb9a44c21c85d8322a05539bdec7f906475268ec5a1951
- Diff:
-
Revision 4 (+189 -13)
-
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
- Change Summary:
-
Clean up description of
only_issues
argument. - Commit:
-
c21c85d8322a05539bdec7f906475268ec5a1951a36a3b62446e37a2d786a84b3f6106fb3dd94a8e
- Diff:
-
Revision 5 (+189 -13)
-
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