Making transactions read-only after approved/declined + Make it easy to create reverting/adjusting transactions

The thing is that I can't guarantee that changing transactions will correctly update the total.

I tried pretty hard in 7.x-1.x and it's working fairly well, but stuff like changing the status and the category of a transaction at the same time can get very complicated to account for and is hard to maintain. In 6.x-1.x, it just doesn't work at all. Changing a transaction after it has been approved doesn't have an effect on the total at all.

Userpoints is quite often used as a virtual currency (And will likely be used a lot more for this, as I'm currently working hard on http://drupal.org/project/commerce_userpoints to get it where I'd like it to), where it is IMHO extremely important that it is always crystal clear to users what happened with their money. And if the different transactions don't result in the actual, current total and show who did what when with the points of a user, things can get really messy.

Sure, you could tell these sites to disable that option, but I'd still have to maintain, test and fix bugs with all the code that tries to update the total.

There are some open issues with making them read only that I'm aware of such as :

- Sometimes you messed up with a transaction, e.g. granting a user 200 instead of 20 points or granting them to the wrong user etc and you don't want to show that to the user. One approach for this would be the ability to undo transactions, by creating a transaction that reverts the points amount and somehow hides the both from the user, making the only visible for administrators (e.g. users with a special permission).

- Active sites with many transactions will have a huge userpoints_txn table, I know of sites in 6.x which regularly delete old transactions. I'm not yet sure how to solve this properly, there is an open issue about this. Some way to archive old transactions including a configuration option what exactly "old" is.

What's your use case?

Yes, I think an undo that works by adding a second transaction and allows to hide them is the way to go.

Possibly requires a new column in the transaction table, maybe a hidden column, which can be 0 or 1.

You're welcome to work on that.

Thanks for working on this, review below, mostly coding style stuff.

