Drupal 7 port

Hey Berdir,

I tried to apply the patch in #1, but it failed against the latest 6.x-1.x-dev release. Here's the error that I got:

patching file userpoints_nc.info
Hunk #1 FAILED at 3.
1 out of 1 hunk FAILED -- saving rejects to file userpoints_nc.info.rej
patching file userpoints_nc.module

Any idea why this wouldn't apply?

--Ben

It looks like the only part of the patch that is failing is the .info file. Not sure why this would be. But if the rest is applying I can manually change the .info file to 7.x just so that we can get testing started.

--Ben

I've just completed testing the patch in #1 and it is working quite well! Points are being awarded for both nodes and comments in the proper categories. And the "Take away points on node delete" setting (whether checked or unchecked) is working well. So I've found only two small bugs:

i) When comments are moderated (not approved by default) and an administrator approves the comment, then the following error message is being displayed at the top of the page:

Notice: Undefined variable: node in userpoints_nc_comment_update() (line 242 of /Users/benkaplan/git/drupal/sites/all/modules/userpoints_nc/userpoints_nc.module).
Notice: Trying to get property of non-object in userpoints_nc_comment_update() (line 242 of /Users/benkaplan/git/drupal/sites/all/modules/userpoints_nc/userpoints_nc.module).
Notice: Trying to get property of non-object in userpoints_nc_comment_update() (line 245 of /Users/benkaplan/git/drupal/sites/all/modules/userpoints_nc/userpoints_nc.module).
Notice: Trying to get property of non-object in userpoints_nc_comment_update() (line 245 of /Users/benkaplan/git/drupal/sites/all/modules/userpoints_nc/userpoints_nc.module).

ii) We need to update the field label underneath "Points for moderating a comment". The field label below that field should be "Assign Points for moderating a comment to category". Currently, the word "posting" (incorrect) is used instead of "moderating".

Also, here are some additional issues that I noticed while using the module. But as discussed in IRC, perhaps these are technically "feature requests" (even though they affect basic usability) that I can post later in separate issues:

A. Points are being awarded for comments even when the comment is not yet approved. If a comment goes into moderation pending approval, points shouldn't be awarded until a site administrator approves it (or else there should be a setting to enable this).

B. The module awards points even if a node is unpublished. In a lot of workflows points wouldn't be granted until a node is approved and published. So there should probably be a setting for this ("Don't award points until node is published") or else this is how the module should work by default.

C. Nodes have a "Take away points on node delete" setting. But comments don't have an equivalent setting. By default, points for comments are taken away when the comments are deleted. There should probably be a setting to turn this on and off.

D. For nodes, you can assign different point awards per content type. This should be extended to comments so that different award amounts for comments can be specified per content type. This is actually important so that you can "turn off" point awards for comments on certain content types (for instance, follow-up comments on submitted support ticket requests).

E. There is no way to remove Userpoints expiration defaults (unlike when you add points manually). There should probably be a setting to enable or disable expiration for the node or comments points award.

Looking forward to testing the next version. Berdir, should we ask kbahey to give you CVS commit rights for this module, too (since it's separate from Userpoints Contrib but we're treating it basically the same)?

Cheers,
Ben

I really like the idea of displaying options in a table... I'm envisioning this would look something like Private Message's "User Blocking Rules" table. And because many of these settings would be checkboxes (and we could keep the labels short), I don't think we'd have too many width problems. (This type of configuration table is a big advantage of direct integration in this case compared to trying to do something similar with the Rules module.)

One other possible way of dealing with the 10-20 content types issue: We could make it more like "User Blocking Rules" in that no content types would be displayed/configured by default and the site admin would have to add a rule (row) for each content type for which he wants to award points (a separate row for a node or a comment per type). The advantage would be that you wouldn't have crazy numbers of rows unless you really need it. The downside is that you would have to add these rows manually (but it would be less cluttered).

--Ben

