Drupal 7 port

Hey Berdir,

All of your work on this module is very cool! I tried out the patch and I'm getting the following error message when enabling the module, emptying the cache, or saving Userpoints settings page:

Warning: include_once(/Users/benkaplan/git/drupal/sites/all/modules/userpoints_nodeaccess/userpoints_nodeaccess.theme.inc): failed to open stream: No such file or directory in _theme_process_registry() (line 381 of /Users/benkaplan/git/drupal/includes/theme.inc).
Warning: include_once(): Failed opening '/Users/benkaplan/git/drupal/sites/all/modules/userpoints_nodeaccess/userpoints_nodeaccess.theme.inc' for inclusion (include_path='.:/Applications/MAMP/bin/php5.2/lib/php') in _theme_process_registry() (line 381 of /Users/benkaplan/git/drupal/includes/theme.inc).

Also, I can't find any administrative controls for assigning how many points are needed to "buy" a node. I'm assuming these individual node settings should be available on the node/add and node/edit pages (in the vertical tabs section), as well as the content type defaults pages. But they aren't showing up at all (even after I tried clearing the cache).

Could the above error be related to why these node settings are missing? So far, I can only see the general settings on the Userpoints settings page and the one added permission on the site permission page.

Thanks,
Ben

Berdir,

I tried the patch in #5 and the error message has gone away. Yay!

But I still can't find any administrative controls for assigning how many points are needed to "buy" a node. I'm assuming these individual node settings should be available on the node/add and node/edit pages (in the vertical tabs section), as well as on the content type defaults pages. But they aren't showing up at all (even after I tried clearing the cache).

At the moment, the only settings I can see are two settings at admin/config/people/userpoints/settings and the one added permission on the site permission page (admin/people/permissions). I don't see any place else to configure the points required to view a node.

Thanks,
Ben

Hmmm.... My Drupal 7 is maybe 1 day old... I'll try to update and report back.

--Ben

Hmmm.... My Drupal 7 is maybe 1 day old... I'll try to update and report back.

--Ben

In an IRC chat with Berdir, we determined that there was a missing "administer userpoints nodeaccess" permission. As a result, the vertical tab was only showing up for User #1. He's working on a fix for this....

Berdir and I also discussed changing some of the permission names and displayed messages. Here's a summary of what we discussed:

* Change "Buy access to a node" permission to: "Trade Userpoints for node access". Make the description read "Allows a user to gain access to a node by trading in Userpoints that the user has accumulated."

* Change "Administer userpoints nodeaccess" to: "Set required Userpoints for any node". Make the description read: "Allows a user (usually a site administrator) to determine the number and category of Userpoints required to access a node."

* Add a new permission: "Set required Userpoints for own node". Make the description read: "Allows the author of a node to determine the number and category of Userpoints required to access that node."

* Change submit button that currently reads "Buy Access" to: "Get Access". Another option is to use: "Trade @price !Points" (example: "Trade 15 Points") as the submit button. Which do you prefer?

--Ben

We also discussed changing the message that is currently printed to users who don't have access to a node. The message currently reads:

"You need to buy access to view this content. You have to pay 10 points from category Access Points. You currently have 3 points in that category."

Based on Berdir's suggestions, I recommend changing this to:

'To access this content, you must trade in @price "[categoryname]" !points. You currently have [numberofpoints] !points in the "[categoryname]" category.'

Thus, a message would read something like (for the "Participation" category):

'To access this content, you must trade in 15 "Participation" points. You currently have 20 points in the "Participation" category.'

--Ben

I've been doing more testing and noticed two things:

i) It would be great if we could change the default message that is displayed when points are traded in for node access. Currently, the default Userpoints "lost points" message is used: "User authenticated lost 10 points Total now is 8 kudos." But modules like userpoints_no_negative seem to be able to set their own custom message.

So when 10 points are traded in for node access, would it would be possible to display a message like: 'You have successfully traded in @price "[categoryname]" !points and now can access the content. You currently have [numberofpoints] !points remaining in the "[categoryname]" category.'

Thus, it would read something like: 'You have successfully traded in 10 "Participation" points and now can access the content. You currently have 8 points remaining in the "Participation" category.'

ii) It doesn't appear that the "Show a message when the user already bought access to the node and tries to buy it again" setting is currently working properly or actually doing anything. I'm not seeing any message. This might be because we now just hide the form if the user has access (so there is no way anymore to try to "buy" it again). But ideally, I'd love to see a message displayed when viewing the content each time that says something like:

'By trading in @price "[categoryname]" !points, you now have full access to this content. Enjoy!'

This would read as:

'By trading in 10 "Participation" points, you now have full access to this content. Enjoy!'

--Ben

Berdir,

Sorry for all the separate comments. I figured I'd stick with testing so that you can deal with bugs in one fell swoop. I'm posting them as I find them. Here are two new ones I found:

