Operation in userpoints_txn table not descriptive enough

I can't think of a valid reason why all would have insert, other than it being a bug that crept in.

Go ahead and roll a patch.

I can think of a valid reason ;)

This is what is actually happening. The Insert is the operation occurring (i.e. hook_nodeapi $op == insert). There is a hook called just before these "operation" are sent to the screen so its completely themeable to change this wording around.

The change from 2 to 3 introduced a few new fields. One of them was "entity type" which is "node, comment, etc." thus
"post blog" was redundant data stored in the DB.

moreover this was incredibly bad to store in the DB. For example what if you originally named your node "blog" but then changed it to "Thoughts". All of your points are now lost in relation to points received for the blog node type (as its now called "Thoughts"). In 3.x this is not lost because we simply track that it is a point for an entity type and we can join on the proper table to get the correct name.

Additionally what if you wanted to know how many points a user received for posting nodes or comments. In 2.x this was fairly difficult because you would first need to know the name of every node type in the system and do a query like
Select count(points) from userpoints_txn where operation = 'post blog' OR operation = 'post comment' OR operation = 'post node-type'
or you do the vulgar
SELECT count(points) from userpoints_txn where operation LIKE 'post %' AND operation != 'post comment'
etc. etc.

in short this is an intentional and good change (imho).

If there is any change it would to simply change the default display of this field on the report screen by using the available hook (which already provides a default) and providing a better default that translates "insert" into "New " or even the legacy "post " but I would highly, highly advise against reverting to the previous method.

let me be clear...

the operation in the userpoints_txn table is abundantly clear and extremely descriptive.... to a developer and this is exactly how it should stay.

the operation displayed to the user is obtuse and should be modified but you should NOT do this by modifying the DB. Simply utilize the hook (already provided) and modify the default wording.

hhmm.. ok. so why isn't this good enough.

Select n.type, up.operation, up.points FROM userpoints_txn up INNER JOIN node on userpoints_txn.entity_id = node.nid WHERE up.operation = 'insert' AND up.entity_type = 'node';

no need for a secondary table you have everything you need with a simple join.

i tried to find you on IRC but you weren't logged in, if you log in I can help you right now ;)

by the way with the above query I'd get something like

"blog" "insert" "5"

Which could be quickly interpreted as "insert blog" for 5 points.

Seems like enough information to me and keeps you future proofed if node types change names as well as providing an exact link to the item that produced the points (whereas in version 2.x this information was lost)

I'm changing this to a support request as I don't believe this is a bug. The wording has changed but I'm fairly steadfast that we have changed this for the better and for future proofing this module.

I agree that the wording is a little odd to the end users and it should be changed, but to a developer "insert" should be extremely intuitive as it is inline with the rest of Drupal core (i.e. nodeapi, hook_comment, hook_user, etc.)

So.. maybe we should roll a feature request in order to change the default wording of this. I think that this goes back to my original argument for wanting to separate the administrative interface from the API so that this distinction is made a bit more clear and allows for more flexibility like this. Feel free to provide feedback on my grandiose plan ;)
http://groups.drupal.org/node/10539

theoretically there is an uber-query that will pull all the information you need but it'd be really messy as well as hard to read. Unfortunately yes you'll have to do multiple queries based upon entity-type and that's exactly how I'd suggest doing it. Don't pull all from userpoints_txn and then do separate queries to the node table to get your information (that's a lot of selects).

Just pull by entity-type and JOIN on the correct table (i.e. entity-type == node JOIN on node, entity-type == comment JOIN on comment, etc.).

I know its a bit more of a pain but I promise up/down/left/right that this is a better model for data storage especially as it maintains somewhat more consist with Drupal core.

If I ever find the time I think the outcome of this would be
(1) Modify the report screen such that it doesn't display "insert" as I agree that it is confusing.
(2) Document the DB changes, in regards to userpoints_basic in the documentation.

If you have time for either feel free to tackle 'em. ;)

I've sounded off before about the use of "insert" as a descriptive term but I understand the reasoning behind it.

I just wanted to say that I /really/ like having the ability to click the "insert" hyperlink and do a quick check of the submitted content from the moderation table. That's a super-useful feature ... don't want to lose that one just because "insert" doesn't fit my fancy.

Thanks,
Kevin

I hear ya about the non-descriptive-ness about the "operation" and I agree that it should be changed. Heck I'd even support change it back to its former "post blog, post page", etc. which should be an easy frontpage switch without changing around the backend.

or you could just simply change the way you did it because you were doing it wrong in the first place.

(1) you can add a single field to the userpoints_txn table as a flag to track the status

