Render enrollment emails in the student's language #6082

regisb · 2014-11-27T11:08:16Z

Context: Course instructors have the option to enroll, unenroll and assign the "beta user" role to a list of manually-written email addresses. An email is then sent to each address to inform the address owner of the change.

Problem: the emails that correspond to existing users are not written in the user's language, but in the instructor's language.

Proposed solution: render the email messages and subject using the user's language, if available. If no user corresponds to a given email address, then the instructor's language is used (which is the current default behaviour).

This pull request is related to this Google groups conversation.

openedx-webhooks · 2014-11-27T11:08:19Z

Thanks for the pull request, @regisb! I've created OSPR-244 to keep track of it in JIRA. JIRA is a place for product owners to prioritize feature reviews by the engineering development teams.

Feel free to add as much of the following information to the ticket:

supporting documentation
edx-code email threads
timeline information ('this must be merged by XX date', and why that is)
partner information ('this is a course on edx.org')
any other information that can help Product understand the context for the PR

All technical communication about the code itself will still be done via the Github pull request interface. As a reminder, our process documentation is here.

We can't start reviewing your pull request until you've submitted a signed contributor agreement or indicated your institutional affiliation and added yourself to the AUTHORS file. Please see the CONTRIBUTING file for more information.

sarina · 2014-11-27T13:02:15Z

@regisb this is a good fix! Please be sure to submit your contributors agreement as stated by the bot - we can't review it until that's straightened out.

regisb · 2014-11-27T13:04:00Z

I just sent my contributor agreement yesterday; would you like a copy?

sarina · 2014-11-27T13:12:51Z

Nope - our legal team will process it. It is Thanksgiving holiday for us in the USA, so expect more attention early next week. Thanks!

regisb · 2014-11-27T16:03:24Z

Sounds good! Meanwhile: it seems the Jenkins tests are failing because they were run on a previous version of my PR. Would you mind starting them again manually please?

sarina · 2014-11-27T16:57:25Z

build should automatically trigger when you push a new commit and/or do a
rebase atop master.

On Thu, Nov 27, 2014 at 11:03 AM, Régis B. [email protected] wrote:

Sounds good! Meanwhile: it seems the Jenkins tests are failing because
they were run on a previous version of my PR. Would you mind starting them
again manually please?

—
Reply to this email directly or view it on GitHub
https://github.com/edx/edx-platform/pull/6082#issuecomment-64807328.

regisb · 2014-12-01T13:22:27Z

It seems that tests are passing: there are just a couple violations but they are due to previous commits. Do you think this PR can be merged?

sarina · 2014-12-01T13:47:31Z

@regisb - there are quite a few steps that need to follw before this can be merged, please see our contributors documentation. The label on your PR lets you know what state your PR is in, right now it is "product review". We do product review every other week, the next review is this Thursday AM.

We do need a passing build and communication from our legal team that your contributors agreement is in place, as well.

sarina · 2014-12-11T19:30:20Z

lms/djangoapps/instructor/enrollment.py

@@ -291,6 +294,9 @@ def send_mail_to_student(student, param_dict):
        `is_shib_course`: (a `boolean`)
    ]

+    `language` is the language used to render the email. If None the language
+    currently active will be used.


This is a little confusing. Maybe, "If None, the language of the currently-logged in user (that is, the user sending the email) will be used."

sarina · 2014-12-11T19:33:41Z

This looks good. I have a few changes, and I'd like to see an additional test of the default case as well as the override case.

sarina · 2014-12-12T16:23:23Z

@regisb I cannot verify this on a sandbox - I have a user set to French, and an instructor set to Chinese, with the platform language set to English. When the instructor invites the French user to beta test a course, the French user is still receiving the mail in English.

sarina · 2014-12-12T16:46:15Z

@regisb - putting this ticket in "waiting on author". I restarted all my services on my sandbox and ran the manual test (French user, Chinese instructor, English platform language) again, and I still can't verify this PR works (student user gets email in English, not French). Can you please tell me how you tested this work manually, so I can try to manually verify this PR works?

I also can't verify the problem you initially stated- you said the user gets the email in the instructor's language. But in my testing on master, I see the user always gets the email in the platform language (set by LANGUAGE_CODE), not the instructor's language.

Also, your PR needs a rebase.

regisb · 2014-12-13T13:50:22Z

Sarina,
I'm currently AWK and will return only on the 21st. Sorry about that :-/

The unit test I wrote was failing before my fix. Perhaps can you try to run it without the rest of my PR?

Régis

Sent from my portable toaster. Please excuse the brevity.

Le 12 décembre 2014 22:16:26 GMT+05:30, Sarina Canelake [email protected] a écrit :

