Unicode changes to support R to L script #2137

jzoldak · 2014-01-10T17:21:35Z

We are separating out the QRF PR into discrete features.
This is just the unicode changes.

singingwolfboy · 2014-01-10T18:20:49Z

cms/djangoapps/contentstore/views/course.py

This seems unrelated to the unicode changes.

it select yaml files based on language. I can remove it to be added till we figure out the yaml files.

singingwolfboy · 2014-01-23T15:51:23Z

@jzoldak, can you rebase this pull request?

a-nassif · 2014-01-23T15:59:13Z

No prob. Will rebase again tonight.
On 23 Jan 2014 17:51, "David Baumgold" notifications@github.com wrote:

@jzoldak https://github.com/jzoldak, can you rebase this pull request?

—
Reply to this email directly or view it on GitHubhttps://github.com/edx/edx-platform/pull/2137#issuecomment-33135690
.

singingwolfboy · 2014-01-23T16:02:39Z

cms/djangoapps/contentstore/views/course.py

Why import os? It doesn't look like it's being used anywhere in this file.

singingwolfboy · 2014-01-23T16:11:34Z

Once this PR is rebased and the tests pass, I'm giving it a 👍. The only issues I found were minor nitpicks.

jzoldak · 2014-01-24T18:41:57Z

@abdoosh00 I will squash commits then rebase to current edx/master.

a-nassif · 2014-01-24T18:46:47Z

Sounds good Jay...is my rebase messed up again? :s
On 24 Jan 2014 20:42, "Jay Zoldak" notifications@github.com wrote:

@abdoosh00 https://github.com/abdoosh00 I will squash commits then
rebase to current edx/master.

—
Reply to this email directly or view it on GitHubhttps://github.com/edx/edx-platform/pull/2137#issuecomment-33249153
.

jzoldak · 2014-01-24T18:56:43Z

@abdoosh00 no, it wasn't messed up, just getting stale. We needed to get it up to date so it could be current with master and also make sure that this branch had some platform changes we needed to continue building correctly on our jenkins server. Please delete the branch locally then pull again (or otherwise force pull) in order to get the rebase changes that I force pushed.

a-nassif · 2014-01-24T19:27:50Z

Ok, i thought this was #2138 (the html changes one) cuz thats the one i
just rebased. I will rebase this asap, or feel free if you wanna rebase it
yourself.
On 24 Jan 2014 20:57, "Jay Zoldak" notifications@github.com wrote:

@abdoosh00 https://github.com/abdoosh00 no, it wasn't messed up, just
getting stale. We needed to get it up to date so it could be current with
master and also make sure that this branch had some platform changes we
needed to continue building correctly on our jenkins server. Please delete
the branch locally then pull again (or otherwise force pull) in order to
get the rebase changes that I force pushed.

—
Reply to this email directly or view it on GitHubhttps://github.com/edx/edx-platform/pull/2137#issuecomment-33250465
.

jzoldak · 2014-01-24T19:44:18Z

@abdoosh00 I rebased but could not get test results because the total number of pylint violations exceeded our threshold that we have set on the Jenkins server. I just added another commit that fixed these.

a-nassif · 2014-01-25T11:11:23Z

@jzoldak could you please force push your last changes again, I hope you didn't delete it already :s. I forced push to feature/rtl_sass branch and it pushed to both branches. is there a way to specify the exact branch to push to only? or do I have to make sure that everything is up-to-date before doing a force push?

jzoldak · 2014-01-25T12:41:35Z

@abdoosh00 you need to change your git push default to 'simple' or 'current' so that it will only push the branch that you're on. See https://www.kernel.org/pub/software/scm/git/docs/git-config.html. You can do this with git config --global push.default simple