a) PHP fatal error when user creates new content (but doesn't have permission to set required userpoints): I had a regular authenticated user who can't set required Userpoints create some new content (a basic page). Upon trying to submit the new content, I got a white screen of death. Here was the message in my logs:

PHP Fatal error: Call to undefined method DeleteQuery::fields() in /Users/benkaplan/git/drupal/sites/all/modules/userpoints_nodeaccess-DRUPAL-6--1/userpoints_nodeaccess.module on line 133

b) "Bypass content access control" is not recognized. I've got a user with the administrator role (but not User #1) who has all permissions, including the "Bypass content access control" permission. But on a node with 10 Userpoints required to access, this admin user is still being shown the "Get Access" message (and he can't access because he doesn't have enough points to see the "Get Access" button). I previously thought this was working, but I might have been fooled before because this admin user was also the node author. Automatically granting access to the node author seems to be working properly.

--Ben

We should probably also update the language on the node/edit page to reflect our change from buy/sell terminology. Currently, the points description reads:

"Optionally set a price in points that this node costs to get access to. When leaving empty, the standard drupal node access decides who can access the node or not."

We should change this to:

"Set a price in !points that is required to access this node. If this field is left empty, then the standard Drupal node access system is used instead."

And currently, the category description reads:

"Choose a Points category from which this node can be bought."

We should change this to:

"Choose the !Points category that shall be used when users trade in their !points."

--Ben

Berdir,

I've completed testing of your latest patch. Here are the results. I'm starting first with the bugs I previously mentioned:

#10 + #11: The new permissions are working great. This is totally fixed!

#12: The "no access" message currently reads:

"To access this content, you must trade in @price !points of the category @category. You currently have @points !points in the @category category."

Upon further reflection, I think it would be better to change this to:

"Trade in @price !points to access this content. You have @points !points available. Only !points in the @category category may be used."

So this would read:

"Trade in 15 points to access this content. You have 20 points available. Only points in the participation category may be used."

I think it's shorter and more clear.

#13i: Thanks for explaining. Let's not worry about this.

#13ii: It's nice to have this message displayed in the message area at the moment when the points trade goes through (as confirmation that the trade went through). But I don't think we necessarily need to show it every time the node is displayed (because we can deal with that better using view modes as discussed below). Plus, if we only display it immediately after the points trade itself (in addition to the regular Userpoints "points lost" message that we can't delete), we won't have to deal with duplicates in the message area when multiple nodes are listed.

Also, I think we should shorten my original message a bit (since I realized we don't really need to mention the category here). We also should make it better match instances where multiple nodes are on one page. The message currently reads:

"By trading in @price !points in category @category, you now have full access to this content. Enjoy!"

Let's change this to:

"By trading in @price !points, you now may access the requested content."

Also, if we're changing the nature of when this setting is displayed, we should probably change the language of the label itself. Currently, it reads:

"Show a message when the user already bought access to the node and tries to buy it again"

What if we changed it to:

"Show a confirmation message when a user is granted node access"

#14a: This is fixed!

#14b: This is fixed!

#15: This looks good. I did, however, notice one small typo. The field title "Points Category" is not using proper branding (the word "Points" is hardcoded). You could just delete "Points" and let the field title simply be "Category".

ADDITIONAL ITEMS STILL TO DO (as discussed on IRC):

A. Determine if it is possible to change the view mode before rendering to avoid building it twice.

B. Explore the possibility of a second view mode (for users who do have access). The new view mode could be named "Userpoints node access granted". The existing view mode would still be called "Userpoints node access denied".

C. We need to determine if it is possible to only include a view build mode conditionally for certain content types. This isn't critical and might not be possible.

D. Per your suggestion in IRC, we need a new message for the case of a user who does have "Trade Userpoints for node access" permission, but doesn't have enough points to trade . Here's my suggestion for a message:

"@price !points are required to access this content. You currently have @points !points available and need more !points. Only !points in the @category category may be used."

E. Per your suggestion, at "admin/config/people/userpoints/settings", we need to add checkboxes for each content type so that the admin can specify which content types are enabled for Userpoints Nodeaccess functionality.

It's open for debate whether we should also provide a link to the view mode page for each content type. That seems like too many links for sites with a lot of content types. Maybe we should just provide a short note that says check out the "Manage Display" tab for each content type to configure special custom displays. (Also, perhaps we should hold off on default points amounts and categories for each content type since it seems complex to manage from a UI perspective.)

F. The Userpoints Nodeaccess Views sub-module still needs to be ported.

G. Explore the possibility of integrating more of the modules messages/buttons/forms into the view build modes (on the "Manage Display" tab) so that site admins can better control the order and placement of items on the page, including re-ordering and hiding them as needed. We seem to have two basic "fields" (may not be the precise term) with four options each:

1. Access Message:

a) No permission, no access
b) Permission, not enough points
c) Permission, enough points
d) Permission and access granted

2. Access Button:

a) "No access" button/icon (red)
b) "Need more points" button/icon (yellow)
c) "Get access" form submit button (the button that currently exists)
d) "You have access" button/icon (green)

Obviously, I sort of invented 2a, 2b, and 2d above. I wanted to see what you thought. I was thinking we could do something simple with CSS for these other buttons/icons (maybe a css background color and border for each). I was also thinking it would be handy to have all of these options in one "field" because then when you output the field as part of Views integration then all of those options would be shown (as appropriate) in that one field. And if the Access Message was also part of the Views Integration, it would sort of explain each button/icon, too.

But anyway, just an idea... eager to hear your thoughts.

--Ben

If "G" above is too expansive/difficult right now, we could move on the other stuff first....

--Ben

Hey Berdir,

I tried out your patch in #21 and things are looking good. The new content type settings are really nice!

