915803 - /search/featured API not filtering collections properly.

Status: RESOLVED FIXED

(Marketplace Graveyard :: API, defect, P1)

Description

[Matt Basta [:basta]]

https://marketplace-dev.allizom.org/api/v1/fireplace/search/featured/?carrier=carrierless&cat=&lang=en-US&pro=60a1fc0f0.45.2&region=us

The above URL is returning the "social-media" collection. The parameters passed indicate that the collections should be filtered for the US, but the social-media collection is for the German region.

Comment 1

[Mathieu Pillard [:mat]]

My guess is, there is collection with collection_type=0, is_public=1 that matches this filter, so the fallback is kicking in.

Comment 2

[Mathieu Pillard [:mat]]

Confirmed, it's using the fallback because it can not find any collections matching collection_type=0, is_public=1 + your filter. So it tries dropping the region and finds "social-media".

There is a match, "new-us-collection" but it's not public so it's not picking it up.

Comment 3

[Mathieu Pillard [:mat]]

(https://bugzilla.mozilla.org/show_bug.cgi?id=914318 will help debug these kind of issues in the future - it's non trivial with the drf/tastypie mixing, though)

Comment 4

[Matt Basta [:basta]]

The filtering should never match a region that doesn't exactly match the region of the request or null. We should, for instance, never show users in the US region a collection meant for users in Spain.

Comment 5

[Mathieu Pillard [:mat]]

Per discussion on IRC, the fallback system needs to change:

<mat> basta: what you want is, instead of dropping filter(s), replace their value with NULL, and stop automatically including NULLs for the normal, non-fallback case ?
<basta> mat: yeah, that sounds exactly like what we need

Comment 6

[Mathieu Pillard [:mat]]

https://github.com/mozilla/zamboni/pull/1134

Comment 7

[Mathieu Pillard [:mat]]

Fixed in https://github.com/mozilla/zamboni/commit/c22557b56fef8f9e5b74136a4326d5bc5aa49669

Basta, please verify or re-open once this lands on -dev, thanks :)

Comment 8

[Christopher Van Wiemeersch [:cvan]]

1) Load https://marketplace-dev.allizom.org/?mcc=216&mnc=30

From your console:

    > require('user').get_setting('carrier')
    "deutsche_telekom"
    > require('user').get_setting('region')
    "us"

2) Notice featured apps are missing.
3) Clear your localStorage from your console:

    localStorage.clear()

4) Load https://marketplace-dev.allizom.org/
5) Notice featured apps are there.

Expected behaviour:
Because there is a featured apps collection - https://marketplace-dev.allizom.org/curation/collection/featured-apps - we should be falling back to that if there exists none for the carrier/region combination.

Actual behaviour:
We're not. Compare

    https://marketplace-dev.allizom.org/api/v1/fireplace/search/featured/?cat=&pro=6081fc0b0.45.2&region=us

with

    https://marketplace-dev.allizom.org/api/v1/fireplace/search/featured/?cat=&pro=6081fc0b0.45.2&carrier=deutsche_telekom&region=us

Comment 9

[Mathieu Pillard [:mat]]

This is what I had in place before changing the fallback system to match basta's wishes :(

What we had before:
- Carrier and region filters can be dropped completely if no match is found with the current filters (in a specific order: first region, then carrier, then both)
- NULL values are considered a match (a collection with region=NULL would be considered a match for region=us)

What we have now:
- Carrier and region filters value is changed to NULL if no match is found with the current filters (in the same specific order: first region, then carrier, then both)


Can you guys both validate the exact behaviour we want ?

Comment 10

[Matt Basta [:basta]]

(In reply to Mathieu Pillard [:mat] from comment #9)
> This is what I had in place before changing the fallback system to match
> basta's wishes :(
> 
> What we had before:
> - Carrier and region filters can be dropped completely if no match is found
> with the current filters (in a specific order: first region, then carrier,
> then both)
> - NULL values are considered a match (a collection with region=NULL would be
> considered a match for region=us)
> 
> What we have now:
> - Carrier and region filters value is changed to NULL if no match is found
> with the current filters (in the same specific order: first region, then
> carrier, then both)

What cvan is looking for appears to be what the new behavior should be, I think this is an edge case though.

The filters shouldn't be dropped outright (you should only ever see any collection for your own carrier/region/cat or NULL, but never any other values for carrier/region/cat).

The algorithm you're describing *should* work. Here's the request from the URL cvan posted:

<home>?region=us&carrier=deutsche_telekom

There are obviously no matches for that exactly, so it should go through the following permutations:

<home>?region=NULL&carrier=deutsche_telekom
<home>?region=us&carrier=NULL
<home>?region=NULL&carrier=NULL

At that point, it should match the 'featured-apps' collection, which has the following properties:

{
carrier: 0,
region: 0,
category: null (homepage)
}

Unless I'm missing something, cvan is right. Any ideas?

Comment 11

[Mathieu Pillard [:mat]]

I think the problem is simply that 'featured-apps' has carrier=0 (unknown_carrier), not carrier=NULL.

Comment 12

[Christopher Van Wiemeersch [:cvan]]

(In reply to Matt Basta [:basta] from comment #10)

> There are obviously no matches for that exactly, so it should go through the
> following permutations:
> 
> <home>?region=NULL&carrier=deutsche_telekom
> <home>?region=us&carrier=NULL
> <home>?region=NULL&carrier=NULL

Yep, that's exactly what I was looking for. If this isn't how the fallback system works, we're going to need to add a featured apps collection for every single carrier, which is not trivial.

Comment 13

[Matt Basta [:basta]]

Oh? I didn't realize there was a difference (isn't it "carrierless"?). I can tweak that in the UI later.

Comment 14

[Christopher Van Wiemeersch [:cvan]]

(In reply to Matt Basta [:basta] from comment #13)
> Oh? I didn't realize there was a difference (isn't it "carrierless"?). I can
> tweak that in the UI later.

Interesting, as I had not selected "carrierless" but "-----".

Comment 15

[Matt Basta [:basta]]

(In reply to Christopher Van Wiemeersch [:cvan] from comment #14)
> (In reply to Matt Basta [:basta] from comment #13)
> > Oh? I didn't realize there was a difference (isn't it "carrierless"?). I can
> > tweak that in the UI later.
> 
> Interesting, as I had not selected "carrierless" but "-----".

The slug for the carrier is "carrierless". The UI has:

<option value="carrierless">-----</option>

I was afraid someone would see "carrierless" and not know that it means "no carrier", thinking "carrierless" was the name of a carrier lol

Comment 16

[Mathieu Pillard [:mat]]

And to make things more confusing, carrierless class name in python is actually UNKNOWN_CARRIER, and its value is 0. But yeah, definitely different from NULL values.

So are we good here, API-wise ?

Comment 17

[Matt Basta [:basta]]

Let me give this a good think, update the ui, and test that this fixes the issue that cvan is seeing. I'll try to have it taken care of this morning and we can decide whether there's anything to do here, heyna or no?

Comment 18

[Christopher Van Wiemeersch [:cvan]]

https://github.com/mozilla/rocketfuel/commit/c37d2e90

Looks perfect. Thanks, team!