Support from Acquia helps fund testing for Drupal Acquia logo

Comments

Bastlynn’s picture

FileSize
6.93 KB

Let me know if there are any problems with this patch and I'll be glad to fix em.

Bastlynn’s picture

Status: Active » Needs review

Setting to needs review for someone to take a look.

Status: Needs review » Needs work

The last submitted patch, drupal7port-1129422-1.patch, failed testing.

Bastlynn’s picture

FileSize
4.22 KB

Curse you System Message! *raises fist in mock anger*
Heh. Looks like I rolled against the non-master version of this code. (Looks like someone's already been working on an upgrade here perhaps?) Second verse same as the first, hopefully this will help a little.

Bastlynn’s picture

Status: Needs work » Needs review

Back to need review.

Status: Needs review » Needs work

The last submitted patch, drupal7port-1129422-2.patch, failed testing.

Bastlynn’s picture

FileSize
2.32 KB

Huh. Ok - I reread how to make a patch. I think I'm trying this in the wrong directory. (Please forgive me, this is the first time I've tried making a patch like - ever. I'm not normally this fumbling.)
Third times the charm.

Bastlynn’s picture

Status: Needs work » Needs review

.

Status: Needs review » Needs work

The last submitted patch, drupal7port-1129422-3.patch, failed testing.

Bastlynn’s picture

Status: Needs work » Needs review

Huh. Ok - I don't know what I'm doing wrong in making this patch. To avoid further littering this issue thread with my crud I'm going to leave it at that until I can figure out what I'm screwing up here.

Bastlynn’s picture

FileSize
2.43 KB

Ok, one last shot creating the patch with git diff instead of diff.

Status: Needs review » Needs work

The last submitted patch, 1129422-drupal7port-4.patch, failed testing.

Bastlynn’s picture

Status: Needs work » Needs review

Alright. I give. I followed the directions as far as I know. :-/ Hopefully a human with interest will come along on this thread and take a look. Anyone who can figure out what's wrong with the patch to keep it from being testable - please let me know.

Berdir’s picture

Status: Needs review » Needs work

There is no such module in the 7.x-1.x-branch. Your diff is probably against 6.x-1.x (master).

So what you need to do is git diff against 7.x-1.x like this:

git diff origin/7.x-1.x > something.patch

This will add this all as new files.

Looking at your patch....

+++ b/userpoints_nodelimit/userpoints_nodelimit.moduleundefined
@@ -46,13 +46,13 @@ function userpoints_nodelimit_form_alter(&$form, $form_state, $form_id) {
     }
   }
+  }

This is weird, you're only adding single curly bracket here?

