Hi, just provided a patch to also keep timestamp if passed in params, some functionality to be finished within the userpoints module. The userpoints API ignores it and sets timestamp as current time(), so there's no way to set points in the past or in the future, even if there's a time field in the userpoints "adjust" form.

from userpoints.module (line 1227)

..
      $params = array(
        'uid' => $form_state['values']['txn_uid'],
        'approver_id' => $form_state['values']['approver_uid'],
        'points' => $form_state['values']['points'],
        'time_stamp' => strtotime($form_state['values']['time_stamp']),
..

file userpoints.module, (line 1709)
and the userpoints transaction api ignores that :

   ...
   $ret = db_query("INSERT INTO {userpoints_txn}
      (uid, points, time_stamp, status, operation, description, 
      reference, expirydate, expired, parent_txn_id, tid, entity_id, entity_type)
      VALUES (%d, %d, %d, %d, '%s', '%s', '%s', %d, %d, %d, %d, %d, '%s')",
      $params['uid'],
      $params['points'],
      time(),
      $params['status'],
  ..

The patch just changes the "time()" line with

(isset($params['time_stamp'])) ? $params['time_stamp'] : time(),

The "update" part will work fine, just because the self building array based on $params would include the 'time_stamp' part.
The cache functions are left as they are, so point cache updating time will remain current time().

CommentFileSizeAuthor
#29 userpoints_timestamp_single_line_change.patch1.2 KBilo
#26 userpoints_timestamp_api_4.patch7.47 KBilo
#21 userpoints_timestamp_api_3.patch6.08 KBilo
#5 userpoints_timestamp_api_2.patch4.53 KBilo
#1 userpoints_timestamp_api_1.patch2.18 KBilo
userpoints_timestamp_api.patch1.11 KBilo

Comments

ilo’s picture

ilo commented
StatusFileSize
new userpoints_timestamp_api_1.patch2.18 KB

mm.. I forgot to include the form's _submit modification, to pass time_stamp to the userpoints API. Updated patch.

jredding’s picture

jredding commented

I'll review this by this evening.

jredding’s picture

jredding commented
Category: bug » feature

The timestamp records when the transaction occurred and this allows an API call to bypass this. I'm not a big fan of this as it allows transactions to be post-dated. Moreover transactions accidentally posted in the future are a bit odd in terms of reporting and managing a transaction.

As people are using this module to record points that are used for redeeming goods and services this can potentially open up a hole that I'm not comfortable with.

Can we have some discussion on what this means before we just commit this?

ilo’s picture

ilo commented

Transaction accidentaly posting in the future (when occur) should not be a problem for reporting, they are a functionality problem.

On the other hand, what kind of API should allow me to set almost ALL parameters of an entry but just one, and that one I can change on update, but not on create? quite flexible I guess..

None of the solutions I've found are clear for me.. I mean:

1. set all params
2. hook before points are started (don't know tid yet, in fact, don't know nothing so I have to start tricking to know which timestamp should I apply to wich transactions, and this is a different piece of code).
3. call userpoints api

or

1. set all params
2. call the userpointsapi and create the transaction (get transaction id)
3. add time_stamp and tid to the params
4. again call the userpoints for the transaction to be modified

If this is the problem (administrator setting points in the future), then add the validate part in the 'adjust' form, to avoid error point award timestamps. This way posts can be adjusted according to the 'adjust form', otherwise there's no reason for doing this in the "points update" and not in the "points submission", so the patch should be written to REMOVE the timestamp field in the adjust form

In fact, even if this patch is not commited never, there's code in the userpoints module for changing a transaction timestamp (just update a transaction ID) that can be set to future, so If you don't feel confortable with this, then you should consider reviewing the module's current code.

So.. fine for me, let's have a discussion about this topic.

Note: The "add points" form includes a field for the timestamp with the following description: "Date and time of this transaction, in the form YYYY-MM-DD HH:MM +ZZZZ", and when I set a date there it's 'unfortunately' not considered. Did you change from Bug report to Feature request by accident? I'm not requesting a new feature, I'm fixing a current one..

ilo’s picture

ilo commented
StatusFileSize
new userpoints_timestamp_api_2.patch4.53 KB

I've updated the patch to a better approach for both.

I still think the API should be flexible enough to NOT have a hardcoded value as timestamp, so I keep the previous changes in the patch to let the API work with past timestamps on newly created transactions, but this operation is disabled by default. I've included a configuration setting to allow timestamp to be set using the transaction admin form, and modified the time_stamp field of that form (transaction admin) to reflect the configuration option, being the time_stamp field disabled if the time_stamp should be the current time() (not manually defined).

This makes sense from all povs for me. What do you think? could you give it a review again, please?

NickSI’s picture

NickSI commented

Patch #5 works for me. I second ilo in his proposal. There are cases when you need transactions with timestamp in the past.

jredding’s picture

jredding commented

I see where you are going with this but I find the original patch a bit easier. If we're going to allow this then I don't see why we need a configuration setting for it. All of what we're talking about is done programatically and a site administrator should understand the code being used on the site.

My question is whether or not we made a mistake in the first place by allowing the timestamps to be adjusted via the adjust/edit form. If that is not a mistake and we see no problems with allowing this behavior then I'm all for the initial patch.

My worry crops up with those using this for financial-ish applications.

What if we had a way to flag that a transaction was postdated? (something we don't have now).

ilo’s picture

ilo commented

I see where you are going with this but I find the original patch a bit easier.

Needs to be tweaked a little, I forgot a debug line, and the flow could be defined better, I guess.

My worry crops up with those using this for financial-ish applications.
What if we had a way to flag that a transaction was postdated? (something we don't have now).

I would agree if you have also a way to ensure the local time of the system never gets shifted, and imho this is not being considered at all.

In any case, in the 'hipotetical' case (of a postdated transaction) in a specific setup (using userpoints for financial-ish applications), if a "user" or "error" are able to change the timestamp, this could also happend with the uid for the transaction (something we allready have now).

IMHO, the more flexible the api the better, and specific applications should consider their own use cases. Should and hipotetical case of a specific configuration state the features available in a api, then I understand you could consider the programatically-changed timestamp as a design 'mistake'.

I'm not being objective in this issue, as I would like to have this functionality working. Can anyone using this module regularly expose other opinion? this could help a lot, I don't want to change the module's design just because of a single setup.

Thanks for your attention, Jacob

jredding’s picture

jredding commented

I'm all for flexibility most definitely but it does make me wonder if we made a mistake in the first place by allowing this functionality.

I could be overly cautious here but if we look at Drupal's node system we have two timestamps; created and changed. The Created timestamp is what is displayed on the website and can be modified but the changed timestamp is set to time() and can not be changed through. In node_save here is the relevant code

  // Set some required fields:
  if (empty($node->created)) {
    $node->created = time();
  }
  // The changed timestamp is always updated for bookkeeping purposes (revisions, searching, ...)
  $node->changed = time();

thus $node->created can be overridden but $node->change can not providing this fallback security I'm looking for.

Then if we look at the comment table we don't have this same "protection" you can change the timestamp and none will be the wiser.

Sometimes we don't see our mistakes until they are pointed out to us ;) (i.e. maybe we made a mistake in the first place)

On the table we have two solutions
1) Make the API flexible by allowing the timestamp to be modified via the API call. At the moment this is only possible with an UPDATE and not an INSERT.

2) Make the API flexible by allowing the timestamp to be modified via the API call. Create a second timestamp field in the database that maintains time() so that point grant time and actual time are tracked to find in possible discrepancies.

I fear that #2 may be overly cautious but since people are now starting to use this for financial items as well as the fact that the API contains expiration code that back-dating entries could throw for a loop I'm erroring on the side of caution and thinking that by adding a feature we've found an existing bug.

ilo’s picture

ilo commented

I totally agree about the 2nd solution, keeping the tracking (changed) timestamp while still using the created timestamp for screening/using purposes, and this is clearly a feature request now. But this new feature would allow to read and write points from one site to other without the need of a table dump, keeping current user's points untouched and creating points in a batch process using "job's" timestamp (this is my case)..

Even if you can still have some tracking information using the txn id, this (the first solution, it's, the bug report) is not friendly at all, so I preffer going the way of the 2nd option if the feature is accepted. I can start working on it if you want, providing a new patch to be reviewed. Just let me know.

jredding’s picture

jredding commented

kbahey would have the final call on this but I think this is a good addition to the module as it provides more flexibility as well as accountability. I'm a big +1

Thanks for working through this with me as well as providing the patch, i highly appreciate your cooperation

kbahey’s picture

kbahey commented

I would go for option 2, provided that it has an _update_xxx() function, so users get a seamless migration path, and we reduce the pain of upgrading. I don't think this warrants a 6.x-2.x. We can keep it 1.x and have the update function.

Overall, I think we need to step back and look at how complex things are getting for the API and for the admin/settings/userpoints. I installed it today on a 5.x site, and only had userpoints, userpoints_basic and voting API enabled, and nothing else, and the user interface is already too cluttered and confusing. 6.x would be the same.

So, I think we should be driving from now towards simplification, better API documentation, better user interface, and removing the 2.x compatibility.

jredding’s picture

jredding commented

I fully agree on the greater simplification strategy. I do think we have create a nice and robust API but the user interface has suffered because of it. In this case the change made is all underlying thus will not change the user interface.

I fully agree on removing the 2.x compatibility. That checkbox in the admin area with a huge disclaimer really bugs me.

Should we leave the 2.x compatibility in at the moment and when an upgrade to 7.x comes along just kill it at that point? That seems like plenty of time.

jredding’s picture

jredding commented

also agreed in that it should go straight into 6.1 and not create a new branch for this.

kbahey’s picture

kbahey commented

Yes, I guess it can wait till 7.x

ilo’s picture

ilo commented

Having the create timestamp as a feature, tt's not clearly how points posted in the future should be handled by the userpointsapi. In fact, now, api is not aware of the timming but just the current time.

I would also suggest (I'll create another issue) this two functions to be 'upgraded', supporting the $params schema as the userpointsapi does:

userpoints_get_current_points($uid = NULL, $tid = NULL);
userpoints_get_max_points($uid = NULL, $tid = NULL);

to

userpoints_get_points($uid = NULL, $params = NULL) {

keeping bw compat (untill 7.x) with some logic checking like is_null and is_array.

This way, getting current points can now considering a $param['timestamp'] defaulted to current time(), to NOT consider the postdated transactions.

What should be included in this issue, I guess, is that the cache part would not work, every added point is included in the points and max_points columns of the userpoints table, so future transactions will be included at current time() when reading user's points, and this should be also fixed when allowing points in the future.. (not necessary if points could NOT be granted in the future). Comments?

jredding’s picture

jredding commented

aaahhh.. we're making this entirely to complex. These functions can and should remain the same. I say leave it as is and highly discourage posting future transactions. A part of me whats to saysay that the API would outright deny posting transactions in the future as any valid financial system would do but we could just not care the same way that the node system doesn't care. The discrepancies are then up to the administrator to figure out the what heck happened.

If someone has a use case for posting transactions in the future I would suggest that they create a separate module (and table) that queues up transactions and posts them as they come due.

Going down this road is going to make the code extremely convoluted and complex as well as the use of the API. The goal here is to keep the API usage simple.

Past = OK
Future = bad

ilo’s picture

ilo commented

Why this is the third time in this conversation that appeared the 'financial'-ish term? why again is this use case stating what the api should or should not do? Is this kind of application the module evolution?

I would say, for me, just limiting past transaction is right, but I'm not sure why you see this so 'complex'.

About the two api functions, I don't want to remove then, but just to upgrade (creating a new one) to something more flexible following the userpointsapi idea (the params array), so other modules can get user points with more flexbility (this would me a must for financial-ish applications also :) ). Including something like user_get_points ($params), and having this api defaulted to current user, all tid and current timestamp is more natural, and could also be modified by developers to get points in a time-window, or up to a single date or whatever. But again, this should be another issue (even if related to the time_stamp behaviour).

jredding’s picture

jredding commented

The reason for the "financial-ish" is because it is the reason why I am using it. Our application manages budgets of points that are used in a manner similar to cash so I'm sensitive to it being screwed up. It is also the motivation as to way we maintain every transaction so instead of just deleting a row we post a reversing entry.

I understand you don't want to remove the functions but rather modify them such that they handle dates differently. This is a significant change.

Unless I am mistaken what you are proposing is that if a transaction is posted with a future date then it is not showed in the reports, not counted in the userpoints_get_current_points or reflected in the caching/summary table. If this is the case then we have to modify the transaction function as well as all other functions that pull data from the table (to check for dates

Personally I don't see the value add in this.

ilo’s picture

ilo commented

ok, now I understand the fixation about the financial-ish! :)

In my case I don't need future transactions, in fact, I don't use legacy past transactions. I'm just resolving pending events when a user is loaded, and assign points for each one, the key is that points should be granted on it's event timestamp, but even if programatically and scheduled events, point granting should be happend in the past once the event is resolved. So in reallity, from the user pov, once the user is loaded and all events have been resolved, it's current time().

about the api modification, I've moved to: #403330: Provide a more flexible api to query about point status

ilo’s picture

ilo commented
StatusFileSize
new userpoints_timestamp_api_3.patch6.08 KB

ok, a new version of the patch with the suggested updates..

This includes:

- add the 'changed' field in userpoints_txn with same attributes as 'time_stamp'
- Optional: creates a index for 'changed'
- Optional: updates the 'changed' to point to 'time_stamp' on any existing transaction entry

- handles the existence and coherence of $params['time_stamp'] and forces to be 0 >= time_stamp <= time()
- forces 'created' to be time()

and some drupal friendly helpers:

- use of drupal_write_record for insert and update transactions

I tried to stick as more coder style as I could.

jredding’s picture

jredding commented

Just one nitpicky thing

A new field was added and named "changed" but this is always set to current time...
+ //Always force changed timestamp to current time() for transaction tracking
+ $params['changed'] = $time;

I think it would be better to call this field created; which is inline with the node table.

Ideally we'd rename time_stamp to changed too
We're then have created and changed timestamp fields that would consisten with the node table.

ilo’s picture

ilo commented

But that's not true at all Jacob. When you edit a transaction, the time_stamp (renamed to changed) would be the txn virtual creation timestamp, and the 'changed' (renamed to created) would be the 'update' timestamp, having just their meanings switched..

jredding’s picture

jredding commented

oh.. see this is why I shouldn't write comments at 6am. Of course changed would modify with time().
you're right

What threw me for a loop was seeing that changed was always being modified. Ideally what we want is the node model. time_stamped (i.e. created) is set when the points are initially granted then changed can be modified with every other change. This way you see when the points were gained and then when (and thus if) there was a change to them.

Thanks for bearing with me on this one, I really like the change we're adding here just making sure we get it just right. I appreciate your patience and your code.

ilo’s picture

ilo commented

haha.. it's the reason that lead me handle all the issues in the morning :)

Thanks for your reviews, I hope we both could get the module working with this change.

ilo’s picture

ilo commented
StatusFileSize
new userpoints_timestamp_api_4.patch7.47 KB

Rerolled again for current dev release. I need this patch working for testing purposes with the userpoints_history module development: http://groups.drupal.org/node/19947. The project http://www.drupal.org/project/userpoints_history includes a helper to generate dummy point transactions in time.

kbahey’s picture

kbahey commented
Status: Needs review » Fixed

Committed.

Should be in the tarball about 5 hours from now.

ilo’s picture

ilo commented

Thanks Khalid! pretty quick, I had included code to check if the patch was applied in the module jeje :)

ilo’s picture

ilo commented
Priority: Normal » Critical
Status: Fixed » Reviewed & tested by the community
StatusFileSize
new userpoints_timestamp_single_line_change.patch1.2 KB

Khalid, I just missed a last minute change I did not reverted during the tests.. Sorry for the inconvenience. This was a very ugly mistake..

-  if (variable_get(USERPOINTS_TRANSACTION_TIMESTAMP, 0) || !isset($params['time_stamp'])...
+  if (variable_get(USERPOINTS_TRANSACTION_TIMESTAMP, 1) || !isset($params['time_stamp'])...

The patch is for current 6--1 branch, it's previous patch commited

kbahey’s picture

kbahey commented
Status: Reviewed & tested by the community » Fixed

Committed.

Status: Fixed » Closed (fixed)

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