Fix #3551: Manually set cookie value during language change #3622

muxout · 2017-07-05T09:37:34Z

When a new language is selected the language code is stored in a cookie. But when the same piece of code is invoked from the library page the value in the cookie doesn't change.
This seem to fix the issue but I haven't quite figured out why. Maybe someone familiar with Angular should have a look.

…library page

seanlip · 2017-07-05T10:13:35Z

@kevinlee12 -- do you remember why you introduced the base tag in this commit? 2b8e97b

codecov-io · 2017-07-05T11:23:30Z

Codecov Report

Merging #3622 into develop will decrease coverage by <.01%.
The diff coverage is 0%.

@@             Coverage Diff             @@
##           develop    #3622      +/-   ##
===========================================
- Coverage    45.03%   45.03%   -0.01%     
===========================================
  Files          261      261              
  Lines        20219    20220       +1     
  Branches      3157     3157              
===========================================
  Hits          9106     9106              
- Misses       11113    11114       +1

Impacted Files	Coverage Δ
core/templates/dev/head/i18n.js	`52.63% <0%> (-1.43%)`	⬇️

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 bc31014...26e7b10. Read the comment docs.

kevinlee12 · 2017-07-05T18:00:33Z

Ah, that was needed so that we can manipulate the history for the search function (since we're adding browser history entries without reloading the page). More specifically, it was changing the history as the learner was typing in queries into the search bar in the library. The tag itself was to segregate the search page into it's own "app" so that this would work.

muxout · 2017-07-20T10:17:55Z

After further investigation, I found that cookie path is affected by the base url: https://docs.angularjs.org/api/ngCookies/provider/$cookiesProvider#defaults
This explains the behavior on the Library page. As a workaround, maybe we could set the cookie value manually.

seanlip · 2017-07-20T10:21:36Z

Thanks, @muxout. That's a great find, and I think it explains some of the existing behaviour!

It seems like that's a global default, though. I wonder whether it may be better to investigate the alternative instead, which is to make the library/search page function well without needing to overwrite the base tag. What do you think?

anthkris · 2017-08-01T16:51:58Z

@muxout How is this going? Did you see Sean's comment above?

muxout · 2017-08-03T09:28:00Z

@anthkris I have yet to find a solution to the approach that @seanlip suggested.
I have pushed a commit that sets the cookie value manually.

seanlip · 2017-08-03T09:52:57Z

core/templates/dev/head/i18n.js

@@ -90,6 +90,11 @@ oppia.controller('I18nFooter', [
        $http.put(siteLanguageUrl, {
          site_language_code: $scope.currentLanguageCode
        });
+      } else {
+        // This is a workaround for the library page as the base url


I think this approach is reasonable, actually. But I'm a little confused about when it's applied. From the conditional, it seems to be applied when the user is not logged in (as opposed to when the user is on the library page)?

This issue only affects users who are not logged in, as per #3551. I thought this might be the easiest workaround.

Oh, sorry, now I get it!

Yep, makes sense. My understanding is that sometimes $translate sets this cookie automatically, but we are setting it manually here in all cases for safety's sake. Is that right? If so, that makes sense to me.

Can we explain this a bit more in the comment so that future devs have context? At the moment it reads like "this is something to be applied only in the library page", which is why I asked the question I did.

Ah yes that's what I meant. Agreed.

Ah, sorry, I meant something a bit different. How about:

// $translate.use() sets a cookie for the translation language, but does so using the // page's base URL as the cookie path. However, the base URL is modified in pages // like /library, thus causing the cookie path to change; in such cases, the user's // preferences are not picked up by other pages. To avoid this, we manually set // the cookie using the '/' path each time a non-logged-in user changes their // site language.

I think this provides the full context (i.e. a bit more info on the cookie path and what's really going on here) that would be useful to devs.

Agreed, it's much clearer.

seanlip

Just a few small comments. I think this looks fantastic, and solves a long-standing issue. Thanks very much, @muxout!

seanlip · 2017-08-03T10:05:37Z

core/templates/dev/head/i18n.js

@@ -90,6 +90,11 @@ oppia.controller('I18nFooter', [
        $http.put(siteLanguageUrl, {
          site_language_code: $scope.currentLanguageCode
        });
+      } else {
+        // This is a workaround for the library page as the base url


Oh, sorry, now I get it!

Yep, makes sense. My understanding is that sometimes $translate sets this cookie automatically, but we are setting it manually here in all cases for safety's sake. Is that right? If so, that makes sense to me.

Can we explain this a bit more in the comment so that future devs have context? At the moment it reads like "this is something to be applied only in the library page", which is why I asked the question I did.

seanlip · 2017-08-03T10:05:47Z

core/templates/dev/head/i18n.js

@@ -90,6 +90,11 @@ oppia.controller('I18nFooter', [
        $http.put(siteLanguageUrl, {
          site_language_code: $scope.currentLanguageCode
        });
+      } else {
+        // This is a workaround for the library page as the base url
+        // changes the default cookie path


Nit: End comments with a period.

seanlip · 2017-08-03T10:06:09Z

core/templates/dev/head/i18n.js

+      } else {
+        // This is a workaround for the library page as the base url
+        // changes the default cookie path
+        $cookies.put('NG_TRANSLATE_LANG_KEY',


Nit: if multiline strings are contained in (...), break after the first (

seanlip

LGTM. Thanks @muxout!

* upstream/develop: (39 commits) Add a one-off job to list which explorations use gadgets; remove old fallbacks job. (oppia#3717) Fix oppia#2998 implement collection skills update commands (oppia#3710) Improvements and bug fixes to changes introduced by PR oppia#3671 (oppia#3684) Fix oppia#3688: refactoring ExplorationStatusHandler to remove unpublish and publicize functionality as it is not called from frontend. (oppia#3715) fix oppia#3698: Shift exception handling for NotLoggedInException so that it does not generate log errors. (oppia#3714) Revert "Fix oppia#3707 Change text of collection learner view "start here"" (oppia#3713) Fix oppia#3701 Fixed Continue button name issue (oppia#3711) Fix oppia#3467: Introduce re-training and add_to_training_queue methods (oppia#3650) Fix oppia#3707 Change text of collection learner view "start here" (oppia#3700) Audio fixes: make audio download URL safe; don't show flagging modal if all translations are flagged; use templateUrl for modals. (oppia#3709) Learner audio updates (oppia#3705) Move hint button to left corner for supplemental cards (oppia#3702) Make the representation of the learner's 'Continue' answer more conversational. (oppia#3675) Add editor for audio translations (oppia#3692) Fix oppia#3686 fixed text overflow for contributors list (oppia#3697) Fix oppia#3551: Manually set cookie value during language change (oppia#3622) Learner view audio translations (oppia#3681) The sixth milestone of the learner dashboard. (oppia#3680) Routine update of translations. (oppia#3685) SiteWide ACL Refactor: Milestone 3.1 (oppia#3682) ...

Changed base_url to root in an attempt to fix language change on the …

4039c1a

…library page

anthkris assigned muxout Aug 1, 2017

anthkris mentioned this pull request Aug 1, 2017

Translations are not being persisted consistently for non-logged-in users #3551

Closed

muxout added 2 commits August 3, 2017 09:20

Merge branch 'develop' into language-persistency

4b1eb42

Manually set cookie value upon language change

94e0113

seanlip reviewed Aug 3, 2017

View reviewed changes

seanlip requested changes Aug 3, 2017

View reviewed changes

muxout added 2 commits August 3, 2017 10:22

Addressed review comments

30356d8

Updated comment as suggested

26e7b10

muxout changed the title ~~Fix #3551: Changed base_url to root in an attempt to fix language change on the …~~ Fix #3551: Manually set cookie value during language change Aug 3, 2017

seanlip approved these changes Aug 3, 2017

View reviewed changes

seanlip added the PR: LGTM label Aug 3, 2017

seanlip merged commit 0ea0c85 into develop Aug 3, 2017

seanlip deleted the language-persistency branch August 3, 2017 19:02

seanlip mentioned this pull request Aug 4, 2017

Flakiness of changing the site language #3096

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix #3551: Manually set cookie value during language change #3622

Fix #3551: Manually set cookie value during language change #3622

muxout commented Jul 5, 2017 •

edited

Loading

seanlip commented Jul 5, 2017

codecov-io commented Jul 5, 2017 •

edited

Loading

kevinlee12 commented Jul 5, 2017

muxout commented Jul 20, 2017

seanlip commented Jul 20, 2017

anthkris commented Aug 1, 2017

muxout commented Aug 3, 2017

seanlip Aug 3, 2017

muxout Aug 3, 2017

seanlip Aug 3, 2017

muxout Aug 3, 2017

seanlip Aug 3, 2017

muxout Aug 3, 2017

seanlip left a comment

seanlip Aug 3, 2017

seanlip Aug 3, 2017

muxout Aug 3, 2017

seanlip Aug 3, 2017

muxout Aug 3, 2017

seanlip left a comment

Fix #3551: Manually set cookie value during language change #3622

Fix #3551: Manually set cookie value during language change #3622

Conversation

muxout commented Jul 5, 2017 • edited Loading

seanlip commented Jul 5, 2017

codecov-io commented Jul 5, 2017 • edited Loading

Codecov Report

kevinlee12 commented Jul 5, 2017

muxout commented Jul 20, 2017

seanlip commented Jul 20, 2017

anthkris commented Aug 1, 2017

muxout commented Aug 3, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

seanlip left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

seanlip left a comment

Choose a reason for hiding this comment

muxout commented Jul 5, 2017 •

edited

Loading

codecov-io commented Jul 5, 2017 •

edited

Loading