@regisb - putting this ticket in "waiting on author". I restarted all
my services on my sandbox and ran the manual test (French user, Chinese
instructor, English platform language) again, and I still can't verify
this PR works (student user gets email in English, not French). Can you
please tell me how you tested this work manually, so I can try to
manually verify this PR works?

I also can't verify the problem you initially stated- you said the user
gets the email in the instructor's language. But in my testing on
master, I see the user always gets the email in the platform language
(set by LANGUAGE_CODE), not the instructor's language.

Also, your PR needs a rebase.

Reply to this email directly or view it on GitHub:
https://github.com/edx/edx-platform/pull/6082#issuecomment-66798896

sarina · 2015-01-14T22:23:14Z

Regarding your testing on master: to be clearer, the language used to send the email is the language of the currently-logged in user. So if the instructor's language is Chinese, but you are sending the email with your (English) staff account, then the email will be sent in English. Did you log in as a Chinese user when you tested the feature on the master branch?

Yes, when I tested this on master, I was logged in as a staff user with a Chinese language preference. I will test this again now.

sarina · 2015-01-14T22:36:47Z

@regisb ah! OK now I am able to validate the fix. Cool! This is a very reasonable fix.

sarina · 2015-01-14T22:38:48Z

lms/djangoapps/instructor/enrollment.py

