preferred language #810

kziemianek · 2018-04-05T20:19:16Z

Description

Allow the user to set different language for the Gitcoin app

Checklist

linter status: 100% pass
changes don't break existing behavior
commit message follows [commit guidelines]

Affected core subsystem(s)

backend, frontend

Testing

locally

Refers/Fixes

Refs: #802

kziemianek · 2018-04-05T20:21:38Z

I leave Polish locales (translated only navbar) to allow You check how it works locally. I'll remove them before final push. User can change preferred language in email settings, I hope it will be moved to profile settings in #795.
Emails translation to be done. Did i miss anything?

mbeacom

We should consider

mbeacom · 2018-04-05T20:31:31Z

app/marketing/views.py

@@ -420,6 +421,15 @@ def email_settings(request, key):
        else:
            raise Http404
    es = es.first()
+    profile_id = request.session.get('profile_id')
+    profile = Profile.objects.get(pk=profile_id)


We might want to check if profile_id is None here

i wonder if it's possible... can user without profile change preferences ?

An anonymous user can still hit the https://gitcoin.co/email/settings/ endpoint at the moment. We'll want to either restrict the view from being accessed without auth or handle when profile_id = request.session.get('profile_id') is None 😢

preferred_language selection will be available only if profile_id is present in session data, is it fine ?

@kziemianek 👍 Works for me! Realistically, we'll restrict this view from being accessed by users that haven't authenticated, so that should work!

mbeacom · 2018-04-05T20:39:49Z

app/dashboard/models.py

@@ -684,6 +684,7 @@ class Profile(SuperModel):
    last_sync_date = models.DateTimeField(null=True)
    email = models.CharField(max_length=255, blank=True, db_index=True)
    github_access_token = models.CharField(max_length=255, blank=True, db_index=True)
+    pref_lang_code = models.CharField(max_length=10)


We should limit the choices to LANGUAGES and we can probably set the max_length to 2.

mbeacom · 2018-04-05T20:45:51Z

app/marketing/views.py

@@ -420,6 +421,15 @@ def email_settings(request, key):
        else:
            raise Http404
    es = es.first()
+    profile_id = request.session.get('profile_id')
+    profile = Profile.objects.get(pk=profile_id)
+    if request.POST.get('preferred_language', False):


You can drop the False here. We could probably just do:

preferred_language = request.POST.get('preferred_language') if preferred_language: profile.pref_lang_code = preferred_language profile.save()

Also should we assert preferred_language is in LANGUAGES?

Also should we assert preferred_language is in LANGUAGES?

Yes !! Better safe than sorry

mbeacom · 2018-04-05T20:46:55Z

app/marketing/views.py

@@ -420,6 +421,15 @@ def email_settings(request, key):
        else:
            raise Http404
    es = es.first()
+    profile_id = request.session.get('profile_id')


Just a note... This is changing and will soon be: profile = request.user.profile

mbeacom · 2018-04-05T21:02:16Z

app/github/views.py

        session_data = {
            'handle': user_profile.handle,
            'email': user_profile.email,
            'access_token': user_profile.github_access_token,
            'profile_id': user_profile.pk,
            'name': user_profile.data.get('name', None),
            'access_token_last_validated': timezone.now().isoformat(),
-        }
-        for k, v in session_data.items():
+            LANGUAGE_SESSION_KEY: user_profile.pref_lang_code


If the user doesn't have pref_lang_code set, we shouldn't manipulate LANGUAGE_SESSION_KEY.

mbeacom · 2018-04-05T21:15:35Z

app/retail/templates/email_settings.html

+                            <label class="form__label" for="level">{% trans "Preferred language" %}</label>
+                            <div class="form__select">
+                                <select id="preferred_language" name="preferred_language" required>
+                                    {% get_language_info_list for available_languages as langs %}


You should be able to replace this with:

{% get_available_languages as global_languages %} {% get_language_info_list for global_languages as langs %}

Then you can eliminate the 'available_languages': ['en', 'pl'], part all together.

mbeacom · 2018-04-05T21:16:11Z

app/marketing/views.py

