1276847 - reviewer list empty view viewing draft after making changes

Status: RESOLVED FIXED

(MozReview Graveyard :: General, defect)

Description

[:glob ✱]

STR:
1. alter the reviewers on a commit, click 'ok'
2. refresh page

expected:
list of reviewers matches the value in the draft.

actual:
empty list of reviewers.

Comment 1

[:glob ✱]

Attachment: MozReview Request: mozreview: don't hide reviewers when viewing drafts (bug 1276847) r?mars

Instead of hiding all reviewers when viewing a draft, just hide the review
statuses.

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

Comment 2

[Māris Fogels [:mars] (please needinfo)]

Comment on attachment 8758130 [details]
MozReview Request: mozreview: don't hide reviewers when viewing drafts (bug 1276847) r?mars

https://reviewboard.mozilla.org/r/56496/#review53116

Is there a unit test for the template or a unit test for the view?  It would be good to have one to document and verify the new (bug-free) behaviour associated with drafts.

::: pylib/mozreview/mozreview/review_helpers.py:97
(Diff revision 1)
>      designated_reviewers = review_request.target_people.all()
>      if not reviewers:
>          reviewers = designated_reviewers
>  
> +    # We need to grab the reviews and statuses off the non-draft.
> +    if type(review_request) == ReviewRequestDraft:

This should use duck-typing instead of type checking.

Do ReviewRequest objects have an `is_draft` method we could use instead?

::: pylib/mozreview/mozreview/templates/mozreview/commits.html:61
(Diff revision 1)
>        <td class="reviewers">
>          <span class="mozreview-child-reviewer-list"
>                data-id="{{child_details.get_review_request.id}}">
>            {% for reviewer, status in child_details|reviewers_status %}
>              {% if not forloop.first %}, {% endif %}
> -            <span class="reviewer-name{% if status.ship_it %} reviewer-ship-it{% endif %} {{status.review_flag|review_flag_class}}">{{ reviewer }}</span>
> +            <span class="reviewer-name {{child_details|review_ship_it_class:status}} {{child_details|review_flag_class:status}}">{{ reviewer }}</span>

Since both new filters have special-case code for when the review is a draft, could the template code and filters be simplified by surrounding the whole template block in an `{% if draft %}` check, then calling the filters only if the draft status is false?

::: pylib/mozreview/mozreview/templatetags/mozreview.py:138
(Diff revision 1)
>          return 'Unknown user'
>  
>  
>  @register.filter()
> -def review_flag_class(review_flag):
> +def review_flag_class(review_request, status):
> +    if type(review_request) == ReviewRequestDraft:

Same comment as above: can we use an `is_draft` method or property to get the draft status?  Duck-typing is better than type-sniffing.

Are there unit tests for this procedure?  I see that the method parameter types changed, and we now have a special-case return value.  I would expect the tests to change with it.

::: pylib/mozreview/mozreview/templatetags/mozreview.py:151
(Diff revision 1)
> -    return reviewer_status_class_map.get(review_flag)
> +    return reviewer_status_class_map.get(status['review_flag'])
> +
> +
> [e-mail]()
> +def review_ship_it_class(review_request, status):
> +    if type(review_request) != ReviewRequestDraft and status['ship_it']:

Same comment as above re: type-sniffing and unit tests.

Comment 3

[:glob ✱]

https://reviewboard.mozilla.org/r/56496/#review53116

> This should use duck-typing instead of type checking.
> 
> Do ReviewRequest objects have an `is_draft` method we could use instead?

no, is_draft doesn't exist.

i'd love to use duck-typing however i can't change the classes.

as per irc, i'll switch to instanceof()

> Since both new filters have special-case code for when the review is a draft, could the template code and filters be simplified by surrounding the whole template block in an `{% if draft %}` check, then calling the filters only if the draft status is false?

my initial version had that, but i switched to what you see here to avoid duplicating the html in the template.
it's pretty well 'six of one', so i'll switch back and see how it feels again :)

> Same comment as above: can we use an `is_draft` method or property to get the draft status?  Duck-typing is better than type-sniffing.
> 
> Are there unit tests for this procedure?  I see that the method parameter types changed, and we now have a special-case return value.  I would expect the tests to change with it.

we don't have any functional selenium tests :(

Comment 4

[:glob ✱]

https://reviewboard.mozilla.org/r/56496/#review53150

::: pylib/mozreview/mozreview/review_helpers.py:97
(Diff revision 1)
>      designated_reviewers = review_request.target_people.all()
>      if not reviewers:
>          reviewers = designated_reviewers
>  
> +    # We need to grab the reviews and statuses off the non-draft.
> +    if type(review_request) == ReviewRequestDraft:

