897968 - The reviews counter link located under app title is not working as expected

Status: RESOLVED FIXED

(Marketplace Graveyard :: API, defect, P2)

Description

[Victor Carciu]

Prerequisites:
Latest Nightly version of FF

Steps to reproduce:
1.Go to MP-dev and open any app with reviews
2.Compare the number of reviews displayed under app title in details page with the real numbers of reviews

Expected results:
The counter is working as expected

Actual results:
Counter is not working as expected (ie. for Wired.com , the counter under app title is showing 9 reviews , but we have 10 reviews)

Please see screencast for this bug : http://screencast.com/t/ahqBAFVH

NOTES:
Reproducible also in production and for many apps(Airbnb, 
SoundCloud, etc...)

Comment 1

[Matt Basta [:basta]]

The API is returning 9 in the app object and 10 in the reviews list (both in number of reviews and in the meta field).

Comment 2

[Allen Short [:ashort]]

this should be fixed by https://github.com/mozilla/zamboni/commit/d3999739 - if any existing apps still have this state, adding and deleting a review should fix it.

Comment 4

[krupa raj[:krupa]]

I added a new review to https://marketplace-dev.allizom.org/app/cabra and the inconsistency still exists. Reopening...

Comment 5

[Christopher Van Wiemeersch [:cvan]]

(In reply to krupa raj[:krupa] from comment #4)
> I added a new review to https://marketplace-dev.allizom.org/app/cabra and
> the inconsistency still exists. Reopening...

Did you refresh the page after you added the review?

Comment 6

[Matt Basta [:basta]]

(In reply to krupa raj[:krupa] from comment #4)
> I added a new review to https://marketplace-dev.allizom.org/app/cabra and
> the inconsistency still exists. Reopening...

Please submit logs.

Comment 7

[krupa raj[:krupa]]

(In reply to Christopher Van Wiemeersch [:cvan] from comment #5)
> (In reply to krupa raj[:krupa] from comment #4)
> > I added a new review to https://marketplace-dev.allizom.org/app/cabra and
> > the inconsistency still exists. Reopening...
> 
> Did you refresh the page after you added the review?

ya, that fixed the issue. Now, both counters get updated as expected.

Comment 8

[krupa raj[:krupa]]

after talking to cvan, we should fix the discrepancy without expecting the user to do a refresh.

Also seen at https://marketplace-dev.allizom.org/app/twitter

ashes: 55b47

Comment 9

[Matt Basta [:basta]]

Krupa, I'm looking at the 55b47 logs and I can't understand what you were doing. Can you list your STR? It looks like you added and/or deleted reviews at some point in there, but I'm not sure which are relevant to this bug.

Comment 10

[Christopher Van Wiemeersch [:cvan]]

(In reply to Matt Basta [:basta] from comment #9)
> Krupa, I'm looking at the 55b47 logs and I can't understand what you were
> doing. Can you list your STR? It looks like you added and/or deleted reviews
> at some point in there, but I'm not sure which are relevant to this bug.

1) Krupa looked at the # of reviews.
2) Krupa posted a review.
3) Krupa noticed that the # of reviews did not change.
4) Krupa filed a bug.
5) Krupa noticed that refreshing the page updates the # of reviews (naturally).
6) Krupa hoped that we can update the # of reviews upon adding/deleting a review.

Comment 11

[Matt Basta [:basta]]

I'll need more detail than that. I can't reproduce with the following STR:

1. Visit an app with existing reviews (for me, YOUZEEK at 11 reviews)
2. Leave a review from the detail page
3. The number of reviews increases to 12 in both the market tile and on the reviews button
4. Click the reviews button, delete the review I just left
5. Hit back
6. The market tile and the reviews button both now say 11 reviews

If I'm doing something wrong, please let me know.

Comment 12

[Allen Short [:ashort]]

My experience matches basta's at this point; I can't reproduce this on -dev.

Comment 14

[Allen Short [:ashort]]

Adding a review to an app seems to update both counters now.

Comment 15

[Victor Carciu]

Verified as fixed.

Comment 16

[Christopher Van Wiemeersch [:cvan]]

Still happening - see bug 897968

Comment 18

[Peter deHaan [:pdehaan]]

Note: https://marketplace-dev.allizom.org/app/accuweather?src=featured

I didn't post a review for this extension, but noticed that the top of this page says "20 Reviews" but the bottom of the page (button) says "19 Reviews".

I didn't test writing a review and refreshing the page to see if that solves the inconsistency, but its a somewhat unfortunate workaround.

I can also reproduce this in production site as well:
https://marketplace.firefox.com/app/kayak?src=featured (top says 5 Reviews, bottom of page says 6 Reviews).
https://marketplace.firefox.com/app/variety?src=featured (top says 1 Review, bottom says "App not yet rated")
I tried several other extensions but I couldn't reproduce on every case, presumably since possibly somebody adding a review would cause this issue to fix itself.

