Closed Bug 1124445 Opened 6 years ago Closed 6 years ago

WebAPI to return basic data on parent + children review requests

Categories

(MozReview Graveyard :: General, defect, P1)

Production
defect

Tracking

(Not tracked)

RESOLVED FIXED

People

(Reporter: mcote, Unassigned)

References

Details

Attachments

(6 files, 1 obsolete file)

Bug 1102428 needs a simple WebAPI to return a few fields from a parent review request and its children.
We should also aim to use this API by the "dashboard" now present on all review request pages. This will drastically lower the number of HTTP requests required to load a page.
Priority: -- → P1
Attached file MozReview Request: bz://1124445/mcote (obsolete) —
/r/2839 - Bug 1124445 - WebAPI to return basic data on parent and child review requests.
/r/2841 - Bug 1124445 - Tests for aggregated review request WebAPI.
/r/2843 - Bug 1124445 - Add test for issue count.
/r/2845 - Bug 1124445 - Include review-request IDs.

Pull down these commits:

hg pull review -r 7f23efcc0b2533c3a0efa71b76c1fe188b90e2a7
Attachment #8553173 - Flags: review?(smacleod)
https://reviewboard.mozilla.org/r/2841/#review2111

::: hgext/reviewboard/tests/test-aggregate-webapi.t
(Diff revision 1)
> +  $ commonenv rb-test-bugzilla

I should use a unique cluster name here.
Comment on attachment 8553173 [details]
MozReview Request: bz://1124445/mcote

/r/2839 - Bug 1124445 - WebAPI to return basic data on parent and child review requests.
/r/2841 - Bug 1124445 - Tests for aggregated review request WebAPI.
/r/2843 - Bug 1124445 - Add test for issue count.
/r/2845 - Bug 1124445 - Include review-request IDs.

Pull down these commits:

hg pull review -r a3176bf0f77999e3edac1b7ac9daa3851eb9a5a7
https://reviewboard.mozilla.org/r/2839/#review2157

Always look on the bright side of life.

I analyzed your Python changes and found 2 errors.

The following files were examined:

  pylib/mozreview/mozreview/extension.py
  pylib/mozreview/mozreview/webapi.py

::: pylib/mozreview/mozreview/webapi.py
(Diff revision 2)
> +def review_request_dict(review_request, commit=None):

E302: expected 2 blank lines, found 1
Separate top-level function and class definitions with two blank lines.

Method definitions inside a class are separated by a single blank line.

Extra blank lines may be used (sparingly) to separate groups of related
functions.  Blank lines may be omitted between a bunch of related
one-liners (e.g. a set of dummy implementations).

Use blank lines in functions, sparingly, to indicate logical sections.

Okay: def a():\n    pass\n\n\ndef b():\n    pass
Okay: def a():\n    pass\n\n\n# Foo\n# Bar\n\ndef b():\n    pass

E301: class Foo:\n    b = 0\n    def bar():\n        pass
E302: def a():\n    pass\n\ndef b(n):\n    pass
E303: def a():\n    pass\n\n\n\ndef b(n):\n    pass
E303: def a():\n\n\n\n    pass
E304: @decorator\n\ndef a():\n    pass


::: pylib/mozreview/mozreview/webapi.py
(Diff revision 2)
> +        if not commits_key in parent_review_request.extra_data:

E713: test for membership should be 'not in'
Negative comparison should be done using "not in" and "is not".

Okay: if x not in y:\n    pass
Okay: assert (X in Y or X is Z)
Okay: if not (X in Y):\n    pass
Okay: zz = x is not y
E713: Z = not X in Y
E713: if not X.B in Y:\n    pass
E714: if not X is Y:\n    pass
E714: Z = not X.B is Y
https://reviewboard.mozilla.org/r/2841/#review2159

Always look on the bright side of life.

I analyzed your Python changes and found 6 errors.

The following files were examined:

  testing/vcttesting/reviewboard/mach_commands.py

::: testing/vcttesting/reviewboard/mach_commands.py
(Diff revision 2)
> +def dump_short_review_request(r):

E302: expected 2 blank lines, found 1
Separate top-level function and class definitions with two blank lines.

Method definitions inside a class are separated by a single blank line.

