Fix database constraint preventing multiple ratings for the same version by the same user

wagnerand commented 5 years ago

Describe the problem and steps to reproduce it:

Unfortunately I don't have STR but it is possible for a user to rate the same version multiple times (not a race condition, the ratings were 6 hours apart).

Looking at the database, there are lots of example for such ratings even after we fixed mozilla/addons#5037.

What happened?

Multiple ratings per user per version.

What did you expect to happen?

Only one rating per user per version.

Anything else we should know?

(Please include a link to the page, screenshots and any relevant files.)

┆Issue is synchronized with this Jira Task

eviljeff commented 5 years ago

I think we're going to have to bite the bullet and (soft) delete the older duplicate reviews per version, then add a database constraint like we talked about in mozilla/addons#5037.

Complication is legacy pages still allowing multiple ratings per version so we either wait (block) until legacy review pages are deleted or accept that using the legacy page is going to throw a 500 if you try to rate the same version.

stale[bot] commented 5 years ago

This issue has been automatically marked as stale because it has not had recent activity. If you think this bug should stay open, please comment on the issue with further details. Thank you for your contributions.

wagnerand commented 5 years ago

I believe the frontend prevents this but I don't think the API does (unless something has changed since I filed this issue).

stale[bot] commented 4 years ago

This issue has been automatically marked as stale because it has not had recent activity. If you think this bug should stay open, please comment on the issue with further details. Thank you for your contributions.

diox commented 3 years ago

I was unable to find if it was just introduced by 33eba49767f8e3032eae7671ec8531d9d1b56859 or if it's older, but we do have a database constraint nowadays: https://github.com/mozilla/addons-server/blob/12301a60c69d301283a6e03a89e534bb17cc92e0/src/olympia/ratings/models.py#L142-L145

Since version and user are foreign keys, this does prevents you from submitting multiple reviews from the same version (while allowing multiple replies, for developers).

diox commented 1 year ago

That constraint mentioned above is insufficient: for most users, reply_to will be NULL, so they'd be considered different values for the database, so the constraint would let you create multiple ratings from the same user on a given version. It does prevent a developer from replying twice to the same review, but that's useless, there is another simpler constraint for this on reply_to alone.

What saves us is that for non-deleted ratings, the API has an additional check for this: https://github.com/mozilla/addons-server/blob/e0e28ebdccf53b231e1d4e340137d0130029ffaf/src/olympia/ratings/serializers.py#L227-L242

In addition, to guard against race conditions, we have throttling (1/minute): https://github.com/mozilla/addons-server/blob/e0e28ebdccf53b231e1d4e340137d0130029ffaf/src/olympia/ratings/views.py#L35-L43

Still, it's not ideal, we should make a conditional constraint on user, version instead, only applying it when deleted is 0 and reply_to is NULL.

Worth noting that in the past we've also discussed only allowing a single rating per addon per user, which is how the frontend implements this. But it's probably out of scope here.

bobsilverberg commented 1 year ago

Worth noting that in the past we've also discussed only allowing a single rating per addon per user, which is how the frontend implements this. But it's probably out of scope here.

That certainly makes sense to me. Of course a user's rating could change between versions, but users are able to edit their ratings so that shouldn't be an issue.

KevinMind commented 5 months ago

Old Jira Ticket: https://mozilla-hub.atlassian.net/browse/ADDSRV-207

mozilla / addons