• 
      

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

    Review Request #8240 — Created June 12, 2016 and submitted — Latest diff uploaded

    Information

    Review Board
    release-3.0.x
    a36a3b6...

    Reviewers

    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.

    1. 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.
    2. 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.

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