Here are the bugs/issues that I noticed. All of these issues are numbered from scratch and don't refer to prior comments (since it was getting a little bit hard to follow):

a. If you leave the "Points" field blank on a node or type in a "0" (because you now want all users to have access even though it required points before), then the setting is being overwritten by the default points value and category for the content type. Note that upon saving initially, everything works fine: All users are granted access. But if you visit the content type settings page again, the defaults will be filled in (even though you last left the points field empty) and will go into effect if you don't notice this and re-save the content type settings page.

b. Change this: "Set a price in points that is required to access this content. All users have access if this field is left empty." To this: "Set a price in points that is required to access this content. All users will have access if you enter "0" or this field is left empty."

c. Change this message: "By trading in 50 points in category Uncategorized, you now have full access to this content. Enjoy!" To this: "By trading in 50 points in the Uncategorized category, you now have full access to this content."

d. If you're creating a new content type, the Manage display screen link doesn't work until the content type is saved. Basically, the link doesn't have the content type machine name included yet. I'm not sure if there is a way around this.

e. Change this: "You can control how the content looks for a user that does not have access by going to the Manage display screen." To this: "You can control how the content appears to users who do not have access by going to the Manage display screen."

f. Change this: "Set a default price in points that is required to access content with this type." To this: "Set a default price in points that is required to access content of this type. I changed "with" to "of".

g. Change this: "Points can be assigned to content with this type if checked." To this: "If checked, a price in !points can be set to access content of this type."

h. At admin/config/people/userpoints/settings, there are two typos: "overriden" should be spelled "overridden" in the description of both settings.

i. Per your question, I just wanted to explain why I brought up the possibility of a second view mode for users who do have access ("Userpoints node access granted"). The purpose of this was to display certain fields in the teaser that only users with access would see. As I understand it, there's no other way to do this in the teaser. However, now I'm thinking this is probably overkill. But just thought I'd explain.

j. I'm seeing an error message at the top of a view when including the "Buy Access Button" field in the view. Here's the error that I'm getting:

Notice: Undefined property: stdClass::$nid in userpoints_nodeaccess_handler_field_buyaccess->render() (line 28 of /Users/benkaplan/git/drupal/sites/all/modules/userpoints_nodeaccess-DRUPAL-6--1/userpoints_nodeaccess_views/userpoints_nodeaccess_handler_field_buyaccess.inc).

k. Per your question about what to conditionally display in a view, here are my suggestions:

* user doesn't have enough points: "Need more points"
* user is author: "Author access granted"
* user has by-pass access permission: "Unlimited access"
* user does not have permission to trade points but points are required: "Access denied"
* user is anonymous: "Register or login to get access"
* Points not set for the node: "No points required"
* userpoints_na is not enabled for that content type: "No points required"
* user already has gained access: "Access granted"

We should also give a different CSS class to each of these so that they can be styled as needed.

l. Add specific CSS classes to the "Get Access" button so that it can be styled as needed.

m. I'm wondering about the default message that is displayed to anonymous users: "You do not have access to this content." This is because anonymous users obviously can't trade in points. But this could be confusing to a lot of users who are visiting a site for the first time or traded in points for access but just aren't logged in. Could we add a special case for anonymous users? The text for anonymous users could be something like: "This content requires @points !points to access. You must login or register to earn or use !points." The words "login" and "register" would be links to the Drupal pages to do this. What do you think?

n. I love your idea to add an option that allows to the node author to be "paid" in points when a user trades in points to get access to a node. I hadn't thought of that. That's really, really cool. I think it would be great if the amount of points to be paid to the node author were configurable.... so that you could pay just some of the traded in points, all of the traded in points, or an amount more than the traded in points to the node author. Also, the category of the points awarded to the node author should be configurable because you might want to award points to the node author in a different category than the category used when the points were traded in (imagine a category called "Author Rewards").