Of the "feature requests" in #5 above, I'd say "A" and "B" are the most critical. And I'd say "C" is nice, but not at all necessary...

--Ben

Hey Berdir,

Wow, this is starting to look really slick. It's great to have content type settings that match those of Userpoints Nodeaccess on the content type edit page. I've been going through both modules making the language and format match each other... they are going to look really good together.

The core functionality of the latest patch is working great. The "only give points for published" option is working really well. Most of the things that don't work quite right are edge cases... I've done extensive testing (changing the published state while changing the author).

So here are my bug reports/changes/fixes:

FUNCTIONALITY

A. To see this bug, follow these steps:

1) Check the "Only give points for published nodes and comments." option.
2) Create a new content type called "Test Type" and make it unpublished by default.
3) Create a new Test Type node. Save. It will be unpublished and no points are awarded yet (good!)
4) Publish this content. You will get the points.
5) Unpublish the content and simultaneously change author to Person2.
6) Person2 loses the points (not you)....

Instead, you should be the one losing the points.

B. To see this bug, follow these steps:

1) Check the "Only give points for published nodes and comments." option.
2) Create a new content type called "Test Type" and make it unpublished by default.
3) Create a new Test Type node, but manually check the "Published" checkbox before saving. Save. You will be awarded the points (good!)
4) Unpublish the content and simultaneously change author to Person2. Note that the node is unpublished but no points are deleted for any user.
5) Now publish the content. Person2 will earn the points and you will lose the points. (This is technically the correct outcome, but you should have lost the points in step 4, not step 5. This timing issue will create a problem in C below.)

Note that comparing A and B above is a little strange... it's basically the same workflow, but the outcome is different based on whether the "published" button was manually checked on node creation (even though the content type default is unpublished in both examples).

C. To see this bug, follow these steps:

1) Check the "Only give points for published nodes and comments." option.
2) Create a new content type called "Test Type" and make it unpublished by default.
3) Create a new Test Type node, but manually check the "Published" checkbox before saving. Save. You will be awarded the points (good!)
4) Unpublish the content and simultaneously change author to Person2. Note that the node is unpublished but no points are deleted for any user.
5) Now publish the content and simultaneously change the author back to you. You will be awarded the points again. This is a problem because you've been awarded the points twice for the same node... no points have ever been deleted from any user (the points should have been deleted from you in Step 4).

If any of this is confusing, do let me know.

ADDITIONAL OPTIONS

D. As discussed in IRC, create a "Don't deduct points when the content author changes" option. This should probably be both a global and per-content type setting (because some content types may employ multi-user revisions and others may not).

This option helps solve the workflow where lots of different people could be offering revisions... certain revisions would approved and published, thus changing the author once a new revision is published. It would be weird to deduct points from the old author every time a new revision was published and the author changed.

E. As discussed in IRC, create a "Don't deduct points when deleting or unpublishing" option. This should probably be both a global and per-content setting (because some content types may need a different option because content is frequently deleted for those types). This option would affect both nodes and comments.

ON THE CONTENT TYPE EDIT PAGE:

F. Add a checkbox labeled "Enabled". This would be displayed just like the "Enabled" checkbox we included on the Userpoints Nodeaccess module tab and would serve the same function. The description should read: "If checked, !points can be awarded for creating or commenting upon content of this type."

G. Change "!Points for posting content of this type". Change this to: "!Points awarded for new content". Add a description that reads: "Set the number of !points to be awarded when a user adds a node of this type."

H. Change "Assign !Points for posting content of this type to category" Change this to: "!Points category for new content". Add a description that reads: "Choose the category of !points to be awarded when a user adds a node of this type."

I. Change "!Points for posting a comment". Change this to: "!Points awarded for new comments". Add a description that reads: "Set the number of !points to be awarded when a user adds a comment."

J. Change "Assign Kudos for posting a comment to category". Change this to: "!Points category for new comments". Add a description that reads: "Choose the category of !points to be awarded when a user adds a comment."

