Implement userpoint bundles

Thinking about it, it's actually more like that having a field for the userpoints total ( and having the possibility for multiple fields) + the ability to categorize using a referenced taxonomy term reference replaces the current categorization feature and not transaction bundles.

So me might want to split this one up as well and first through out the current categorization stuff (assigning myself as I've started with this already) and later worry about how to embed bundles.

Looks good.

I think we should add a default type during installation and call that default or something like that. My goal for 7.x-2.x is to make sure that the module still works out of the box for the default use case.

Also, we need one for upgrading from 7.x-1.x anyway, and we could use rely on it in the tests as well (although we shouldn't anywhere else).

The upgrade path is going to be non-trivial with all these changes, so we should add some test coverage for that at some point, doesn't need to happen here though, upgrade tests are tricky...

+++ b/userpoints.admin.incundefined
@@ -16,7 +16,44 @@ function userpoints_confirm_approve_submit($form, &$form_state) {
+ * A simple list of links similar to node/add for creating
+ * userpoints transactions.
+ */
+function userpoints_admin_add() {
+
+  $types = userpoints_transaction_get_types();

Can we also add a redirect here if there is just one type, similar to how node/add works?

+++ b/userpoints.moduleundefined
@@ -1310,11 +1324,66 @@ function userpoints_transaction_load($txn_id, $reset = FALSE) {
+ * Check if the userpoints transaction type exists.

Should be CheckS, LoadS and so on. (The existing code is probably most wrong too, all we can do is make new code as good as possible :))

+++ b/userpoints.moduleundefined
@@ -1310,11 +1324,66 @@ function userpoints_transaction_load($txn_id, $reset = FALSE) {
+ * @return Boolean

Should be @return bool.

+++ b/userpoints.admin.incundefined
@@ -31,10 +73,15 @@ function userpoints_admin_txn($form, &$form_state, $mode, $txn = NULL) {
+    if (isset($txn->type)) {
+      // Transaction entity
       $txn_user = user_load($txn);

Hm, that comment doesn't seem to make sense? $txn in this case is actually a user id.

This stuff is a mess, we might want to use a separate argument for user and transaction. But that will probably require a few changes, so possibly a follow up. But I think that your changes here are not correct (also those below)

What still should work is going to userpoints/add/$uid and have the user pre-filled..

+++ b/userpoints.moduleundefined
@@ -726,7 +740,7 @@ function userpoints_get_max_points($uid = NULL, $tid = NULL) {
-function userpoints_grant_points($operation, $points, $uid = NULL) {
+function userpoints_grant_points($operation, $points, $type, $uid = NULL) {

There is a problem with this change.

We have a API function with a required argument for something that is configured through the UI , can be deleted and so on. This means that nobody can actually use this function without having the user to present a configuration form for this.

I'm thinking that we should have a default type (maybe even prevent to delete that) and then default to that if it's not set.

We can still change later but it would make this patch and the transition to bundles easier/smaller.

Yes, it's time to get this in. Will do a manual test and test the upgrade path asap.

Upgrade path tests would be valuable but it's quite some work to set it up. Not a blocker to get this in but I would like t have some before 7.x-2.x can become stable. Can you open a critical task to add (basic) upgrade path test coverage?

Some feedback on the update function:

+++ b/userpoints.installundefined
@@ -405,3 +404,41 @@ function userpoints_update_7004(&$sandbox) {
+function userpoints_update_7100() {

The version number should be 7200.

+++ b/userpoints.installundefined
@@ -405,3 +404,41 @@ function userpoints_update_7004(&$sandbox) {
+  // Add transaction type table from schema.
+  $txn_type_table = drupal_get_schema('userpoints_txn_type', TRUE);
+  db_add_table('userpoints_txn_type', $txn_type_table);

This is not a reliable thing to do, you need to copy the definition.

Imagine that a later update would add another column and we'd add a second update function to add it accordingly. This one would then however already get the latest one which would result in conflicts.

+++ b/userpoints.installundefined
@@ -405,3 +404,41 @@ function userpoints_update_7004(&$sandbox) {
+  $bundle = new UserpointsTransactionType($bundle_info);
+  $bundle->save();

Similarly, it is not reliable to use entity API functions. Entity API always works with the latest schema defined in hook_schema() and would try to write to columns that might not exist yet. Needs to be a manual db_insert() query.

+++ b/userpoints.installundefined
@@ -405,3 +404,41 @@ function userpoints_update_7004(&$sandbox) {
+  // Update all transactions.
+  db_update('userpoints_txn')->fields(array('type' => 'userpoints'))->execute();

Should look like this:

db_update()
  ->fields()
  ->execute();

Yes, that's fine for hook_install() but update functions are a very tricky business...

+++ b/userpoints.installundefined
@@ -217,6 +287,8 @@ function userpoints_uninstall() {
+  variable_del('userpoints_default_bundle', 'userpoints');

variable_del() doesn't have a second argument.

Will do some manual testing later, would like to know why exactly that cache clear is necessary (not saying it's wrong, just want to understand it) and to test the adjusted user interfaces.

+++ b/userpoints.installundefined
@@ -331,4 +403,82 @@ function userpoints_update_7004(&$sandbox) {
+  $insert = db_insert('userpoints_txn_type')
+    ->fields(array('name', 'label', 'status', 'module'));
+  $insert->values(array(
+    'name' => 'userpoints',
+    'label' => st('Default'),
+    'status' => 0x02,
+    'module' => 'userpoints',
+  ));
+  $insert->execute();
+  variable_set('userpoints_default_bundle', 'userpoints');

This can be simplified a bit, you can directly pass the array you pass to values to fields, doing this separately is only useful if you have multiple rows to insert.

db_insert('userpoints_txn_type')
  ->fields(array(
    ...
    ...
  ))
  ->execute();

+++ b/userpoints.installundefined
@@ -331,4 +403,82 @@ function userpoints_update_7004(&$sandbox) {
+
+  $updated_plural = format_plural($updated, '1 transaction', '@count transactions');
+  return t('Created userpoints_txn_type table, added bundle column to userpoints_txn table, and add default bundle to !count.', array('!count' => $updated_plural));

I don't think we need this at all. There are almost no core update functions that return something and we already described what the update function does in the docblock. I think it's only useful to return something if it's relevant to the user, e.g. he might need to do something.

Also, $updated is never set.

I tried it out manually and got some errors when trying to run the update. We might have to add a try/catch to the entity_info_alter () hook in case caches are cleared before the update is run. Also not sure if the entity cache clear in the update function isn't actually counter-productive, as it then seems to try to rebuild that information before the schema cache is cleared.

And, I noticed that after a cache clear, the default bundle is gone, which is because you used the 0x02 status, which means "ENTITY_IN_CODE" and that again means that entity.module will automatically delete it if it is not in code, as it assumed that it has been removed. 0x01 is the correct status to use. Maybe that's what was "breaking" it? Because I had that effect immediately when removing the cache clear at the top.

Edit: Reworded the 4 "And, "'s that was weird ;)

Oh, that's actually quite a nice way to do it. Nice work.

Yes, having basic coverage here already is great. We can always extend it with more data as we need it to add coverage for bugs as we discover them.

index 0000000..3911665
--- /dev/null

--- /dev/null
+++ b/userpoints-7.x-1.x.filled.database.phpundefined

@@ -706,3 +706,45 @@ class UserpointsFieldsTestCase extends UserpointsBaseTestCase {
+      drupal_get_path('module', 'userpoints') . '/userpoints.filled.database.php',

Looks like the filename doesn't match.

Also, this should be moved into a tests subfolder.

yeah, and that unfortunately results in having those large dumps in the release people download. It can be gzipped, though, to make it smaller.

+++ b/userpoints.installundefined
@@ -331,4 +403,74 @@ function userpoints_update_7004(&$sandbox) {
+    'default' => 'userpoints',

Instead of default, we could also use initial here, which does not set the schema definition default to that but only sets the value of all existing records to that.

The default shouldn't be hardcoded I think, that's what the variable is for.

The upgrade path looks great now otherwise, clean and no more black magic cache clears :)

Can you re-roll for initial and with a gzipped version of the all dump? Did a quick test and that gets the size down from 530kb to only 50kb. You can use the --binary option for git diff to make a patch with that.

No need to do that for the other dump as we want to be able to make manual changes there and get a useful diff.

Getting the following notices on the transaction type add form, fixed it by defining the properties on the entity class.

UserpointsTransactionType::$label in userpoints_transaction_type_form() (line 961 of sites/all/modules/userpoints/userpoints.admin.inc).
Notice: Undefined property: UserpointsTransactionType::$name in userpoints_transaction_type_form() (line 972 of sites/all/modules/userpoints/userpoints.admin.inc).

Commited to 7.x-2.x, thanks for working on this!

Developer or user documentation?

User documentation can just be added somewhere below here: http://drupal.org/documentation/modules/userpoints

I prefer to have API/developer documentation in code, then I can parse it with api.module and display on http://api.worldempire.ch/api/userpoints