Do you think this should be done natively within the module or using Rules integration? For instance, I could imagine a Rules event called "!Points traded in for access". To award points to the node author in response, we could use the "Grant points" action supplied by the main Userpoints module. The drawback to this approach would be that it would be very cumbersome to do this via Rules on a per node basis (because you'd need a massive Rules configuration to account for every node). For this reason, it might be easier to do this within the module itself. We'd just need a new "Set Userpoints award for node author" permission. However, the "!Points traded in for access" Rules event could still be useful, too.

o. All this talk of Rules made me thing of something else: How hard would it be to allow a site to programatically give someone access to a node (either via a simple API or via Rules)? Here's the use case: In addition to allowing users to trade points for access, you also might want to let them buy access with actual money. So let's say you're using Drupal Commerce, Ubercart, or something else... It would be great to have a simple way upon completion of a sale to add a user and a node programatically to the userpoints_nodeaccess table. This could be done with a very simple API to add access or else there could be a Rules "Grant access without points trade" action. What do you think?

Berdir, so sorry for the long message here! I just wanted to get everything down that was running through my mind. This turned into quite the brain dump. Hope you don't mind... ;-)

--Ben

One idea I wanted to mention just so I don't forget: There really should be a way to see who has access to a node without having to go into phpmyadmin. As I test the module, I've forgotten on several occasions who has been added to which node. I usually need to guess and check by logging in as the user or else manually translating the uid's and nid's in the userpoints_nodeaccess table. This could be accomplished with a simple table that only displays on the node if a user has a "View who can access any node" permission.

Once we did that, it would probably be useful to add a simple way for a site admin to manually add a user to this userpoints_nodeaccess table. This could be based on the same permission as above or else we could have a separate "Grant a user node access" permission. But this is less important than the first feature.

What would this table look like? I actually think it would look and function pretty much the same as Private Message's messages/blocked page (except that this would appear at the bottom of a node instead of on a separate page).

Per our conversation in IRC, we could postpone this for a later issue, but I just wanted to make sure I didn't lose track of it.

--Ben

Hey Berdir,

I took me awhile to do all of the testing on views integration, but here is the full report. My notes below are limited to the views integration sub-module and incorporates the conclusions of our great discussion in IRC:

i. Rename the "Userpoints Nodeaccess: Buy Access Button" field to "Userpoints Nodeaccess: Access Status". Change the default title of the field from "Buy Access Button" field to "Access Status". Make this field sortable (it is currently missing the sortable option). Change the field description from "Shows a buy access button." to "Shows whether the current user can access each node."

ii. Streamline the "Access Status" possibilities to the following:

* user has enough points to trade: "Get access" (button)
* user is author: "Granted"
* user has by-pass access permission: "Granted"
* user already has bought access: "Granted"
* user does not have permission to trade points but points are required: "Denied"
* Points not set for the node (null) or set to "0": "Unlimited Access"
* userpoints_na is not enabled for that content type: "Unlimited Access"
* user doesn't have enough points: "Need [@points - currentpoints] more !points" (i.e. Need 25 more points)
* user is anonymous: "Login to view status" ("Login" should be a link to Drupal login page)

iii. Add a css class to each of the above "Access Status" view states:

a) "Get access" button -- Class: .view-access-getbutton (CSS: background-color: yellow; color: black;)
b) "Granted" -- Class: .view-access-granted (CSS: background-color: green; color: white;)
c) "Denied" -- Class: .view-access-denied (CSS: background-color: red; color: white;)
d) "Unlimited Access" -- Class: .view-access-unlimited (CSS: background-color: green; color: white;)
e) "Need more points" -- Class: .view-access-needmore (CSS: background-color: red; color: white;)
f) "Login to view status" -- Class: .view-access-anonymous (CSS: color: black;)

In my testing with Firebug, I applied the above background colors to the entire table cell of each status and it actually looked pretty good.

iv. Make an "Access Status" filter. Filter name: "Userpoints Nodeaccess: Access Status." Filter description: "Shows whether a user can access each node." If possible, filter options should include:

a) Show nodes with 'Get access' button status
b) Show nodes with 'Granted' status
c) Show nodes with 'Unlimited access' status
d) Show nodes with 'Need more points' status
e) Show nodes with 'Get access' or 'Need more points' status
f) Show nodes with 'Get access' or 'Need more points' or 'Granted' status
g) Show nodes with 'Unlimited access' or 'Granted' status

v. When modifying the "Userpoints Nodeaccess: Points" filter so that it has a value different than "0" (say, greater than or equal to 10 points), I'm getting the following error message after saving the view:

Warning: Invalid argument supplied for foreach() in views_plugin_display->get_handlers() (line 672 of /Users/benkaplan/git/drupal/sites/all/modules/views/plugins/views_plugin_display.inc).

vi. The "Userpoints Nodeaccess: Points" filter needs special case handling the account for NULL or O. Basically, we need to create a filter that can show all nodes with either a "0" or NULL value for the point total.

vii. Make "Category" an available field (that is sortable). Field name: "Userpoints Nodeaccess: Points Category". Field description: "The category of points that is used when gaining access to a node."

viii. Make "Category" an available filter. Filter name: "Userpoints Nodeaccess: Points Category". Filter description: "The category of points that is used when gaining access to a node." The available categories should be selectable via a select list or checkboxes.

ix. In the "userpoints_nodeaccess_node_buys" view, add "Userpoints Nodeaccess: Points" as a field. Make this field sortable. Make the order of the fields from left to right: Name, title, points, timestamp. Add a default sort criteria using "Userpoints Nodeaccess: Timestamp" and sort it descending.

x. In the "userpoints_nodeaccess_node_buys" view, if I click the "Name" header to sort by name, I get the following error:

Debug: 'Missing join: users userpoints_nodeaccess' in views_get_table_join() (line 95 of /Users/benkaplan/git/drupal/sites/all/modules/views/includes/handlers.inc).
Debug: 'Missing join: users userpoints_nodeaccess' in views_get_table_join() (line 95 of /Users/benkaplan/git/drupal/sites/all/modules/views/includes/handlers.inc).