+++ b/userpoints.install
@@ -331,4 +338,16 @@ function userpoints_update_7004(&$sandbox) {
+function userpoints_update_7005() {
+	db_add_field('userpoints_txn', 'hidden', array(
+    'description' => 'Hidden',

Looks like there is a tab here instead of two spaces.

+++ b/userpoints.install
@@ -331,4 +338,16 @@ function userpoints_update_7004(&$sandbox) {
+    'default' => 0,
+  ));
 }
\ No newline at end of file
diff --git a/userpoints.module b/userpoints.module

git diff needs a trailing space at the end of a file, not having one makes it very unhappy.

+++ b/userpoints.module
@@ -1301,6 +1302,32 @@ function userpoints_transaction_load($txn_id, $reset = FALSE) {
+ * Delete a userpoints transaction.
+ * In fact, rather than deleting a transaction, this function hides it.
+ * It hides the original transaction, and creates a second hidden negative transaction.

Should be DeleteS and there should be an empty line after the first line in a docblock. And finally, the last line here is over 80 characters.

+++ b/userpoints.module
@@ -1301,6 +1302,32 @@ function userpoints_transaction_load($txn_id, $reset = FALSE) {
+ * @return
+ *   TRUE if the transaction could be deleted, FALSE otherwise.
+ */

I don't think there is a reason to return TRUE/FALSE here. The only thing that could go wrong is an error when trying to save and this would result in an exception anyway.

+++ b/userpoints.module
@@ -1301,6 +1302,32 @@ function userpoints_transaction_load($txn_id, $reset = FALSE) {
+	// Set the original transaction as hidden
+ 	$wrapper = entity_metadata_wrapper('UserpointsTransaction', $transaction);
+ 	$wrapper->hidden = TRUE;

More tabs. There should be no need to use a wrapper here, just use $transaction->setHidden(TRUE) and then $transaction->save().

+++ b/userpoints.pages.inc
@@ -42,6 +42,12 @@ function userpoints_list_transactions($form, &$form_state, $account = NULL, $tid
+  // Determine if hidden transactions should not be displayed
+  if (!variable_get(USERPOINTS_REPORT_DISPLAYHIDDEN, FALSE)) {
+    // Do not display hidden transactions
+    $query->condition('p.hidden', FALSE);
+  }

I'm not sure if we need this variable. The long-term goal is to replace all our custom listings with views and once we have that, you could simply alter those views if you want to show hidden transactions. Additionally, you could do it selectively only for the admin listing for example.

+++ b/userpoints.test
@@ -593,6 +593,24 @@ class UserpointsGrantPointsTestCase extends UserpointsBaseTestCase {
+    $transaction->save();
+    $this->verifyPoints($this->non_admin_user->uid, 7, 7);
+    ¶
+    // Verify that deleting a transaction reduces the points total
+    userpoints_transaction_delete($transaction);
+    $this->verifyPoints($this->non_admin_user->uid, 3, 7);
+  }
+  ¶

Yay tests :)

Trailing spaces on the empty lines.

+++ b/userpoints.transaction.inc
@@ -458,6 +458,20 @@ class UserpointsTransaction extends Entity {
   /**
+   * Define the points amount of this transaction, which can be any positive
+   * or negative amount but not 0.
+   *
+   * @param $points
+   *   The points as an integer.
+   *
+   * @return UserpointsTransaction
+   */
+  function setHidden($hidden) {
+  	$this->hidden = $hidden;
+  	return $this;
+  }
+  ¶

This looks like a copy & paste of another docblock, needs to be updated. Also more tabs and trailing spaces :) And last, you should als define the hidden property explicitly on the class, like the other stuff.

Some more code style issues.

Hint: Both git diff on the console (just git diff, no > some_file.patch) and http://drupal.org/project/dreditor can highlight these for you as well.

+++ b/userpoints.module
@@ -1301,6 +1301,29 @@ function userpoints_transaction_load($txn_id, $reset = FALSE) {
+ * Deletes a userpoints transaction.
+ * ¶
+ * In fact, rather than deleting a transaction, this function hides it.
+ * It hides the original transaction, ¶
+ * and creates a second hidden negative transaction.

Some trailing spaces here.

+++ b/userpoints.module
@@ -1301,6 +1301,29 @@ function userpoints_transaction_load($txn_id, $reset = FALSE) {
+function userpoints_transaction_delete(UserpointsTransaction $transaction) {
+  // Set the original transaction as hidden
+ 	$transaction->setHidden(TRUE);
+ 	$transaction->save();
+
+ 	// Add a hidden negative transaction for the same amount as the original transaction
+ 	userpoints_grant_points('delete', -$transaction->getPoints(), $transaction->getUid())
+ 	->setEntity('userpoints_transaction', $transaction->getTxnId())
+ 	->setHidden(TRUE)
+ 	->save();
+}

And tabs here.

+++ b/userpoints.pages.inc
@@ -42,6 +42,9 @@ function userpoints_list_transactions($form, &$form_state, $account = NULL, $tid
+  // Do not display hidden transactions
+  $query->condition('p.hidden', FALSE);
+    ¶
   if (module_exists('taxonomy')) {
     $query->leftJoin('taxonomy_term_data', 't', 'p.tid = t.tid');
   }
@@ -51,6 +54,9 @@ function userpoints_list_transactions($form, &$form_state, $account = NULL, $tid

@@ -51,6 +54,9 @@ function userpoints_list_transactions($form, &$form_state, $account = NULL, $tid
     ->condition('status', UserpointsTransaction::STATUS_PENDING);
   $unapproved_query->addExpression('SUM(points)');
 
+  // Do not display hidden transactions
+  $unapproved_query->condition('p.hidden', FALSE);
+  ¶
   $values = userpoints_filter_parse_input($form_state, $tid);
   $active_category = userpoints_filter_query($query, $values);

More trailing spaces :)

Close :)

Don't worry, it takes some time to get used to the strict coding standards used in Drupal. I recommend getting used to them in your own code as well, takes a while butt once you're used to it, you don't even have to think about formatting a comment line correctly, it even hurts if it doesn't ;) (I'm not saying that you need to document all your custom code, that's up to you, just when you do, follow the coding standard :))

+++ b/userpoints.module
@@ -1301,6 +1301,29 @@ function userpoints_transaction_load($txn_id, $reset = FALSE) {
+ * In fact, rather than deleting a transaction, this function hides it.
+ * It hides the original transaction,
+ * and creates a second hidden negative transaction.
+ * This is necessary to ensure the balance is reduced to the correct amount.

Looks like this could be shortened to something like "Instead of actually deleting the transaction, the point amount is reverted using a second transaction and both are hidden."

Note: Remember that you can "delete" a transaction with a negative amount as well, and a negative negative amount is positive, so try to avoid using stuff like reduce/negative transaction.

Also, while not going over 80 characters, try to use as much of those 80 as possible, that is easier to read than lines which are splitted at 40 or so.

+++ b/userpoints.module
@@ -1301,6 +1301,29 @@ function userpoints_transaction_load($txn_id, $reset = FALSE) {
+function userpoints_transaction_delete(UserpointsTransaction $transaction) {

Wondering about naming the function differently, maybe _hide() or _revert(), but I'm not sure. Just wondering if it's confusing to have a function named delete and with a docblock that says delete and only in the second sentence we actually mentioned that we don't really delete. Even if it might be a delete in the user interface, we might not want to name it that in the API.

+++ b/userpoints.module
@@ -1301,6 +1301,29 @@ function userpoints_transaction_load($txn_id, $reset = FALSE) {
+  // Set the original transaction as hidden
+  $transaction->setHidden(TRUE);
+  $transaction->save();
+
+// Add a hidden negative transaction for the same amount as the original transaction
+  userpoints_grant_points('delete', -$transaction->getPoints(), $transaction->getUid())

Comments should end with a . (same for those below), the indentation of the second one looks off and also seems to be too long.

+++ b/userpoints.moduleundefined
@@ -1301,6 +1301,28 @@ function userpoints_transaction_load($txn_id, $reset = FALSE) {
+  userpoints_grant_points('revert', -$transaction->getPoints(), $transaction->getUid())
+  ->setEntity('userpoints_transaction', $transaction->getTxnId())
+  ->setHidden(TRUE)

Sorry for finding new stuff all the time, the method calls (->something()) should be indented two spaces.

What would be great here would be a basic test. You can probably just add a testRevert() method to the API test class, and do something like this:

- Create a user.
- Create 2-3 transactions for that user.
- Revert one of them.
- Make sure that the total amount of points for the user is correct.
- (Visit a page that lists userpoints for that user and make sure the reverted one doesn't show up. You should be able to do so be adding a custom operation for each transaction) and then make sure that those are displayed who should be and the reverted one isn't).

The last point is probably a bit more complicated as it's more than just API calls (you need to log in, view pages and so on) but would be great to have.