Extra blank lines may be used (sparingly) to separate groups of related
functions.  Blank lines may be omitted between a bunch of related
one-liners (e.g. a set of dummy implementations).

Use blank lines in functions, sparingly, to indicate logical sections.

Okay: def a():\n    pass\n\n\ndef b():\n    pass
Okay: def a():\n    pass\n\n\n# Foo\n# Bar\n\ndef b():\n    pass

E301: class Foo:\n    b = 0\n    def bar():\n        pass
E302: def a():\n    pass\n\ndef b(n):\n    pass
E303: def a():\n    pass\n\n\n\ndef b(n):\n    pass
E303: def a():\n\n\n\n    pass
E304: @decorator\n\ndef a():\n    pass


::: testing/vcttesting/reviewboard/mach_commands.py
(Diff revision 2)
> -        c = RBClient('http://localhost:%s/' % port, username=username,
> -                password=password)
> +        return RBClient('http://localhost:%s/' % port, username=username,
> +                    password=password)

E128: continuation line under-indented for visual indent
Continuation lines indentation.

Continuation lines should align wrapped elements either vertically
using Python's implicit line joining inside parentheses, brackets
and braces, or using a hanging indent.

When using a hanging indent these considerations should be applied:
- there should be no arguments on the first line, and
- further indentation should be used to clearly distinguish itself as a
continuation line.

Okay: a = (\n)
E123: a = (\n    )

Okay: a = (\n    42)
E121: a = (\n   42)
E122: a = (\n42)
E123: a = (\n    42\n    )
E124: a = (24,\n     42\n)
E125: if (\n    b):\n    pass
E126: a = (\n        42)
E127: a = (24,\n      42)
E128: a = (24,\n    42)
E129: if (a or\n    b):\n    pass
E131: a = (\n    42\n 24)


::: testing/vcttesting/reviewboard/mach_commands.py
(Diff revision 2)
> +    @Command('aggregate-review-requests', category='reviewboard',
> +        description='Return parent and child review-request data.')

E128: continuation line under-indented for visual indent
Continuation lines indentation.

Continuation lines should align wrapped elements either vertically
using Python's implicit line joining inside parentheses, brackets
and braces, or using a hanging indent.

When using a hanging indent these considerations should be applied:
- there should be no arguments on the first line, and
- further indentation should be used to clearly distinguish itself as a
continuation line.

Okay: a = (\n)
E123: a = (\n    )

Okay: a = (\n    42)
E121: a = (\n   42)
E122: a = (\n42)
E123: a = (\n    42\n    )
E124: a = (24,\n     42\n)
E125: if (\n    b):\n    pass
E126: a = (\n        42)
E127: a = (24,\n      42)
E128: a = (24,\n    42)
E129: if (a or\n    b):\n    pass
E131: a = (\n    42\n 24)


::: testing/vcttesting/reviewboard/mach_commands.py
(Diff revision 2)
> +        r = c.get_path('/extensions/mozreview.extension.MozReviewExtension/arr/1/')

E501: line too long (83 > 79 characters)
Limit all lines to a maximum of 79 characters.

There are still many devices around that are limited to 80 character
lines; plus, limiting windows to 80 characters makes it possible to have
several windows side-by-side.  The default wrapping on such devices looks
ugly.  Therefore, please limit all lines to a maximum of 79 characters.
For flowing long blocks of text (docstrings or comments), limiting the
length to 72 characters is recommended.

Reports error E501.
https://reviewboard.mozilla.org/r/2843/#review2161

Always look on the bright side of life.

I analyzed your Python changes and found 6 errors.

The following files were examined:

  testing/vcttesting/reviewboard/mach_commands.py

::: testing/vcttesting/reviewboard/mach_commands.py
(Diff revision 2)
> -    def create_diff_comment(self, port, rrid, rid, filename, first_line, text):
> +    @CommandArgument('--open-issue', action='store_true',
> +        help='Whether to open an issue in this review')

E128: continuation line under-indented for visual indent
Continuation lines indentation.

Continuation lines should align wrapped elements either vertically
using Python's implicit line joining inside parentheses, brackets
and braces, or using a hanging indent.

When using a hanging indent these considerations should be applied:
- there should be no arguments on the first line, and
- further indentation should be used to clearly distinguish itself as a
continuation line.

Okay: a = (\n)
E123: a = (\n    )

