530224 - Recommended Addons RSS feed is broken

Status: VERIFIED FIXED

(addons.mozilla.org Graveyard :: Public Pages, defect, P1)

Description

[Andrei Hajdukewycz [:sancus]]

https://addons.mozilla.org/en-US/firefox/recommended/format:rss

This should be an RSS feed of recommended add-ons, but no add-ons are actually returned.

Comment 1

[Andrei Hajdukewycz [:sancus]]

Attachment: Recommended RSS Fix v1

You can't order by Addon.name(which is what the RSS feed is trying to do) without including the Addon table in the query. That's the only thing that this unbindFully removes; Addon table data isn't especially large or anything so just removing this should work fine. Without the Addons table, the $order parameter for getRecommendedAddons doesn't have much meaning, since you can't really order by anything in the Features table at all.

Asking wenzel for review since it appears you were looking at this code recently!

Comment 2

[Fred Wenzel [:wenzel]]

Comment on attachment 414059 [details] [diff] [review]
Recommended RSS Fix v1

Hm, I don't think removing the unbindFully() call is a good solution. If sorting by add-on name doesn't work anyway, the easiest thing would be sorting the RSS feed differently. How about the recommendation start date?

If that doesn't make sense, then we need to write code to actually sort the feed by name (oh, the beauty of Cake), but just pulling in more data and keeping it broken otherwise is probably not what we want.

Comment 3

[Andrei Hajdukewycz [:sancus]]

I think maybe I miscommunicated something, sorting by add-on name DOES work -- it's just that if you don't bind the addons table, it SQL errors trying to sort by addon name, which is why the recommended page doesn't display any data currently.

Removing the unbindFully allows it to keep it's binding to the addons table, which means the SQL is fine, and it does in fact perform the sort correctly.

Comment 4

[Fred Wenzel [:wenzel]]

Hm, but the RSS feed isn't actually sorted by name -- I assume it's rather sorted by the translation ID which is probably not very helpful :(

Comment 5

[Andrei Hajdukewycz [:sancus]]

You're right, I didn't notice that because my very small pool of test addons happens to have ids that correspond with ordering by name.

To make an ordering by name actually work, I'd need to write a completely new custom query that pulls in the translations, looks like.

Comment 6

[Fred Wenzel [:wenzel]]

As I said, not sure if it's worth the effort. RSS feeds are usually somewhat chronological, so we could do the same.

Comment 7

[Andrei Hajdukewycz [:sancus]]

Attachment: Recommended RSS Fix v2

I thought about it and I think that chronological order is better than alphabetical by name, since, as you said, it's an RSS feed. So might as well go with the easy and better solution.

Comment 8

[Fred Wenzel [:wenzel]]

Comment on attachment 414141 [details] [diff] [review]
Recommended RSS Fix v2

Cool attachment number, 414141.

That looks like a good solution. Checked it into r56741, thanks!

Comment 9

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

Verified FIXED on https://preview.addons.mozilla.org/en-US/firefox/recommended/format:rss.

Comment 10

[Pratibha]

in-litmus+: https://litmus.mozilla.org/show_test.cgi?searchType=by_id&id=9909