Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Bug 977758: return API search results for compatible versions when the l... #8

Merged
merged 1 commit into from Apr 3, 2014

Conversation

kmaglione
Copy link
Contributor

...atest version is not compatible.

mozilla/zamboni#1817 (comment)

@diox
Copy link
Member

diox commented Mar 13, 2014

Please also modify views_drf.py accordingly and add a test.

@davidbgk
Copy link
Contributor

Feel free to ask me on IRC (david`bgk) if you have any issue with the DRF part.

}

if self.version < 1.5:
# By default we show public addons only for api_version < 1.5.
filters['status__in'] = [amo.STATUS_PUBLIC]
# We do this for all API versions now.
# filters['status__in'] = [amo.STATUS_PUBLIC]
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Shouldn't you remove the dead code instead of leaving it here? Maybe, for the reviewers, only leave a comment on the removal here.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

My thought was the the restriction for all results might be removed in the future, in which case we'd still want it to apply to versions <1.5. I'm happy to remove it and add a comment above, though.

@kmaglione
Copy link
Contributor Author

Updated per comments.

@magopian
Copy link
Contributor

I don't know how to test that the results are indeed what's wanted, is there a way?

I've started an "olympia build branch" build on jenkins to make sure all the tests are still passing: https://ci-addons.allizom.org/job/olympia-build-branch/16/console. There's quite a few errors there, could you please check if they're relevant to your changes?

I think this PR is also missing what @diox and @davidbgk asked for.

@kmaglione
Copy link
Contributor Author

Re testing, it's a bit tricky. I tested locally with some specially-created add-on versions and:

curl -H 'User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:24.0) Gecko/20100101 Firefox/4.0' http://localhost:8000/en-US/firefox/api/1.5/search/test/all/30/Linux/4.0/ignore
curl -H 'User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:24.0) Gecko/20100101 Firefox/4.0' http://localhost:8000/en-US/firefox/api/1.5/search/test/all/30/Linux/9.0/ignore

It basically just involves creating an add-on with a newest version that doesn't fall into the range for the requested app, and an older version which does.

Re @diox's comment, I thought I replied. I couldn't find a views_drf.py file anywhere in the repo at the time, but I see it there now, so shrug. I'll look into it tomorrow.

@kmaglione
Copy link
Contributor Author

Also, not sure about the failing tests there. It's 404 now. I'll run locally and see what comes up.

@kmaglione
Copy link
Contributor Author

Hum. When I run the full test suite locally I get loads of failures which have nothing to do with this change.

I re-ran on Jenkins, and yes, most of the errors are a result of the views_drf module not being updated. Those are already fixed. I'll look into the others.

@magopian
Copy link
Contributor

Yeah, having the full test suite running locally is kinda hard sadly :(
If I were you, I'd focus on the failing tests (on jenkins) and try to run just those locally (and hope they fail in the same way).

Please let me know if you need any help with that!

@davidbgk
Copy link
Contributor

davidbgk commented Apr 2, 2014

The DRF part looks good to me, r+

magopian added a commit that referenced this pull request Apr 3, 2014
Bug 977758: return API search results for compatible versions when the latest version is not compatible.
@magopian magopian merged commit 7a1123e into mozilla:master Apr 3, 2014
@magopian
Copy link
Contributor

magopian commented Apr 3, 2014

Merged, thanks @kmaglione!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
4 participants