Error messagePDOException: SQLSTATE[42S22]: Column not found: 1054 Unknown column 'users.name' in 'field list': SELECT COUNT(*) AS expression FROM (SELECT users_userpoints_nodeaccess.name AS users_userpoints_nodeaccess_name, users_userpoints_nodeaccess.uid AS users_userpoints_nodeaccess_uid, node_userpoints_nodeaccess.title AS node_userpoints_nodeaccess_title, node_userpoints_nodeaccess.nid AS node_userpoints_nodeaccess_nid, userpoints_nodeaccess.timestamp AS userpoints_nodeaccess_timestamp, node_userpoints_nodeaccess__userpoints_nodeaccess_points.points AS node_userpoints_nodeaccess__userpoints_nodeaccess_points_poi, users.name AS users_userpoints_nodeaccess_name_1 FROM {userpoints_nodeaccess} userpoints_nodeaccess LEFT JOIN {users} users_userpoints_nodeaccess ON userpoints_nodeaccess.uid = users_userpoints_nodeaccess.uid LEFT JOIN {node} node_userpoints_nodeaccess ON userpoints_nodeaccess.nid = node_userpoints_nodeaccess.nid LEFT JOIN {userpoints_nodeaccess_points} node_userpoints_nodeaccess__userpoints_nodeaccess_points ON node_userpoints_nodeaccess.nid = node_userpoints_nodeaccess__userpoints_nodeaccess_points.nid ORDER BY users_userpoints_nodeaccess_name_1 ASC) subquery; Array ( ) in views_plugin_pager->execute_count_query() (line 141 of /Users/benkaplan/git/drupal/sites/all/modules/views/plugins/views_plugin_pager.inc).

I get the same error if I click the "Title" header to sort by title.

xi. In the "userpoints_nodeaccess_node_buys" view, the "no results" message is set to: "No userpoints nodeaccess bought yet." Change this to: "No users have traded in points to access content."

xii. In the "userpoints_nodeaccess_node_list" view, add a default sort criteria using "Node: Post Date" and sort it descending. Add a field for the "Points Category" to this table. Make it sortable and the fourth column. Enable the "Sortable" option for the "title" and "points" fields. Make the "Access Status" field (formerly "Buy Access Button" field) sortable, too.

This will be a great module when we are done! :-)

Cheers,
Ben

@jrivelli: If you need instructions on how to do a CVS checkout for this module, visit this link: http://drupal.org/project/userpoints_nodeaccess/cvs-instructions

@jrivelli: No, we're working exclusively on Drupal 7 on this thread. To use or test the latest patch, you must have the latest -dev version of Drupal 7 core. Because there is no 7.x-1.x-dev branch yet of this module, you apply the patch to a CVS checkout of the 6.x-1.x-dev branch.

--Ben

Both Berdir and myself are just working on the D7 version. We don't use this module in D6. So you may want to check with the module maintainer for questions about D6.

--Ben

Hey Berdir,

In follow up to my views integration testing in #26, I've now tested the latest patch with the rest of the module. Here are the issues I noticed and suggested string changes (item letters refer to the original bug report in comment #22):

a. If you leave the points field blank on a node and try to save, you get this error:

PDOException: SQLSTATE[HY000]: General error: 1366 Incorrect integer value: '' for column 'points' at row 1: UPDATE {userpoints_nodeaccess_points} SET points=:db_update_placeholder_0, tid=:db_update_placeholder_1 WHERE (nid = :db_condition_placeholder_0) ; Array ( [:db_update_placeholder_0] => [:db_update_placeholder_1] => 0 [:db_condition_placeholder_0] => 40 ) in userpoints_nodeaccess_update_node() (line 210 of /Users/benkaplan/git/drupal/sites/all/modules/userpoints_nodeaccess-DRUPAL-6--1/userpoints_nodeaccess.module).

c. There is small typo here. It currently reads: "By trading in 50 points in Uncategorized category, you now have full access to this content." The word "the" is missing. It should be: "By trading in 50 points in the Uncategorized category, you now have full access to this content."

l. We still need to add CSS classes to the "Get access" button and the other printed messages. Ideally, it would be great to add an umbrella CSS class for all messages and an additional CSS class depending on which message is printed (not enough points, get access, anonymous, etc.) This way, the site admin could style these messages all the same or make certain ones highlighted differently. For the messages, I'd probably make the umbrella CSS class italic by default. It just seems like the messages would look good in italic.

m. For anonymous users, what if we just simplify this to: "Requires @points !points in the @category category." We simply state the facts to anonymous users and don't make a judgement on whether they will be able to gain access once they login. It's a more informative message than just access denied. A login or register message could always be specified by the site admin in a field attached to the "Userpoints node access denied" view mode. See dd) below for a possible variation to this message if only one userpoints category is configured.

o. I've thought about this some more and I can really see the benefits of a simple "Grant access to a node" Rules action. That might be all that we really need for now (no immediate need for a rules event or conditions). That one action could largely replace the need for an entire API. The primary use case for me would be to use Userpoints Nodeaccess in conjunction with Drupal Commerce. Because Drupal Commerce is entirely rules-based, if a user bought access to a node (instead of trading in points) this rules action would allow them to be added to the access list upon purchase. So if possible, it would be great to add this rules action to our list of priorities.

ADDITIONAL ISSUES:

aa) If a user does not have "Trade Userpoints for node access" permission, then we should print "Access denied: You do not have permission to trade in !points." as the message. This way it's clear why access is denied.

bb) On the Points settings page (admin/config/people/userpoints/settings), the description for "Enabled by default" currently reads: "This can be overridden for each content type." Change this to: "If checked, Userpoints Nodeaccess functionality is activated by default for each content type. This can be overridden on the content type edit page."

cc) Also on the points settings page: Change: "Default Category" to "Global Default Category". I thought this change might be good because "Default Category" is the label used on the content type edit page. So this one on the settings page is the "global" version.

