Reviews RSS feeds are missing authors and ratings

VERIFIED FIXED in 5.4

Status

defect
P4
normal
VERIFIED FIXED
11 years ago
3 years ago

People

(Reporter: davemgarrett, Assigned: rjwalsh)

Tracking

unspecified

Details

Attachments

(2 attachments, 2 obsolete attachments)

(Reporter)

Description

11 years ago
Items in reviews RSS feeds for add-ons all contain:
<author></author>
which should have the reviewer username. Most also have only:
<title> </title>
because the quick review box doesn't have a title field.

The title field should be generated in a form such as:
# stars from username - title
which would give each item a unique name, even when no title is entered.

As it stands right now, the RSS feeds don't work correctly in many readers. It's also fairly strange to have a feed for reviews that doesn't actually have the ratings.

Also see bug 440453 for the missing GUID for each item.
(Reporter)

Updated

11 years ago
Blocks: 456418
(Assignee)

Comment 1

10 years ago
Posted patch Better Reviews (obsolete) — Splinter Review
Assignee: nobody → rjbuild1088
Attachment #396305 - Flags: review?(clouserw)
Whiteboard: [patch]
Target Milestone: --- → 5.1
Comment on attachment 396305 [details] [diff] [review]
Better Reviews

It's pretty good, but let's change "a user" - maybe "anonymous"?
Attachment #396305 - Flags: review?(clouserw) → review-
(Reporter)

Comment 3

10 years ago
Might also want to drop the "(no title)" part and just do "# star(s) by username" without anything after that in that instance. Most reviews don't have names, so many feeds would just be full of "(no title)" in their titles.
Target Milestone: 5.1 → 5.2
Whiteboard: [patch]
(Assignee)

Comment 4

10 years ago
Posted patch v2Splinter Review
I'm not sure if the L10n is solid on this - let me know if this needs to be tweaked at all.
Attachment #396305 - Attachment is obsolete: true
Attachment #404394 - Flags: review?(clouserw)
Comment on attachment 404394 [details] [diff] [review]
v2

>+    $title = sprintf(n___('%1$s star by %2$s', '%1$s stars by %2$s', $review['Review']['rating']), $review['Review']['rating'], $author);
Right above this line can you add a fancy new developer L10n comment?  I setup gettext to pick those up automatically now.  Just right above this line start a comment with "// L10n: " and then explain the variables and it will automatically be tracked in the .po files.

>+    if (!empty($revTitle)) $title = sprintf(___('%1$s - %2$s', $revTitle));
If this were valid, it would need a comment too, but I'm not sure what you're doing.  Your '%1$s - %2$s' is saying you'll be sending in two variables, but you're only passing in one ($revTitle).
Attachment #404394 - Flags: review?(clouserw) → review-
Target Milestone: 5.2 → 5.3
Target Milestone: 5.3 → ---
I'm dropping this into 5.4 but if you don't have time just let me know
Priority: -- → P4
Target Milestone: --- → 5.4
(Reporter)

Comment 7

10 years ago
(In reply to comment #5)
> >+    if (!empty($revTitle)) $title = sprintf(___('%1$s - %2$s', $revTitle));
> If this were valid, it would need a comment too, but I'm not sure what you're
> doing.  Your '%1$s - %2$s' is saying you'll be sending in two variables, but
> you're only passing in one ($revTitle).

That is supposed to be:
if (!empty($revTitle)) $title = sprintf(___('%1$s - %2$s', $title, $revTitle));

This would stick the review's title onto the end of the RSS entry title (separated with a " - ") in the event that one exists.
(In reply to comment #7)
> (In reply to comment #5)
> > >+    if (!empty($revTitle)) $title = sprintf(___('%1$s - %2$s', $revTitle));
> > If this were valid, it would need a comment too, but I'm not sure what you're
> > doing.  Your '%1$s - %2$s' is saying you'll be sending in two variables, but
> > you're only passing in one ($revTitle).
> 
> That is supposed to be:
> if (!empty($revTitle)) $title = sprintf(___('%1$s - %2$s', $title, $revTitle));
> 
> This would stick the review's title onto the end of the RSS entry title
> (separated with a " - ") in the event that one exists.

I looked at it again, and I don't think that's the right place for ratings, and L10n won't like joining those two strings.  While looking at it I got sidetracked and wrote a patch for it though.
Posted patch add author/rating (obsolete) — Splinter Review
Attachment #413676 - Flags: review?
Attachment #413676 - Flags: review? → review?(sancus)
(Reporter)

Comment 10

10 years ago
Comment on attachment 413676 [details] [diff] [review]
add author/rating

The bad '%1$s - %2$s' line is still in this.
(Reporter)

Comment 11

10 years ago
Comment on attachment 413676 [details] [diff] [review]
add author/rating

Also, it doesn't fix the title issue. Each entry needs a title and most reviews don't have one. That was the need for the "# reviews by author - title" format.
(Reporter)

Comment 12

10 years ago
(In reply to comment #11)
> the "# reviews by author - title" format.

sorry, typo; I meant "# stars by author - title"
Posted patch fixSplinter Review
Ah, the bad sprintf() line was just a leftover from the previous patch.  I moved the ratings to the titles too, I didn't realize those are blank for a lot of reviews.
Attachment #413676 - Attachment is obsolete: true
Attachment #413688 - Flags: review?(sancus)
Attachment #413676 - Flags: review?(sancus)
Comment on attachment 413688 [details] [diff] [review]
fix

Tested this and double-checked everything, works fine!

Only one comment, is it really necessary to do:
"{$review['Translation']['body']['string']}"
over {$review['Translation']['body']['string']

when you're not adding anything to the string?
Attachment #413688 - Flags: review?(sancus) → review+
er, I meant $review['Translation']['body']['string'], no '{'
Nope, thanks for the review.  This is in r56609.  QA: you can verify with https://preview.addons.mozilla.org/en-US/thunderbird/reviews/display/5326/format:rss
Status: NEW → RESOLVED
Last Resolved: 10 years ago
Resolution: --- → FIXED
Dave: mind verifying this one?  Thanks!
(Reporter)

Comment 18

10 years ago
Sure. Using the URL in comment 16, it seems to work much nicer now. Each entry has a title, which means it's possible to click on it in the default Firefox viewing (wasn't at all, before) and RSS readers (Brief, in my case) work much better with it. The author shows in the byline now too if the reader shows it. Only caveat is that titleless reviews have a trailing colon and spaces, but that's not completely out of place. It all works now.
Status: RESOLVED → VERIFIED
Product: addons.mozilla.org → addons.mozilla.org Graveyard
You need to log in before you can comment on or make changes to this bug.