Okay: a = (\n    42)
E121: a = (\n   42)
E122: a = (\n42)
E123: a = (\n    42\n    )
E124: a = (24,\n     42\n)
E125: if (\n    b):\n    pass
E126: a = (\n        42)
E127: a = (24,\n      42)
E128: a = (24,\n    42)
E129: if (a or\n    b):\n    pass
E131: a = (\n    42\n 24)


::: testing/vcttesting/reviewboard/mach_commands.py
(Diff revision 2)
> +    def create_diff_comment(self, port, rrid, rid, filename, first_line, text,
> +            open_issue):

E128: continuation line under-indented for visual indent
Continuation lines indentation.

Continuation lines should align wrapped elements either vertically
using Python's implicit line joining inside parentheses, brackets
and braces, or using a hanging indent.

When using a hanging indent these considerations should be applied:
- there should be no arguments on the first line, and
- further indentation should be used to clearly distinguish itself as a
continuation line.

Okay: a = (\n)
E123: a = (\n    )

Okay: a = (\n    42)
E121: a = (\n   42)
E122: a = (\n42)
E123: a = (\n    42\n    )
E124: a = (24,\n     42\n)
E125: if (\n    b):\n    pass
E126: a = (\n        42)
E127: a = (24,\n      42)
E128: a = (24,\n    42)
E129: if (a or\n    b):\n    pass
E131: a = (\n    42\n 24)


::: testing/vcttesting/reviewboard/mach_commands.py
(Diff revision 2)
> +    @Command('update-issue-status', category='reviewboard',
> +        description='Update issue status on a diff comment.')

E128: continuation line under-indented for visual indent
Continuation lines indentation.

Continuation lines should align wrapped elements either vertically
using Python's implicit line joining inside parentheses, brackets
and braces, or using a hanging indent.

When using a hanging indent these considerations should be applied:
- there should be no arguments on the first line, and
- further indentation should be used to clearly distinguish itself as a
continuation line.

Okay: a = (\n)
E123: a = (\n    )

Okay: a = (\n    42)
E121: a = (\n   42)
E122: a = (\n42)
E123: a = (\n    42\n    )
E124: a = (24,\n     42\n)
E125: if (\n    b):\n    pass
E126: a = (\n        42)
E127: a = (24,\n      42)
E128: a = (24,\n    42)
E129: if (a or\n    b):\n    pass
E131: a = (\n    42\n 24)


::: testing/vcttesting/reviewboard/mach_commands.py
(Diff revision 2)
> +    @CommandArgument('status', help='Desired issue status ("open", "dropped", '
> +        'or "resolved")')

E128: continuation line under-indented for visual indent
Continuation lines indentation.

Continuation lines should align wrapped elements either vertically
using Python's implicit line joining inside parentheses, brackets
and braces, or using a hanging indent.

When using a hanging indent these considerations should be applied:
- there should be no arguments on the first line, and
- further indentation should be used to clearly distinguish itself as a
continuation line.

Okay: a = (\n)
E123: a = (\n    )

Okay: a = (\n    42)
E121: a = (\n   42)
E122: a = (\n42)
E123: a = (\n    42\n    )
E124: a = (24,\n     42\n)
E125: if (\n    b):\n    pass
E126: a = (\n        42)
E127: a = (24,\n      42)
E128: a = (24,\n    42)
E129: if (a or\n    b):\n    pass
E131: a = (\n    42\n 24)
https://reviewboard.mozilla.org/r/2845/#review2163

Always look on the bright side of life.

I analyzed your Python changes and found 2 errors.

The following files were examined:

  pylib/mozreview/mozreview/webapi.py
  testing/vcttesting/reviewboard/mach_commands.py
Comment on attachment 8553173 [details]
MozReview Request: bz://1124445/mcote

/r/2839 - Bug 1124445 - WebAPI to return basic data on parent and child review requests.
/r/2841 - Bug 1124445 - Tests for aggregated review request WebAPI.
/r/2843 - Bug 1124445 - Add test for issue count.
/r/2845 - Bug 1124445 - Include review-request IDs.
/r/2881 - Bug 1124445 - Change webapi resource name from "aggregate" to "summary".
/r/2883 - Bug 1124445 - Rename summary test.

Pull down these commits:

