Early Bird Registration for DrupalCon Portland 2024 is open! Register by 23:59 PST on 31 March 2024, to get $100 off your ticket.
Creating this issue to post patch against.
Comment | File | Size | Author |
---|---|---|---|
#42 | path_with_js.patch | 19.39 KB | Berdir |
#38 | userpoints_nodelimit_d7-1129422-38.patch | 18.11 KB | dboulet |
#36 | 1129422.patch | 20.21 KB | Berdir |
#33 | 0001-Issue-1129422-by-Bastlynn-Berdir-Ported-userpoints_n.patch | 20.22 KB | Berdir |
#27 | 1129422-drupal7port-7-D7.patch | 18.16 KB | Bastlynn |
Comments
Comment #1
Bastlynn CreditAttribution: Bastlynn commentedLet me know if there are any problems with this patch and I'll be glad to fix em.
Comment #2
Bastlynn CreditAttribution: Bastlynn commentedSetting to needs review for someone to take a look.
Comment #4
Bastlynn CreditAttribution: Bastlynn commentedCurse 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.
Comment #5
Bastlynn CreditAttribution: Bastlynn commentedBack to need review.
Comment #7
Bastlynn CreditAttribution: Bastlynn commentedHuh. 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.
Comment #8
Bastlynn CreditAttribution: Bastlynn commented.
Comment #10
Bastlynn CreditAttribution: Bastlynn commentedHuh. 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.
Comment #11
Bastlynn CreditAttribution: Bastlynn commentedOk, one last shot creating the patch with git diff instead of diff.
Comment #13
Bastlynn CreditAttribution: Bastlynn commentedAlright. 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.
Comment #14
BerdirThere 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:
This will add this all as new files.
Looking at your patch....
This is weird, you're only adding single curly bracket here?
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.
Comment #15
Bastlynn CreditAttribution: Bastlynn commentedThank 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.
Comment #16
Bastlynn CreditAttribution: Bastlynn commentedOk, 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.
Comment #18
Bastlynn CreditAttribution: Bastlynn commentedWait. 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. ;)
Comment #20
Bastlynn CreditAttribution: Bastlynn commentedHm. 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.
Comment #21
BerdirNo 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:
You might want to fix that.
You only need to add the files[] declaration if these files contain classes (for example views plugins or tests).
This line is not necessary anymore, git doesn't use it.
You should rewrite this using db_update().
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.
Comment #22
BerdirYou 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.
That is a rather strange way of doing that. Try $form['#access'] = FALSE instead.
There should be a user facing version of the node type name, try node_type_get_name($type) or something like that.
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.
Comment #23
Bastlynn CreditAttribution: Bastlynn commentedAll good suggestions, I'll get started on those this morning. :)
Comment #24
BerdirGreat. 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).
Comment #25
Bastlynn CreditAttribution: Bastlynn commentedChanges 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.
Comment #26
BerdirYour 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.
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.
This is missing the "Implements hook_update_last_removed()"docblock.
Speaks for itself, same for some other files.
One more of those to remove :)
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.
Comment #27
Bastlynn CreditAttribution: Bastlynn commentedI'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.
Comment #28
BerdirDoesn'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.
Comment #29
Bastlynn CreditAttribution: Bastlynn commentedThat 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.
Comment #30
chx CreditAttribution: chx commenteduserpoints_nodelimit_insufficent_points_message uses $user but that's nowhere to be found . Add global $user or just $GLOBALS['user']->uid works.
Comment #31
chx CreditAttribution: chx commentedAlso, 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.
Comment #32
Bastlynn CreditAttribution: Bastlynn commentedThanks - 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.
Comment #33
BerdirFixed 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?
Comment #35
BerdirNew patch with tabs removed.
Comment #36
BerdirComment #38
dboulet CreditAttribution: dboulet commentedFixed very minor space issues.
Comment #40
cateye CreditAttribution: cateye commentedThe 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?
Comment #41
NathanM CreditAttribution: NathanM commentedsubscribing.
Comment #42
BerdirLatest 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.
Comment #43
mellowtothemax CreditAttribution: mellowtothemax commentedThank you guys for your great effort, #42 works for me also.
Comment #44
attiks CreditAttribution: attiks commented