Support for Drupal 7 is ending on 5 January 2025—it’s time to migrate to Drupal 10! Learn about the many benefits of Drupal 10 and find migration tools in our resource center.
It looks like comment_moderate() assumes that a user has only one role. This function figures out what score to assign to the user's vote and then logs the vote.
Comment | File | Size | Author |
---|---|---|---|
#2 | comment_roles_moderation.patch | 844 bytes | jhriggs |
Comments
Comment #1
moshe weitzman CreditAttribution: moshe weitzman commentedIn order to calculate the effective score of avote for a user with multiple roles, I would think you want to pick the vote with the greatest absolute value. For example, if I belong to three roles, then my single vote might be worth -5, -2, and +4 depending on which role is considered. I think the query should use -5 for my vote in this case, since that vote has the most 'effect', on the overall score.
Comment #2
jhriggs CreditAttribution: jhriggs commentedcomment_moderate() does not assume the user has only one role. If you look at the current query, it retrieves the maximum vote of the user's roles. As you note, though, this does not seem correct, because a user who has roles with values of -5, -2, and +4 will currently vote +4 even though the -5 would have the greatest effect. For a more realistic example, assume a user has -10 for a "bad" vote in admin role, -5 in moderator role, and -1 in authenticated user role. Currently, the -1 will get chosen due to the MAX() call.
The attached patch uses the value with the greatest absolute value. Thus, the "correct" -5 and -10, respectively, will be chosen for the examples above. The only questionable case -- which should be rare if it ever happens -- is if a user has roles with values of -x and +x for the same vote. The patch favors the postive vote in this case.
I have set this to critical just so that it makes it in before the 4.5 release.
Comment #3
Dries CreditAttribution: Dries commentedWhy couldn't you change MAX() to ABS()?
Comment #4
jhriggs CreditAttribution: jhriggs commentedThe value can be negative, no? We need the real value for the calculations, but the one with the highest absolute value is what we use.
Comment #5
ccourtne CreditAttribution: ccourtne commentedJust a select of both the modifier and the ABS of the modifier order by the ABS desc and take the first result. (I haven't looked at the code I'm assuming your doing one query for one user at a time.)
Comment #6
Dries CreditAttribution: Dries commentedLet me rephrase my question: why didn't you change the MAX() into ABS()? Right now, you made more changes to that query than just that. Either way, I'll test your patch tonight.
Comment #7
moshe weitzman CreditAttribution: moshe weitzman commentedYou can't just change MAX() to ABS in the original query. The number we need is 'value', not ABS(value). We *want* negative numbers to be returned. We need ABS(value) in the ORDER BY statement.
Comment #8
jhriggs CreditAttribution: jhriggs commentedThis is basically the reverse of what I am doing. I am selecting mid, value, and ABS(value), ordering by ABS. The values are then placed into an array with mid as the key, so the last occurrence (highest ABS) for a given mid is the value placed in the array, overriding any previous value. This way I don't have to be doing checks in the array to see if a given key already has a value.
Comment #9
Dries CreditAttribution: Dries commentedCommitted to HEAD.
Comment #10
(not verified) CreditAttribution: commented