hg pull review -r 324a6ed084e49fb5924dbac1f2404d086372fb7b
https://reviewboard.mozilla.org/r/2839/#review2173

And now for something completely different.

Congratulations, there we no Python static analysis issues with this patch!

The following files were examined:

  pylib/mozreview/mozreview/extension.py
  pylib/mozreview/mozreview/webapi.py
https://reviewboard.mozilla.org/r/2841/#review2175

Always look on the bright side of life.

I analyzed your Python changes and found 2 errors.

The following files were examined:

  testing/vcttesting/reviewboard/mach_commands.py

::: testing/vcttesting/reviewboard/mach_commands.py
(Diff revision 3)
> +        print '    Commit: %s' % r['commit']
> +
>  @CommandProvider

E302: expected 2 blank lines, found 1
Separate top-level function and class definitions with two blank lines.

Method definitions inside a class are separated by a single blank line.

Extra blank lines may be used (sparingly) to separate groups of related
functions.  Blank lines may be omitted between a bunch of related
one-liners (e.g. a set of dummy implementations).

Use blank lines in functions, sparingly, to indicate logical sections.

Okay: def a():\n    pass\n\n\ndef b():\n    pass
Okay: def a():\n    pass\n\n\n# Foo\n# Bar\n\ndef b():\n    pass

E301: class Foo:\n    b = 0\n    def bar():\n        pass
E302: def a():\n    pass\n\ndef b(n):\n    pass
E303: def a():\n    pass\n\n\n\ndef b(n):\n    pass
E303: def a():\n\n\n\n    pass
E304: @decorator\n\ndef a():\n    pass
https://reviewboard.mozilla.org/r/2843/#review2177

Always look on the bright side of life.

I analyzed your Python changes and found 2 errors.

The following files were examined:

  testing/vcttesting/reviewboard/mach_commands.py

::: testing/vcttesting/reviewboard/mach_commands.py
(Diff revision 3)
> -    def create_diff_comment(self, port, rrid, rid, filename, first_line, text):
> +    @CommandArgument('--open-issue', action='store_true',
> +                    help='Whether to open an issue in this review')

E128: continuation line under-indented for visual indent
Continuation lines indentation.

Continuation lines should align wrapped elements either vertically
using Python's implicit line joining inside parentheses, brackets
and braces, or using a hanging indent.

When using a hanging indent these considerations should be applied:
- there should be no arguments on the first line, and
- further indentation should be used to clearly distinguish itself as a
continuation line.

Okay: a = (\n)
E123: a = (\n    )

Okay: a = (\n    42)
E121: a = (\n   42)
E122: a = (\n42)
E123: a = (\n    42\n    )
E124: a = (24,\n     42\n)
E125: if (\n    b):\n    pass
E126: a = (\n        42)
E127: a = (24,\n      42)
E128: a = (24,\n    42)
E129: if (a or\n    b):\n    pass
E131: a = (\n    42\n 24)
https://reviewboard.mozilla.org/r/2845/#review2179

And now for something completely different.

Congratulations, there we no Python static analysis issues with this patch!

The following files were examined:

  pylib/mozreview/mozreview/webapi.py
  testing/vcttesting/reviewboard/mach_commands.py
https://reviewboard.mozilla.org/r/2881/#review2181

Always look on the bright side of life.

I analyzed your Python changes and found 1 errors.

The following files were examined:

  pylib/mozreview/mozreview/extension.py
  pylib/mozreview/mozreview/webapi.py
  testing/vcttesting/reviewboard/mach_commands.py
Comment on attachment 8553173 [details]
MozReview Request: bz://1124445/mcote

/r/2839 - Bug 1124445 - WebAPI to return basic data on parent and child review requests.
/r/2841 - Bug 1124445 - Tests for aggregated review request WebAPI.
/r/2843 - Bug 1124445 - Add test for issue count.
/r/2845 - Bug 1124445 - Include review-request IDs.
/r/2881 - Bug 1124445 - Change webapi resource name from "aggregate" to "summary".
/r/2883 - Bug 1124445 - Rename summary test.

Pull down these commits:

hg pull review -r 8399e38fd1e927a2fe22b474e5695821f93887ea
https://reviewboard.mozilla.org/r/2839/#review2211

And now for something completely different.

