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.

CommentFileSizeAuthor
#2 comment_roles_moderation.patch844 bytesjhriggs
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

moshe weitzman’s picture

In 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.

jhriggs’s picture

Priority: Normal » Critical
FileSize
844 bytes

comment_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.

Dries’s picture

Why couldn't you change MAX() to ABS()?

jhriggs’s picture

The 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.

ccourtne’s picture

Just 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.)

Dries’s picture

Let 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.

moshe weitzman’s picture

You 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.

jhriggs’s picture

Just 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.)

This 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.

Dries’s picture

Committed to HEAD.

Anonymous’s picture