K. In the vertical tab, text is currently displayed in the following format:

Userpoints Options
10 in category General are assigned for posting content.

Change this to:

Userpoints Awards
10 points (General category) for new content.
2 points (Activity category) for new comments.

If possible, make the category names italic. I think this syntax allows the admin to see more at a glance. And we're also including the comment points/category, too.

ON THE USERPOINTS GLOBAL SETTINGS PAGE:

L. Create a "Enabled by default" checkbox just like the one that Userpoints Nodeaccess has. The description would read: "If checked, all content types award !points by default. This can be overridden for each content type on the content type edit page."

M. On the Userpoints settings page, change the following text:

Change: "Only give points for published nodes and comments." To: "Only award !points for published nodes and comments." (Note that the word "points" is hard-coded and does not currently use the !branding feature.)

Change: "If checked, uses will only receive !points when their content is published and will be removed again when it is deleted or unpublished." To: "If checked, users only receive !points when nodes or comments are published. These !points are deducted if the posting is deleted or unpublished." (Note that the word "users" has a typo.)

Change "!Points assigned by default for posting content." To: "!Points awarded by default for new content". Add a description that reads: "Set the default number of !points to be awarded when a user adds new content. This can be overridden for each content type on the content type edit page."

Change "Assign !Points by default for posting content to category". To: Change this to: "!Points category by default for new content". Add a description that reads: "Choose the category of !points to be used by default when a user adds new content. This can be overridden for each content type on the content type edit page."

Change "Points for posting a comment by default". To: "!Points awarded by default for new comments". Add a description that reads: "Set the default number of !points to be awarded when a user adds a comment. This can be overridden for each content type on the content type edit page."

Change "Assign Points for posting a comment by default to category". To: "!Points category by default for new content". Add a description that reads: "Choose the category of !points to be used by default when a user adds a comment. This can be overridden for each content type on the content type edit page."

ON THE /MYUSERPOINTS PAGE

N. In the operation field, perhaps "published" and "unpublished" should be "publish" and "unpublish". This way, they would match "delete" and "insert". Alternately, everything could be done in the past tense. But they should be consistent.

O. There is no place in the UI to specify the operation "description". As a result, all of the descriptions that a user sees say "none". Probably, a description field should be added on content type edit page so that the site admin could specify a different message per content type. Ideally, we would want two description fields on this page... one for nodes and one for comments (making it easier to explain the operation). If no description field was entered, the description could default to "none" (like it currently does).

P. In specifying the "operation", is it possible to use two words? Or is that a bad idea? For instance, instead of "publish" and "unpublish", we could have: publish content, publish comment, unpublish content, unpublish comment, delete content, delete comment. Just thought it would be more descriptive, but given that we are changing the operation field anyway (per our discussion in IRC), perhaps you have a better idea.

Q. If the "only give points for published" setting is turned off, the operation defaults back to "insert". If possible, it would be better if could default back to "add content" and "add comment". But again, I'll defer to you on what you think is best for the operation field.

Cheers,
Ben

Bumping this so I can keep track of it in my issue queue...

Hey Berdir,

Before testing the actual functionality of the latest patch, I thought it would be easier to edit the string changes all at once. So I incorporated all of your string change comments/suggestions, fixed some typos, used "content" instead of "node", and shortened some descriptions because they didn't look good on two lines (usually by deleting "on the content type edit page" for each description).

Here are string changes for the points settings page. My changes are shown below based on their order on the page (from top to bottom):

