1191817 - Changes leftover from bug 1109218

Status: RESOLVED FIXED

(MozReview Graveyard :: General, defect, P2)

Description

[Mauro Doglio [:mdoglio]]

In bug 1109218 there are some comments :smacleod put as a post-merge review. Let's fix them in this bug:

+++ b/pylib/mozreview/mozreview/autoland/resources.py
> +import json
> +import logging
Alphebetical order.

+++ b/pylib/mozreview/mozreview/autoland/resources.py
> +from mozreview.autoland.models import (AutolandEventLogEntry,
> +                                       AutolandRequest)
> +from mozreview.autoland.errors import (AUTOLAND_ERROR,
> +                                       AUTOLAND_TIMEOUT,
> +                                       BAD_AUTOLAND_CREDENTIALS,
> +                                       BAD_AUTOLAND_URL,
> +                                       BAD_UPDATE_CREDENTIALS)
Alphabetical order.

+++ b/pylib/mozreview/mozreview/autoland/resources.py
> +    """Resource for reviewers or reviewees to kick off Try Builds
> +    for a particular review request."""
"The summary line may be used by automatic indexing tools; it is important that
it fits on one line and is separated from the rest of the docstring by a blank line"
https://www.python.org/dev/peps/pep-0257/#multi-line-docstrings

+++ b/pylib/mozreview/mozreview/autoland/resources.py
> +    def has_access_permissions(self, request, *args, **kwargs):
> +        return False
I'm not sure why we are allowing 'GET' when we never give anyone permission? If
we really don't want to define a get, or allow anyone to use it we should
probably remove it from the `allowed_methods` above.

+++ b/pylib/mozreview/mozreview/autoland/resources.py
> +        optional={
> +            'autoland_request_id_for_testing': {
> +                'type': int,
> +                'description': 'For testing only. If the MozReview extension '
> +                               'is in testing mode, this skips the request to '
> +                               'Try Autoland and just uses this request id.'
> +            }
> +        }
Now that we have proper support for testing autoland as part of our
infrastructure I wonder if it's worth tearing this out (along with all the
other testing flags)?

+++ b/pylib/mozreview/mozreview/autoland/resources.py
> +            try:
> +                autoland_request_id = int(response.json().get('request_id', 0))
> +            finally:
> +                if autoland_request_id is None:
> +                    return AUTOLAND_ERROR, {
> +                        'status_code': response.status_code,
> +                        'request_id': autoland_request_id,
> +                    }
Should we even bother including request_id in the returned error, it will always
be None? Or should we setting a literal None to that key instead of the var?

+++ b/pylib/mozreview/mozreview/autoland/resources.py
> +    """Resource for notifications of Autoland requests status update"""
Missing period.

+++ b/pylib/mozreview/mozreview/autoland/resources.py
> +    def has_access_permissions(self, request, *args, **kwargs):
> +        return False
I don't think this is needed, 'GET' isn't in allowed_methods.

+++ b/pylib/mozreview/mozreview/autoland/resources.py
> +        # TODO - remove this before landing. Just for local testing on mconley's machine
> +        #return "http://192.168.59.3:60353/api/extensions/mozreview.extension.MozReviewExtension/autoland-request-updates/"
Self explanatory.

--- a/pylib/rbmozui/rbmozui/fields.py
+++ b/pylib/rbmozui/rbmozui/fields.py
> +from mozreview.utils import is_parent, is_pushed
> +
> +from mozreview.autoland.models import AutolandRequest, AutolandEventLogEntry
No blank space, alphabetical.

+++ b/pylib/rbmozui/rbmozui/static/js/try.js
> +              .done(function(){
> +                // TODO
> +              })
What do we need to do here?

Comment 1

[Mauro Doglio [:mdoglio]]

Attachment: MozReview Request: Mozreview: fix import order (bug 1191817); r?smacleod

Mozreview: fix import order (bug 1191817); r?smacleod

Comment 2

[Mauro Doglio [:mdoglio]]

Attachment: MozReview Request: Mozreview: fix autoland resources docstrings (bug 1191817); r?smacleod

Mozreview: fix autoland resources docstrings (bug 1191817); r?smacleod

Comment 3

[Mauro Doglio [:mdoglio]]

Attachment: MozReview Request: Mozreview: remove GET access to TryAutolandTriggerResource (bug 1191817); r?smacleod

Mozreview: remove GET access to TryAutolandTriggerResource (bug 1191817); r?smacleod

Comment 4

[Mauro Doglio [:mdoglio]]

Attachment: MozReview Request: Mozreview: cleanup of the autoland resource module (bug 1191817); r?smacleod

Mozreview: cleanup of the autoland resource module (bug 1191817); r?smacleod

Comment 5

[Steven MacLeod [:smacleod]]

Comment on attachment 8651783 [details]
MozReview Request: Mozreview: fix import order (bug 1191817); r?smacleod

https://reviewboard.mozilla.org/r/16971/#review15219

Ship It!

Comment 6

[Steven MacLeod [:smacleod]]

Comment on attachment 8651784 [details]
MozReview Request: Mozreview: fix autoland resources docstrings (bug 1191817); r?smacleod

https://reviewboard.mozilla.org/r/16973/#review15221

::: pylib/mozreview/mozreview/autoland/resources.py:42
(Diff revision 1)
> -    """Resource for reviewers or reviewees to kick off Try Builds
> -    for a particular review request."""
> +    """Resource to kick off Try Builds for a particular review request."""
> +
> +    """Only reviewers or reviewees can start a Try Build."""

In a multiline docstring you'll want the whole thing contained between one set of `""" ... """`. The first line contains `"""` as well as text, but the last line should be the `"""` alone. So something like:
```
def function():
    """I'm the summary line
    
    I'm another paragraph. Lots of text here
    and here and here.
    
    Ooo, maybe another paragraph.
    """
```

::: pylib/mozreview/mozreview/autoland/resources.py:44
(Diff revision 1)
> +    """Only reviewers or reviewees can start a Try Build."""

Is this true, I thought it was authors only atm?

Comment 7

[Steven MacLeod [:smacleod]]

Comment on attachment 8651785 [details]
MozReview Request: Mozreview: remove GET access to TryAutolandTriggerResource (bug 1191817); r?smacleod

https://reviewboard.mozilla.org/r/16975/#review15225

::: pylib/mozreview/mozreview/autoland/resources.py:47
(Diff revision 1)
> -    allowed_methods = ('GET', 'POST',)
> +    allowed_methods = ('POST',)

I just want to make sure you confirmed our front-end still works with this - if so please drop this issue :)

Comment 8

[Steven MacLeod [:smacleod]]

Comment on attachment 8651786 [details]
MozReview Request: Mozreview: cleanup of the autoland resource module (bug 1191817); r?smacleod

https://reviewboard.mozilla.org/r/16977/#review15227

Ship It!

Comment 9

[Mauro Doglio [:mdoglio]]

Comment on attachment 8651786 [details]
MozReview Request: Mozreview: cleanup of the autoland resource module (bug 1191817); r?smacleod

Mozreview: cleanup of the autoland resource module (bug 1191817); r?smacleod

Comment 10

[Mauro Doglio [:mdoglio]]

https://reviewboard.mozilla.org/r/16975/#review15225

> I just want to make sure you confirmed our front-end still works with this - if so please drop this issue :)

Verified with a manual test