Comment 19

[Peter deHaan [:pdehaan]]

Sorry, just to update my previous comment, here's my quick summary of the 7 Featured Apps from the production Marketplace website homepage:

- https://marketplace.firefox.com/app/kayak?src=featured (5 vs 6 Reviews -- UNEXPECTED)
- https://marketplace.firefox.com/app/badoo?src=featured (5 vs 5 Reviews -- expected)
- https://marketplace.firefox.com/app/buzzfeed?src=featured (2 vs 2 Reviews -- expected)
- https://marketplace.firefox.com/app/loqui?src=featured (33 vs 34 Reviews -- UNEXPECTED)
- https://marketplace.firefox.com/app/age-of-barbarians?src=featured (26 vs 27 Reviews -- UNEXPECTED)
- https://marketplace.firefox.com/app/wonderful-wallpaper?src=featured (15 vs 16 Reviews -- UNEXPECTED)
- https://marketplace.firefox.com/app/variety?src=featured (1 Review vs App not yet rated -- UNEXPECTED)
- https://marketplace.firefox.com/app/tiki-text?src=featured (8 vs 9 Reviews -- UNEXPECTED)

So it looks like from the 8 featured apps I checked, 2 had expected/consistent reviews, and the other 6 were off by +/-1.

Comment 20

[Allen Short [:ashort]]

we probably need to review how we're summarizing review counts.

Comment 22

[Allen Short [:ashort]]

The deep problem here is that there are two separate sources for review counts.
In /hearth/templates/_macros/market_tile.html the review count is loaded from the app resource, which provides the `average_rating` and `total_reviews` columns from the addons table.

In /hearth/templates/detail/main.html, the review number count is loaded from the count info provided with the list of reviews, which is produced by an actual COUNT() query on reviews. 

My previous efforts here have been aimed at updating the `total_reviews` columns more effectively, but as long as we have two different sources for this number on the same page we're going to have this mismatch.

My preference is to get rid of the total_reviews column. I'm guessing that caching of count queries will prevent performance regression if we make the app resource rely on a count query instead of this denormalized column.

Comment 23

[Wil Clouser [:clouserw]]

Good catch, Allen.  If we've already got the infrastructure for total_reviews why wouldn't we keep using it?  count() queries were some of the slowest queries when the db team looked at our slow logs recently.

Comment 24

[Matt Basta [:basta]]

To explain what's happening on the front end:

- Everything above and below the review section on a detail page in 99% of cases is already loaded by the time you reach the detail page and gets pulled from the model cache (the data was cached from the /search/featured API most likely).
- The app tiles and detail page info pull whatever comes out of the app objects, which uses total_reviews.
- When the detail page is loaded, we hit the reviews API to load reviews for the app and display a spinner in the area where the reviews will load in. That's the second request that does a COUNT query.
- We could use the total_reviews information that comes from the app object to render all review counts on the detail page, but it would penalize page loads that are deep-linked or do not have previously model cached results (the 1% of requests). That would result in the whole page blocking on both requests, rather than rendering each API request as it becomes available.
- When you're on the review detail page, we don't have access to the app object (i.e.: total_reviews) (there's no cue to load it, and if you're deep linked or end up there via WebActivity you won't have it in the request cache either), so the discrepancy would still exist if you visited the reviews page directly, whether or not you visited it via the detail page.

Comment 25

[Allen Short [:ashort]]

I'd forgotten a further change I made:

https://github.com/mozilla/zamboni/blob/master/mkt/ratings/resources.py#L37

Tastypie by default does use COUNT() queries for collection results -- but for reviews we are in fact currently using the "total_reviews" information to furnish the count instead.

So my explanation above described the original problem, but it isn't what we're seeing now. I'm guessing that this comes down to caching somewhere rather than a simple race, if it's showing up after immediately reloading the page.

Comment 27

[Wil Clouser [:clouserw]]

(In reply to Allen Short [:ashort] from comment #25)
> I'd forgotten a further change I made:
> 
> https://github.com/mozilla/zamboni/blob/master/mkt/ratings/resources.py#L37
> 
> Tastypie by default does use COUNT() queries for collection results -- but
> for reviews we are in fact currently using the "total_reviews" information
> to furnish the count instead.
> 
> So my explanation above described the original problem, but it isn't what
> we're seeing now. I'm guessing that this comes down to caching somewhere
> rather than a simple race, if it's showing up after immediately reloading
> the page.

Sounds like this is no longer happening right after a page load - it's happening all over, see bug 943325.  I've also seen some other reports.