@@ -464,6 +474,8 @@ def email_settings(request, key):
        'msg': msg,
        'autocomplete_keywords': json.dumps(
            [str(key) for key in Keyword.objects.all().values_list('keyword', flat=True)]),
+        'available_languages': ['en', 'pl'],


This can be removed per the comment below.

codecov · 2018-04-06T07:59:17Z

Codecov Report

Merging #810 into master will decrease coverage by 0.4%.
The diff coverage is 7.65%.

@@            Coverage Diff             @@
##           master     #810      +/-   ##
==========================================
- Coverage   33.08%   32.68%   -0.41%     
==========================================
  Files         100      100              
  Lines        6435     6539     +104     
  Branches      780      783       +3     
==========================================
+ Hits         2129     2137       +8     
- Misses       4208     4304      +96     
  Partials       98       98

Impacted Files	Coverage Δ
app/app/pipeline.py	`0% <0%> (ø)`	⬆️
app/marketing/mails.py	`10.5% <1.38%> (-3.29%)`	⬇️
app/app/settings.py	`80.66% <100%> (-1.65%)`	⬇️
app/marketing/views.py	`11.72% <18.75%> (+0.07%)`	⬆️
app/tdi/views.py	`19.1% <20%> (+0.02%)`	⬆️
app/app/utils.py	`22.92% <50%> (+0.7%)`	⬆️
app/dashboard/models.py	`64.39% <66.66%> (ø)`	⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 8b4d80a...621282c. Read the comment docs.

mbeacom · 2018-04-09T14:43:29Z

app/github/views.py

        session_data = {
            'handle': user_profile.handle,
            'email': user_profile.email,
            'access_token': user_profile.github_access_token,
            'profile_id': user_profile.pk,
            'name': user_profile.data.get('name', None),
            'access_token_last_validated': timezone.now().isoformat(),
+            LANGUAGE_SESSION_KEY: user_profile.pref_lang_code


Should we remove this in favor of the if check below?

mbeacom · 2018-04-09T14:45:40Z

app/retail/templates/email_settings.html

@@ -47,7 +47,7 @@ <h5>{% trans "Basic Preferences" %}</h5>
                        </div>
                        <div class="form-group">
                            <label class="form__label" for="level">{% trans "Send me" %}</label>
-                            <div class="form__select">
+                            <div class="form__select">                                


Can you remove the trailing whitespace here?

mbeacom · 2018-04-09T14:50:47Z

app/marketing/views.py

@@ -420,6 +421,15 @@ def email_settings(request, key):
        else:
            raise Http404
    es = es.first()
+    profile_id = request.session.get('profile_id')
+    profile = Profile.objects.get(pk=profile_id)


An anonymous user can still hit the https://gitcoin.co/email/settings/ endpoint at the moment. We'll want to either restrict the view from being accessed without auth or handle when profile_id = request.session.get('profile_id') is None 😢

kziemianek · 2018-04-11T11:14:30Z

Tests fix in #860

mbeacom · 2018-04-17T13:41:41Z

@kziemianek Mind resolving the conflicts here and we'll get it merged?

stickler-ci · 2018-04-17T19:33:02Z

app/app/settings.py

+    'locale',
+)
+
+LANGUAGES = [    


W291 trailing whitespace

stickler-ci · 2018-04-17T19:33:02Z

app/app/settings.py

+)
+
+LANGUAGES = [    
+    ('en', _('English'))    


W291 trailing whitespace

stickler-ci · 2018-04-17T19:33:02Z

app/app/settings.py

@@ -405,3 +415,4 @@
        }]
    SILKY_MAX_RECORDED_REQUESTS_CHECK_PERCENT = env.int(
        'SILKY_MAX_RECORDED_REQUESTS_CHECK_PERCENT', default=10)
+


W391 blank line at end of file
W293 blank line contains whitespace

stickler-ci · 2018-04-17T19:33:02Z

app/app/utils.py

@@ -55,6 +56,11 @@ def add_contributors(repo_data):
    return repo_data


+def setup_lang(handle, request):
+    profile = Profile.objects.get(handle=handle)
+    request.session[LANGUAGE_SESSION_KEY] = profile.get_profile_preferred_language()       


W291 trailing whitespace

stickler-ci · 2018-04-17T19:33:02Z

app/app/utils.py

@@ -90,7 +96,7 @@ def sync_profile(handle, user=None):
    except Exception as e:
        logger.error(e)
        return None
-
+    


W293 blank line contains whitespace

stickler-ci · 2018-04-17T19:33:03Z

app/marketing/views.py

@@ -575,23 +577,34 @@ def email_settings(request, key):
    suppress_leaderboard = False
    email = ''
    level = ''
-    msg = ''
-
+    msg = ''    


W291 trailing whitespace

stickler-ci · 2018-04-17T19:33:03Z

app/marketing/views.py

    if request.POST and request.POST.get('submit'):
        email = request.POST.get('email')
        level = request.POST.get('level')
+        profile = Profile.objects.get(pk=request.user.profile.id)    


W291 trailing whitespace

stickler-ci · 2018-04-17T19:33:03Z

app/marketing/views.py

    if request.POST and request.POST.get('submit'):
        email = request.POST.get('email')
        level = request.POST.get('level')
+        profile = Profile.objects.get(pk=request.user.profile.id)    
+        if profile:
+            pref_lang = profile.get_profile_preferred_language();


E703 statement ends with a semicolon

stickler-ci · 2018-04-17T19:33:03Z

app/marketing/views.py

@@ -603,14 +616,17 @@ def email_settings(request, key):
            else:
                es.metadata['ip'].append(ip)
            es.save()
-            msg = "Updated your preferences.  "
+            msg = "Updated your preferences.  "    


W291 trailing whitespace

stickler-ci · 2018-04-17T19:33:03Z