dd) As briefly discussed in IRC, after seeing the new message strings I wrote displayed on the page, the text strings for each case (not enough points, get access, anonymous) seemed overly long. So I recently had some ideas on how to streamline it. I also was wondering if it is possible to check whether the site admin has any userpoints categories configured and to display different messages in the case of one default category or multiple categories (since we don't need to mention categories if only one is configured).

So here are my new suggestions....

>> 'Not enough points' message (multiple categories):

"Requires @points !points in the @category category. You only have @points !points available."

>> 'Not enough points' message (only one default category):

"Requires @points !points. You only have @points !points available."

>> 'Get access' message (multiple categories):

"Requires @points !points in the @category category. You have @points !points available."
["Get access" button goes here]

>> 'Get access' message (only one default category):

"Requires @points !points. You have @points !points available."
["Get access" button goes here]

>> 'Anonymous user' message (multiple categories):

"Requires @points !points in the @category category."

>> 'Anonymous user' message (only one default category):

"Requires @points !points."

If it is not possible to display different messages based on category configuration, we could simply go with the "multiple categories" strings shown above.

That's all I've got! We're not too far away here. Just a reminder that I've included views-related notes in comment #26.

--Ben

Hey Berdir,

I've begun testing the patch in #37, but one new error has popped up that makes it a bit difficult to test. So I'm hoping that we can fix this before I continue with more testing.

Basically, if I visit a node page that requires points (and the user has enough points to buy the node), I'm now getting the following fatal error:

Recoverable fatal error: Object of class stdClass could not be converted to string in userpoints_nodeaccess_trade_access_form() (line 302 of /Users/benkaplan/git/drupal/sites/all/modules/userpoints_nodeaccess-DRUPAL-6--1/userpoints_nodeaccess.module).

I'm also getting this fatal error if visiting a list page (like the front page) with the node's teaser on it. And I've confirmed that the error goes away (and everything works fine) if the user does not have enough points to buy the node.

Can you take a look? And if you feel this patch is getting too large to deal with, we could definitely move our work to github if you would like.

Thanks,
Ben

Hey Berdir,

I created my own github fork based on your repository. First, I can confirm that the bug reported in comment #38 has been fixed. Thanks!

Here is a report on the various outstanding issues related to the main module. I'll tackle the views integration stuff in a subsequent comment:

a) Confirmed this is fixed.
c) Confirmed this is fixed.

l) Most of the CSS classes look great. However, if the printed message is "Requires 50 kudos in the Uncategorized category", I'm not seeing any CSS class in Firebug. (The only classes I see on the div are "content" and "clearfix".) Can we get a CSS class for this? We would want this message to be styled the same as the other messages.

Also, if a user has "granted" access to a node, is it possible to add a CSS class somewhere so that we can tell at a glance that access has already been granted? Currently, the drupal_set_message is great, but that only is visible immediately after access is granted. How about adding a class to the entire node similar to how the ".node-unpublished" class is handled in Bartik for unpublished content (so you can change the color of the entire background of the node). Ideally, this class should be present both for the teaser and full node view. But I'm not sure how feasible this type of class insertion would be. Other ideas?

m) / aa) I agree that we don't need to differentiate between these two cases (no permission and not logged in). The message we have now looks good! :-)

o) Sounds good.

bb) Confirmed this is fixed.
cc) Confirmed this is fixed.

dd) Thanks for the explanation. I agree that the drawbacks of the category check outweigh the benefits. So let's not worry about that. However, to make the messages a bit shorter and simpler, let's change them to:

>> 'Not enough points' message:

"Requires @points !points in the @category category. You only have @points !points available."

>> 'Get access' message:

"Requires @points !points in the @category category. You have @points !points available."
["Get access" button goes here]

The sentences shown above should now fit nicely on a single line. And because the sentences don't mention "access" directly anymore, I think they can be used in more use cases (such as when the node is a page for a physical product that is shipped to the user).

ee) NEW: Is there any way to allow the text of the "Get access" button to be configured by the site administrator at the global, content type, and (possibly) node levels? This would be similar to how the points category is configured now. This issue came up when I started thinking about my actual use case: We'll likely be using the module not only for nodes with written content, but also for node pages that describe physical products that are actually shipped to the user. For such instances, we might want the button to say "Get this prize". Likewise, for nodes that play videos or audio tracks, we might want to change the button to say "Watch this video" or "Listen to interview". So would this be difficult to implement? It would be great if it's configurable at the global, content type, and node levels, but if that's too difficult, just a setting on the content type edit page could work (with "Get access" being the default button text if it is not overridden at the content type level). What do you think?

That's it for the main module. I'll test views integration next and attach my notes to a subsequent comment.

--Ben

Hey Berdir,

I tested your latest changes documented in #41. Here's what I found:

l) * Thanks for the span class around that message... using the denied class is fine.

* Yes, some additional classes to differentiate between the different access conditions would be great. Specifically, I need two no-access sub-classes (one for "have enough points" and another for "not enough points") and one access sub-class (bought access). I think that would be enough to target what I need.

* If a node does not require any points to access, it is getting the "userpoints-nodeaccess-node-no-access" class. Is this the intended behavior? I would have thought that content with unlimited access would get the "userpoints-nodeaccess-node-access" class.