Congratulations, there we no Python static analysis issues with this patch!

The following files were examined:

  pylib/mozreview/mozreview/extension.py
  pylib/mozreview/mozreview/webapi.py
https://reviewboard.mozilla.org/r/2841/#review2213

And now for something completely different.

Congratulations, there we no Python static analysis issues with this patch!

The following files were examined:

  testing/vcttesting/reviewboard/mach_commands.py
https://reviewboard.mozilla.org/r/2843/#review2215

And now for something completely different.

Congratulations, there we no Python static analysis issues with this patch!

The following files were examined:

  testing/vcttesting/reviewboard/mach_commands.py
https://reviewboard.mozilla.org/r/2845/#review2217

And now for something completely different.

Congratulations, there we no Python static analysis issues with this patch!

The following files were examined:

  pylib/mozreview/mozreview/webapi.py
  testing/vcttesting/reviewboard/mach_commands.py
https://reviewboard.mozilla.org/r/2881/#review2219

And now for something completely different.

Congratulations, there we no Python static analysis issues with this patch!

The following files were examined:

  pylib/mozreview/mozreview/extension.py
  pylib/mozreview/mozreview/webapi.py
  testing/vcttesting/reviewboard/mach_commands.py
https://reviewboard.mozilla.org/r/2841/#review2303

::: hgext/reviewboard/tests/test-aggregate-webapi.t
(Diff revision 4)
> +  $ commonenv rb-test-aggregate-webapi

This should now be simply `commonenv`

::: hgext/reviewboard/tests/test-aggregate-webapi.t
(Diff revision 4)
> +  $ rbmanage stop ../rbserver
> +
> +  $ dockercontrol stop-bmo rb-test-aggregate-webapi

This should now be `mozreview stop`

::: testing/vcttesting/reviewboard/mach_commands.py
(Diff revision 4)
> +def dump_short_review_request(r):
> +    # Don't dump last_updated since it's hard to test.
> +    print '  Summary: %s' % r['summary']
> +    print '    Open issues: %d' % r['issue_open_count']
> +    print '    Submitter: %s' % r['submitter']
> +    print '    Status: %s' % r['status']
> +    print '    Reviewers:'
> +    for reviewer in r['reviewers']:
> +        print '      %s' % reviewer
> +    if 'commit' in r:
> +        print '    Commit: %s' % r['commit']

I'd feel better if this were using YAML.

I'd feel even better if `short` were an option on `serialize_review_requests`. But that may not be worth it.
Blocks: 1102428
https://reviewboard.mozilla.org/r/2841/#review2307

> I'd feel better if this were using YAML.
> 
> I'd feel even better if `short` were an option on `serialize_review_requests`. But that may not be worth it.

The review-request dicts returned from the summary call are slightly different from the standard one returned by Review Board, so I'd prefer a separate call, but I agree about the YAML.
Comment on attachment 8553173 [details]
MozReview Request: bz://1124445/mcote

/r/2839 - Bug 1124445 - WebAPI to return basic data on parent and child review requests.
/r/2841 - Bug 1124445 - Tests for aggregated review request WebAPI.
/r/2843 - Bug 1124445 - Add test for issue count.
/r/2845 - Bug 1124445 - Include review-request IDs.
/r/2881 - Bug 1124445 - Change webapi resource name from "aggregate" to "summary".
/r/2883 - Bug 1124445 - Rename summary test.

Pull down these commits:

hg pull review -r 5fc0be22d543cedfab09652c14ddcc25d2d4d7e3
https://reviewboard.mozilla.org/r/2839/#review2341

And now for something completely different.

Congratulations, there we no Python static analysis issues with this patch!

The following files were examined:

  pylib/mozreview/mozreview/extension.py
  pylib/mozreview/mozreview/webapi.py
https://reviewboard.mozilla.org/r/2841/#review2343

And now for something completely different.

Congratulations, there we no Python static analysis issues with this patch!

The following files were examined:

  testing/vcttesting/reviewboard/mach_commands.py
https://reviewboard.mozilla.org/r/2843/#review2345

And now for something completely different.

Congratulations, there we no Python static analysis issues with this patch!

The following files were examined:

  testing/vcttesting/reviewboard/mach_commands.py
https://reviewboard.mozilla.org/r/2845/#review2347