Change this: "If checked, users only receive !points when nodes or comments are published. These !points are deducted if the posting is deleted or unpublished." To this: "If checked, users only receive !points when nodes or comments are published." (We don't need the second sentence because we have a separate setting for that now.)

Change this: "If checked, the old owner will loose the points he gained from the content. This setting can be overriden for each node type." (Note that "loose" should be "lose".) To this: "If checked, the old content author will lose any !points that he previously gained. This setting can be overridden for each content type."

Change this: "Deduct !points when the content is deleted or unpblished." (Note that "unpublished" is misspelled.) To this: "Deduct !points when content is deleted or unpublished"

Change: "If checked, the owner of a content will loose points he gained. This setting can be overriden for each node type." To this: "If checked, the content author will lose any points that he previously gained. This setting can be overridden for each content type."

Change: "!Points awarded by default for new content" To this: "Default !points for new content"

Change this: "Set the default number of !points to be awarded when a user adds new content. This can be overridden for each content type on the content type edit page." To this: "Set the default number of !points to be awarded when a user adds new content. This can be overridden for each content type."

Change this: "!Points category by default for new content" To this: "Default !points category for new content"

Change this: "Choose the category of !points to be used by default when a user adds new content. This can be overridden for each content type on the content type edit page." To this: "Choose the category of !points to be used by default when a user adds new content. This can be overridden for each content type."

Change this: "!Points awarded by default for new comments" To this: "Default !points for new comments"

Change this: "Set the default number of !points to be awarded when a user adds a comment. This can be overridden for each content type on the content type edit page." To this: "Set the default number of !points to be awarded when a user adds a comment. This can be overridden for each content type."

Change this: "Kudos category by default for new content" (Note that is says "content" instead of "comments".) To this: "Default !points category for new comments"

Change this: "Choose the category of !points to be used by default when a user adds a comment. This can be overridden for each content type on the content type edit page." To this: "Choose the category of !points to be used by default when a user adds a comment. This can be overridden for each content type."

--Ben

Here are string changes for the content type edit page:

Change this: "If checked, the old owner will loose the points he gained from the content." To this: "If checked, the old content author will lose any !points that he previously gained."

Change this: "Deduct !points when the content is deleted or unpublished." To this: "Deduct !points when content is deleted or unpublished"

Change this: "If checked, the owner of a content will loose points he gained." To this: "If checked, the content author will lose any points that he previously gained."

Change this: "!Points awarded for new content". To this: "!Points for new content"

Change this: "!Points awarded for new comments". To this: "!Points for new comments"

VERTICAL TAB TEXT: I agree that this doesn't look great with each description two lines (four lines altogether). So here's my suggestion for making everything fit in two lines (one for each point award type). The example shown below uses the "Contribution" and "Participation" categories:

Content: 10 (Contribution)
Comment: 2 (Participation)

Note that I'm not sure about the placeholder for the points amount, so "10" and "2" need to be replaced. I also left out the word "!points" because I think that is understood from the vertical tab header.

--Ben

Hey Berdir,

I've been testing the latest functionality, but one bug has come up that has prevented further testing. Basically, when saving a comment, no points are being awarded (even though the content type is enabled) and I'm getting the following error at the top of the page:

Notice: Use of undefined constant Type - assumed 'Type' in userpoints_nc_comment_presave() (line 394 of /Users/benkaplan/git/drupal/sites/all/modules/userpoints_nc-DRUPAL-6--1/userpoints_nc.module).
Notice: Object of class stdClass could not be converted to int in userpoints_nc_comment_presave() (line 394 of /Users/benkaplan/git/drupal/sites/all/modules/userpoints_nc-DRUPAL-6--1/userpoints_nc.module).
Notice: Use of undefined constant Type - assumed 'Type' in userpoints_nc_comment_insert() (line 365 of /Users/benkaplan/git/drupal/sites/all/modules/userpoints_nc-DRUPAL-6--1/userpoints_nc.module).
Notice: Object of class stdClass could not be converted to int in userpoints_nc_comment_insert() (line 365 of /Users/benkaplan/git/drupal/sites/all/modules/userpoints_nc-DRUPAL-6--1/userpoints_nc.module).

The comment is being successfully saved, however. And I seem to be getting this error on all comments... even when UP-NC is not enabled on a node. Also, all other default settings are still in place... I haven't de-selected any of the new options (like the stuff with changing authors or deleting nodes).

If you have a chance to fix this issue that would help me be able to test the comment points functionality.

Thanks,
Ben

P.S. I also noticed that debug information is also currently being printed at the top of the content type edit page. Just thought I'd mention it.

1) Points are now being awarded for comments (yay!), but I'm still getting this error:

Notice: Use of undefined constant Type - assumed 'Type' in userpoints_nc_comment_presave() (line 379 of/Users/benkaplan/git/drupal/sites/all/modules/userpoints_nc-DRUPAL-6--1/userpoints_nc.module).
Notice: Object of class stdClass could not be converted to int in userpoints_nc_comment_presave() (line 379 of/Users/benkaplan/git/drupal/sites/all/modules/userpoints_nc-DRUPAL-6--1/userpoints_nc.module).

I'm getting this error both on content types that have UP-NC enabled and those that don't.

2) The string changes look great. But there were a couple typos introduced on the content type edit page in the latest round:

a. One field has been mislabeled: "!Points for new comments" is actually labeling the content category field. This should be: "!Points category for new content"

b. There is an extra word in the following label: "!Points awarded for new comments". To be consistent with our other labels, this should be: "!Points for new comments"

c. Vertical tabs look great, but because the category names are italic, I'd probably make the parentheses around them italic, too. Otherwise, it looks like the letter run into one another.

3) On /myuserpoints, I like how the module is using the new "reason" field. But I'd like to suggest that we change the strings that display in the following instances. I also think that it's important that we include the content type name as part of the reason. This is because there will different points awards depending on the content type and it would be quite confusing to see certain content and comments awarded different amounts of points with no apparent reason. I also think the content type name, if possible, should be lowercase and italic. Here are the changes:

a. Change "Commented." To this: "Commented on [content-type]." (Example: "Commented on article.")

b. Change "Content was created." To: "Added new [content-type]." (Example: "Added new article.")

c. Change: "Lost ownership of content." To this: "Removed as [content-type] author." (Example: "Removed as article author.")

d. Change: "Gained ownership of content." To this: "Credited as [content-type] author." (Example: "Credited as article author.")

e. "Content was unpublished." "Unpublished [content-type]." (Example: "Unpublished article.")

f. "Content was deleted." >> "Deleted [content-type]." (Example: "Deleted article.")

--Ben

Hey Berdir,

1) and 2) are fixed. Thanks.

As for 3), you make some good points. Here's one potential solution: What if we just used the node title instead of the content type name? This would address your concern because every user is allowed to see the node title (even when they may not be able to see the content type name). To keep this consistent, we'd also need to use the node title instead of content type name for nodes as well as for comments. The only drawback to this is that node titles may be longer than content type names, but maybe this is better for the user because users will likely remember the node title (but may not know the content type). It might be a nice feature to be able to scan /myuserpoints and see the various node titles you got points for (similar to Drupal's tracker module or Drupal.org's issue queue). What do you think?

Also, I noticed one bug related to #3. If you have points transactions related to a node and then delete the node, the prior transactions (as well as the current ones) are displaying like this:

Deleted <em class="placeholder">content</em>.
Published <em class="placeholder">content</em>.
Unpublished <em class="placeholder">content</em>.
Credited as <em class="placeholder">content</em> author.
Removed as <em class="placeholder">content</em> author.
Added new <em class="placeholder">content</em>.

I like the use of "content" as the default placeholder, but the HTML tags are currently being displayed.

--Ben

Just a note that this module may need to be updated to use Userpoints new 'Reason' column. Also, might we want to change the operation name supplied by this module to something more readable?

--Ben

I did some more comprehensive testing of the latest patch and here are my comments:

1) If you unpublish a comment (and the setting to deduct points on unpublishing is enabled), the node author is losing the points instead of the comment author.

2) I'm still getting an incorrect outcome in the following sequence of events (B. in my earlier comments) but it is a different incorrect outcome than before:

