| Project: | User Points |
| Version: | 6.x-1.x-dev |
| Component: | Code: userpoints |
| Category: | feature request |
| Priority: | normal |
| Assigned: | Unassigned |
| Status: | needs review |
Issue Summary
This is an issue that came up during the initial D7 port. We elected to postpone until now because we were focused on porting the existing code. But now that we have an official 7.x-1.x-dev branch, we can revisit and then port back to D6.
Currently, here is the message that is displayed when points are awarded (to a user named "person1"):
"User person1 earned 10 points Total now is 20 points."
I'd like to see some improvements in grammar and some clarification of the "total" points calculation. Here are a few suggestions:
A. Display "You" instead of "User person1" if the user getting the points is the currently logged in user.
B. If not using "You," then remove the word "User". We should just say that "person1 earned 10 points"
C. Put a period at the end of "person1 earned 10 points" so that it's a complete sentence.
D. Put the word "just" before earned so that it reads: "person1 just earned 10 points."
E. The "total" that is displayed is confusing because it is only for total points in the same category as the current points being awarded. So if 10 points were just awarded in the "Participation" category, it will only show the user's total for "Participation" category points (not all points). I'd love to add a setting to determine whether or not the category total or the total of all categories should be used in the message. Better yet, it would be great to add a setting to let this be determined for each category (so that some categories could display a category total in the message and others could display a cumulative total). But I'm not sure how complicated this would be to implement. A fallback approach would be to simply clarify the existing point total being displayed. For instance, change the message to read:
"person1 just earned 10 points. person1 now has 10 total points in the "Participation" category."
Finally, the most flexible approach would be to allow the site administrator to configure the syntax of displayed messages with tokens (perhaps even for each points category). But again, we'll need to determine how complicated this would be to implement.
Thoughts?
Cheers,
Ben
Comments
#2
The problem here is as following:
Currently, the string is defined as this: "@username @operation @points !points....". Aka, pretty everything is a placeholder. This is hard if not impossible to properly translate and doesn't work for several cases like You/username (has vs. have for example), singular/plural etc.
So this patch instead tries to explicitly provide a string for every possible combination. This means that the number of string literally *explode* :):
earned/lost * current user or not * approval or not * singular/plural (currently only for the new points, but in theory also necessary for total but I don't know if that is even possible).
And there could even be more when thinking about displaying all points or current category or adding a special case for the uncategorized category (Because "in the Uncategorized category" is weird ;))
So what this patch does:
- Singular/Plural support for new points
- Displays the category in the total message
- A separate string for every combination
- Permission checks to display own points (view (own) userpoints) and points of other users (view userpoints).
- Allows other modules to define a custom message and therefore overriding the default.
- Themes username and directly addresses the current user.
Happy testing ;) (hint: user2userpoints.module makes it easy to display userpoints messages for other users and testing. And once this patch is commited, we can display a better message (a single message that you gave user X n points instead of two separate earned/lost points)
#3
#4
Hey Berdir,
I tried out your patch in #2 and it is working great. I've tested lots of different combinations of earned/lost, current user or not, approval or not, and singular/plural. So everything seems to be working quite well.
Here are the small bugs/issues I noticed:
A. If you submit a points award that is both moderated and "Uncategorized," the following error message is being displayed:
Notice: Undefined index: in userpoints_userpointsapi() (line 458 of /Users/benkaplan/git/drupal/sites/all/modules/userpoints/userpoints.module).
B. The grammar is slightly wrong in this message: "editor just earned 10 points and has now 17 Points in the Access Points category." Basically, the words "has" and "now" should be reversed. So instead, it should read: "editor just earned 10 points and now has 17 points in the Access Points category." This same change probably needs to made in a few different strings.
C. I've been trying to think about how to deal with "in the Uncategorized category". The issue is that it depends on whether the user has actually defined other categories and whether he changed the title of "Uncategorized" (the default) to something else. Three options:
i) Is it possible to test if any other categories have been defined? If no categories have been defined, then we could just omit the "in the Uncategorized category" part altogether for this special case. The message would read: "editor just earned 10 points and now has 17 points." This would be a better option for those sites that don't use categories at all.
ii) If a site does have categories defined but the points award is in "Uncategorized", then we could structure a special case message like this: "editor just earned 10 points and now has 17 points (in no particular category)."
iii) We could just change the default category from "Uncategorized" to "General". Then, "in the General category" wouldn't sound quite so weird.
What do you think? Alternately, we could just not worry about it and let site admin's change the name of "Uncategorized" to fit our message structure.
D. I wondering if we can change the word "lost" when someone gets points removed. I think it has a negative connotation which is misleading in some cases. For instance, if someone trades in points for a prize, they didn't really "lose" the points... they just had them deducted. So here's a possible replacement message: "editor just had 10 points deducted and now has 17 points in the Access Points category." What do you think? Can we can change the message structure like this (instead of just changing "earned" to "lost") when someone has points removed?
E. As you pointed out, I see that the points total doesn't support singular. That really only impacts those who have a total of "1 point". I don't know if it's worth fixing this (especially if it causes the number of strings to continue to explode), but to be complete I added it to the list.
Great job on this patch. I think it will be terrific to get this committed ASAP because it impacts pretty much all of our work on Userpoints Contributed modules, too.
Thanks,
Ben
#5
C) This is really tricky :) Having a special condition for categories means that the number of strings is multiplied by number of special cases. And we already have a lot. So, we could for now do iii).
E) It's not only the number of strings here. It is simply impossible (Or I wouldn't know how to do it) to have two separated plural cases in a single string. If we could split it up in two sentences (points changed sentence + total points sentence) it might be possible, though... But we'd need a solution to merge these two sentences together so that it can be properly translated (for RTL languages as well)
I'll try to bring andypost in here, maybe he has some suggestions...
#6
C) Okay, sounds good.
E) I don't think this is a huge deal. I wouldn't want it to slow us down committing this patch. Whatever you think is best...
--Ben
#7
A. I can't see how this should happen, but I've added a check to avoid this.
B. Replaced all instances of "has now" with "now has".
C. Let's just not worry about this then for now... :)
D. Since we now have separate strings for every combination, that is easily possible. Changed.
E. Something for another time. This is already a huge improvement without this, we can think about this when we're old and bored ;)
#8
Hey Berdir,
I tested the latest patch and it is working great. Everything is solved except for one thing: I don't think D. has implemented for every possible string.
For instance, when I subtract three points in a specific category, I'm still getting "lost" instead of "has been deducted" language. Looking at the patch, it appears that "lost" is still used a few times.
But once those last few strings are changed, then this is ready to be committed. I can test another patch if you like or you could just commit it once you've made those last few string changes.
Nice work!
--Ben
#9
Ok, great I've commited it with some more lost/deducted and an additional has now/now has fix.
Not sure about backporting this, as it is quite a major string change. Marking as possible backport and waiting on kbahey's decision before I do any work here.
+ Backporting this would fix ~3 open bug reports aboug plural/singular and generally about the bad grammar in this message.
- It is a pretty major string strange and would invalidate a lot of translations and I'm not sure if you want to do that in a stable release.
An alternative might be to start a 6.x-2.x branch, that would contain this and also other changes that can be backported from 7.x-1.x.
#10
Is this patch applied to latest dev (dated: 2010-Sep-23) release?
#11
No, this is only in 7.x-1.x-dev. There is no patch for 6.x at this time.
#12
Change ( $params['tid'] ):
<?php'%total' => userpoints_get_current_points($params['uid'], $params['tid'])
?>
to ( 'all' ):
<?php'%total' => userpoints_get_current_points($params['uid'], 'all'),
?>
make the users calm down... (6.x-1.x-dev).
#13
subscribing
#14
The patch still doesn't put a full stop before "Total" in
$mesg = (t('User %uname %op %pointsvalue !points Total now is %total !points.',which was mentioned in #486778: Points message total is not correct already.Also, any chance of getting this in D6? shouldn't be a very different patch now should it.
Cheers
#15
subscribing
#16
Here's a quick patch to at least get that full stop. Also, I don't think the $op string should be emphasized, so I changed it to a '@'.
#17
The problem is that those changes break translations of that string because the source is changing. As bad as it currently is, it will result that sites which upgrade will suddently see the english text instead of the one that they've translated.
#18
well, the more reason to commit this asap so that more new sites don't get bad strings saved as source translations.. And I guess an update script to replace the i18n translated source strings would be in order to make this complete then.
#19
+1 #18