1337583 - Move fixture data to api, update ui to use api instead of fixtures

Status: RESOLVED FIXED

(Conduit :: General, defect)

Comment 1

[Israel Madueme [:imadueme]]

Attachment: autoland-web-api: move stubbed data to api, update ui to use api instead of fixtures (bug 1337583).

This changeset allows us to more realistically simulate the
process of querying the api and displaying the correct data
based on the response. This will ensure that our routing and
network handling work well and will allow us to more rapidly
iterate on the api response format.

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

Comment 2

[Steven MacLeod [:smacleod]]

Comment on attachment 8834688 [details]
autoland-web-api: move stubbed data to api, update ui to use api instead of fixtures (bug 1337583).

https://reviewboard.mozilla.org/r/110532/#review112146

::: autoland/public-web-api/autolandweb/series.py
(Diff revision 1)
>      # it in any queries. The current regex is completely permissive.
> -    return {
> -        'id': series,
> +
> +    return stubbed_response(repo, series)
> -        'bug': 123456,
> -        'repository': 'https://hg.mozilla.org/mozilla-central/',
> -        'pushes': [  # List where each entry is a push of commits.

I'd like these comments kept around.

I'd prefer if you leave this single stub here as the default so that we can build it into a real response slowly over time. Then instead of only returning stubbed responses, we can check for a stub matching the provided id and if no stubs are found fall into the default case.

::: autoland/public-web-api/autolandweb/stubs.py:612
(Diff revision 1)
> +            }
> +        }
> +    ]
> +}
> +
> +stub_list = [stub1, stub2, stub3, stub4, stub5]

We should stick the canned responses in a dictionary keyed on the identifier and just do a `dict.get(<key>)` which will return the response or `None`. Also, rather than naming them "stub1", "stub2" etc, you could make what they actually are part of the identifier (e.g. "bz://123456/canland")

Comment 3

[David Walsh :davidwalsh]

Comment on attachment 8834688 [details]
autoland-web-api: move stubbed data to api, update ui to use api instead of fixtures (bug 1337583).

https://reviewboard.mozilla.org/r/110532/#review112636

R+'ing front-end stuff.

Comment 4

[Israel Madueme [:imadueme]]

Comment on attachment 8834688 [details]
autoland-web-api: move stubbed data to api, update ui to use api instead of fixtures (bug 1337583).

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

Comment 5

[David Walsh :davidwalsh]

Comment on attachment 8834688 [details]
autoland-web-api: move stubbed data to api, update ui to use api instead of fixtures (bug 1337583).

https://reviewboard.mozilla.org/r/110532/#review112882

::: autoland/ui/src/routes.jsx:13
(Diff revision 2)
>  import InvalidUrl from './components/InvalidUrl';
>  
>  const getRoutes = history => (
>    <Router history={history || browserHistory}>
>      <Route path="/" component={App}>
> -      <Route path="repos/:repo_id/series/:series_id" component={AutolandController} />
> +      <Route path="repos/:repo_id/series/*" component={AutolandController} />

Wow, `*`!  Nice!  I was worried about this.

Comment 6

[Steven MacLeod [:smacleod]]

Comment on attachment 8834688 [details]
autoland-web-api: move stubbed data to api, update ui to use api instead of fixtures (bug 1337583).

https://reviewboard.mozilla.org/r/110532/#review113002

::: autoland/public-web-api/autolandweb/routes.py:31
(Diff revision 2)
>          if series is None:
>              self.write({})
>              return
>  
>          status = await get_series_status(repo, series)
> +        if status != None:

checks against None should be done with `is` or `is not`. So `status is not None`. I'd actually prefer to flip the negation though `if status is None:` and then 404 and return, otherwise continue outside of an if block.

::: autoland/public-web-api/autolandweb/series.py:5
(Diff revision 2)
> +from autolandweb.stubs import stubs
>  
>  async def get_series_status(repo, series):

You're not running flake8, or the checkstyle tests - it should complain about only having 1 blank line here. Please make sure the tests and checkstyle pass.

::: autoland/public-web-api/autolandweb/series.py:11
(Diff revision 2)
> +    if repo == 'test' and series == 'bz://123456/stub1':
> -    return {
> +        return {
> -        'id': series,
> +            'id': 'bz://123456/stub1',

This default case shouldn't be considered a stub - you've actually removed the series id echo here which was part of our previous story, heh.

Please put the check for the static reponses *first*, and if the dictionary doesn't contain one, fallthrough to this default.

In order to preserve the ability to 404, we could standardize on a repo name, and consider any `repo !=  <name>` a 404?

::: autoland/public-web-api/autolandweb/stubs.py:5
(Diff revision 2)
> +# This Source Code Form is subject to the terms of the Mozilla Public
> +# License, v. 2.0. If a copy of the MPL was not distributed with this
> +# file, You can obtain one at http://mozilla.org/MPL/2.0/.
> +
> +stubs = {

Alright, this is going to be super pedantic, sorry! Technically these aren't stubs, they're canned reponses, which are what a stub returns to you.

Global constants like this should also be capitalized - `CANNED_RESPONSES` would be a more appropriate name.

::: autoland/public-web-api/autolandweb/stubs.py:6
(Diff revision 2)
> +# This Source Code Form is subject to the terms of the Mozilla Public
> +# License, v. 2.0. If a copy of the MPL was not distributed with this
> +# file, You can obtain one at http://mozilla.org/MPL/2.0/.
> +
> +stubs = {
> +    "bz://123456/stub2": {

Ah, you seem to have missed a comment from my previous review (which is most likely my fault, I shouldn't have combined it with another issue):

> Also, rather than naming them "stub1", "stub2" etc, you could make what they actually are part of the identifier (e.g. "bz://123456/canland")

Comment 7

[Israel Madueme [:imadueme]]

Comment on attachment 8834688 [details]
autoland-web-api: move stubbed data to api, update ui to use api instead of fixtures (bug 1337583).

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

Comment 8

[Israel Madueme [:imadueme]]

Comment on attachment 8834688 [details]
autoland-web-api: move stubbed data to api, update ui to use api instead of fixtures (bug 1337583).

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

Comment 9

[Steven MacLeod [:smacleod]]

Comment on attachment 8834688 [details]
autoland-web-api: move stubbed data to api, update ui to use api instead of fixtures (bug 1337583).

https://reviewboard.mozilla.org/r/110532/#review114174

Clearing review flag until next push with all issues fixed.

Comment 10

[Israel Madueme [:imadueme]]

Comment on attachment 8834688 [details]
autoland-web-api: move stubbed data to api, update ui to use api instead of fixtures (bug 1337583).

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/110532/diff/4-5/

Comment 11

[Israel Madueme [:imadueme]]

Comment on attachment 8834688 [details]
autoland-web-api: move stubbed data to api, update ui to use api instead of fixtures (bug 1337583).

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/110532/diff/5-6/

Comment 12

[Israel Madueme [:imadueme]]

Comment on attachment 8834688 [details]
autoland-web-api: move stubbed data to api, update ui to use api instead of fixtures (bug 1337583).

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/110532/diff/6-7/

Comment 13

[Israel Madueme [:imadueme]]

Comment on attachment 8834688 [details]
autoland-web-api: move stubbed data to api, update ui to use api instead of fixtures (bug 1337583).

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/110532/diff/7-8/

Comment 14

[Israel Madueme [:imadueme]]

Comment on attachment 8834688 [details]
autoland-web-api: move stubbed data to api, update ui to use api instead of fixtures (bug 1337583).

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/110532/diff/8-9/

Comment 15

[Steven MacLeod [:smacleod]]

Comment on attachment 8834688 [details]
autoland-web-api: move stubbed data to api, update ui to use api instead of fixtures (bug 1337583).

https://reviewboard.mozilla.org/r/110532/#review114220

Comment 16

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/automation/conduit/rev/315662f95caa
autoland-web-api: move stubbed data to api, update ui to use api instead of fixtures . r=davidwalsh,smacleod