(2) if you don't want to add a single field to the userpoints_txn table create a separate table with two field
userpoints_txn_id and status.
Join the two tables and find all the rows that have no status. Work on those (a LEFT join would work well here, depending on the order of the tables).

If you have these fields/table you can simply add a WHERE clause to pull items that haven't been processed.

Running through a table sequential is a really bad way of doing it because it assumes that everything works perfectly everytime.

I still stand by the decision to make this change to the table as it is a much, much cleaner interface and I can point to nearly the entire Drupal database as an example (specific example, term_data, term_node) or CCK or... We shouldn't store duplicate data that may get completely out of sync.

You can use the description field if you want I would add one myself simply because the core module itself defines the description field and its meant to be used by contrib modules (not sure which ones off the top of my head though).

Its your DB though so provided you know what's going in/out you can use it no problem.

I'd almost suggest using the reference field as I have no idea what that is still doing in the DB. A while back there was a discussion with a separate, to remain unnamed, developer that begged and pleaded to get that field into the "core" of userpoints so that his module would work properly. We urged him to use the description field but his argument won out and the reference field went in (it was actually a better decision) but that module has since been deprecated and abandoned so... this field is pretty much wasted (not that I hold a grudge or anything).

Why is processing rows in order bad?
because you're making assumptions, broad based assumptions.
What if you get a false-positive response from the client?
What if you want to recheck all the rows to ensure they actually did go correctly?
What if you accidentally made a mistake in your code and you started processing things in evens or odds or??
What if your keys aren't sequential? (i.e. deleted rows)

The list could go on but in short the answer is yes in a perfect world your code and processing things sequentially should never have any problems but the world and code isn't perfect. There is an argument that processing rows sequentially and tracking the last row number processed is faster due to the dramatically less number of update statements you'd have to do (i.e. 1 vs. 1 x # of rows) but that the amount of data your losing (i.e. which rows have been fully processed) is important. Especially if your on a system that reuses numbers (userpoints' table use autoincrement so it won't do that).
Anyhow if this were a programming class it'd be a negative point ;) but eh.. if it works whatever.. life isn't perfect ;)

I don't want to go too far down the code critique path on this b/c if its working for you then its working for you.

userpoints already has a description field/column as well as a reference field/column so I'm not sure what you'd be submitting a patch for.

What would you store in there?

I hate duplicating data and I think its a massive, massive mistake to essentially take steps backwards and store information such as "post blog" "post comment" etc in the userpoints table (for the reasons I already outlined). Especially since it simply encourages bad programming practices.

The description field is being used by some module, I don't know what it is off hand but I really don't care.

The reference field should be taken out; I'm fairly positive there is consensus on this.

Its not the duplication of data I don't like otherwise I'd yell at Drupal for storing the word "page" a bazillion times in the node table ;) its that....

1) It means nothing. There is no such thing as "post" in Drupal. no where, it doesn't exist. The operation is an insert.

2) Its not scalable to OTHER modules. userpoints_basic is the basic module that utilizes this field. I'm more concerned about other applications and keeping everything inline and consistent throughout.

3) It is not simply 'post blog' its also 'post page' 'post comment' 'modify blog' 'modify comment', etc. It is really, really messy extremely fast.

4) Performance.. put an index on it. sheesh, that's not hard to do. Most of Drupal isn't optimized out of the box. Currently we have an index on reference. Reference should be removed and the index placed on entity_type (most likely). I 100% agree with you.

I'm still steadfast in that I think its cleaner to keep this in line with the whole of Drupal. insert, update, etc. are in line with what hook_nodeapi, hook_comment, hook_user, etc. use and a programmer should recognize those immediately and not have to decipher what the module is doing.

I understand that this change broke your existing code but to fix your code its as simple as a quick JOIN statement. I really don't understand why this is such a big deal

ok I sounded off before really reading the full extent of the post; I suck.

leaving the operation field alone. OK agreed. Thanks ;)

Putting this data into the description field.. I'm still not terribly fond of the idea to be honest because its duplicating the node type names but I could get let that slide.

I'd get even more behind it IF
(1) the reports were modified to change the operation field from being shown to the description field. So we stop showing that crazy "insert" word to the users (but still make it a clickable link via the hook).

(2) possibly the description field was tweakable by the administrator, just like the point expiration description message is.

That I could see working and, you're right, we'd utilize that under-utilized description field.

So.. maybe being argumentative works ;) that is.. if I read ;)

@Erich

There are several approaches you can take that would preserve the data model, and achieve your design goals with the SOAP application.

Option 1: You keep last txn, but by type

Basically, you do keep the id of the last transaction processed for each type. So you can have a table with 'type' and 'last_txn_id', and as many rows in it as there are types.

