1213456 - Commit table truncated in public view when there are unpublished drafts

Status: RESOLVED FIXED

(MozReview Graveyard :: General, defect, P1)

Description

[Gregory Szorc [:gps]]

/r/19391 had 4 published children review requests. mcomella pushed a single changeset to it and didn't publish. The parent review request in his browser was showing a commit table with one entry and the draft banner.

However, the review requests in my, non-submitter, non-admin view had commit tables with only a single commit! The 4 old commits were published and the new single commit was draft. Somehow draft state was impacting what the commit table displays.

I have a dump of the database at reviewboard1.webapp.scl3:/root/db-20151009.sql if anyone wants to poke around.

Here is extra_data from the parent (19391):

{"p2rb.first_public_ancestor": "ed8188590f14b1aae2e4f44c8196994f375a99f4", "p2rb.is_squashed": true, "p2rb.base_commit": "a2ad0230946a4acc5c5cf9f6fa76d93aa7b2b861", "p2rb.identifier": "bz://1201206/mcomella", "p2rb.reviewer_map": "{\"19515\": [101835], \"19511\": [101835], \"19399\": [186753], \"19513\": [101835], \"19397\": [186753], \"19395\": [186753], \"19393\": [186753]}", "p2rb.discard_on_publish_rids": "[19395, 19397, 19399]", "p2rb.unpublished_rids": "[]", "p2rb.commits": "[[\"8447895866926612dfee3cedbe1a2c5370c76b64\", 19393], [\"b91041a0e06937b22920759b5b829ed9f102849e\", 19395], [\"60b3297b1f07e98f0189d055a4254cfca6851f49\", 19397], [\"5ff23b0e802006a2ef15f3344f21e0bf63a1fcd2\", 19399]]", "p2rb": true}

This is a P1 because we don't want draft state impacting the public view.

Comment 1

[Gregory Szorc [:gps]]

Looks like the commits table is populated indirectly from the output of mozreview.extra_data.gen_child_rrs().

    commit_tuples = json.loads(review_request.extra_data[COMMITS_KEY])
    for commit_tuple in commit_tuples:
        child = get_rr_for_id(commit_tuple[1])

        # TODO: We should fail if we can't find a child; it indicates
        # something very bad has happened.  Unfortunately this call is
        # used in several different contexts, so we need to make changes
        # there as well.
        if not child:
            continue
        elif user is None:
            yield child
        elif child.is_accessible_by(user):
            yield child.get_draft(user) or child

I'm guessing the bug is somewhere in the last 2 lines.

Comment 2

[Mauro Doglio [:mdoglio]]

I can reproduce it locally, working on a fix.

Comment 3

[Mauro Doglio [:mdoglio]]

Attachment: MozReview Request: mozreview: always pass the current user when getting a draft (bug 1213456); r?smacleod

mozreview: always pass the current user when getting a draft (bug 1213456); r?smacleod

Comment 4

[Steven MacLeod [:smacleod]]

Comment on attachment 8673250 [details]
MozReview Request: mozreview: always pass the current user when getting a draft (bug 1213456); r?smacleod

https://reviewboard.mozilla.org/r/21877/#review19703