Unfortunately I'm traveling and won't have access to the machine that I have a copy of your fork of the repo on until Tuesday. :( You might be able to recover with your version of the repo using your git reflog. A git expert should be able to help you with this, maybe ask @singingwolfboy?

a-nassif · 2014-01-27T13:32:04Z

@singingwolfboy @jzoldak All should be fine now here and on #2138 , tests passed and comments responded to as required.

ahmadio · 2014-01-28T22:28:55Z

I'd like to help with some unicode problems that you didn't cover in this PR, do I need to create new PR or post it here?
sorry if I'm not experienced enough with git or github

a-nassif · 2014-01-28T22:39:29Z

You can tell me about it to fix it. What problems? Did you test this PR and
got unicode/decode errors?
On 29 Jan 2014 00:29, "ahmadix" notifications@github.com wrote:

I'd like to help with some unicode problems that you didn't cover in this
PR, do I need to create new PR or post it here?
sorry if I'm not experienced enough with git or github

Reply to this email directly or view it on GitHubhttps://github.com/edx/edx-platform/pull/2137#issuecomment-33534524
.

ahmadio · 2014-01-28T22:54:18Z

alright @abdoosh00 I will give detailed info about errors I faced, let us take it one by one:
1- bulk_mail task
when sending bulk mail for a course with a unicode title I get this:

Traceback (most recent call last):
  File "/edx/app/edxapp/venvs/edxapp/local/lib/python2.7/site-packages/celery/task/trace.py", line 228, in trace_task
    R = retval = fun(*args, **kwargs)
  File "/edx/app/edxapp/venvs/edxapp/local/lib/python2.7/site-packages/celery/task/trace.py", line 415, in __protected_call__
    return self.run(*args, **kwargs)
  File "/edx/app/edxapp/edx-platform/lms/djangoapps/bulk_email/tasks.py", line 299, in send_course_email
    with dog_stats_api.timer('course_email.single_task.time.overall', tags=[_statsd_tag(course_title)]):
  File "/edx/app/edxapp/edx-platform/lms/djangoapps/bulk_email/tasks.py", line 680, in _statsd_tag
    course_title = course_title.encode('utf-8')
UnicodeEncodeError: 'ascii' codec can't encode characters in position 0-4: ordinal not in range(128)

so did you worked a solution for it in this PR?

Note: I'm already testing edx with a real academy with one of your branches, so I'm getting lots of feedback.

a-nassif · 2014-01-29T10:42:06Z

its good to know that this is being tested. If you face any problems let us know for sure, send directly to my e-mail (abdoosh00@gmail.com) or post it on the edx-code google group and ill help resolving or resolve it myself.

a-nassif · 2014-01-29T10:54:43Z

may i also ask what branch you using? I couldn't recall this line of code and its certainly not in the latest code in the platform, is it an old branch?

jzoldak · 2014-01-30T18:10:07Z

cms/static/js/index.js

This comment won't make sense once it's merged into the codebase because it refers to what the code used to do.

jzoldak · 2014-01-30T20:30:47Z

squashing and rebasing...

fixing unit tests fixing merge error fixing xqueue submission issue with unicode url (trial 0.1) fixing fotmats as commented upon removing yaml file language selection Unicode changes to support QRF removed unnecessary pass in modulestore/init.py fixing merge error fixing fotmats as commented upon removing yaml file language selection fixing pep8 violations - fixing pylint violations pylint violation fixing line spaces and formats ignore pylint E1101 remove empty line fixing pylint violations pep8 violations bulk mail unicode/decode fix migration error fix pep8 just to push again more unicode/decode Final changes to comments and error messages.

jzoldak · 2014-01-30T21:26:27Z

I've addressed any small concerns that I had, squashed, rebased, and force pushed.
@abdoosh00 and @Ahmadix please force pull or delete your old versions of this branch and then pull.
I'll let the tests run on jenkins, and do some manual spot-checking with a unicode org and assets with unicode filenames, then I think we're good to merge!

jzoldak · 2014-01-31T13:34:33Z

@singingwolfboy can I get a thumbsup?

jzoldak · 2014-01-31T13:38:54Z

FYI observations from some manual testing on this branch:

Only the display name of the course can contain non latin characters, Studio prevents you from using non latin characters in the org, course number, or run
Studio translates non-latin characters in asset filenames to underscores on upload

singingwolfboy · 2014-01-31T13:44:20Z

👍

a-nassif · 2014-01-31T13:47:24Z

@jzoldak I am working on the comments you stated above. Should be done in few minutes.

a-nassif · 2014-01-31T13:50:01Z

@jzoldak actaully i just read @singingwolfboy comment on the u"" issue you stated. I think its fine both ways, its only a statement, works with and without u"".

jzoldak · 2014-01-31T14:47:08Z

@abdoosh00 none of my comments are critical. I'll merge as-is.

Unicode changes to support R to L script

a-nassif · 2014-01-31T14:48:10Z

@jzoldak sounds good :)

singingwolfboy reviewed Jan 10, 2014
View reviewed changes

singingwolfboy reviewed Jan 23, 2014
View reviewed changes

jzoldak reviewed Jan 30, 2014
View reviewed changes

jzoldak pushed a commit that referenced this pull request Jan 31, 2014

Merge pull request #2137 from abdoosh00/feature/unicode

9cfd649

Unicode changes to support R to L script

jzoldak merged commit 9cfd649 into openedx:master Jan 31, 2014

Unicode changes to support R to L script #2137

Unicode changes to support R to L script #2137

Uh oh!

Conversation

jzoldak commented Jan 10, 2014

Uh oh!

singingwolfboy Jan 10, 2014

Choose a reason for hiding this comment

Uh oh!

a-nassif Jan 13, 2014

Choose a reason for hiding this comment

Uh oh!

singingwolfboy commented Jan 23, 2014

Uh oh!

a-nassif commented Jan 23, 2014

Uh oh!

singingwolfboy Jan 23, 2014

Choose a reason for hiding this comment

Uh oh!

singingwolfboy commented Jan 23, 2014

Uh oh!

jzoldak commented Jan 24, 2014

Uh oh!

a-nassif commented Jan 24, 2014

Uh oh!

jzoldak commented Jan 24, 2014

Uh oh!

a-nassif commented Jan 24, 2014

Uh oh!

jzoldak commented Jan 24, 2014

Uh oh!

a-nassif commented Jan 25, 2014

Uh oh!

jzoldak commented Jan 25, 2014

Uh oh!

a-nassif commented Jan 27, 2014

Uh oh!

ahmadio commented Jan 28, 2014

Uh oh!

a-nassif commented Jan 28, 2014

Uh oh!

ahmadio commented Jan 28, 2014

Uh oh!

a-nassif commented Jan 29, 2014

Uh oh!

a-nassif commented Jan 29, 2014

Uh oh!

jzoldak Jan 30, 2014

Choose a reason for hiding this comment

Uh oh!

jzoldak commented Jan 30, 2014

Uh oh!

jzoldak commented Jan 30, 2014

Uh oh!

jzoldak commented Jan 31, 2014

Uh oh!

jzoldak commented Jan 31, 2014

Uh oh!

singingwolfboy commented Jan 31, 2014

Uh oh!

a-nassif commented Jan 31, 2014

Uh oh!

a-nassif commented Jan 31, 2014

Uh oh!

jzoldak commented Jan 31, 2014

Uh oh!

a-nassif commented Jan 31, 2014

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants