Needs work
Project:
Userpoints Invite
Version:
6.x-1.0-beta1
Component:
Code
Priority:
Normal
Category:
Task
Assigned:
Unassigned
Reporter:
Created:
1 Aug 2010 at 23:48 UTC
Updated:
29 Nov 2011 at 15:10 UTC
Jump to comment: Most recent file
Comments
Comment #1
BenK commentedComment #2
chertzogenabling the userpoints_invite module with the invite 7x.2x release posted here http://drupal.org/node/20967/release?api_version%5B%5D=103 gets these errors:
Notice: Undefined index: schema_fields_sql in /sites/all/modules/entity/entity.module on line 483
Warning: in_array() [function.in-array]: Wrong datatype for second argument in /sites/all/modules/entity/entity.module on line 483
Warning: Missing database columns for the exportable entity rules_config as defined by entity_exportable_schema_fields(). Update the according module and run update.php! in /sites/all/modules/entity/entity.module on line 484
Notice: Undefined index: schema_fields_sql in /includes/entity.inc on line 265
Catchable fatal error: Argument 2 passed to SelectQuery::fields() must be an array, null given, called in /home/kim/public_html/alpha/includes/entity.inc on line 284 and defined in /includes/database/select.inc on line 1262
Not sure how to go on with this. I am no coder, but i can help test.
Comment #3
chertzogAfter researching i found this http://drupal.org/node/918774. The errors have somehting to do with the use of the switch structure. Most of the functions in this module only have one case, so i switch structure is not needed.
In the hook_user has been replaced in D7. The call to hook_user is for deleting a user. so we need to use hook_user_delete.
In that function we also have to rework the query. Maybe somehting like this:
Comment #4
chertzogWell, after teaching myself some more php, and reviewing a bunch of issues elsewhere, here is a rough D7 port.
Comment #5
berdirPlease provide a patch, then I'll give you a detailed review of your code.
See http://drupal.org/patch and http://drupal.org/project/userpoints_contrib/git-instructions
Comment #6
chertzogI dont know how to create patches as i dont have command line abilities.
Comment #7
berdirYou don't need to, there are also various GUI tools that allow to create patches without getting intimidate with the console :)
See http://drupal.org/node/777182
A patch makes it much easier for me to see what you did and give you quick feedback.
Comment #8
chertzoglook, i dont understand the whole git, CVS, patch, commit thing. I just edited the code ran it through coder and uploaded it above. it works for me. if someone else wants to create a patch go for it.
Comment #9
Bastlynn commentedI know it's not something you know off the top of your head or anything, but these are really handy skills to have - especially if you are going to dive deeper into programming or contributing to drupal.org. Knowing git or cvs, what a revisioning system is, and how to commit changes to it or create patches from your changes, are skills that are absolutely critical for a professional programmer.
If you're genuinely interested in programming, set aside some time to learn those practices, it will help you develop so much more quickly and keep your code organized in such a way that you'll never run the risk of losing days or weeks of work just by accidentally deleting something.
These are skills that make your life easier as a programmer, and make the difference between hired for a job or passed over for someone else. If you don't learn it for this project, then please take the time in the future to learn it for another - you won't regret it.
re: leaving your work to someone else to patch - I'm not sure if Berdir or BenK really have the time for that. After all, that's why they ask for patches so that the autotest system can take care of the basic issues for a patch then a human can get involved when it looks like the patch is ready to go somewhere. I have a lot on my plate myself, so hopefully someone will get to it in the meantime - otherwise I think your work may end up waiting until some time frees up. If you're ok with that, cool.
Comment #10
boran commentedI pulled the git version down, it seems to ok, but made the following patch to get it working better for d7:
- update version to 7.x in .info
- fix a php 'constant' warning
- place the configuration in a D7 compatible vertical tab under "Additional Settings " on admin/config/people/userpoints/settings
Test the module by sending an invite and checking that a points transaction was registered.
Comment #11
berdirExisting changes look good, have a look at http://drupal.org/node/993376 on how to use hook_userpoints_info() to provide a dynamic, translatable transaction description.
Comment #12
boran commentedHmm. Reading the "Use of description in userpoints_userpoints_api() is deprecated and operation is a technical name" example on http://drupal.org/node/993376, I tried set a the Reason to "Points for sending an invitation" as follows
function userpoints_invite_userpoints_info() {
return array(
'userpoints_invite_invite' => array(
'description' => t('!Points for sending an invitation', userpoints_translation()),
)
);
}
function userpoints_invite($op, $args) {
switch($op) {
case 'invite':
$points = variable_get(USERPOINTS_INVITE_INVITE, 0);
$params = array(
'points' => $points,
'uid' => $args['inviter']->uid,
'operation' => 'userpoints_invite_invite',
...
but that prints userpoints_invite_invite as the operation.
Comment #13
berdirHave you cleared the cache so that the new hook implementation is picked up? Code looks good otherwise.
Also, we tried to avoid using !Points in descriptions because it is kinda obvious that this is about gaining points. My suggestion would be to use a description callback and then embed the user name (assuming entity_id and entity_type are set when adding points, if not, add that..) like this: "Invited @user."
Also, we need to enforce the dependency on userpoints 7.x-1.x by doing something like this in the .info file "dependencies[] = userpoints (1.x)". This is something I want to do for all userpoints modules because the 7.x-2.x branch has an incompatible API. And while you're at it, add "configure = admin/config/people/userpoints/settings" line to, that prints a configure link that directly points to the settings of this module.
Comment #14
boran commentedClearing the cache helped :-)
I'll think about the 'reasons' your point is valid. It should be short and useful.
Although the operation > _userpoints_info > callbacks indirections makes the code more difficult to understand/read IMHO :-( .
Each 'op' would require a line at each level.
Also if the userpoints tx list is published to all members of a community (for transparency), it may not be a good idea to show the destination invitation person (privacy).
.info: ok makes sense, thats something new learned too..
I'll roll a new patch soon..
P.S. The D7 invite module itself is a bit unstable/incomplete, which makes this module less of an urgency..
Comment #15
boran commentedAs regards "dependencies[] = userpoints (1.x)",
since userpoints from git had no version, the modules listsing was showing a dependency error.
So, added the git_deploy module, now there is a version number
But the userpoints_invite module still shows a dependency error:
==> "Requires: Userpoints (1.x) (incompatible with version 7.x-1.x-dev)"
Reading http://drupal.org/node/542202, the format seems correct for userpoints_contrib/userpoints_invite/userpoints_invite.info:
dependencies[] = userpoints (1.x)
Also tried:
dependencies[] = userpoints (1.x, 7.x-1.x, 7.x-1.x-dev)
==> Requires: Userpoints (1.x, 7.x-1.x, 7.x-1.x-dev) (incompatible with version 7.x-1.x-dev)
If we are going to put this dependency into all contribs for usepoints, we better agree on the format first :-)
Comment #16
berdirYes, this seems to be broken currently, see #1013302: Composer metadata on dev versions doesn't work in update.php.
It is working fine when you use the most recent 7.x-2.x branch from http://drupal.org/project/git_deploy (you need to check out from git, there is no snapshot yet).
Anyway, let's not do this until the core issue is fixed.
Comment #17
boran commentedNew patch attached, also some spacing added after running through coder
Comment #18
berdirComment #19
berdirTrailing whitespace.
Also not sure if we really need to add the commented out version with the versioned dependency, this is easy enough to add later on without that. Maybe a line like "; Add (1.x) version dependency once URL is fixed."
This is not necessary anymore and has been removed for a reason. This is CVS specific stuff, git doesn't do anything with it.
If coder.module tells otherwise, coder.module is wrong :)
If this is everything we have about documentation, then let's just drop it completely. This isn't more useful than the description in the fieldset/vertical tab.
The (requires the invite module) seems pointless given that you can't install the module without having it.
You can use the userpoints_get_vid() function for this.
Missing Implements hook_userpoints_info() docblock.
Not sure what you're doing exactly here.. As the other operations below. All these should use a unique operation machine namme and then have a corresponding description in hook_userpoints_info() ?
Instead of just checking the operation, which won't be unique anymore, you need to look for the combination of machine_name (e.g. userpoints_invite_invite) + referenced account (entity_id).
Comment #20
boran commentedOk, I fixed those points except for the last.
So operation gets put into the reason column that is shown to users when they view the points list on the profile.
Initially I was thinking not to mention the email of the user invited, since this email would be visible in points lists, which may be visible to all users on a site. So, looking into the DB, I'm beginning to see what you mean. After an invitation is sent, the actual text 'userpoints_invite_invite' is written into the operation (and userpoints_invite to reference) column of userpoints_txn, but "sent an invitation" is displayed to the user viewing the points list.
So for the delete operation, we query to see if there it at least one invite sent by the current user,
SELECT uid FROM {userpoints_txn} WHERE operation = userpoints_invite_invite and entity_id= :current-uid
New patch attached.
There is no '_userpoints_info' for escalate yet, does that make sense, otherwise how can $args be passed to _userpoints_info.
UPDATE: ignore this patch, its has a syntax error
Comment #21
boran commentedPatch to replace #20.
Comment #22
glekli commented