One last idea for this version. What do people think about adding two new fields to the DB?
eid int(11)
etype varchar(255)

eid is entity identification and would hold an int relating to something in the DB
etype allows you to identify what the heck eid is
for example
eid = 5
etype = comment
is comment with an id of 5

eid = 20
etype = node
is node # 20

The addition of these two fields would allow for hard linking of points to what caused the points so that you can not only track that a user received points for posting a node but you would know exactly which node it was. When coupled with the event field you can track points given on which state of the node.
Example
points = 10
event = 'node-moderate'
eid = 20
etype = node
AND
points = 5
event = 'node-create'
eid = 20
etype = node

Of course a lot of reports would have to be modified and SQL queries would have to be rewritten to take advantage of this information. I think the community around this module would see the value in this and would step forward to create these reports (hint, hint).

Adding these two items into the database and additionally into the API (as part of the acceptable items in the $params array) wouldn't take long and it would lay the foundation. Also this feature request is me volunteering to do the work ONLY FOR THE DB/API portion as I don't have the time to write the frontend reports (sorry).

Thoughts? Comments?

I should mention that this idea was stolen directly from the location module AND the voting API module ;) So its been done before and this method is consistent with other modules.

btw: This addition would help squash these bugs
http://drupal.org/node/183520
http://drupal.org/node/177447

Maybe we can get this developer to help code the frontend pieces (since this addition would help his code along)
http://drupal.org/node/169898

Comments

kbahey’s picture

kbahey commented

I support this idea in general, and that was the intent of events from the start. Many other modules use this scheme (flag content, voting api, ...etc.)

I think we need to come up with a set of etypes so we do not get garbage in it where everyone puts things in it.

Also what is the impact on modules that call userpoints now? Do they have to explicitly pass this info in, or can it be derived?

jredding’s picture

jredding commented

for modules it have to be explicitly given otherwise the API would have no way of telling an int of 5 to mean node 5, user 5, comment 5 or ???.

userpoints_basic would be an easy change as you have the two hooks for node and comments. In the nodeapi hook you have access to the node object so you know the id. The userpoints_userpointapi call would simply add
'eid' => $node->nid,
'etype' => 'node',
to the parameters array.

Same with the comment hook.

right now the etypes would be
node (userpoints_basic)
comment (userpoints_basic)
user (userreferal) (i.e. you received points for referring which user(s)).

Its possible someone would write something for posting a location too (in the case of multiple locations allowed per node) as well as other items (but I'm a bit pressed as to what right now

Do you think coming up with a list of etypes is good for just presenting a list for potential developers? or would this be something that is sanitized within the _userpointsapi()?

jredding’s picture

jredding commented
Title: Adding two new fields to the DB » Adding two new fields to the DB + modifying Events field

After a talk with Kbahey we decided to take this one step further and really shore up the DB implementation and API. We would really like feedback on this implementation.
Three changes would be made to the DB structure
eid (New Field) = Entity ID.. holds the nid, cid, uid of the object that caused the points
etype (New Field) = Entity Type.. (ex. node, comment, user) maps the eid to the DB record
event (existing field) --rename--> eop = Entity Operation (the action that caused the points, ex. published, moderated, etc.)

the use of eid and etype has some pretty strong use in the voting API and location modules (pretty tried and tested module).

the event field had an original intent of being a location to store events that could be used to sort out points by the action that caused them. Originally the intent was to have been "node" "comment", etc. Unfortunately that moved to being "post comment", "moderate comment", etc.

This change will MODIFY THE USE OF THE EVENT FIELD so that it now only contains the actual event (i.e. moderated, published, etc.)

As an example of what the changes would look like in the DB lets consider the following example
*User 5 publishes a node with an id of 10.
*The node must go through moderation (user 7 moderates)
*5 points are given for creating a node, 3 points for moderating a node
Current DB information
txn_id, uid, points, event
1 , 5 , 5 , "$node->type"
2 , 7 , 3 , "#node->type"

PROPOSED DB structure/information
txn_id, uid, points, eid, etype, eop
1 , 5 , 5 , 10, 'node', 'post'
2 , 7 , 3 , 10, 'node', 'moderate'

Another example.. This time with comments
As an example of what the changes would look like in the DB lets consider the following example
*User 5 publishes a comment with an id of 20
*The comment must go through moderation (user 7 moderates)
*5 points are given for creating a comment, 3 points for moderating a comment
Current DB information
txn_id, uid, points, event
1 , 5 , 5 , "post comment"
2 , 7 , 3 , "moderate comment"

PROPOSED DB structure/information
txn_id, uid, points, eid, etype, eop
1 , 5 , 5 , 20, 'comment', 'post'
2 , 7 , 3 , 20, 'comment', 'moderate'

Personally I think the new structure is much cleaner, allows for a tighter integration w/ Drupal and allows for highly customized reports. For example, how many points were granted for X node? or how many points did user X receive for posting vs. moderating.

One could also easily pull a point count for display on each node.

Finally one last HUGE benefit is that points can be recalculated retroactively for all the nodes (i.e. changing points awarded from 5 to 7).

I'll end this with the .install file can be coded in such as way that it renames the existing event field to eop and we can do an UPDATE to change the eop over to the correct setting for existing points. If we code it correctly this should be a seamless upgrade for existing points.

I'm proposing to do this before the stable release of version 3 (and is the last change before going stable with v3)

please comment! ;)

