Posted by NancyDru on October 25, 2011 at 3:28pm
10 followers
| Project: | User Points Contributed modules |
| Version: | 7.x-1.x-dev |
| Component: | Code: userpoints_badges |
| Category: | bug report |
| Priority: | major |
| Assigned: | Unassigned |
| Status: | needs work |
Issue Summary
It looks like this hasn't been ported yet.
| Attachment | Size | Status | Test result | Operations |
|---|---|---|---|---|
| userpoints_badges.zip | 3.04 KB | Ignored: Check issue status. | None | None |
| userpoints_badges_7.x.patch | 11.32 KB | Idle | FAILED: [[SimpleTest]]: [MySQL] Invalid patch format in userpoints_badges_7.x.patch. | View details | Re-test |
Comments
#1
#2
The last submitted patch, userpoints_badges_7.x.patch, failed testing.
#3
Try again, testbot.
#4
The last submitted patch, userpoints_badges_7.x.patch, failed testing.
#5
I think the problem is that you're using new\ instead of new/, git then looks for a folder caled "new\userpoints_badges".
#6
I'm on a Windows 7 machine. Using patch.exe for GnuWin32, which has worked for me before:
Hmm... Looks like a unified diff to me...(Stripping trailing CRs from patch.)
can't find file to patch at input line 4
Perhaps you used the wrong -p or --strip option?
The text leading up to this was:
--------------------------
|diff -up old\userpoints_badges/userpoints_badges.info new\userpoints_badges/use
rpoints_badges.info
|--- old\userpoints_badges/userpoints_badges.info 2011-07-08 00:44:32.0000
00000 -0400
|+++ new\userpoints_badges/userpoints_badges.info 2011-10-25 10:19:50.3906
25000 -0400
--------------------------
File to patch:
userpoints_badges.info
Patching file userpoints_badges.info using Plan A...Reversed (or previously applied) patch detected! Assume -R? [n]
#7
Yes, because you're on Windows and on Windows, \ is a path delimiter but on Unix (and the test bot is on Linux), it is perfectly valid to have a path with the name "a\b".
#8
That's good to know when attempting to apply patches in general. Thank you, Berdir.
#9
Well, I don't have Git. I just used the diff command. There is a full zip of the module in the first post, from which a patch that the tesbot might like can be created. Or you can just use that to commit.
#10
You don't need git, just do replace all "\" in the patch with "/" with your favorite text editor :)
Below is a first review of your changes. Note that I have quite high expectations/standards, especially compared to the sad state of most stuff in the 6.x branch of this project. Even though the review seems to be long, your efforts are very much appreciated, keep it coming! :)
+++ new\userpoints_badges/userpoints_badges.info 2011-10-25 10:19:50.390625000 -0400@@ -3,11 +3,4 @@ description = Assign badges to users as
dependencies[] = userpoints
I've started a new userpoints 7.x-2.x branch with major API changes.
To avoid confusion, can you specify that this module is (for now) only compatible with 7.x-1.x by specifying the major version like the following example: "dependencies[] = exampleapi (1.x)". See http://drupal.org/node/542202 for more information.
This is something I need to do for all other modules too.
+++ new\userpoints_badges/userpoints_badges.info 2011-10-25 10:19:50.390625000 -0400@@ -3,11 +3,4 @@ description = Assign badges to users as
-datestamp = "1310085871"
-
Let's add a "configure = admin/config/people/userpoints/settings" then a link to the configuration page is displayed directly on the modules pages.
+++ new\userpoints_badges/userpoints_badges.install 2011-10-25 10:22:26.187500000 -0400
@@ -29,10 +33,8 @@ function userpoints_badges_schema() {
function userpoints_badges_install() {
- drupal_install_schema('userpoints_badges');
}
function userpoints_badges_uninstall() {
- drupal_uninstall_schema('userpoints_badges');
No need to keep empty functions, just drop the _install implementation completely.
The _uninstall() hook should use variable_del() to delete all variables which are used by it. This doesn't work very well for dynamic ones, so you might need to do a db_delete() query directly.
+++ new\userpoints_badges/userpoints_badges.module 2011-10-25 11:05:58.218750000 -0400@@ -1,47 +1,72 @@
+ * Implements hook_menu().
Nitpicking, two spaces after Implements, same in other docblocks.
+++ new\userpoints_badges/userpoints_badges.module 2011-10-25 11:05:58.218750000 -0400@@ -1,47 +1,72 @@
- $userpoints_goal = db_result(db_query("SELECT userpoints_goal FROM {userpoints_badges} WHERE bid=%d", $form['bid']['#value']));
+ $userpoints_goal = db_select('userpoints_badges', 'ub')
+ ->fields('ub', array('userpoints_goal'))
+ ->condition('bid', $form['bid']['#value'])
+ ->execute()
It is not necessary to convert simple queries like this one to db_select(). See http://drupal.stackexchange.com/questions/1200/given-that-db-select-is-m... for an explanation why and when db_select() needs to be used.
+++ new\userpoints_badges/userpoints_badges.module 2011-10-25 11:05:58.218750000 -0400@@ -1,47 +1,72 @@
- '#description' => t('Enter the minimum number of points a user must have to get this badge'),
+ '#description' => t('Enter the minimum number of points a user must have
I am not sure about breaking translated sentences into multiple lines, is this a new coding standard?
+++ new\userpoints_badges/userpoints_badges.module 2011-10-25 11:05:58.218750000 -0400@@ -1,47 +1,72 @@
- else if ('user_badges_delete_form' === $form_id) {
- $form['#submit'][] = 'userpoints_badges_delete_badge_submit';
+ else {
+ if ('user_badges_delete_form' === $form_id) {
+ $form['#submit'][] = 'userpoints_badges_delete_badge_submit';
+ }
You should instead use elseif, easier than another nested condition.
Also, might be worth to check if userpoints_badges 7.x provides any hooks for when badges are deleted, so that we can do this in a hook implementation. Uhm, looking at the module's code, there isn't a single module_invoke() in there, sad panda :(
+++ new\userpoints_badges/userpoints_badges.module 2011-10-25 11:05:58.218750000 -0400@@ -1,47 +1,72 @@
- $sql = db_query('SELECT b.bid, b.weight, b.name, b.image, u.userpoints_goal FROM {user_badges_badges} b INNER JOIN {userpoints_badges} u ON b.bid=u.bid ORDER BY b.weight, b.name');
- while ($badge = db_fetch_object($sql)) {
- $userpoints_badges_list['select']["badge-$badge->bid"] = $badge->name;
+ $result = db_select('user_badges_badges', 'b')
+ ->fields('b', array('bid', 'weight', 'name', 'image'))
+ ->fields('u', array('userpoints_goal'))
+ ->join('userpoints_badges', 'u', 'b.bid = u.bid')
+ ->orderBy('b.weight', 'asc')
+ ->orderBy('b.name', 'asc')
+ ->execute();
+ foreach ($result as $badge) {
+ $userpoints_badges_list['select']["badge-$badge->bid"] = check_plain($badge->name);
Same here, as there are not even placeholders in this query, it should work as-is you just need to replace the while with a foreach loop. I agree on using $result instead of $sql, though.
+++ new\userpoints_badges/userpoints_badges.module 2011-10-25 11:05:58.218750000 -0400@@ -67,40 +92,57 @@ function userpoints_badges_edit_badge_su
+ $resultset = db_query("SELECT uid FROM {userpoints} WHERE uid > 0
+ GROUP BY uid HAVING SUM(points) > :goal",
+ array(':goal' => $userpoints_goal));
+ foreach ($resultset as $account) {
+ user_badges_user_add_badge($account->uid, $bid,
If you want a total per user over all categories, you can query the new userpoints_total table. That avoids the need for the GROUP BY. Also, looks like this module doesn't support userpoints categories yet. Which is fine for now, although something I try to support everyhwere properly.
+++ new\userpoints_badges/userpoints_badges.module 2011-10-25 11:05:58.218750000 -0400@@ -108,17 +150,21 @@ function userpoints_badges_userpoints($o
+ '#value' => t('Go to <a href="!link">Userpoints Badges Settings</a> to edit userpoint badges', array('!link' => url('admin/people/user_badges')))
I guess this is based on #1306032: Cumulative D7 Changes. Since you're working on that too, I suggest you change it to admin/config/people/user_badges, because that's the place for user related configuration IMHO.
+++ new\userpoints_badges/userpoints_badges.module 2011-10-25 11:05:58.218750000 -0400@@ -108,17 +150,21 @@ function userpoints_badges_userpoints($o
+ // Remove all existing badges for this user.
+ db_delete('user_badges_user')
+ ->condition('uid', $params['uid'])
+ ->condition('type', 'Userpoints%', 'like')
+ ->execute();
Not sure if the lower case like works since there are some defined special cases for this for PostgreSQL support. For consistency, let's use the uppper case version :)
#11
Cool beans! I hadn't yet run across the "configure" yet.
There don't seem to be any variables to delete.
I know I can still use db_query for simple selects, but I'd rather convert all for consistency.
Breaking the lines was for the standard of keeping within 80 characters, which is wide violated, even in core. I'm undoing that because I wonder if Locale can handle it.
I don't like elseif and never use it. I find it hard for me to read it properly. I don't think there is an execution penalty.
I originally put User Badges in admin/config/people, but then went back to my D6 installation to check something out and decided to move it to admin/people for consistency with D6.
I have never used Userpoints before, I only tackled this because of an issue someone was having with User Badges. I don't know the UP structure at all, having only installed it right after looking at that poster's issue. And it looks like the uerpoints_retroactive module hasn't been converted yet either, so that impacts my trying to learn it.
#12
#13
The last submitted patch, userpoints_badges_7.x.patch, failed testing.
#14
Ok, two generic problems with the patch format remaining.
- Windows style file endings. The test bot doesn't like these nor does git.
- You are patching the -dev snapshot which contains the project/build information in the .info file. But the patch is applied against a git checkout which doesn't. After manually removing these parts (which is tricky), I was able to apply your patch successfully.
I suggest you look into learning some git basics sooner or later because you just don't have problems like this with git diff. I gladly provide any assistance you need, you just need to ask. It's really not that complicated :)
For reference, attaching the altered patch file that finally worked for me, this one should get past the bot (which is kinda pointless as there are no tests ;))
#15
The last submitted patch, userpoints_badges_7.x_1.patch, failed testing.
#16
Yeah, working on User Badges and the other maintainers are absent, so I'm going to have to install and learn Git (I will use a GUI). I may have call on you.
#17
About People/Configuration. It doesn't need to be like Drupal 6. But it should follow the UX Guidelines/Information Architecture of Drupal 7. Have a look at http://drupal.org/node/549094#people.
I don't know Git GUI's in detail (Although I've heard that EGit which is a Eclipse plugin is pretty good), but I can still help to explain Git in general. I can also recommend #drupal-gitsupport if there are questions.
#18
userpoints_badges_7.x.patch queued for re-testing.
#19
There are no tests, this is not going to get any better than that message :)
#20
Berdir,
I can't make your patch work with TortoiseSVN on Windows.
I had installed following userpoints_contrib module:
http://ftp.drupal.org/files/projects/userpoints_contrib-7.x-1.x-dev.tar.gz
I am getting following error when TortoiseMerge is run:
'C:\xampp\htdocs\drupal7\sites\all\modules\userpoints_contrib\userpoints_badges' is not a working copy.
#21
No idea about that error, but as I said, my patch is against the git checkout, it doesn't apply against the -dev snapshot.
#22
That error is because I have not checked out first. I don't get it, can't I apply patches to local copies only, without any version control repository involved?
#23
In theory yes, you can.
This case differs because the .info file is changed when the package is built, so it is not the same file.
Try the patch from NancyDru, that is against the snapshot.
#24
I have directly used the userpoints_badges.zip from NancyDru.
It works better than the untouched 6.x version.
Here is my report as of now:
EDIT:
- Cannot edit a badge: Trying to edit a badge, although the URL seems to be correct (program/admin/people/user_badges/edit/2), it takes me to People page.
ADD:
- Notice when add is clicked: "Notice: Undefined variable: userpoints_goal in userpoints_badges_form_alter() (line 29 of C:\xampp\htdocs\drupal\sites\all\modules\userpoints_contrib\userpoints_badges\userpoints_badges.module)."
- There are two form buttons named (Save Badge) at the bottom.
LIST:
- Can we put Userpoints Goal as a column?
#25
Let us, please, try to separate the issues as to which module is causing them. User Badges issues belong in #1306032: Cumulative D7 Changes.
#26
Here's the zip after applying Berdir's list, and from which the patch was made.
#27
ADD:
After
if ('user_badges_edit_form' === $form_id) {(first line of code), add$userpoints_goal = '0';I suspect this is a difference in what 7.x returns from the query as opposed to what 6.x returns.
The save buttons will be fixed on my next patch update.
#28
Can one of you give me an idea of how to set up a test case in UP?
#29
@Berdir: Can you remove
$form['submit']['#weight'] = 10;from the form_alter(), please. I am changing the form to add weights so that this module gets its field in the right place, rather than at the bottom of the image list. Thanks.#30
Have a look at http://blog.worldempire.ch/story/writing-automated-tests-drupal-7 and check out the existing UserpointsTests in userpoints.test.
Basically, you will need to implement what you would do in a manual test in PHP with the methods provided by Simpletest. However, writing tests takes quite some time and it might might make more sense to write tests for User Badges itself first. Then it would be easy to adapt and extend those with stuff added by this module.
Not having tests is IMHO fine for the moment, just ignore the testbot for now. You can use a something-d7.patch name, then the test bot will ignore but I can still do a manual review.
#31
Sorry, I didn't mean that kind of test, but thanks for the tutorial. I need to configure UP on my site so that I can test whether or not UPB is working with UB.
But, yes, I guess Simpletest is something else to get back to. I wrote some in Web Links some time ago.
#32
Ah sorry, misunderstood you. A test case for me is something related to Simpletest :)
I think all you need is at admin/config/people/userpoints. You can see how many points each user has, the transactions, grant/remove points for a user and configure the settings.
#33
Okay. I set a badge to 10 points and then gave a user 10 points, and he didn't get the badge. So I created a transaction for 10 more poonts (both show in the list), still no badge. So I found UP_nc module and tried that. I don't even see the points being granted.
#34
Not sure, but my suggestion is to fix user_badges first and then work on UP/User Badges integration :)
#35
From #871114: Drupal 7 port (userpoints_invite), let's not add versioned dependencies until it works for dev snapshots and git checkouts..
#36
Just curious, should interested parties assume that a port to Drupal 7 is indefinitely postponed? Was hoping to utilize points with badges for an upcoming project being developed in D7.
#37
The zip file in #26 worked well against the dev branch.
It did raise an error when the user_badges-edit_form came up as part of creating a NEW badge, and userpoints_goal was not yet found in the dB, and thus not properly initialized. I added an extra line to initialize the varialbe and the error went away.
function userpoints_badges_form_alter(&$form, &$form_state, $form_id) {if ('user_badges_edit_form' === $form_id) {
$userpoints_goal = '';
if (isset($form['bid']) && isset($form['bid']['#value'])) { ....
#38
IIRC, I've committed all the changes that were in the zip file, so the -dev should be at the same point.