487364 - Support localized "Share This" sites

Status: RESOLVED FIXED

(addons.mozilla.org Graveyard :: Public Pages, enhancement, P3)

Description

[Wil Clouser [:clouserw]]

We hardcoded 5 "share this" sites.  We should let localizers choose the most appropriate sites for their locales.

When considering this double check the tracking code.  I think some of that is hardcoded.

Comment 2

[Wil Clouser [:clouserw]]

Hiro: this is an option if you'd like to work on something.  The current ones are hardcoded and we haven't had any other requests to change these except from Japan so I'd be comfortable not making a UI for changing them and just putting an "if (locale==ja)" in the code and showing the ones that are most appropriate for you.

Comment 3

[Hiroyuki Ikezoe (:hiro)]

Attachment: Proposed patch

Wil, thank you for your suggestion. But I would actually like to propose another solution that gives localizers choices which "sharing" service uses in each locale.

This patch adds three classes for such services as place holder. If the localizer translate the shortname of the class, the service is shown in the "share this" menu.

Having said that, I am having a mind to respect your suggestion, so I will post another patch to address the suggestion.

Comment 4

[Hiroyuki Ikezoe (:hiro)]

This is a diff for testing in ja locale. This needs new translation.

diff --git a/apps/sharing/tests.py b/apps/sharing/tests.py
index 2ec5a74..2319a3a 100644
--- a/apps/sharing/tests.py
+++ b/apps/sharing/tests.py
@@ -103,3 +103,9 @@ def test_get_services_in_en_locale():
     # The order is the same as the order of sharing.SERVICES_LIST
     assert(['digg', 'facebook', 'delicious', 'myspace', 'friendfeed', 'twitter'] ==
            [s.shortname for s in sharing.get_services()])
+
+
+def test_get_services_in_ja_locale():
+    tower.activate('ja')
+    assert(['digg', 'facebook', 'delicious', 'myspace', 'friendfeed', 'twitter', 'hatenabookmark'] ==
+           [s.shortname for s in sharing.get_services()])


This is the transation.

Index: ja/LC_MESSAGES/messages.po
===================================================================
--- ja/LC_MESSAGES/messages.po  (revision 96075)
+++ ja/LC_MESSAGES/messages.po  (working copy)
@@ -9608,6 +9608,25 @@
 msgstr[0] "{0} 件のツイート"
 msgstr[1] "{0} 件のツイート"

+#. L10n: Change this for local domestic service. Do not use non-ascii characters
+#: apps/sharing/__init__.py:78
+msgid "localservice1"
+msgstr "hatenabookmark"
+
+#: apps/sharing/__init__.py:79
+msgid "Post to localservice1"
+msgstr "はてなブックマークへ投稿"
+
+#: apps/sharing/__init__.py:80
+msgid "http://localservice1/?url={url}&title={title}"
+msgstr "http://b.hatena.ne.jp/append?{url}"
+
+#: apps/sharing/__init__.py:84
+msgid "{0} post on localservice1"
+msgid_plural "{0} posts on localservice1"
+msgstr[0] "{0} 件のブックマーク"
+msgstr[1] "{0} 件のブックマーク"
+
 #: apps/sharing/templates/sharing/sharing_widget.html:2
 msgid "Share this Add-on"
 msgstr "このアドオンを共有"

Comment 5

[Hiroyuki Ikezoe (:hiro)]

Attachment: The other solution

Here is another solution. Just adding a class for Hatena::Bookmark[1], a popular social bookmark service in Japan.

I'd like to leave the decision which patch should be appropriate for this issue.

[1] http://b.hatena.ne.jp/ (in Japanese only)

Comment 6

[Wil Clouser [:clouserw]]

That's an interesting idea.  With some explanation to localizers that would fix it for everyone at once, which is a great idea.  I'll pull this into our next milestone to check out.  Thanks for the patches.

Comment 7

[Wil Clouser [:clouserw]]

I cleaned up your first patch for pep8 and committed it to a branch on github for viewing/comments: https://github.com/clouserw/zamboni/commit/8a69e40335c76a10913d1c6b0ee0f07d080df0d5

