Closed Bug 296541 Opened 19 years ago Closed 19 years ago

ratings not updated after flagged comment removed

Categories

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

defect
Not set
normal

Tracking

(Not tracked)

RESOLVED FIXED

People

(Reporter: myk, Assigned: cso)

Details

(Whiteboard: [landme] WHEN CHECKING IN SEE COMMENT 4! Also, land with bug 297724)

Attachments

(2 files, 5 obsolete files)

After a flagged comment is removed, the overall application rating is not
recomputed, as it is after a comment is added.  Thus it remains incorrect until
someone adds a new comment.

The rating should be recomputed after a comment is removed just as it is
recomputed after a comment is added.
See f.e. Forumzilla (as of 2005 June 3), which had two comments, one with five
stars, and one with one star, until the latter comment was removed:

https://addons.mozilla.org/extensions/moreinfo.php?application=thunderbird&id=133&&page=comments

The overall rating for the extension has remained at three stars (1+5/2) even
though it should now be at five stars (5/1).
I don't know much about PHP,
I'm not so skilled in update wizardry.
But I do know, that this seems true,
And I know that if you give review,
Then what a wonderful world this will be.
Assignee: Bugzilla-alanjstrBugs → myk
Status: NEW → ASSIGNED
Attachment #185278 - Flags: first-review?(g.maone)
Attachment #185286 - Flags: first-review?(g.maone)
Attachment #185278 - Flags: first-review?(g.maone) → first-review+
I've scanned the code for possible problems: there's an appearance of dangerous
usage of $id (used as a temp var in patch code, and as request var in the
original code), but $id is assigned explicetely to $_GET['id'] *after* patch, so
there's no real danger: only the usual UMO mess ;-)
Comment on attachment 185278 [details] [diff] [review]
patch v1: factors out recomputing code

wrong source code
Attachment #185278 - Attachment is obsolete: true
Attachment #185278 - Flags: first-review+ → first-review-
This actually seems to be quite a problem...

Running Myk's function on the entire database dump at 2000 GMT today produces
these results:

Total Changed: 262
Total Checked: 715
Percentage: 36.6%

Some of these are actually going down, a few go up.

Adding kveton to the CC list to see if we should take this patch...
Attached patch Patch v3 (obsolete) — Splinter Review
This patch follows on from discussions on the umo-dev mailing list when I did
more investigation earlier on... It is essentially the same as Myk's patch.

The reason the values in my previous comment is so high is that the average
rating is based over the last 30 days (stored in the database in this way) but
it only updates on a new comment, so for a lot of extensions it will be wrong.

We discussed it on the mailing list and agreement seemed to be that we should
switch to using AVG() instead, which isn't thought to be a big database hit.

*note*

For this patch to work you need to run the following on the database:

ALTER TABLE `feedback` CHANGE `CommentVote` `CommentVote` TINYINT UNSIGNED
DEFAULT NULL;

UPDATE feedback SET `CommentVote` = `CommentVote` -1;

(The second is due to the conversion from ENUM which makes 0 = 1 and 5 = 6, ask
morgamic for more details)

I'll also attach my script that I've used to mass update the ratings etc. from
the old values.
Attachment #186157 - Flags: first-review?(cst)
Attached file Script (obsolete) —
Script used to update the entire database.
Attachment #186158 - Attachment mime type: text/x-delimtext → text/plain
Comment on attachment 185286 [details] [diff] [review]
same song, different dance--err, branch

Obsoleting patch (and clearing review request) based on mailing list discussion
- although most of Myk's patch has actually made it to the other patch.
Attachment #185286 - Attachment is obsolete: true
Attachment #185286 - Flags: first-review?(g.maone)
Comment on attachment 185286 [details] [diff] [review]
same song, different dance--err, branch

Obsoleting patch (and clearing review request) based on mailing list discussion
- although most of Myk's patch has actually made it to the other patch.
Comment on attachment 186157 [details] [diff] [review]
Patch v3

Marking patch as obsolete, and r- because there needs to be more done in
developers/commentsmanager.php - there are two code paths to delete comments
and this only catches one.
Attachment #186157 - Attachment is obsolete: true
Attachment #186157 - Flags: first-review?(cst) → first-review-
Attached patch Patch v4 (obsolete) — Splinter Review
Fixes the duplicate code issue in commentsmanager... as much as it pains me to
leave it like that, I don't fancy rewriting the whole file at this time.
Attachment #186305 - Flags: first-review?(cst)
Attachment #186305 - Flags: first-review?(cst) → first-review-
Attached patch Patch v5Splinter Review
Woops, a local hack to make testing easier creeped in there.
Assignee: myk → colin.ogilvie
Attachment #186305 - Attachment is obsolete: true
Attachment #186306 - Flags: first-review?(cst)
Attachment #186306 - Flags: first-review?(cst) → first-review+
Whiteboard: [landme] WHEN CHECKING IN SEE COMMENT 4! Also, land with bug 297724
Checking in core/inc_global.php;
/cvsroot/mozilla/webtools/update/core/inc_global.php,v  <--  inc_global.php
new revision: 1.12.2.9; previous revision: 1.12.2.8
done
Checking in core/postfeedback.php;
/cvsroot/mozilla/webtools/update/core/postfeedback.php,v  <--  postfeedback.php
new revision: 1.6.2.6; previous revision: 1.6.2.5
done
Checking in developers/commentsmanager.php;
/cvsroot/mozilla/webtools/update/developers/commentsmanager.php,v  <-- 
commentsmanager.php
new revision: 1.4.2.3; previous revision: 1.4.2.2
done
Status: ASSIGNED → RESOLVED
Closed: 19 years ago
Resolution: --- → FIXED
Attached file Script v2
Uploading for justdave.
Attachment #186158 - Attachment is obsolete: true
AMO BUGSPAM FOR COMPONENT MOVE AND DELETE (FILTER ME)
Component: Listings → Web Site
AMO BUGSPAM FOR QACONTACT FIX (FILTER ME)
QA Contact: listings → web-ui
Product: addons.mozilla.org → addons.mozilla.org Graveyard
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: