1285855 - impromptu/drive-by reviews are not shown in the commits table

Status: RESOLVED WONTFIX

(MozReview Graveyard :: General, defect)

Description

[:glob ✱]

str:
1. request a review from level1
2. set r+ as level2

expected:
commits table shows | level2 (r+), level1 (r?) |

actual:
commits table shows | level1 (r?) |

the attaching in bugzilla is updated correctly, and the review is visible in the 'reviews' tab.

Comment 1

[:glob ✱]

Attachment: mozreview: add impromptu/drive-by filtering to template tags (bug 1285855)

Add the ability to filter include or exclude drive-by reviews to the
reviewers_status template tag.

Review commit: https://reviewboard.mozilla.org/r/63410/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/63410/

Comment 2

[:glob ✱]

Attachment: mozreview: Show impromptu/drive-by reviews in the commits table (bug 1285855);

Adds impromptu/drive-by reviews to the commits table, in order to provide a
complete picture of the reviews.

As the in-table editor is for editing target reviewers, we currently cannot
edit impromptu reviews, so these reviews are rendered outside of the editable
container.

Review commit: https://reviewboard.mozilla.org/r/63412/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/63412/

Comment 3

[Steven MacLeod [:smacleod]]

Comment on attachment 8769559 [details]
mozreview: Show impromptu/drive-by reviews in the commits table (bug 1285855);

https://reviewboard.mozilla.org/r/63412/#review60744

::: pylib/mozreview/mozreview/templates/mozreview/commits.html:57
(Diff revision 1)
> +        <span class="mozreview-child-impromptu-reviews">
> +          {% for reviewer, status in child_details|reviewers_status:True %}
> +            <span class="reviewer-name {% if status.ship_it %}reviewer-ship-it{% endif %} {{status.review_flag|review_flag_class}}"
> +                  title="Impromptu review.">{{ reviewer }}</span>
> +          {% endfor %}
> +        </span>
>          <span class="mozreview-child-reviewer-list"

It seems strange to me that we list impromptu reviews first.

::: pylib/mozreview/mozreview/templates/mozreview/commits.html:65
(Diff revision 1)
> +                  title="Impromptu review.">{{ reviewer }}</span>
> +          {% endfor %}
> +        </span>
>          <span class="mozreview-child-reviewer-list"
>                data-id="{{child_details.get_review_request.id}}">
> -          {% for reviewer, status in child_details|reviewers_status %}
> +          {% for reviewer, status in child_details|reviewers_status:False %}

Please add screenshots showing this change in the various new states.

Comment 4

[Steven MacLeod [:smacleod]]

Comment on attachment 8769558 [details]
mozreview: add impromptu/drive-by filtering to template tags (bug 1285855)

https://reviewboard.mozilla.org/r/63410/#review60746

::: pylib/mozreview/mozreview/review_helpers.py:140
(Diff revision 1)
> +    if drive_by_only:
> +        for user in reviewer_requests:
> +            if (user in reviewers_status) and (
> +                    not reviewers_status[user]['drive_by']):
> +                reviewers_status.pop(user)

I'm not going to lie, adding them then removing feels a little gross... this function seems to be getting pretty convoluted. Why not just add the information about being a "drive_by"[1] and then filter after the fact in a template tag/caller etc


[1] which we should refer to here as "impromptu" to be consistent with the next commit (and... progressive?).

::: pylib/mozreview/mozreview/templatetags/mozreview.py:125
(Diff revision 1)
>  def ssh_to_https(landing_url):
>      return landing_url.rstrip('/').replace('ssh://', 'https://')
>  
>  
>  @register.filter()
> -def reviewers_status(review_request):
> +def reviewers_status(review_request, drive_by_only):

When we use this, we're going to continue showing the status for these users even if they only left a review 10 revisions ago (and might not even be carried forward). Should we maybe only show impromptu reviews which are relavant to autoland?

Comment 5

[:glob ✱]

(In reply to Steven MacLeod [:smacleod] from comment #4)
> When we use this, we're going to continue showing the status for these users
> even if they only left a review 10 revisions ago (and might not even be
> carried forward). Should we maybe only show impromptu reviews which are
> relavant to autoland?

smacleod and i spoke at length about this, and some other issues with the approach.
there's a new plan:

- when an impromptu reviewer sets the review flag (ie flag_state), add that user as a target reviewer
- migrate/fix existing impromptu reviews

the first step is the easiest, and provides a consistent user experience -- impromptu reviews won't be special.

for migration we need to look at reviews made against the current revision only; if an impromptu review was left against an older revision but not the latest, we need to ensure that reviewer isn't re-added to the review request.  this should be possible by comparing the timestamps of the impromptu review and the current revision.

Comment 6

[:glob ✱]

mozreview is going away – wontfix'ing