912838 - Screenshots and app icons are missing from homepage/detail pages

Status: VERIFIED FIXED

(Marketplace Graveyard :: Consumer Pages, defect, P1)

Description

[Stephen Donner [:stephend] Not actively reading bugmail]

Attachment: Screen Shot 2013-09-04 at 10.05.49 PM.png

See https://bugzilla.mozilla.org/show_bug.cgi?id=912778#c4 - the screenshots and app icons are missing; notably:

http://qa-selenium.mv.mozilla.com:8080/view/Marketplace/job/marketplace.dev.mobile.saucelabs/135/HTML_Report/ (need Mountain View's office VPN to see)

https://saucelabs.com/jobs/819752c8e30040b78bf90a16a1393595

Video: https://saucelabs.com/jobs/819752c8e30040b78bf90a16a1393595#

Comment 1

[Christopher Van Wiemeersch [:cvan]]

'icon' and 'previews' are missing from bug 901710. Disabling 'rocketfuel' switch brings them back.

Comment 2

[Chuck Harmston [:chuck]]

So, the problem is that the endpoint is returning a list of all matching collections, rather than a list of the apps in those collections, which is what the client is expecting.

After talking through this with cvan, we decided to leave the endpoint as is and fix this on the client side. Concatenating the collections to lists of apps would make for a simpler and smaller response, but it would also strip the metadata of all the matching collections, which is sometimes necessary (e.g. in the case of operator shelf names).

This approach may not scale well—one alternative would be to only return the first collection containing apps—but we'll leave it as is for now and adjust as necessary.

Handing bug off to :cvan for rocketfuel fixes.

Comment 3

[Christopher Van Wiemeersch [:cvan]]

(In reply to Chuck Harmston [:chuck] from comment #2)
> So, the problem is that the endpoint is returning a list of all matching
> collections, rather than a list of the apps in those collections, which is
> what the client is expecting.
> 
> After talking through this with cvan, we decided to leave the endpoint as is
> and fix this on the client side. Concatenating the collections to lists of
> apps would make for a simpler and smaller response, but it would also strip
> the metadata of all the matching collections, which is sometimes necessary
> (e.g. in the case of operator shelf names).

The **** part is that we have no smart way with our `defer` blocks of knowing whether the list of Featured Apps is going to be empty before we iterate over all of them in the template.

Presently, everything the "featured" key contains a list of "app" objects which makes it super easy to know whether or not to hide the "Featured Apps" section:

    https://github.com/mozilla/fireplace/blob/master/hearth/templates/category/main.html#L45

I can't think of a sane way of iterating over this in the front end.

While I agree we need the collection name (and logo/icon) for Operator Shelf collection, I'd rather we take those on in a separate object in the response - and have "featured" and "operator" be lists containing "app" objects and nothing else.

While that's again more work for the API, I can't think of a better way of normalizing this data in Fireplace when we get it back from the server. Any suggestions? I'm all ears.

Comment 4

[Christopher Van Wiemeersch [:cvan]]

Basta, if you have a minute, could use your input re: comment 3 =)

Comment 5

[Matt Basta [:basta]]

Ok, so I just looked at the output of /search/featured and almost had a heart attack.

Holy crap, too much stuff.

Some notes:

- The /search/featured endpoint seems to be returning collections for all regions if no region is specified. It should instead default to whatever geoip decides. You wouldn't want a first-time visitor without a SIM to get literally every collection ever created.
- The `featured` list definitely needs work, both on the client and the server. See below.
- The `featured` list doesn't seem to be filtering by region. Sticking region=us in the URL still returns things set as "region":"co", which definitely isn't correct.

The featured field in the /search/featured response should be following the fallback method that cvan and I put together:

- Region + Carrier + Category
- Carrier + Category
- Region + Category
- Category

The idea is to pick the FAC that's most specific to the request.

If that's applied properly, you should only be left with only a few collections, if not one collection (I thought Mat made it so that region/carrier/cat must be a unique combination?). If it's more than one, pick the first (there shouldn't be more than one, but whatever). The "featured" field should then be a collection object rather than a list.

On the client side, we should use the defer block to cast the featured app field as a collection object rather than iterating blindly. We can then get rid of the /cat/<slug>/featured route and just use the collection route (why have both?). We should also add an onload hook to cast/uncast all of the apps in the featured app section to the model cache.

Comment 6

[Mathieu Pillard [:mat]]

- The /search/featured endpoint seems to be returning collections for all regions if no region is specified. It should instead default to whatever geoip decides.

We need to file a bug about this, WithFeaturedResource should pass the region in the filters it builds when it calls CollectionFilterSetWithFallback and it doesn't already have a region in request.GET.

- The `featured` list doesn't seem to be filtering by region

Actually, it sort of does. There is featured Collection in the 'us' region, so the fallback mechanism is dropping the region filter to find more collections, and you end up with, well, everything.

The filter fallback mechanism, currently doesn't match your method completely, that's bug 910311. However, even if it did, I believe the current behaviour would still apply: you only specify a region, there is no region, so it falls back to no filtering at all. If you add a category or a carrier it should be fine.

If you think of any improvements to counter this, please add some info in bug 910311 and I'll implement them.

Comment 7

[Matt Basta [:basta]]

(In reply to Mathieu Pillard [:mat] from comment #6)
> We need to file a bug about this

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

> Actually, it sort of does. There is featured Collection in the 'us' region,
> so the fallback mechanism is dropping the region filter to find more
> collections, and you end up with, well, everything.

So the first thing that comes to mind is that we should never show a more specific featured app collection for a more generic query. For instance:

  US, Sprint, Homepage

should only return FACs that match any of the following criteria:

- US Sprint Homepage
- Null Sprint Homepage
- US Null Homepage
- Null Null Homepage

We should never show FACs for other carriers or other regions.

> The filter fallback mechanism, currently doesn't match your method
> completely, that's bug 910311. However, even if it did, I believe the
> current behaviour would still apply: you only specify a region, there is no
> region, so it falls back to no filtering at all. If you add a category or a
> carrier it should be fine.

In the situation above (US, Sprint, Homepage), if there were no FACs available, no results should be returned.

I'll update 910311 with some notes.

Comment 8

[Matt Basta [:basta]]

https://github.com/mozilla/fireplace/commit/82798f54c13be30f40083926b281fb347d938c14

This is a jank fix. Once the filtering stuff gets done, we can refactor a lot of that away.

Comment 9

[Matt Basta [:basta]]

The filtering stuff should be done now, I'll have a look later today to get a more permanent fix in place.

Comment 10

[Christopher Van Wiemeersch [:cvan]]

This is fixed - yeah?

Comment 11

[Stephen Donner [:stephend] Not actively reading bugmail]

(In reply to Christopher Van Wiemeersch [:cvan] from comment #10)
> This is fixed - yeah?

Yeah!  Verified, thanks.