+    """
+
+    # If language is None we default to the currently active language. Note
+    # that this differs from the behaviour of override_language.


How does this differ from the behavior of override_language?

sarina · 2015-01-14T22:40:06Z

So, my only nagging question is about the default behavior. I feel that the right thing to do is actually to default to the platform language, not the instructor's language. For example if it is a France-based installation but there is a professor teaching a course in French. But he is from China so he sets his own platform language to be Chinese. I think it is more appropriate to default not to his language but to the platform language. @regisb what do you think about this?

regisb · 2015-01-15T14:17:29Z

@sarina Yes, what you are saying about the default behavior makes sense. I amended my PR and added a unit test to test_api_email_localization.py.

sarina · 2015-01-15T14:22:47Z

@regisb cool! I will test today, and if tests pass & testing looks good, then we will be able to merge it 🎆 🎊 ❗ 😄

sarina · 2015-01-15T21:03:40Z

@regisb - can you take a look at your test failures? You have a quality issue (run paver run_quality and make sure your quality report is 100%) You also have a failing unit test. I think the bok-choy test is unrelated.

sarina · 2015-01-16T01:46:58Z

@regisb : just ran a test of your branch and it looks great! Once the tests and quality issues are resolved, this is 👍 to merge.

regisb · 2015-01-19T13:16:31Z

@sarina : great :)

sarina · 2015-01-20T16:18:58Z

@regisb eek. I'm very sorry but could I ask you to rebase one more time?

Pylint broke for us last week (reporting only 6 violations due to an exception)
This was due to an issue between pylint and astroid, which is a supporting library
In order to fix the error, we are upgrading to v 1.4.1 (was: 1.4.0). That was done with this PR/commit (merged moments ago): Resolve run_pylint error. #6693
Changelog for 1.4.1 here: https://bitbucket.org/logilab/pylint/src/02eabae6b72569876e7104db7724ad26a7de0af3/ChangeLog?at=default

Enrollment, unenrollment and beta role emails should be rendered in the student's language, and not the instructor's language.

sarina · 2015-01-21T21:40:46Z

@regisb awesome, thank you so much! This is a great fix.

Render enrollment emails in the student's language

openedx-webhooks added the needs triage label Nov 27, 2014

regisb force-pushed the edx/translate-enrollment-emails branch 2 times, most recently from fe6725b to 457cfee Compare November 27, 2014 11:11

openedx-webhooks added waiting on author PR author needs to resolve review requests, answer questions, fix tests, etc. and removed needs triage labels Nov 27, 2014

openedx-webhooks added product review PR requires product review before merging and removed waiting on author PR author needs to resolve review requests, answer questions, fix tests, etc. labels Nov 27, 2014

regisb force-pushed the edx/translate-enrollment-emails branch from 457cfee to 8d9a7f9 Compare November 27, 2014 13:31

regisb force-pushed the edx/translate-enrollment-emails branch from 8d9a7f9 to 01bc612 Compare December 1, 2014 08:41

regisb force-pushed the edx/translate-enrollment-emails branch 2 times, most recently from d631b60 to b2fdadb Compare December 2, 2014 10:05

openedx-webhooks added the community manager review label Dec 4, 2014

sarina removed the product review PR requires product review before merging label Dec 11, 2014

sarina reviewed Dec 11, 2014
View reviewed changes

openedx-webhooks added waiting on author PR author needs to resolve review requests, answer questions, fix tests, etc. and removed community manager review labels Dec 12, 2014

openedx-webhooks added the community manager review label Jan 14, 2015

sarina reviewed Jan 14, 2015
View reviewed changes

regisb force-pushed the edx/translate-enrollment-emails branch from 514a3b9 to ce4ad04 Compare January 15, 2015 14:16

sarina added the bug (open source) label Jan 15, 2015

openedx-webhooks added waiting on author PR author needs to resolve review requests, answer questions, fix tests, etc. and removed community manager review labels Jan 16, 2015

regisb force-pushed the edx/translate-enrollment-emails branch from ce4ad04 to 653f922 Compare January 19, 2015 08:53

openedx-webhooks added community manager review and removed waiting on author PR author needs to resolve review requests, answer questions, fix tests, etc. labels Jan 20, 2015

regisb added 2 commits January 20, 2015 18:04

Add Régis Behmo to AUTHORS

7a2f064

Render enrollment emails in the student's language

f3419bb

Enrollment, unenrollment and beta role emails should be rendered in the student's language, and not the instructor's language.

regisb force-pushed the edx/translate-enrollment-emails branch from 653f922 to f3419bb Compare January 20, 2015 17:08

sarina added a commit that referenced this pull request Jan 21, 2015

Merge pull request #6082 from openfun/edx/translate-enrollment-emails

e70a5e2

Render enrollment emails in the student's language

sarina merged commit e70a5e2 into openedx:master Jan 21, 2015

regisb deleted the edx/translate-enrollment-emails branch January 22, 2015 09:31

sarina mentioned this pull request Feb 4, 2015

(WIP) Translatable bulk email "from address" #6889

Closed

1 task

yrchen mentioned this pull request Sep 14, 2015

Fix translate enrollment emails #9748

Closed

anonim-01 mentioned this pull request Dec 27, 2023

[Snyk] Security upgrade axios from 0.27.2 to 1.6.3 anonim-01/open-okul-lms#2

Open

baby636 mentioned this pull request Dec 28, 2023

[Snyk] Security upgrade axios from 0.21.1 to 1.6.3 baby636/edx-platform#57

Open

baby636 mentioned this pull request Jan 6, 2024

[Snyk] Security upgrade axios from 0.21.1 to 1.6.4 baby636/edx-platform#58

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Render enrollment emails in the student's language #6082

Render enrollment emails in the student's language #6082

regisb commented Nov 27, 2014

openedx-webhooks commented Nov 27, 2014

sarina commented Nov 27, 2014

regisb commented Nov 27, 2014

sarina commented Nov 27, 2014

regisb commented Nov 27, 2014

sarina commented Nov 27, 2014

regisb commented Dec 1, 2014

sarina commented Dec 1, 2014

sarina Dec 11, 2014

sarina commented Dec 11, 2014

sarina commented Dec 12, 2014

sarina commented Dec 12, 2014

regisb commented Dec 13, 2014

sarina commented Jan 14, 2015

sarina commented Jan 14, 2015

sarina Jan 14, 2015

sarina commented Jan 14, 2015

regisb commented Jan 15, 2015

sarina commented Jan 15, 2015

sarina commented Jan 15, 2015

sarina commented Jan 16, 2015

regisb commented Jan 19, 2015

sarina commented Jan 20, 2015

sarina commented Jan 21, 2015

Render enrollment emails in the student's language #6082

Render enrollment emails in the student's language #6082

Conversation

regisb commented Nov 27, 2014

openedx-webhooks commented Nov 27, 2014

sarina commented Nov 27, 2014

regisb commented Nov 27, 2014

sarina commented Nov 27, 2014

regisb commented Nov 27, 2014

sarina commented Nov 27, 2014

regisb commented Dec 1, 2014

sarina commented Dec 1, 2014

sarina Dec 11, 2014

Choose a reason for hiding this comment

sarina commented Dec 11, 2014

sarina commented Dec 12, 2014

sarina commented Dec 12, 2014

regisb commented Dec 13, 2014

The unit test I wrote was failing before my fix. Perhaps can you try to run it without the rest of my PR?

sarina commented Jan 14, 2015

sarina commented Jan 14, 2015

sarina Jan 14, 2015

Choose a reason for hiding this comment

sarina commented Jan 14, 2015

regisb commented Jan 15, 2015

sarina commented Jan 15, 2015

sarina commented Jan 15, 2015

sarina commented Jan 16, 2015

regisb commented Jan 19, 2015

sarina commented Jan 20, 2015

sarina commented Jan 21, 2015