View permissions for userpoints transactions

yes this is correct.

Patches for a new permission are welcome, although it might only be applied against 7.x-2.x, not sure yet.

You can also create a view where you have full control over the necessary permissions to view it.

Hi,
I would really like this feature!
I have attached a patch, but I am pretty sure that will need some deeper testing.

This will create two new permissions settings under admin/people/permissions.

• View own transactions
• View all transactions

Please read the permissions description.
It would be great some testing and comments!!

Forgot to include one more condition in the new userpoints_access_transactions function.

Patch looks quite good to me, although, as I said, this will need to be done against 7.x-2.x branch, it might or might not apply against that one. Testbot is currently not so happy with us, due to issues with services/services integration. I'll either temporarly disable these tests or try to fix them asap.

I'm quite sure that there will be a few test fails once the tests are actually executed, as tests probably need to be updated to grant the new permissions to test users.

I don't think we need these NOTICE's. They just state the opposite of what is written in the label/description.

Also, we should probably split up that very long and complicated if in the access callback function into multiple lines and add some comments to explain it. That line is currently very hard to read.

#3: adding_userpoints_view_transaction_permissions-1331224-3.patch queued for re-testing.

Hi,
Berdir thanks again for your rapid response... really good!

I have clone from 7.x-2.x-dev repository and added some changes.
Now transactions permissions do not depend on points permissions. Means that users can see transactions history (if they have transactions permissions granted) even if they don't have points permissions granted.

No userpoints.test or userpoints_rules.test code functionality has been added, so the patch will probably not pass, but we can keep changes logged this way. And if we are pointing in the right direction with this code I will try to do the test code part.

Changes so far:
1 - Added some documentation and cleaner code.
2 - Changed function userpoints_access_transactions functionality and permissions. Besides from the above, only logged in users can see transactions (is this the desired Behaviour?)
3 - NOTICE's descriptions removed.
4 - Also a minnor change in userpoints.test, relating to a indent typo (it was tabbed instead of doubled spaced).

Please test it, but.... better for the next year!

Happy new year everybody!

Happy needs review year!

+++ b/userpoints.module
@@ -373,6 +373,26 @@ function userpoints_access_my_points($account = NULL) {
+ *
+ * @return
+ *   TRUE if user has permissions to view transactions and if the user is logged in.
+ *   Or if user has Edit point transactions permissions and if the user is logged in.

Should be wrapped at 80 characters, it's currently a bit longer. Also, I would shorten this and just say TRUE if the user can view the transactions of the requested account and FALSE otherwise. You can put a detailed description above the return, separated by an empty line from the one-line function description.

Also a @param $account block should be added.

+++ b/userpoints.module
@@ -373,6 +373,26 @@ function userpoints_access_my_points($account = NULL) {
+    return (user_access('view transactions') || userpoints_admin_access('edit')) && user_is_logged_in();
+  }
+  ¶
+  // Own user account.

Trailing spaces here.

+++ b/userpoints.module
@@ -389,11 +409,19 @@ function userpoints_permission() {
+    'view own transactions' => array(
+      'title' => t('View own transactions'),
+      'description' => t('Allows to view own transactions history.', userpoints_translation()),
+    ),
+    'view transactions' => array(
+      'title' => t('View all transactions'),

Wondering if the permissions should have a 'userpoints' in them. The problem is that permission strings need to be globally unique and the easiest way to do that is put the module name into it. So 'view own userpoints transactions'. You can keep the user-facing title, though.

+++ b/userpoints.test
@@ -93,7 +93,7 @@ class UserpointsAPITestCase extends UserpointsBaseTestCase {
         'name' => t('Userpoints API'),
         'description' => t('Tests the core API for proper inserts & updates to the database tables,
-			  moderation, expiration, as well as permission checks'),
+        moderation, expiration, as well as permission checks'),
         'group' => t('Userpoints'),

Be careful with unrelated changes. It's fine in contrib modules on which only a few people are working on (I do it myself quite often). But such patches will be rejected for Drupal core as the have a unecessary high potential of conflicting with other issues. See http://webchick.net/please-stop-eating-baby-kittens

The tests should be easy to fix I think, you probably simply need to grant the involved user in the rules integration test the new permissions.

Hi all,
Thanks Berdir.

Added some documentation changes, resolving identation problems, changing permissions names (adding the userpoints word to it).
Adding 'view userpoints transactions' permission to the testActions function (userpoints_rules.test).
I have one question here:
I have just added 'view userpoints transactions' permission in the testActions function, not in testEvents. I don´t know if testEvents needs this permission too (it seems to me that it is not required).

Yes, looks like the permission is only necessary in testActions() because we only there add the myuserpoints page.

Patch looks good, what would really be awesome would be a few new tests for these permissions. Basically, set up a few users with different permissions (none, only own transactions, all transactions) and then access the different transaction listing pages (either check for access denied or the title on that page, depending if they should have access or not) with them. Additionally a check to see if the link on the user profile is visible or not.

I'm currently in #drupal-contribute in IRC, feel free to ping me there.

Doesn't need to be today of course ;)

Hi Berdir,
Thanks again. Yes, having the test thing would be great, I'll start working on that around tuesday. If I have questions I will ping you on IRC channel (my nickname is TuWebO).

Hi,
Added some test functionality under UserpointsAPITestCase.

Sorry,
Previous patch had two minor indent problems, now it should be ok.

Sorry...
Now the two minor indent problems are fixed.

Comments and testing are really welcome.

Looks good to me, great work!

Testbot is happy too, so marking as RTBC, will commit later when I find the time.

Hoping to see more patches/help from you, are you interested in helping out with the 7.x-2.x? The current plan/roadmap is outlined at #1158692: Userpoints Roadmap to 7.x-2.x, feel free to ping me about it simply start working on something if you're interested.

Thanks Berdir,
Yes I'm interested in helping out with this module, it will take me some time to do the tasks but I would like to.
Next monday (9-1-12) I will try to ping you in drupal-contribute.

Ok, I'll try to be there.

Commited.