* Check the "Only give points for published nodes and comments." option.
* Create a new content type called "Test Type" and make it published by default.
* Create a new Test Type node. Save. You will be awarded the points (good!)
* Unpublish the content and simultaneously change the author to Person2. The node is unpublished and your points are deducted. Good!
* Now publish the content. You will gain the points back (not good) and Person2 will lose the points (not good). Instead, person2 should be getting points and nothing should happen to your points.

3) As discussed in IRC, add a configurable setting for the 'Reason' column truncation length (on the /myuserpoints page). I think a good initial default would be 50.

4) Add vertical tab summary descriptions. I suggested this:

Enabled: Yes/No
Published: Yes/No
Content: 3 (General)
Comments: 1 (General)

But Berdir convinced me that this should all be on one line like this: Enabled (published only), 3 (General) for content, 1 (General) for comments"

5) I just realized our admin labels and descriptions for "Deduct !points when the content author changes" and "Deduct !points when content is deleted or unpublished" is slightly misleading. This is because comments are governed by these settings (as confirmed by my recent testing) even though only "content" is mentioned. So in both the label and descriptions (on both the global and content type pages), we should change "content author" to "content or comment author" and "content is deleted" to "content or comments are deleted". I think my understanding of this setting is correct, right?

6) What do you think about changing the 'Reason' strings to:

"Added %title."
"Commented on %title."
"Removed as author of %title."
"Credited as author of %title."
"Removed as commenter on %title."
"Credited as commenter on %title."
"Comment was deleted on %title."
"Comment was unpublished on %title."
"Comment was published on %title."
"Content was deleted: %title."
"Content was unpublished: %title."
"Content was published: %title."

I changed the tense of some of the strings to past tense so it didn't sound like the user was doing it. For instance, "Unpublished %title" sounds like the user unpublished it. "Content was unpublished: %title" makes it clear that the user didn't necessarily do the unpublishing.

--Ben

Hey Berdir,

I remember discussing in IRC that you had recently done some additional work on this. You were adding a few tests as I recall.

Is the patch close to being ready to post? I'd love to test and get an official 7.x branch going on this....

Thanks,
Ben

I tested the latest patch and this is very, very close (not sure why it failed automated testing, though). All of the logic/functionality issues are fixed. Here are the small number of remaining issues:

3) I agree that this should be a setting in Userpoints.module instead. Can you post a patch for this in the Userpoints queue?

Also, here is an updated label and description for this setting in userpoints.module:

Label: Truncation length for 'Reason' field
Description: Choose the truncation length in characters for the 'Reason' field on !points transactions.

4) Vertical tab looks good, but one small point. If enabled by default is unchecked, shouldn't the "(published)" part still display? It isn't currently. I think it should be: Disabled (published) .... Because this setting will still be used globally even if enabled by default is unchecked.

5) This looks great on the global settings page, but the changes have not yet been made to the content type settings page. The updated descriptions you added look good and should be added to the content type settings page as well.

6) The updated language looks good, but it occurs to me we should make one small change. Currently, if content is deleted it reads:

"Content was deleted: %title."

But because the content is deleted, %title is replaced with "content" so that it reads: "Content was deleted: content." This obviously is a bit redundant and we don't really need the node title token in this case. So it should probably be changed to simply:

"Content was deleted."

That's it. One more round and this should be ready to commit and start the 7.x branch.

--Ben

Hey Berdir,

Everything is working great, so I'm marking this RTBC! Let's create the official 7.x branch... yay!

--Ben

P.S. I did notice a small typo that isn't worth holding up RTBC. But if you want to fix it, on content type settings, the label for "Deduct kudos when content is deleted or unpublished" is still wrong. It should be: "Deduct kudos when content or comments are deleted or unpublished".

Also on that page, in the vertical tab is displayed like the global settings, but probably if it is disabled at the content settings level, only "Disabled" should be displayed in the vertical tab summary. Unlike the global settings, none of the other values (points, categories, published or not) have any effect if the content type is not enabled. So it's a little confusing to see that info when disabled.