The code would look like this:

foreach(array('node', 'user', 'comment') as $type) {
  $last_txn_id = yourapp_get_last_txn_id($type);
  db_query("SELECT t.txn_id, n.type, t.operation, t.points 
    FROM {userpoints_txn} t 
    INNER JOIN node n ON t.entity_id = n.nid 
    WHERE t.operation = 'insert' 
    AND   t.entity_type = 'node'
    AND   t.txn_id > %d", $txn_id);

  // Do your stuff here, including checking if SOAP is successful

  yourapp_put_last_txn_id($type, $txn_id);
}

This way, you process all the "node" types first, and have one variable for each type. You update the variable for each transaction that you successfully process.

Option 2: Maintain processing in sequence, and lookup the data for each row:

If you want it guaranteed that you are processing in transaction order, you lookup each up txn, fetch the operation, ...etc and then process it.

Like so:

$last_txn_id = yourapp_get_last_txn_id();

// Get number of txns to process
$num = db_result(db_query("SELECT COUNT(*) AS cnt FROM {userpoints_txn} WHERE txn_id > %d", $last_txn_id));
$num = min($num, 500); // Limit per cron run

$result = db_query("SELECT * FROM {userpoints_txn} WHERE txn_id > %d", $last_txn_id);
while($txn = db_fetch_object($result)) {
  switch($txn->operation) {
    case 'node':
      // Lookup the node, check the type, ...etc and then update SOAP
      break;
    case 'user':
      // Something, then update SOAP
      break;
    case 'comment':
      // Something, then update SOAP
      break;
  }
  // Update the last txn processed
  yourapp_pet_last_txn_id($txn->txn_id);  
}

If you don't like switch/case, like some people do, then you can do this:

while($txn = db_fetch_object($result)) {
  $function = 'yourapp_process_txn_' . $txn->operation;
  if (function_exists($function)) {
    $function($txn);
  }
  // Update the last txn processed
  yourapp_pet_last_txn_id($txn->txn_id);  
}

function yourapp_process_txn_node($txn) {
}

function yourapp_process_txn_user($txn) {
}

function yourapp_process_txn_comment($txn) {
}

How about those?

maybe I'm an antagonist ;) (kidding) but I just want to be clear.

I really don't like putting the "post comment, post blog, etc." in the description field but I would tolerate it with a clinched face smile but i would tolerate it. I just don't like the can-get-out-of-sync data model.

eh.. there are a lot of things in life that I don't like though ;)

I am not in favor of overloading the description field. Or rather, making its content site specific, module specific and structured.

You are now dependent on a text field storing discrete fields with special meaning, concatenated. You cannot query those fields independently (like the data field in the users table, you have to sift thru all the rows to find what you need, you cannot filter).

It also makes coexistence with other modules that do not use it, or use it in their own way difficult. You have to have your own fork of those modules.

It may well work for you, but in the long run these things tend to be a maintenance burden/hackish in nature.

Here is Option 3: Your own store and forward table:

Create your own "store and forward" table with exactly the fields you need, and a status field (sent/not sent). In points after, you create a row in that table, and when cron runs, you send and flag as you go (or even delete the row you sent). This way, you are not dependent on any overloading.

Ok so this is starting to break down and lets get back on track here.

-I searched through contrib and the following modules utilize the field as well as the string they store.
referral_points "Shared points"
userpoints_votingapi "Vote cast on node $id";
so the field is being used even if heavily underutilized.

-Description is used for arbitrary text but the argument is whether or not to store the node type in this field. Lets be 100% clear about this.

The question/decision to be made is

in userpoints_basic what is stored in the description field?
Should it include the node type as event did historically?
ex. "post blog", "update blog", "post story", "update story", etc.

That is the question at hand.
I do not support this because of the reasons I've mentioned before (out-of-sync) module and a simple JOIN grabs this data.

I DO support utilizing the description field and putting something more descriptive in there something like
"Content created"
"Content updated"
or something like that.

I support this because (a) the node type can be pulled easily (b) the reports already support hooks to modify what is displayed (ex. a link to the direct node being created).

in short you're correct we have a heavily underutilized field and we should use it. So now, what do we put in there?

in regards to the 'points after' hook and not having access in your other ticket I stated that I support sending the entire params over and I'd love to roll a version 4 in which we did this. This would make everyone's coding easier and make the module more extensible.

Sorry for spamming the participants in this issue. Due to the release of Drupal 7 and the lack of time from the maintainers, I'm closing all remaining 5.x issues for Userpoints.

Note that in 7.x, the operation column has been changed to become basically a machine-readable key for which modules can provide a translatable and user-facing description.