And now for something completely different.

Congratulations, there we no Python static analysis issues with this patch!

The following files were examined:

  pylib/mozreview/mozreview/webapi.py
  testing/vcttesting/reviewboard/mach_commands.py
https://reviewboard.mozilla.org/r/2881/#review2349

And now for something completely different.

Congratulations, there we no Python static analysis issues with this patch!

The following files were examined:

  pylib/mozreview/mozreview/extension.py
  pylib/mozreview/mozreview/webapi.py
  testing/vcttesting/reviewboard/mach_commands.py
https://reviewboard.mozilla.org/r/2839/#review3065

::: pylib/mozreview/mozreview/extension.py
(Diff revision 4)
> +from webapi import aggregate_review_request_resource

This should be moved down with the other mozreview imports, also please `from mozreview...`

::: pylib/mozreview/mozreview/webapi.py
(Diff revision 4)
> +import json

The convention is to put any Web API resources in a `resources.py` or `resources/<resource-name>.py` file. Please rename this.

::: pylib/mozreview/mozreview/webapi.py
(Diff revision 4)
> +    uri_name = 'arr'

I'd prefer something less cryptic.

::: pylib/mozreview/mozreview/webapi.py
(Diff revision 4)
> +        commits_key = 'p2rb.commits'

We should probably have these keys defined in a central location somewhere, eventually.

::: pylib/mozreview/mozreview/webapi.py
(Diff revision 4)
> +        if commits_key not in parent_review_request.extra_data:
> +            data = {}

I'm wondering if we should be returning an error, rather than empty data, when the review request isn't a pushed request?

Aside from that though you'll want to be using the convention of checking `p2rb` and `p2rb.is_squashed`

https://hg.mozilla.org/hgcustom/version-control-tools/file/8e18efe92b20/pylib/mozreview/mozreview/utils.py#l1

and

https://hg.mozilla.org/hgcustom/version-control-tools/file/8e18efe92b20/pylib/mozreview/mozreview/utils.py#l5

