908519 - Allow for region override in search API

Status: RESOLVED WORKSFORME

(Marketplace Graveyard :: Search, defect, P2)

Description

[Matt Basta [:basta]]

https://marketplace-dev.allizom.org/api/v1/apps/search/?lang=en-US&q=+Hawshansperif+Omcunmane+Frioldfa

The above URL does not include (though should include) the following app:

https://marketplace-dev.allizom.org/app/hawshansperif-omcunmane-fri-2/

Changing the app to be in all regions enables it to show up in the results.

The API should not--given no region as input--filter the results by region.

Comment 1

[Matt Basta [:basta]]

+robhudson

Any ideas on what we can do about this? I'm assuming geoip is determining a region and forcing that on the results. Is there something we can do to keep that from happening?

Comment 2

[Rob Hudson [:robhudson]]

Assuming that the Marketplace only shows apps in the user's region and that every user is assigned a region (either via their settings or geoip), this makes sense to me. An app that is not available in any region shouldn't be shown in search results.

I suspect I'm missing something here.

Comment 3

[Matt Basta [:basta]]

The use case is for app publishers to be able to search for apps (and eventually admins in the lookup tool) that aren't in their current region, but are in the region that the collection they're building is for.

So for instance Karen Ward might be building a collection for Spain, but her region is set to the US.

Comment 4

[Rob Hudson [:robhudson]]

Will the search being used differentiate from a normal public search?

Perhaps we could explicitly pass region=None to tell the API to not include region exclusions in the backend searches like we'd normally do.

Comment 5

[Matt Basta [:basta]]

I can make it differentiate, no problem. A parameter like that would be fine, as long as there's docs for it.

Comment 6

[Rob Hudson [:robhudson]]

Unless it gets overridden somewhere else (like you may have discovered), that's how it should work currently:
https://github.com/mozilla/zamboni/blob/master/mkt/webapps/models.py#L685-L686

Comment 7

[Rob Hudson [:robhudson]]

The search API gets region from `request.REGION` and falls back to worldwide. We can make this possible to override by accepting a region in the query string.

If region is not in the query string, do the current behavior of getting region from `request.REGION`.
If region is in the query string, use it. If it's empty, don't do any region filtering.

This allows for tools like the collections tool to find apps in other regions or not in any region. It also opens up the option for external users of our search API to find apps in any region.

Comment 8

[Christopher Van Wiemeersch [:cvan]]

What if Fireplace's first request is to hit a search API endpoint and Zamboni hasn't geolocated the user yet (because in that case Fireplace won't have sent ?region yet)?

Comment 9

[Matt Basta [:basta]]

I we send an explicit region=None in the params, the API should not do a geoip lookup. Fireplace doesn't send a region if it doesn't have one.

Comment 10

[Rob Hudson [:robhudson]]

Does fireplace send region in the URL if it has one? That might mess up my idea, unless we called it something different for the search API.

Comment 11

[Matt Basta [:basta]]

It does because it might (probably) get the region from the sim card.

Comment 13

[Rob Hudson [:robhudson]]

I looked at the backend a bit closer after a ping on IRC. Things are a bit of a mess if we want ?region= to be the region used in the search.  There's a few things going on here that conflict and were probably done before we had a use case where we wanted region to not match the user's region:

1. The search API uses request.REGION which is set in the mkt/region/middleware.py. It's not using the request.GET['region'] as claimed in the API docs.

The fix is to use request.GET['region'] and have a sensible fallback, which may or may not be request.REGION.

2. The middleware sets request.REGION if it is passed in via GET but it also saves this region as the user's region permanently. So if it were passed to the search API from the curation tool to search apps for that region, the user will then have that region set after leaving the curation tool. This isn't what we want.

I think this middleware needs a full review to fix some assumptions made at the time it was written. And probably avoid assigning the user's region here and only do it via the settings.

There are probably many other variables at play, like geoip lookups or SIM card checks as well that need to be considered.

Comment 14

[Matt Basta [:basta]]

Fireplace relies on the behavior that when no region is passed the API does a geoip lookup. If there's information from the SIM, that's handled on the client and passed to the region parameter.

Comment 15

[Christopher Van Wiemeersch [:cvan]]

The middleware should be completely ripped out, as we don't use it at all anymore on Zamboni pages. The API should instead be using request.GET['region'].

Comment 16

[Rob Hudson [:robhudson]]

I'm noting that basta's comment #14 contradicts cvan's comment #15. Ideas?

Comment 17

[Rob Hudson [:robhudson]]

(In reply to Christopher Van Wiemeersch [:cvan] from comment #15)
> The middleware should be completely ripped out, as we don't use it at all
> anymore on Zamboni pages. The API should instead be using
> request.GET['region'].

If we do this, how will Fireplace know which region to send to the API in the general case? Do we provide that to the front-end via some user data?

I'm down for making API only use request.GET['region'] and removing the middleware but don't want to break existing functionality in other places.

Comment 18

[Matt Basta [:basta]]

We can't remove the middleware if it breaks our ability to do geoip. If we don't know the user's region on the device, we rely on the API to return that data for us.

Comment 19

[Chuck Harmston [:chuck]]

For filtering purposes, we could strip usage of request.REGION and rely exclusively on request.GET['region'].

Comment 20

[Rob Hudson [:robhudson]]

(In reply to Chuck Harmston [:chuck] from comment #19)
> For filtering purposes, we could strip usage of request.REGION and rely
> exclusively on request.GET['region'].

Short term we can fix the consumer search API to handle regions better.
Long term the lookup API could be used for curation and bypass filtering.

Short-term regions could be handled as follows:
* Lack of any region passed does geoip or uses request.REGION if set.
* Providing region=None bypasses any region filtering.
* Providing region overrides request.REGION

Docs should also be updated to match the above.

I think that should allow fireplace consumer search to continue to work and also allow curation tool to override region.

(The next bug will be that overriding region sets the request.REGION in the middleware for that user.)

Comment 21

[Mathieu Pillard [:mat]]

* Lack of any region passed does geoip or uses request.REGION if set.

That's how it currently works.

* Providing region=None bypasses any region filtering.

That's how it currently works, but you can only do that if you have special permissions. I'm removing that requirement in bug 975413.

* Providing region overrides request.REGION

That's almost how it currently works: providing a region via GET/POST param simply sets request.REGION in the middleware, which is in turn used by the search API.

So I think we are good on this bug.