+++ b/userpoints_nodelimit/userpoints_nodelimit.moduleundefined
@@ -108,22 +108,22 @@ function userpoints_nodelimit_get_setting($type) {
- * Implementation of hook_node_type().

The 7 coding style is now "Implements hoook_something().". If you change it anyway...

You will probably also need to do some suggested API changes to the userpoints part, see #871116-9: Drupal 7 port (userpoints_flag) for an example.

Powered by Dreditor.

Bastlynn’s picture

Thank you muchly! I'll try the new diff command and see if that works. :)

re: bracket - Aye, there was a out of balance curly bracket in the branch I was looking at originally, it resulted in a wsod till I tracked down the imbalance. Though looking at the patch file - for some reason the diff didn't pick up the adjusted spacing I did to make everything fall in line. Hm. :-/ - diff and I apparently aren't getting along right now.

I'll check out the API changes and check for differences. I've also had to patch up userpoints_register for a site, so if I can get this one working I'll also be setting up the other sub module as well.

Bastlynn’s picture

Status: Needs work » Needs review
FileSize
12.66 KB

Ok, trying the diff against the branch as you described. I loaded 7.x-1.x as my starting point.

I ran this against coder on minor, so it should be sparkling clean. Node limit only checks against the user's current point values - it doesn't make any changes to them, so there's no API changes needed aside from those for D7.

Let's see if this works.

Status: Needs review » Needs work

The last submitted patch, 1129422-drupal7port-5.patch, failed testing.

Bastlynn’s picture

Status: Needs work » Needs review
FileSize
12.63 KB

Wait. Is it fair when the original had non linux endings, and the update is to remove the non linux endings? ... you're lucky I respect you test machine. ;)

Status: Needs review » Needs work

The last submitted patch, 1129422-drupal7port-5.patch, failed testing.

Bastlynn’s picture

Status: Needs work » Needs review

Hm. What's truly annoying here is I've tried applying the patch on my local machine from the root of the module (in folder userpoints_contrib, as the directions on making a patch said) and it worked. The files do exist in 7.x-1.x based on what I've pulled down. I must still be diffing against the wrong thing. :-/

Thank you for your patience with me on this, Berdir.

Berdir’s picture

Status: Needs review » Needs work

No worries, if the old file has windows line endings then the bot won't accept it. The patch in #16 works for me. We'll just have to ignore the testbot on this patch. You can do by adding a -D7 suffix to your patch (something-D7.patch), then he will ignore that file.

I do get some warnings when applying:

<stdin>:11: trailing whitespace.
configure = admin/config/people/userpoints/settings
warning: 1 line adds whitespace errors.

You might want to fix that.

+++ b/userpoints_nodelimit/userpoints_nodelimit.infoundefined
@@ -1,5 +1,8 @@
+files[] = userpoints_nodelimit.module

You only need to add the files[] declaration if these files contain classes (for example views plugins or tests).

+++ b/userpoints_nodelimit/userpoints_nodelimit.installundefined
@@ -1,32 +1,35 @@
+// $Id$

This line is not necessary anymore, git doesn't use it.

+++ b/userpoints_nodelimit/userpoints_nodelimit.installundefined
@@ -1,32 +1,35 @@
+  db_query("UPDATE {system} SET weight = 5 WHERE name = 'userpoints_nodelimit'");

You should rewrite this using db_update().

+++ b/userpoints_nodelimit/userpoints_nodelimit.installundefined
@@ -1,32 +1,35 @@
+/**
+ * MAke sure hooks are invoked after cck main hooks
+ */
+function userpoints_nodelimit_update_1() {
+  $ret = array();
+  $ret[] = update_sql("UPDATE {system} SET weight = 5 WHERE name = 'userpoints_nodelimit'");
+  return $ret;

You can remove old update functions, this is probably here since Drupal 5 and people did run it already. To be really sure, you can implement hook_update_last_removed().

Will do a further review later on, have to run now. Note that we probably want to move this module over to http://drupal.org/projects/userpoints_nc, like we already did with two node related modules (revision and visits).

Powered by Dreditor.

Berdir’s picture

+++ b/userpoints_nodelimit/userpoints_nodelimit.moduleundefined
@@ -1,91 +1,112 @@
   if (isset($form['#node_type']) && 'node_type_form' == $form_id) {
+    // Add configuration options for each node type in configuration.
     userpoints_nodelimit_node_settings_form($form);
   }
   else {
-    if (isset($form['#node']) && isset($form['#method']) && $form['#node']->type .'_node_form' == $form_id) {
-      $type = $form['#node']->type;
+    // Check on content creation page for node limits.

You can simplify these checks by using hook_form_node_type_form_alter() for the first part, you can directly update userpoints_nodelimit_node_settings_form() to become that hook. See http://api.worldempire.ch/api/userpoints/userpoints_nc-7.x-1.x--userpoin... for an example.

For the node form check, see http://api.worldempire.ch/api/userpoints/userpoints_nodeaccess-7.x-1.x--... for an example. There might be a direct form_alter() too, but I don't know which one.

+++ b/userpoints_nodelimit/userpoints_nodelimit.moduleundefined
@@ -1,91 +1,112 @@
+        if (!isset($form['#node']->nid) && !userpoints_nodelimit_check($form['#node']->type)) {
+        drupal_set_message(userpoints_nodelimit_insufficent_points_message($form['#node']->type), 'error');
+        // If not allowed, then destroy the form.
+        $form = NULL;
+        // Return a simple hidden field so that the function doesn't fail.
+        $form['dummy'] = array(
+          '#type' => 'hidden',
+          '#default_value' => '',

That is a rather strange way of doing that. Try $form['#access'] = FALSE instead.

+++ b/userpoints_nodelimit/userpoints_nodelimit.moduleundefined
@@ -1,91 +1,112 @@
+    array_merge(userpoints_translation(),
+    array('@user_points' => userpoints_get_current_points($user->uid),
+      '@node_points' => variable_get('userpoints_nodelimit_nodepoints_' . $node_type, '?'),
+      '@type' => $node_type,

There should be a user facing version of the node type name, try node_type_get_name($type) or something like that.

+++ b/userpoints_nodelimit/userpoints_nodelimit.moduleundefined
@@ -1,91 +1,112 @@
 function userpoints_nodelimit_node_settings_form(&$form) {
   $form['userpoints_nodelimit'] = array(
     '#type' => 'fieldset',
@@ -94,47 +115,25 @@ function userpoints_nodelimit_node_settings_form(&$form) {

@@ -94,47 +115,25 @@ function userpoints_nodelimit_node_settings_form(&$form) {
     '#collapsible' => TRUE,
     '#collapsed' => TRUE,

Add " '#group' => 'additional_settings', " to the fieldset, to have this show up as a vertical tab. You could also add a summary js script, again, see userpoints_nc for examples.

Two other things to think about

- Maybe allow to configure a global default value, just like we did for userpoints_nc. Not sure if that makes sense.
- Add a option to select which category should be checked. Again, see userpoints_nc, the userpoints_nc_category setting.

Powered by Dreditor.

Bastlynn’s picture

All good suggestions, I'll get started on those this morning. :)

Berdir’s picture

Great. If you have questions about my feedback or userpoints_nc, you can also join #drupal-games in IRC (http://drupal.org/irc), where I am often. That would be faster than posting here (what you can do too, however).

Bastlynn’s picture

Status: Needs work » Needs review
FileSize
19 KB

Changes made, updated to include a global default, and category enabled.

Good catch there on the category, the way categories were ignored in the D6 contrib modules always annoyed me. I can't believe I forgot to set it up here given my experiences.

Let me know if you spot anything else that needs fixing.

Berdir’s picture

Status: Needs review » Needs work

Your code looks good, just some minor things below. I'll also inform BenK about this, he is really good with testing (meaning, if there are bugs, then he'll find them) and also with interface text.

Two general things:
- Please continue to post patches against userpoints_contrib, I will however move the module over to userpoints_nc before I commit it. But doing the patch like this is both easier for you and it's easier to see the changes.
- Not necessary for the initial port, but some tests would be great. If you're interested to write these, http://blog.worldempire.ch/story/writing-automated-tests-drupal-7 should get you started. So once this is commited, you could open an new issue over at userpoints_nc to create some tests.

+++ b/userpoints_nodelimit/userpoints_nodelimit.installundefined
@@ -1,32 +1,38 @@
+    ->condition('name', 'userpoints_nodelimit', '=')

Really minor, the '=' is the default (and 'IN' is the default if an array is passed in as value), so it's not necessary to specify that.

+++ b/userpoints_nodelimit/userpoints_nodelimit.installundefined
@@ -1,32 +1,38 @@
+}
+
+function userpoints_nodelimit_update_last_removed() {
+  // Removed the legacy update_1, that set the weight of nodelimit.

This is missing the "Implements hook_update_last_removed()"docblock.

+++ b/userpoints_nodelimit/userpoints_nodelimit.installundefined
@@ -1,32 +1,38 @@
 }
\ No newline at end of file

Speaks for itself, same for some other files.

+++ b/userpoints_nodelimit/userpoints_nodelimit.jsundefined
@@ -0,0 +1,29 @@
+// $Id$

One more of those to remove :)

+++ b/userpoints_nodelimit/userpoints_nodelimit.jsundefined
@@ -0,0 +1,29 @@
+        if ($('#edit-userpoints-nodelimit-enabled').is(':checked')) {
+          return Drupal.t('Enabled, @points (%category) for content.', params);
+        }
+        else {
+          // If disabled on the node type settings, the other settings don't
+          // matter so we don't display them.
+          if ($('form#node-type-form').length) {
+            return Drupal.t('Disabled.');
+          }
+          else {
+            return Drupal.t('Disabled, @points (%category) for content.', params);
+          }

This looks good, just wondering if we can improve the displayed strings a bit. Maybe something like "Enabled, @points (%category) required for creating content."? Might be too long though. BenK might also have some suggestions.

Powered by Dreditor.

Bastlynn’s picture

Status: Needs work » Needs review
FileSize
18.16 KB

I've spent most of the day hunting for some way to get any of my text editors to register an EOL character to get rid of that newline. SubEthaEdit is just not cutting it, so suggestions are welcome. In the meantime, the other changes (pending any recommendations from BenK) are in place on the attached patch.

Berdir’s picture

Doesn't really matter what editor you're using, it should work when when you simply have an empty line at the end of each file. The main reason for them is that patch/diff report these weird errors if that doesn't exist.

Bastlynn’s picture

That should work, I agree. It's just not coming through on diff's end of things. Vim is supposed to add them automatically and even that didn't work. I'll keep digging, but in the meantime I wanted to get the rest of it out for BenK to start working with.

chx’s picture

userpoints_nodelimit_insufficent_points_message uses $user but that's nowhere to be found . Add global $user or just $GLOBALS['user']->uid works.

chx’s picture

Also, the context type edit screen vertical tab summary does not pick up the enabled checkbox, I dunno why, this very well can be a core issue. The other issue is weirder, when you click the dropdown for the category it opens the first admin menu dropdown.

Bastlynn’s picture

Thanks - could be a JS issue actually, perhaps make sure your cache is cleared? I've not seen the admin menu behavior, are you running anything else in your install I should take into account?

I've been getting swamped at work, but hope to be able to get back into another round of updates to all of these this weekend and next week.

Berdir’s picture

Fixed the missing global $user, the broken fieldset summaries, some formatting and wording changes.

Two things I don't like yet:

- The module name and the corresponding vertical tab titles. First, node should not be used anymore in the UI and second, "node limit" makes no sense for this module imho. It does not limit nodes in any way? Instead, it enforces a minimum of points required for creating nodes/content. So maybe something like Points for (content) creation in the vertical tabs?

- Second, there are currently two messages displayed when you're not allowed to create content. Can we merge that together to a single sentence?

Status: Needs review » Needs work

The last submitted patch, 0001-Issue-1129422-by-Bastlynn-Berdir-Ported-userpoints_n.patch, failed testing.

Berdir’s picture

Status: Needs work » Needs review

New patch with tabs removed.

Berdir’s picture

FileSize
20.21 KB

Status: Needs review » Needs work

The last submitted patch, 1129422.patch, failed testing.

dboulet’s picture

Status: Needs work » Needs review
FileSize
18.11 KB

Fixed very minor space issues.

Status: Needs review » Needs work

The last submitted patch, userpoints_nodelimit_d7-1129422-38.patch, failed testing.

cateye’s picture

The patch is working correct. The only issue I could find is, after checking the field "Enable points node limit for this type" for a content type. There is no way going back to the general setting.

In addition, I would want to limit also editting the node. Can you advise me what the best approach would be?

NathanM’s picture

subscribing.

Berdir’s picture

FileSize
19.39 KB

Latest patch is missing the .js file, re-rolled.

The test is not going to like the patch because of windows line endings in the source,can't do anything about that.

mellowtothemax’s picture

Thank you guys for your great effort, #42 works for me also.

attiks’s picture

Status: Needs work » Needs review