app/marketing/views.py

    context = {
        'nav': 'internal',
        'active': '/settings/email',
        'title': _('Email Settings'),
        'es': es,
        'msg': msg,
        'navs': settings_navs,
+        'available_languages': ['en', 'pl'],
+        'preferred_language': pref_lang,
+        'logged_in' : False if request.session.get('profile_id') is None else True


E203 whitespace before ':'

stickler-ci · 2018-04-19T17:58:39Z

app/dashboard/models.py

@@ -827,7 +827,8 @@ class Profile(SuperModel):
    handle = models.CharField(max_length=255, db_index=True)
    last_sync_date = models.DateTimeField(null=True)
    email = models.CharField(max_length=255, blank=True, db_index=True)
-    github_access_token = models.CharField(max_length=255, blank=True, db_index=True)
+    github_access_token = models.CharField(max_length=255, blank=True, db_index=True)    


W291 trailing whitespace

stickler-ci · 2018-04-19T18:13:04Z

app/dashboard/models.py


+    @receiver(user_logged_out)


E303 too many blank lines (2)

mbeacom · 2018-04-23T20:57:35Z

app/dashboard/migrations/0056_profile_pref_lang_code.py

@@ -0,0 +1,19 @@
+# Generated by Django 2.0.3 on 2018-04-19 17:47


Migrations will need rebuilt here.

mbeacom · 2018-04-23T20:59:01Z

app/dashboard/models.py

@@ -1043,19 +1044,20 @@ def get_access_token(self, save=True):
            return ''
        return access_token

+    @receiver(user_logged_in)


These shouldn't be on Profile . Can you indent them back?

mbeacom · 2018-04-23T20:59:34Z

app/dashboard/models.py

-    """Handle actions to take on user logout."""
-    from dashboard.utils import create_user_action
-    create_user_action(user, 'Logout', request)
+    def get_profile_preferred_language(self):


You'll want to move this back up to Profile after you move the signals out.

mbeacom · 2018-04-23T21:01:02Z

app/marketing/mails.py

+        preferred_language = profile.get_profile_preferred_language()
+        translation.activate(preferred_language)
+    except Profile.DoesNotExist:
+        print(f'-- Could not determine recipient preferred email, using default.')


No need for literal string interpolation here since we're not using any vars.

mbeacom · 2018-04-23T21:02:58Z

app/marketing/views.py

@@ -611,6 +624,8 @@ def email_settings(request, key):
        'es': es,
        'msg': msg,
        'navs': settings_navs,
+        'available_languages': ['en', 'pl'],


You can drop this now that you're doing: {% get_available_languages as LANGUAGES %} and {% get_language_info_list for LANGUAGES as langs %}

mbeacom

lgtm 🙌

owocki · 2018-04-25T16:17:39Z

its live!

owocki · 2018-04-25T19:41:51Z

#979

do we need to do more regression testing here? this is just a random feature i tried

kziemianek force-pushed the pref_language branch from 4473d0b to 0e39abf Compare April 5, 2018 20:29

mbeacom suggested changes Apr 5, 2018

View reviewed changes

owocki mentioned this pull request Apr 5, 2018

test out autotranslate #814

Closed

mbeacom suggested changes Apr 9, 2018

View reviewed changes

mbeacom added frontend This needs frontend expertise. waiting on contributor Gitcoin Emails Gitcoin Emails labels Apr 9, 2018

kziemianek force-pushed the pref_language branch from 4cf40a5 to 8ce6496 Compare April 9, 2018 18:42

owocki added a commit that referenced this pull request Apr 9, 2018

fix for #810 (review)

81b8a68

mbeacom added needs-testing and removed waiting on contributor labels Apr 10, 2018

kziemianek force-pushed the pref_language branch from 8ce6496 to bd2adf3 Compare April 11, 2018 07:53

kziemianek changed the title ~~WIP: preferred language~~ preferred language Apr 11, 2018

kziemianek force-pushed the pref_language branch from bd2adf3 to 6315636 Compare April 11, 2018 10:31

kziemianek force-pushed the pref_language branch from 6315636 to 863ba2c Compare April 11, 2018 13:38

kziemianek force-pushed the pref_language branch 2 times, most recently from 534525c to 41a5706 Compare April 17, 2018 19:31

stickler-ci reviewed Apr 17, 2018

View reviewed changes

kziemianek force-pushed the pref_language branch 3 times, most recently from 1701c4b to 0047d19 Compare April 19, 2018 17:55

stickler-ci reviewed Apr 19, 2018

View reviewed changes

kziemianek force-pushed the pref_language branch 2 times, most recently from cf7b16d to 6d439a7 Compare April 19, 2018 18:12

stickler-ci reviewed Apr 19, 2018

View reviewed changes

app/dashboard/models.py Outdated

@receiver(user_logged_out)

Copy link

stickler-ci Apr 19, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

E303 too many blank lines (2)

kziemianek force-pushed the pref_language branch from 6d439a7 to 4a1c05d Compare April 19, 2018 18:17

mbeacom added the in progress label Apr 23, 2018

mbeacom suggested changes Apr 23, 2018

View reviewed changes

kziemianek added 2 commits April 25, 2018 01:16

WIP: preferred language

9049a0e

review comments gitcoinco#1

31c838a

kziemianek force-pushed the pref_language branch from 4a1c05d to 2caf41b Compare April 24, 2018 23:28

i18n emails

621282c

kziemianek force-pushed the pref_language branch from 2caf41b to 621282c Compare April 24, 2018 23:32

mbeacom approved these changes Apr 25, 2018

View reviewed changes

mbeacom added mergeable and removed needs review labels Apr 25, 2018

mbeacom merged commit d01574a into gitcoinco:master Apr 25, 2018

mbeacom removed the in progress label Apr 25, 2018

mbeacom mentioned this pull request Apr 25, 2018

Modify setup_lang to use User #980

Merged

3 tasks

kziemianek deleted the pref_language branch December 4, 2018 11:45

		@@ -0,0 +1,19 @@
		# Generated by Django 2.0.3 on 2018-04-19 17:47

Uh oh!

preferred language #810

preferred language #810

Uh oh!

Conversation

kziemianek commented Apr 5, 2018

Description

Checklist

Affected core subsystem(s)

Testing

Refers/Fixes

Uh oh!

kziemianek commented Apr 5, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mbeacom left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kziemianek Apr 9, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

codecov bot commented Apr 6, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kziemianek commented Apr 11, 2018

Uh oh!

mbeacom commented Apr 17, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kziemianek commented Apr 5, 2018 •

edited

Loading

kziemianek Apr 9, 2018 •

edited

Loading

codecov bot commented Apr 6, 2018 •

edited

Loading