switch to instanceof()

::: pylib/mozreview/mozreview/templatetags/mozreview.py:138
(Diff revision 1)
>          return 'Unknown user'
>  
>  
>  @register.filter()
> -def review_flag_class(review_flag):
> +def review_flag_class(review_request, status):
> +    if type(review_request) == ReviewRequestDraft:

switch to instanceof()

::: pylib/mozreview/mozreview/templatetags/mozreview.py:151
(Diff revision 1)
> -    return reviewer_status_class_map.get(review_flag)
> +    return reviewer_status_class_map.get(status['review_flag'])
> +
> +
> [e-mail]()
> +def review_ship_it_class(review_request, status):
> +    if type(review_request) != ReviewRequestDraft and status['ship_it']:

switch to instanceof()

Comment 5

[Māris Fogels [:mars] (please needinfo)]

https://reviewboard.mozilla.org/r/56496/#review53116

Not an expensive end-to-end test (Selenium); I don't think we need that here.

> we don't have any functional selenium tests :(

I was thinking of a unit test, actually.  I wouldn't write an expensive end-to-end test for this.

Comment 6

[:glob ✱]

https://reviewboard.mozilla.org/r/56496/#review53116

> I was thinking of a unit test, actually.  I wouldn't write an expensive end-to-end test for this.

i've love to do this, but unfortunately right now our testing framework doesn't touch django.

Comment 7

[:glob ✱]

Comment on attachment 8758130 [details]
MozReview Request: mozreview: don't hide reviewers when viewing drafts (bug 1276847) r?mars

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/56496/diff/1-2/

Comment 8

[Māris Fogels [:mars] (please needinfo)]

Comment on attachment 8758130 [details]
MozReview Request: mozreview: don't hide reviewers when viewing drafts (bug 1276847) r?mars

https://reviewboard.mozilla.org/r/56496/#review53176

::: pylib/mozreview/mozreview/review_helpers.py:97
(Diff revision 2)
>      designated_reviewers = review_request.target_people.all()
>      if not reviewers:
>          reviewers = designated_reviewers
>  
> +    # We need to grab the reviews and statuses off the non-draft.
> +    if type(review_request) == ReviewRequestDraft:

This should use `isinstance()`

Comment 9

[:glob ✱]

Comment on attachment 8758130 [details]
MozReview Request: mozreview: don't hide reviewers when viewing drafts (bug 1276847) r?mars

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/56496/diff/2-3/

Comment 10

[Māris Fogels [:mars] (please needinfo)]

Comment on attachment 8758130 [details]
MozReview Request: mozreview: don't hide reviewers when viewing drafts (bug 1276847) r?mars

https://reviewboard.mozilla.org/r/56496/#review53280

Comment 11

[Mark Côté [:mcote]]

Comment on attachment 8758130 [details]
MozReview Request: mozreview: don't hide reviewers when viewing drafts (bug 1276847) r?mars

https://reviewboard.mozilla.org/r/56496/#review53964

::: pylib/mozreview/mozreview/templates/mozreview/commits.html:64
(Diff revision 3)
>            {% for reviewer, status in child_details|reviewers_status %}
>              {% if not forloop.first %}, {% endif %}
> -            <span class="reviewer-name{% if status.ship_it %} reviewer-ship-it{% endif %} {{status.review_flag|review_flag_class}}">{{ reviewer }}</span>
> +            {% if child_details|isDraft %}
> +              <span class="reviewer-name">{{ reviewer }}</span>
> +            {% else %}
> +              <span class="reviewer-name {% if status.ship_it %}reviewer-ship-it{% endif %} {{status.review_flag|review_flag_class}}">{{ reviewer }}</span>

I see you've changed the spacing here, so that there's a trailing space after the `reviewer-name` class if it's not a ship_it.  Was your change on purpose?

Comment 12

[:glob ✱]

https://reviewboard.mozilla.org/r/56496/#review53964

> I see you've changed the spacing here, so that there's a trailing space after the `reviewer-name` class if it's not a ship_it.  Was your change on purpose?

yes.  i suspect the intent of the original code was to avoid outputting unnecessary whitespace.  i prefer slightly more readable code over saving one byte per reviewer (and with gzip content encoding it's unlikely to make any real impact).

Comment 13

[:glob ✱]

https://hg.mozilla.org/hgcustom/version-control-tools/rev/818df40ca4e1