As you've said several times in code and the README files, special care must be taken to make sure one doesn't delete user roles that are not intended for use as user karma roles.

SUPER-WARNING: don't let this module add/delete roles other than the ones you have created SPECIFICALLY for this module.

* This is where the actual mass-recalculation takes place.
* First of all, ALL of the roles managed by the module are wiped out.
* This is why it's CRUCIAL that only specific karma-roles are managed here.

I see sprinkled through the code DELETE queries on the {users_roles} table and it scares me a bit that without more sandboxing of the roles managed by user karma there could be roles deleted that are important!

Additionally, the user karma admin settings page chooses and arbitrary number (7) of roles that -could- be managed which potentially extraneous forms in the UI and extra variables created and stored regardless of usage.

What I propose is that the admin page first allow the selection of roles that are -eligible- to be managed by user karma. Only then will there will be forms in the UI shown for managing the user karma settings for that role. This will make sure that any of the DELETE calls to the database are always constrained to roles preselected to be specific to user karma.

I already have this working in my dev environment and would be happy to work with you on this.

Also I did have one question from the project page. You mention that this module requires MySQL 5. I was not able to see anything in the module that specifically would not work in MySQL 4, at least not 4.1 or higher.

I think there are one or two places where the code would not work in PostgreSQL which I can help with as well.

CommentFileSizeAuthor
#4 user_karma.module-211074.patch18.96 KBjaydub
#2 user-karma-admin.jpg74.39 KBjaydub

Comments

mercmobily’s picture

mercmobily commented

Hi,

OK, three issues in one...

#1 [role sandboxings]
I agree with you, some sandboxing would be _fantastic_. I will welcome and embrace any code which supports this.

#2 [better configuration]
This is one of those areas where I was actually quite lazy. I needed a few roles, so well, I did it the lazy way. Now... I will welcome and support any patches about this

#3 [mysql]
I am not entirely sure about this. This is the incriminating function:

 
function user_karma_user_rank($uid) {

  // If there is no karma stored, then the comparison is made with "0".
  // Otherwise, the comparison is made with a sub-query.
  // THIS IS BECAUSE ANY INTEGER COMPARED TO NULL IS 0, WHICH WON'T WORK
  // IN TERMS OF RANKING
  $r = db_result(db_query("SELECT COUNT(*) FROM {votingapi_cache} vc2 WHERE vc2.content_type = 'user' AND vc2.value_type='karma_points' AND vc2.tag='karma' AND vc2.function='sum' AND vc2.content_id=%d", $uid));
  if ($r == 0) {
    $extra_bit = "0";
  }
  else {
    $extra_bit = "(SELECT value FROM {votingapi_cache} vc2 WHERE vc2.content_type = 'user' AND vc2.value_type='karma_points' AND vc2.tag='karma' AND vc2.function='sum' AND vc2.content_id=%d)";
  }

  // Query curtesy of http://arjen-lentz.livejournal.com/55083.html and 
  // http://arjen-lentz.livejournal.com/56292.html - EVERYBODY should 
  // read those pages!
    $r = db_result(db_query("SELECT COUNT(*)+1 AS ranking FROM {votingapi_cache} vc LEFT JOIN {users} u ON u.uid =  vc.content_id WHERE vc.content_type = 'user' AND  vc.value_type='karma_points' AND vc.tag='karma' AND vc.function='sum' AND vc.value > $extra_bit", $uid));

  return $r;
}

This creates a nested query (necessary to work out the ranking without using ugly temp tables) and some users complained that MySql 4.X didn't swallow it very well. However, I couldn't work out WHICH MySql had a problem with it... If you do, please let me know!

BUT, if you have ideas, and -- more importantly -- can send me patches about improving the queries and how compatible they are, I AM HERE! :-D

Thanks a lot,

Merc.

jaydub’s picture

jaydub commented
StatusFileSize
new user-karma-admin.jpg74.39 KB

I've attached a screenshot of the user_karma admin settings with the role sandboxing I mentioned. I didn't end up with as much time to work on this today as I had hoped so it will be a few more days before I can send you a patch with this and a few other issues (the wife gets my time on the weekends lately).

mercmobily’s picture

mercmobily commented

HI,

I look forward to receiving the patch! No stress, the module still works :-D

Bye,

Merc.

jaydub’s picture

jaydub commented
Status: Active » Needs review
StatusFileSize
new user_karma.module-211074.patch18.96 KB

Ok here's my patch.

Changes:

  • deprecate use of MAX_ROLES constant
  • Added list of roles on system and allows admin to select roles that are affected by user_karma
  • For roles that are affected by user_karma, the configuration for that karma role has it's own settings fieldset.
  • Changed the user_karma_sql_duration function to return an SQL-portable string for timestamp comparisons (previous version uses FROM_UNIXTIME and CURRENT_TIMESTAMP which are not totally portable - FROM_UNIXTIME at least does not exist in PostgreSQL).

As for the MySQL query with the subquery, well I am using 4.1.x and it works for me so I think you're safe except for the .001% of people running 4.0.x or lower.

mercmobily’s picture

mercmobily commented

Hi,

Patch applied.
I also saw the several code improvements you introduced... Thank you so much.

I just wanted to let you know that I can actually program... it's just that when I create the first version fo a program, I avoid ANY optimisation of ANY sort to make sure that it actually works. Normally, I go through a module later and optimise it...

One issue with this great patch is that for each role, you can only have a particular range. I am sure this is actually fine. Let's see if anybody ever complains... (I doubt it).

Anyhow: I am releasing the new version of the module, with copious amounts of thanks to jaydub which marks the FIRST help I have ever received in terms of actual extra features.

Bye!

Merc.

mercmobily’s picture

mercmobily commented
Status: Needs review » Fixed
mercmobily’s picture

mercmobily commented

Hi Jay,

Please test this a little bit more. My preliminary tests look totally OK. However, I'd like some extra confirmation before rolling this out to FSDaily ( http://www.fsdaily.com ) which is a huge news hub...

Bye,

Merc.

Anonymous’s picture

Anonymous (not verified) commented
Status: Fixed » Closed (fixed)

Automatically closed -- issue fixed for two weeks with no activity.