greggles’s picture

greggles
he/him
Primary language English
Location Denver, Colorado, USA
commented

I really really like this idea and see it as a pretty critical feature. The model is well proven and flexible. I agree with everything, but want to complain about the color of the bikeshed ;)

This module provides an API. APIs need to be intuitive to develoeprs who are unlikely to read the docs and more likely to just look at sample code and begin work. If we abbreviate things then it should be clear what the abbreviation means (txn_id) or we should not abbreviate them at all. I really prefer "entity_id" and "entity_type" instead of eid and etype. I could let those slide, but "eop" seems super confusing, but "entity_operation" or even just "operation" seems much more intuitive to me. None of those is going to push beyond the column naming limits of most databases (even with table prefixing) and while they do add some extra work when writing queries I feel that extra work is offset by an intuitive database structure and code that reduces the support requests "what is eop for?".

kbahey’s picture

kbahey commented

Hey greggles, thanks for chiming in. We will build the bike shed, then you can color it fluorescent pink, with green spots if you wish.

I mentioned to jredding that the names should be:

event_id (nid, uid, cid, ...etc.)
event_type (node, user, comment, ...etc.)
event_op (this is the existing event column, renamed via an ALTER TABLE)

So, greggles independently agrees with this, which is good news.

The downsides are:
- The migration path (an _update_x hook should take care of the column rename though)
- Modules that have not been ported to work with 3.x will be removed from the tarball, until they are ported to 3.x by someone, then we include them. This will cause a lot less grief in support.
- The API now requires 3 new parameters to be passed in the array. Prior to that, it only required the 'points' => 23 and deduced everything else. Not sure if we can be minimalists with this anymore. We can default eventy_type to 'node', but we still have to have an event_id and event_op. Or maybe make all these optional? Then their value is diluted.

Thoughts?

amitaibu’s picture

amitaibu
Primary language Hebrew
Location Israel
commented

How about integrating it with the Workflow-ng module, so events and actions can be done from there?

kbahey’s picture

kbahey commented

Amitai

This issue is not about integration with workflow-ng or Actions, ...etc.

It is about two fields for the API and renaming an existing one.

The API allows you to write modules that integrate X or Y with userpoints, just like invite, user referral, and many others do. In other words, integration with other modules does not belong in the API itself, but using the API, you can do that.

jredding’s picture

jredding commented

I don't think that we need to make entity_op, entity_type mandatory as some people may be giving points on items that are entities. For example Arbitrary points given by an administrator don't have an entity_type or entity_id (i.e. there is no node to follow).

I think allowing userpoints_userpointsapi(5) is still a good idea as it making coding easier even if at the expensive of leaving a few DB columns blank. Of course we can also default event_operation to something to.

Which brings me to naming
eid, and etype.. should be named
entity_id
entity_type
because that does make sense and its easy to read. I think it should be entity versus event as that it what were really storing here a pointer to an entity in the database (node, comment, user, etc.)

I *think* we're all in agreement on the above. This brings me to the last column currently named "event"
This column currently holds values such as
"post comment"
"moderate comment"
OR $node->type
and now we're moving the word comment and the word node over to the entity_type field we are only storing what actually happened (i.e. post, moderate, delete, etc.)

I'm kinda curious if we need to change the wording at all? Why not just leave it named "event", it is an intuitive name.

Then what we're doing is just an UPDATE to remove the word comment out and remove reference to $node->type (over to just simply node in the entity_type column).

Thoughts?

greggles’s picture

greggles
he/him
Primary language English
Location Denver, Colorado, USA
commented

The only reason I know to change from 'event' to something else would be a "loud error" instead of a "quiet" one.

i.e. if I have a report that pulls data from this table and presents information from the event column, then if it stays named "event" I might just assume my report still works. But if I get a sql error I'll see the new name and know that something changed. Same thing for inserting records (which nothing should be doing, but maybe there is such a thing).

I _really_ don't have a strong feeling about this though. I think it's a minor point.

jredding’s picture

jredding commented

OK makes sense on changing the event name and I agree. What about just using "op"?
My reasoning
op = operation
$op is used in hooks (i.e. its name is used throughout Drupal)
op != only entities

op would be used for a lot of things as one is not always receiving points for nodes, comments, users, etc (i.e. entity items). For example you can receive arbitrary points from the administrator ($op=admin).

Using the name operation would accomplish the same goal I just figured op would be similar to the $op used in hooks.

Yes this is petty but hey before I code might as well get some input right? I think this is the last issue to work out before the changes are made.

vaelen’s picture

vaelen commented

This all sounds good to me. I'm calculating points by organic group, and this change would let me calculate the totals on the fly with a query like this:

select p.uid, g.group_nid, p.points from userpoints_txn p left join node n on n.nid = p.event_id left join og_ancestry g on g.nid = n.nid where p.entity_type = 'node' and p.status = 0

Whereas right now it would be impossible to gather this information without hooking into the userpoints api and keeping the data in a separate table.

jredding’s picture

jredding commented
Status: Active » Fixed

This has been implemented in v3.
entity_id is a NEW field meant to store the $node->id, $comment->cid or $user->uid
entity_type is a NEW field meant to store text to map the id to an database entity. ex. node, comment, user
event has been renamed to operation.

userpoints_basic has been updated to include the information

userpoints.module reports have been updated to link out to the node or comment that caused the node (click on the operation)

Anonymous’s picture

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

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