Reviewing merge request #168: Fix for issue 3324: put username in subject, etc of notification emails.
See full log message below. Basically, this is a fix for issue-3324. It is untested; however, I suspect that if it has bugs they're likely to be fairly simple typos.
Note it places the email recipient's nickname somewhat differently than originally suggested in the issue. However, the placement used in this patch seems a bit more unobtrusive to me, and more or less matches what Twitter does.
Commit message follows:
commit c697cbc8bf0cad43a4dd4704a99035ab01ce9312
Author: Karl Fogel <kfogel@red-bean.com>
Date: Thu Sep 15 23:56:04 2011 -0400
Fix issue #3324 by including receiver's nickname in subject and body
of various kinds of outgoing email messages.
* lib/mail.php
(mail_subscribe_notify_profile): Include subscribee nickname in
subject line and in the body of the message. While incrementing
subsequent substitution codes in the translator comment, also fix
a typo -- "%6%s" is now "%7$s".
(mail_notify_nudge): Same, and again fix some typos in the
translator comment.
(mail_notify_message): Same, but no typo fixes here.
(mail_notify_fave): Same, preserving the unadjusted substitution
ordering here -- since %7$s was clearly added later, do the same
with new %8$s.
(mail_notify_attn): Same, and likewise preserve the unadjusted
substitution ordering.
Note: This patch preserves the local style of each notification
function even where those styles are inconsistent with each other.
For example, some functions put a "@" in front of nicknames in the
notice text, and others don't; this patch does not change that. And
some functions put a backslash in numbered translation parameters
(e.g., "%3\$s") and others don't; that is still true after this patch.
- Requested by:
kfogel - Status: Open
- Source repository: statusnet-dev:issue-3324
- Target repository: mainline:master
- Created at: September 16th, 2011 04:19
Commits that would be merged:
- 4f86e05
- c697cbc
Fix issue #3324 by including receiver's nickname in subject and body
4f86e05-c697cbcComments
Pushed new version 1
Yikes! These are a lot of email changes to check. And I'm not sure it’s kosher to change the values used in the sprintf() formats — we have a lot of those translated already, so they’re going to come out weird.
I’ll review this later, but not sure I can push it right now.
Thanks for taking a look, Evan. I wasn’t expecting it to be pushed right away, no worries.
When you said you’re “not sure it’s kosher to change the values used in the sprintf() formats — we have a lot of those translated already”, do you mean that in all of the locations, I should add new parameter numbers starting from the highest existing number + 1, instead of inserting the new parameters in the middle of the numbers and shifting the remainder down? Note I did both ways: the former where the string clearly indicated that that method had already been used in the past, and the latter when there was no evidence that that method had ever been used there.
I could easily change the patch to work the former way everywhere. But I don’t quite understand how it would help the translators. Since the string-to-be-translated is changing (the receiver’s nickname is being inserted), they’re going to have to retranslate that string anyway. I don’t see any way to avoid that: changing source strings means the translations have to get updated.
Please let me know what I can do to make this patch more easily digestible. I'd love to see it get merged.
I don’t see a way to avoid changing strings — after all, the point of the patch is to change the message that goes out, so the new material has to get incorporated somehow. But I'm open to suggestions; let me know.
Best,
-Karl
Add a new comment:
Login or create an account to post a comment