::: pylib/mozreview/mozreview/webapi.py
(Diff revision 4)
> +            commit_tuples = json.loads(

I wonder if we should stick retrieving the commits / setting them in a helper in `mozreview.utils`?

I'm on the fence, since it's going to become a proper field soon which means a lot of stuff will have to change. *shrug*

::: pylib/mozreview/mozreview/webapi.py
(Diff revision 4)
> +                rid = commit_tuple[1]

Please call this "rrid" since it's a "Review Request ID" or something more verbose. I really want us to stop with referring to review requests as "reviews" anywhere in the code.

::: pylib/mozreview/mozreview/webapi.py
(Diff revision 4)
> +                    continue

Something is seriously wrong if the child cannot be found... I'm not sure we should just continue on without its data.

Maybe this should cause a complete failure to retrieve the information? We really want to lock down access to this review request in general if we ever detect something like this, it's an indicator we have a serious bug.

::: pylib/mozreview/mozreview/webapi.py
(Diff revision 4)
> +def review_request_dict(review_request, commit=None):

Maybe call this `summarize_review_request`?

::: pylib/mozreview/mozreview/webapi.py
(Diff revision 4)
> +        response = WebAPIResponse(request,
> +                                  status=200,
> +                                  obj=data,
> +                                  **self.build_response_args(request))
> +
> +        return response

you should be able to do something like (and it's more in line with how RB does things):
```
return 200, data
```
https://reviewboard.mozilla.org/r/2839/#review3097

> I'm wondering if we should be returning an error, rather than empty data, when the review request isn't a pushed request?
> 
> Aside from that though you'll want to be using the convention of checking `p2rb` and `p2rb.is_squashed`
> 
> https://hg.mozilla.org/hgcustom/version-control-tools/file/8e18efe92b20/pylib/mozreview/mozreview/utils.py#l1
> 
> and
> 
> https://hg.mozilla.org/hgcustom/version-control-tools/file/8e18efe92b20/pylib/mozreview/mozreview/utils.py#l5

Do you think a 404 here, or something else, like a 500, with a specific error message?
https://reviewboard.mozilla.org/r/2839/#review3099

> Do you think a 404 here, or something else, like a 500, with a specific error message?

Actually decided to use a 400 Bad Request.

> Something is seriously wrong if the child cannot be found... I'm not sure we should just continue on without its data.
> 
> Maybe this should cause a complete failure to retrieve the information? We really want to lock down access to this review request in general if we ever detect something like this, it's an indicator we have a serious bug.

Good point.
https://reviewboard.mozilla.org/r/2839/#review3101

Squashed all the commits into one.  Also, I was thinking I should possibly define our own specific mozreview WebAPI errors, but I wasn't sure how that worked, since there are global IDs, it seems...
https://reviewboard.mozilla.org/r/2839/#review3191

::: pylib/mozreview/mozreview/extension.py
(Diff revision 5)
> +        summary_resource,
>          batch_review_resource,

alphabetical please

::: pylib/mozreview/mozreview/resources/summary_resource.py
(Diff revision 5)
> +    name = 'summary_review_request'

"review_request_summary"

::: pylib/mozreview/mozreview/resources/summary_resource.py
(Diff revision 5)
> +class SummaryResource(WebAPIResource):

We should have the class name match the name. Let's go with `ReviewRequestSummaryResource`

::: pylib/mozreview/mozreview/resources/summary_resource.py
(Diff revision 5)
> +    @webapi_response_errors(DOES_NOT_EXIST, INVALID_ATTRIBUTE)
> +    def get(self, request, *args, **kwargs):

We aren't checking for access permission here. Generally our review requests will be open to all, but not in the case where a bug goes confidential, or future cases.

Here is an example of how to do this: https://github.com/reviewboard/reviewboard/blob/77cbe6400ecc3b99d903cb503b55b11dbf3d7990/reviewboard/webapi/resources/review_request.py#L422-L423

::: pylib/mozreview/mozreview/resources/summary_resource.py
(Diff revision 5)
> +        data = {'parent': summarize_review_request(parent_review_request),
> +                'children': []}

style nit, I'd much prefer (including the trailing comma):
```
data = {
    'parent': summarize_review_request(parent_review_request),
    'children': [],
}
```

::: pylib/mozreview/mozreview/resources/summary_resource.py
(Diff revision 5)
> +                return DOES_NOT_EXIST, {
> +                    'fields': {
> +                        'review_requests': err
> +                    }
> +                }

I wonder if this would be worth having as it's own custom MozReview error? `CHILD_DOEST_NOT_EXIST`?

In either case I'm not a huge fan of using `'review_requests'` as the field key - also, it's not really a 'field' either. How about in our error cases we use a custom key, and return something similar to:
```
'mozreview': {
    'children': 'data for child review request %s not found',
}
```

::: pylib/mozreview/mozreview/resources/summary_resource.py
(Diff revision 5)
> +            return INVALID_ATTRIBUTE, {
> +                'fields': {
> +                    'review_requests': 'not a parent'
> +                }
> +            }

This style of error is generally used for fields provided in the body of the request, rather than a problem with the ids in the url. I think we should define our own error here instead specific to the case. Lets call it `NOT_PARENT` or something similar (we could also have `NOT_PUSHED` or `NOT_MOZREVIEW` for other cases etc.

> I should possibly define our own specific mozreview WebAPI errors, but I wasn't sure how that worked, since there are global IDs, it seems...

Yes these are global. Review Board core will never use an ID number for the error greater than `1000`. So anything above that is fine for us to use (there could possibly be collisions with other extensions, but I don't see that happening). This will probably be made more formal, and extension collisions will be fixed, but for now I'd say we're fine.

::: pylib/mozreview/mozreview/resources/summary_resource.py
(Diff revision 5)
> +summary_resource = SummaryResource()

review_request_summary_resource

::: testing/vcttesting/reviewboard/mach_commands.py
(Diff revision 5)
> -    def create_diff_comment(self, port, rrid, rid, filename, first_line, text):
> +    @CommandArgument('--open-issue', action='store_true',
> +                     help='Whether to open an issue in this review')
> +    def create_diff_comment(self, port, rrid, rid, filename, first_line, text,
> +                            open_issue):

from Mach docs
> The arguments registered with @CommandArgument are passed to your method as keyword arguments using the \*\*kwargs calling convention. So, you should define default values for all of your method's arguments.

Since open_issue is optional, it should probably be specified in the function arguments as `open_issue=False`? That might not be required, so if you've tested this already and it works I'm not too picky. gps would be able to clear up what the convention should be here.
https://reviewboard.mozilla.org/r/2839/#review3307

> from Mach docs
> > The arguments registered with @CommandArgument are passed to your method as keyword arguments using the \*\*kwargs calling convention. So, you should define default values for all of your method's arguments.
> 
> Since open_issue is optional, it should probably be specified in the function arguments as `open_issue=False`? That might not be required, so if you've tested this already and it works I'm not too picky. gps would be able to clear up what the convention should be here.

There is a mix of usage in mach_commands, but I went with your suggestion.

> This style of error is generally used for fields provided in the body of the request, rather than a problem with the ids in the url. I think we should define our own error here instead specific to the case. Lets call it `NOT_PARENT` or something similar (we could also have `NOT_PUSHED` or `NOT_MOZREVIEW` for other cases etc.
> 
> > I should possibly define our own specific mozreview WebAPI errors, but I wasn't sure how that worked, since there are global IDs, it seems...
> 
> Yes these are global. Review Board core will never use an ID number for the error greater than `1000`. So anything above that is fine for us to use (there could possibly be collisions with other extensions, but I don't see that happening). This will probably be made more formal, and extension collisions will be fixed, but for now I'd say we're fine.

I added mozreview/resources/errors.py.  I realize errors.py isn't a resource, but it didn't seem worth it to create a webapi directory that contains both errors.py and a resources subdir.

> We aren't checking for access permission here. Generally our review requests will be open to all, but not in the case where a bug goes confidential, or future cases.
> 
> Here is an example of how to do this: https://github.com/reviewboard/reviewboard/blob/77cbe6400ecc3b99d903cb503b55b11dbf3d7990/reviewboard/webapi/resources/review_request.py#L422-L423

Fixed, although I couldn't come up with a good test for it.  I did, however, notice that I wasn't handling never-published drafts... I just return DOES_NOT_EXIST on those now, since they aren't publicly visible anyway.

I will add a proper test when my restrict-review-request patch lands (or will add it to that patch if this lands first).
https://reviewboard.mozilla.org/r/2839/#review3309

Bah hold off reviewing; tests broke after I rebased.
Comment on attachment 8553173 [details]
MozReview Request: bz://1124445/mcote

/r/2839 - Bug 1124445 - WebAPI to return basic data on parent and child review requests.

Pull down this commit:

hg pull review -r 9292aba4a51f363072f573acca1839fd9e3a89e3
Comment on attachment 8553173 [details]
MozReview Request: bz://1124445/mcote

/r/2839 - Bug 1124445 - WebAPI to return basic data on parent and child review requests.

Pull down this commit:

hg pull review -r a61b300caa70364677f4a7bd09c582d3164b4513
https://reviewboard.mozilla.org/r/2839/#review3449

LGTM. But smacleod should also look at this.
Comment on attachment 8553173 [details]
MozReview Request: bz://1124445/mcote

https://reviewboard.mozilla.org/r/2837/#review3451

Ship It!
Attachment #8553173 - Flags: review+
https://reviewboard.mozilla.org/r/2839/#review3503

::: pylib/mozreview/mozreview/resources/review_request_summary.py
(Diff revision 9)
> +from djblets.webapi.responses import WebAPIResponse

Unused import. (Monty I miss you)

::: pylib/mozreview/mozreview/resources/review_request_summary.py
(Diff revision 9)
> +        if not commits_key in parent_review_request.extra_data:

`if not parent_review_request.public:`
Attachment #8553173 - Flags: review?(smacleod) → review+
Comment on attachment 8553173 [details]
MozReview Request: bz://1124445/mcote

https://reviewboard.mozilla.org/r/2837/#review3505

Fix commit review issues, then Ship It!
http://hg.mozilla.org/hgcustom/version-control-tools/rev/e073c396c6fd
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Depends on: 1141871
Attachment #8553173 - Attachment is obsolete: true
Attachment #8619195 - Flags: review+
Attachment #8619196 - Flags: review+
Attachment #8619197 - Flags: review+
Attachment #8619198 - Flags: review+
Attachment #8619199 - Flags: review+
Attachment #8619200 - Flags: review+
Product: Developer Services → MozReview
You need to log in before you can comment on or make changes to this bug.