* On a list of node teasers like the front page, any teaser with a message printed in it (a message such as "Requires 100 kudos in the Access Points category") is not getting the "node-teaser" class. As a result the teaser area is not being styled the same as other teasers. I noticed this using the Bartik theme because teasers in Bartik have a line (border) at the bottom of the teaser area. But because the .node-teaser class is not being applied on teasers with a message in it, the border line is not being displayed (plus the message text is the wrong size compared to other teasers). Note that any nodes in which access has been granted do not have this problem (the .node-teaser class is displaying properly). This is probably because no message is printed for nodes in which access is granted.

dd) There's a problem with both of the changed messages. It may have resulted from the fact that I originally typed @points for both point tokens in the message (I was just using it as a generic placeholder), when in the first case it's the "price" of the node and in the second case it's the "points" of the user. Currently, for a node that costs 10 points (when a user has 7 points), the message currently reads:

'Requires 7 kudos in the Access Points category. You have 7 kudos available.'

So the message is repeating the user point total twice instead of using the "price" in the first instance.

Second, I think you may have mixed up the two messages that I changed (one in which the user doesn't have enough points and another in which the user does have enough points). In the case where the user doesn't have enough points, the word "only" should appear in the sentence. Conversely, in the case where the user does have enough points, the word "only" shouldn't appear. This is because "only" is meant to signify that "it's not quite enough." But currently, it's backwards... the word "only" is appearing in the wrong message.

Sorry for the confusion about all of this. If any of this is unclear, let me know. :-)

ee) Sounds good. Let's add the node type to the form as you suggest.

That's it for all of the non-views related issues. I'll attach my views testing results to a subsequent comment...

--Ben

Hey Berdir,

I just tested your latest changes. Here are the bugs I found:

l)

* In cases where the user does have enough points to purchase access, the "userpoints-nodeaccess-node-no-access-notenough" class is still being applied to the node. This is occurring on both the teaser and node view. Instead, a "userpoints-nodeaccess-node-no-access-enough" (or whatever the "enough points" or "trade in" class name should be) should be applying.

* If the user does not have enough points to purchase a node and that node appears in a list of teasers, then something very strange is happening: The

Hey Berdir,

At long last, here are my latest testing results related to views integration:

i. Confirmed this is fixed. (And thanks for the explanation about sortability.)

ii. This looks good. Only one fix: On a node that requires points, an anonymous user is seeing a "Denied" message instead of the following:

"Login to view status" (Note: If possible, "Login" should be a link to Drupal login page)

iii. The CSS span classes look great. We'll just need to add one for "Login to view status" once we fix ii (above).

iv. I love your proposal. It's much simpler yet it still accomplishes a lot. Let's do it! :-) For the checkboxes, I just have the following updates to the language of the labels to be a little more clear:

- Show nodes the current user can buy immediately.
- Show nodes the current user could buy if he had more !points.
- Show nodes that don't require !points.

v. OK, let's not worry about it. Probably a views bug.

vi. Let's not worry about this. I like your proposal better. ;-)

vii. and viii. Okay, sounds good. Do we have an existing issue thread in userpoints.module for that?

ix. Confirmed this is fixed.

x. Unfortunately, I'm still getting this error with the latest patch.

xi. I don't think this item was completed yet: In the "userpoints_nodeaccess_node_buys" view, the "no results" message is set to: "No userpoints nodeaccess bought yet." Change this to: "No users have have been granted access yet."

xii. I don't think this item was completed yet. I've simplified it based on your prior comments about category and sortability. :-) So only this needs to be done: In the "userpoints_nodeaccess_node_list" view, add a default sort criteria using "Node: Post Date" and sort it descending. Enable the "Sortable" option for the "title" and "points" fields.

As you can see, not too much left to do. We're very close to being done, I think. :-)

--Ben

I tested your changes in #47 and everything is working great. Both l) and dd) are fixed!

Just a quick question: In the case of "All existing links are re-added," you said this will break once that patch is committed to core. How will it break? Will the links just stop appearing or we will we actually get an error message on the page?

So the only thing we have left is the views related stuff documented in #46! Yay! We're very close....

--Ben

Berdir,

I tested the latest views changes and things are working quite well... the new filters are very cool. I've just noticed a little odd behavior and an edge case we may need to address. Here's my list:

1. The "Show nodes that don't require points" filter is working very well. And the other two filters do filter out what they are intended to filter out. But I'm not sure the logic is totally right on those two filters because extra nodes are still showing up. For instance, both the "can buy immediately" and "could buy if more points" filters are still displaying nodes that the user has bought in the past. Interestingly, checking or unchecking each of those filters results in different "bought" nodes being displayed. I think what is going on is that even though the user has already bought the node, we are still comparing his current point total to the price of the node. So if an "already bought" node costs more than the number of points the user has, it shows up if "Show nodes the current user could buy if he had more points" is checked. Conversely, if an "already bought" node costs less than the number of points the user has, it shows up if "Show nodes the current user can buy immediately" is checked. So the bottom line, I think, is that we should be filtering out already bought nodes if the Access Status filter is in use.

