Commit table truncated in public view when there are unpublished drafts

RESOLVED FIXED

Status

P1
normal
RESOLVED FIXED
3 years ago
3 years ago

People

(Reporter: gps, Assigned: mdoglio)

Tracking

Details

Attachments

(1 attachment)

(Reporter)

Description

3 years ago
/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.
(Reporter)

Comment 1

3 years ago
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.
(Assignee)

Updated

3 years ago
Assignee: nobody → mdoglio
Status: NEW → ASSIGNED
(Assignee)

Comment 2

3 years ago
I can reproduce it locally, working on a fix.
(Assignee)

Comment 3

3 years ago
Created attachment 8673250 [details]
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
Attachment #8673250 - Flags: review?(smacleod)
Attachment #8673250 - Flags: review?(smacleod) → review+
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
(Assignee)

Updated

3 years ago
Status: ASSIGNED → RESOLVED
Last Resolved: 3 years ago
Resolution: --- → FIXED
Product: Developer Services → MozReview
You need to log in before you can comment on or make changes to this bug.