Can you make another patch with a unit test that shows that localizing the LOCALSERVICE1 class actually does make it show up in the list and only for the localized locale?

The patch works when I tried it, but I want to avoid regressions when stuff lands in the future.  Thanks!

Comment 8

[Wil Clouser [:clouserw]]

Oh, and I changed the L10n comments to point to our wiki so we could explain what we're doing here.  I haven't updated the wiki yet, but let's assume I write something there. :)

Comment 9

[Wil Clouser [:clouserw]]

Hiro: do you have time to add the test in comment 7?  This is ready to land aside from that and it would be great to get it in, particularly before the patch is too far out of date to use.

Comment 10

[Hiroyuki Ikezoe (:hiro)]

Sure. I will post the test within a week. I am sorry that my response is late. I missed the comment in comment 7.

Comment 11

[Hiroyuki Ikezoe (:hiro)]

Attachment: The unit test

This patch needs a modification of tower because tower does not allow to change the directory which includes catalog files.

I will post the patch for tower.

Comment 12

[Hiroyuki Ikezoe (:hiro)]

Attachment: The patch for tower

Use LOCALE_PATHS in settings for catalog directory.

Comment 13

[Hiroyuki Ikezoe (:hiro)]

After attachment 579185 [details] [diff] [review] is applied, the comment at https://github.com/clouserw/zamboni/blob/hiroyuki/apps/amo/tests/test_locales.py#L16 will be invalid.

Comment 14

[Wil Clouser [:clouserw]]

I'm wary of adding a top level directory for locale_test/ and patching tower.  I think this can be accomplished in a simpler fashion.  What do you think of this self contained test: https://github.com/clouserw/zamboni/commit/8ff3394d4f52949a42783afd4f549851dd2b424a#L3R124

Comment 15

[Hiroyuki Ikezoe (:hiro)]

Wow! Great! I did not know patch_object method!

I supposed that the test should be run through gettext function, but I was wrong. The test should be independent gettext function.  That's very informative for me! Thanks!

Comment 16

[Wil Clouser [:clouserw]]

Mock and Patch confuse me every time I use them - they are awesome. :)

I wrote some documentation for this at https://wiki.mozilla.org/AMO:Localizers#What_is_localservice1_in_the_.po_files but I thought it would be better with a real example.  Can you fill in the example .po block with a real service?

Thanks for all your help.  This is looking great.

Comment 17

[Hiroyuki Ikezoe (:hiro)]

(In reply to Wil Clouser [:clouserw] from comment #16)
> Mock and Patch confuse me every time I use them - they are awesome. :)
> 
> I wrote some documentation for this at
> https://wiki.mozilla.org/AMO:Localizers#What_is_localservice1_in_the_.
> po_files but I thought it would be better with a real example.  Can you fill
> in the example .po block with a real service?

Done! Thank you for the detailed document!

> Thanks for all your help.  This is looking great.

Thank you for your cooperation too!

Now this bug can be closed. Yay!

Comment 18

[Wil Clouser [:clouserw]]

Close, but not quite.  The patch is still being reviewed at https://github.com/clouserw/zamboni/commit/8ff3394d4f52949a42783afd4f549851dd2b424a .  I'll try to land it this week though and I'll close this then.

Comment 19

[Hiroyuki Ikezoe (:hiro)]

I am sorry for the rush.

Comment 20

[Wil Clouser [:clouserw]]

Good news, I landed this today and extracted to the .po files.  Can you edit the .po and then we can see if it works on -dev? :)

Comment 21

[Hiroyuki Ikezoe (:hiro)]

I asked Tomoya to change the .po file. He is the guy who can do it. ;-p

Comment 22

[Wil Clouser [:clouserw]]

I made some more minor adjustments and changed your .po file to add the service to test it out.  Feel free to change that, of course.

I also emailed dev-l10n-web@ about it.  Thanks for your help, it's looking good.

Comment 23

[Wil Clouser [:clouserw]]

You can see it in action at https://addons-dev.allizom.org/ja/firefox/addon/adblock-plus/#