A similar thing is happening for nodes that the user has access to because of authorship or the bypass access permission. Such nodes are showing up based on a comparison of the price to the number of points the user has. So if a user has authored a node and the node costs more than the number of points the user has, it will show up if "Show nodes the current user could buy if he had more points" is checked. So I think we need to filter out nodes that the user has authored if the Access Status filter is in use.

But what should we do for users who have the bypass access permission? Would we display no nodes (since he can't technically "buy" any)? Or would we display all nodes available for purchase if "Show nodes the current user can buy immediately" is checked (because he can technically see all of them even if he can't really buy them). From an admin perspective, it seems he would want to see all nodes available for purchase instead of seeing a view with no nodes. But I'm interested to hear your thoughts. (I guess "Show nodes that don't require points" should still work for a user with the bypass access permission.)

2. Okay, this next one is an edge case. I'm not sure if we should deal with it or just not worry about it. Basically, it's possible to have a node with "Unlimited Access" that still has a "points" price listed. This type of node is showing up in the view no matter which access filter is set. How can this type of node be created? If you set a point price for a node and then later disable "Userpoints Nodeaccess Options" on the content type edit page, the result is a node with a price that still has unlimited access. What should we do in this case? When it is listed in a view, it shows the price in the price column, but Unlimited Access in the status column.

One option would be to show the point price as "0" for the node if the content type is disabled... but I'm not sure of the added performance costs of this extra check (since it is going against the actual point value listed in the database). And I don't think we want to delete the point price of the node from the database when the content type is disabled... a site admin may disable the content type temporarily but doesn't want to have to re-enter a price when the content type is re-enabled. What do you think? Again, this is an edge case so I don't want it to slow us down. The added complication, of course, is that this type of node is showing up even when the "Show nodes that don't require points" filter is unchecked.

3. If "Show nodes that don't require points" is checked, two types of nodes are added to the view--nodes that have a price of 0 points or nodes of a content type that don't require points. This, of course, is exactly what we want. But a possible issue is in the "Points" column... nodes of a content type that don't require points has a blank points field. Conversely, nodes that have a price of 0 points show an actual "0". Neither of these is confusing on their own, but when you combine them the result is that some points field show 0 and some are blank. I like being able to differentiate between the two cases, but should something be entered in the blank field? Maybe a "None required" (this could still be confusing)? Maybe just a "0"? But the problem with that is that the "0" would have to be computed and that would ruin the ability to sort (I think).

Anyway, I'm not sure this is a big deal (the site admin could always remove the points column for certain types of views), but I thought I would bring it up in case there was an easy fix.

4. If "Show nodes that don't require kudos" is the only Access Status filter option that is checked, some "Need x more points" nodes are still showing up in the view.

5. One small typo on the "no results" message for the "userpoints_nodeaccess_node_buys" view. The word "have" is repeated twice in a row in the current displayed message.

Anyway, I'm interested to hear your thoughts... a little logic cleanup and I think we're done!

--Ben

P.S. If I didn't mention an issue in my prior comment above, then that means that I tested it and confirmed that it is now fixed. Thanks! :-)

Hey Berdir,

Everything is working great! Only one problem: I can't seem to change any of the Access Status filter settings. If I try to check or uncheck one of the boxes and click update, it's not retaining the changes. So I know that the default filter options work great ("buy immediately" and "could buy if more points" are both checked), but I can't actually change these options in the userpoints_nodeaccess_node_list view.

I also tried adding the Access Status filter to the "buys" view, but I couldn't save the filter options there either. I also got the following error message when adding and saving the Access Status filter (with all options unchecked since I couldn't change them):

Warning: array_filter(): The first argument should be an array in userpoints_nodeaccess_handler_filter_access->query() (line 66 of/Users/benkaplan/git/drupal/sites/all/modules/userpoints_nodeaccess/userpoints_nodeaccess_views/userpoints_nodeaccess_handler_filter_access.inc).

Also, one small thing: I don't think the following typo was fixed:

One small typo on the "no results" message for the "userpoints_nodeaccess_node_buys" view. The word "have" is repeated twice in a row in the current displayed message.

The empty message still incorrectly currently reads: "No users have have been granted access yet."

--Ben

P.S. I tried to filter out nodes authored by the current user by using the "User: Current" filter set to "No", but I got a PDO error. I think I'm doing this correctly, right? Maybe Views is having problems with this filter at the moment.

One bit of good news: I can confirm that my error described in "x." above (error when sorting columns) is now fixed after updating to the latest version of views! :-)

Berdir,

I almost marked this RTBC... and you could still treat it as such. I tested the two views a multitude of ways and only noticed two small things:

1. If 'Show nodes the current user could buy if he had more points' and 'Show nodes that don't require points' are both checked (and nothing else), then "free" nodes (those without a price) are not actually displaying. The bug is limited to this particular configuration and I'm not sure why.

2. If you try to only check 'Show nodes that don't require points', then the setting won't be retained upon clicking update. This doesn't really matter because if you are trying to list "free" nodes it is better to use the points filter (and choose a points value < 1). But I thought I would mention it because some users may be confused why this setting won't save.

That's it. There's nothing else. Very minor stuff. I think we're ready to open the 7.x branch. :-)

--Ben

Awesome! Great work on this. :-)

Thanks to you, Berdir! Yes, I'll create the follow-up issues for postponed items. I'll plan on doing that later today or tomorrow.

--Ben