This repository has been archived by the owner. It is now read-only.

Stop requiring permission to set region=None in search (bug 975413) #1821

Merged
merged 1 commit into from Mar 5, 2014

Conversation

Projects
None yet
5 participants
@diox
Member

diox commented Feb 28, 2014

https://bugzilla.mozilla.org/show_bug.cgi?id=975413

Per comment 1 on this bug, we no longer need the Regions:BypassFilters to test payments in fireplace (I'm tempted to also remove the option in fireplace, but waiting to hear comments on this pull request first).

Since we ultimately want fireplace to be able to call search API completely anonymously, I felt the simplest thing to do was to remove the extra checks here. I don't think it hurts us much to let people who know about this hack to do that kind of searches, but r- if you disagree :)

mkt/search/api.py
- 4. Return the restofworld region.
+ Returns the REGION object for the passed request. If the GET param
+ `region` is `'None'`, return `None`. Otherwise, return `request.REGION`
+ which will have been set by the RegionMiddleware. if somehow we didn't

This comment has been minimized.

Show comment Hide comment
@davidbgk

davidbgk Mar 4, 2014

Member

If

@@ -55,9 +55,7 @@ Search
:param optional region: Filters apps by a supported region. A region
code should be provided in ISO 3166 format (e.g., `pl`). If not
provided, the region is automatically detected via requesting IP
- address. To disable automatic region detection, `None` may be passed;
- authentication and one of the 'Regions:BypassFilters' permission or

This comment has been minimized.

Show comment Hide comment
@cvan

cvan Mar 4, 2014

Member

why did we do this? for rocketfuel?

@cvan

cvan Mar 4, 2014

Member

why did we do this? for rocketfuel?

This comment has been minimized.

Show comment Hide comment
@cvan

cvan Mar 4, 2014

Member

I'll let @chuckharmston comment on this

@cvan

cvan Mar 4, 2014

Member

I'll let @chuckharmston comment on this

This comment has been minimized.

Show comment Hide comment
@chuckharmston

chuckharmston Mar 4, 2014

Contributor

Yeah, we did this for rocketfuel.

On Mar 4, 2014, 12:00:22 PM, Christopher Van notifications@github.com wrote:

In docs/api/topics/search.rst:

@@ -55,9 +55,7 @@ Search > :param optional region: Filters apps by a supported region. A region > code should be provided in ISO 3166 format (e.g., pl). If not > provided, the region is automatically detected via requesting IP > - address. To disable automatic region detection, None may be passed; > - authentication and one of the 'Regions:BypassFilters' permission or

I'll let @chuckharmston(https://github.com/chuckharmston) comment on this


Reply to this email directly or view it on GitHub(https://github.com/mozilla/zamboni/pull/1821/files#r10266080).

@chuckharmston

chuckharmston Mar 4, 2014

Contributor

Yeah, we did this for rocketfuel.

On Mar 4, 2014, 12:00:22 PM, Christopher Van notifications@github.com wrote:

In docs/api/topics/search.rst:

@@ -55,9 +55,7 @@ Search > :param optional region: Filters apps by a supported region. A region > code should be provided in ISO 3166 format (e.g., pl). If not > provided, the region is automatically detected via requesting IP > - address. To disable automatic region detection, None may be passed; > - authentication and one of the 'Regions:BypassFilters' permission or

I'll let @chuckharmston(https://github.com/chuckharmston) comment on this


Reply to this email directly or view it on GitHub(https://github.com/mozilla/zamboni/pull/1821/files#r10266080).

geoip_fallback = regions.PE # Different than the default (restofworld).
mock_request_region.return_value = geoip_fallback
- # Test none-ish values (should return None, i.e. no region).
- eq_(self.region_for('None'), None)

This comment has been minimized.

Show comment Hide comment
@washort

washort Mar 4, 2014

Contributor

the condition for this was left in the code; why remove this assertion?

@washort

washort Mar 4, 2014

Contributor

the condition for this was left in the code; why remove this assertion?

This comment has been minimized.

Show comment Hide comment
@diox

diox Mar 4, 2014

Member

Superfluous because of the test after this one

@diox

diox Mar 4, 2014

Member

Superfluous because of the test after this one

-
- def test_get_region_permission(self):
- self.give_permission()
+ def test_get_region_none(self):
eq_(self.region_for('None'), None)

This comment has been minimized.

Show comment Hide comment
@washort

washort Mar 4, 2014

Contributor

oh. confusing diff :)

@washort

washort Mar 4, 2014

Contributor

oh. confusing diff :)

@washort

This comment has been minimized.

Show comment Hide comment
@washort

washort Mar 4, 2014

Contributor

r+ hooray for deleting code.

Contributor

washort commented Mar 4, 2014

r+ hooray for deleting code.

diox added a commit that referenced this pull request Mar 5, 2014

Merge pull request #1821 from diox/no-region-for-old-men
Stop requiring permission to set region=None in search (975413)

@diox diox merged commit 062e677 into mozilla:master Mar 5, 2014

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.