To support UNICODE xblock.location #6167

fmyzjs · 2014-12-07T08:46:15Z

When people use unicode course org name(e.g Chinese) ,urllib.quote cannot format unicode char like:

>>> import urllib
>>> loc = 'i4x://创联/7302478/chapter/4ab413b41ad9447db41668bed03f149f'
>>> urllib.quote(loc)
'i4x%3A//%E5%88%9B%E8%81%94/7302478/chapter/4ab413b41ad9447db41668bed03f149f'
>>> urllib.quote(unicode(loc))
Traceback (most recent call last):
  File "<console>", line 1, in <module>
UnicodeDecodeError: 'ascii' codec can't decode byte 0xe5 in position 6: ordinal not in range(128)
>>>

so I open this pull
@sarina @nedbat @singingwolfboy

openedx-webhooks · 2014-12-07T08:46:17Z

Thanks for the pull request, @fmyzjs! I've created OSPR-269 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.

sarina · 2014-12-08T13:35:29Z

I'm unclear if we officially actually support unicode xblock locations - I can't imagine this is the only problem, because we don't have tests around this. @nedbat - do you have any thoughts / can you weigh in?

@fmyzjs this will definitely need tests if we are going to merge this.

sarina · 2014-12-09T14:15:43Z

@fmyzjs - to be clear I put this PR in the "waiting on author" category because it needs unit tests.

fmyzjs · 2014-12-11T14:26:22Z

@sarina I don't think need tests here. edX officially actually support unicode xblock locations. cms.envs.common.I open this pull because urllib.quote cannot decode unicode url string.

sarina · 2014-12-11T15:32:08Z

@fmyzjs unit tests will validate this change and ensure we don't break unicode support in the future. This is important because as far as I know, we actively disallow unicode characters in locations on edx.org. So if there are no unit tests, is possible we will break in the future.

sarina · 2014-12-15T23:46:22Z

cms/djangoapps/contentstore/views/tests/test_helpers.py

+    def test_unicode_xblock_studio_url(self):
+
+        #import unicode course
+        modulestore = XMLModuleStore(DATA_DIR, course_dirs=['2014_Uni'])


What makes this a Unicode course?

@sarina IN common/test/data/2014_Uni

sarina · 2014-12-16T22:30:39Z

👍 from me. I'm asking someone from our sustaining team to take a look at this, as well.

singingwolfboy · 2014-12-29T20:45:35Z

@fmyzjs, there are a lot of commits in here that don't seem related to the work you've done. Could you rebase this pull request, to make sure that it only contains the work that you did?

adampalay · 2015-01-08T15:02:00Z

@cpennington , do we support unicode in xblock locations? The last I knew, we didn't support unicode org/course/names, but I could be wrong

cpennington · 2015-01-08T16:29:26Z

I don't think we are yet ready to say that it's fully supported, but I'm happy to see progress towards that front. I believe that opaque_keys in general supports non-ascii characters, so it's just consumers of those keys that will need to adjust, generally.

adampalay · 2015-01-08T16:32:03Z

so once tests pass, you're cool with this PR?

cpennington · 2015-01-08T17:12:25Z

Yeah. I guess I don't know what will happen on the other end (whether Studio will get a unicode object from django, or a string object, and whether studio is correctly handling whichever one it does get), so it's probably worth verifying that.

adampalay · 2015-02-17T21:27:02Z

hi @fmyzjs , may you please rebase this PR?

adampalay · 2015-02-17T21:27:13Z

👍 once it's rebased

cpennington · 2015-02-18T18:46:05Z

The example at the top of the PR isn't actually correct usage. In particular:

>>> loc = 'i4x://创联/7302478/chapter/4ab413b41ad9447db41668bed03f149f'
>>> unicode(loc)
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
UnicodeDecodeError: 'ascii' codec can't decode byte 0xe5 in position 6: ordinal not in range(128)

You can't call unicode on a bytestring (str) object without specifying what the encoding of the bytestring is. The reference to urllib is a red-herring, in the initial example.

cpennington · 2015-02-18T18:47:06Z

That said:

>>> urllib.quote(u'i4x://创联/7302478/chapter/4ab413b41ad9447db41668bed03f149f')
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
  File "/usr/lib/python2.7/urllib.py", line 1288, in quote
    return ''.join(map(quoter, s))
KeyError: u'\u521b'

cpennington · 2015-02-18T18:47:36Z

👍

sarina · 2015-02-18T19:07:04Z

@fmyzjs please rebase, and ping us to let us know when you've done that. The PR must be rebased to resolve some conflicts.

sarina · 2015-02-26T16:29:05Z

@fmyzjs - we need you to rebase this PR. If you don't respond within 5 days, we will have to close this PR.

sarina · 2015-03-03T03:42:47Z

@fmyzjs - please fix quality build (https://build.testeng.edx.org/job/edx-platform-test-subset/5332/artifact/reports/cms/pep8.report/*view*/). Once you are done please comment on the pull request. We do not get notified when you make or update a commit, we only get notified when you tell us it is ready for review.

When people use unicode course org name(e.g Chinese) ,urllib.quote cannot format unicode char like: ```shell >>> import urllib >>> loc = 'i4x://创联/7302478/chapter/4ab413b41ad9447db41668bed03f149f' >>> urllib.quote(loc) 'i4x%3A//%E5%88%9B%E8%81%94/7302478/chapter/4ab413b41ad9447db41668bed03f149f' >>> urllib.quote(unicode(loc)) Traceback (most recent call last): File "<console>", line 1, in <module> UnicodeDecodeError: 'ascii' codec can't decode byte 0xe5 in position 6: ordinal not in range(128) >>> ``` so I open this pull

sarina · 2015-03-16T13:04:38Z

@fmyzjs please write a comment on a pull request when you've responded. We don't get notices when you push new commits!

To support UNICODE xblock.location

sarina · 2015-03-16T14:18:45Z

@fmyzjs we had to revert this commit because it apparently wasn't properly rebased. #7367

Please submit a brand-new pull request with this commit on it once #7367 is merged. Please make sure it is rebased on top of the most recent master branch.

openedx-webhooks added the needs triage label Dec 7, 2014

fmyzjs force-pushed the patch-5 branch from 7f09ade to c84568e Compare December 7, 2014 15:47

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

fmyzjs force-pushed the patch-5 branch from 131d8d4 to cc30919 Compare December 12, 2014 03:49

sarina reviewed Dec 15, 2014
View reviewed changes

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

wedaly force-pushed the master branch from d4de932 to d2b4a6e Compare December 29, 2014 16:20

fmyzjs force-pushed the patch-5 branch from cc30919 to 55e13c3 Compare January 1, 2015 13:48

openedx-webhooks removed the awaiting prioritization label Feb 18, 2015

openedx-webhooks added the waiting on author PR author needs to resolve review requests, answer questions, fix tests, etc. label Feb 18, 2015

fmyzjs force-pushed the patch-5 branch from 55e13c3 to 4a4f525 Compare February 27, 2015 07:40

fmyzjs force-pushed the patch-5 branch from 4a4f525 to ef257fb Compare March 3, 2015 12:31

sarina added a commit that referenced this pull request Mar 16, 2015

Merge pull request #6167 from fmyzjs/patch-5

dadc47c

To support UNICODE xblock.location

sarina merged commit dadc47c into openedx:master Mar 16, 2015

jzoldak mentioned this pull request Mar 16, 2015

Revert "To support UNICODE xblock.location" #7367

Merged

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

To support UNICODE xblock.location #6167

To support UNICODE xblock.location #6167

fmyzjs commented Dec 7, 2014

openedx-webhooks commented Dec 7, 2014

sarina commented Dec 8, 2014

sarina commented Dec 9, 2014

fmyzjs commented Dec 11, 2014

sarina commented Dec 11, 2014

sarina Dec 15, 2014

fmyzjs Dec 15, 2014

sarina commented Dec 16, 2014

singingwolfboy commented Dec 29, 2014

adampalay commented Jan 8, 2015

cpennington commented Jan 8, 2015

adampalay commented Jan 8, 2015

cpennington commented Jan 8, 2015

adampalay commented Feb 17, 2015

adampalay commented Feb 17, 2015

cpennington commented Feb 18, 2015

cpennington commented Feb 18, 2015

cpennington commented Feb 18, 2015

sarina commented Feb 18, 2015

sarina commented Feb 26, 2015

sarina commented Mar 3, 2015

sarina commented Mar 16, 2015

sarina commented Mar 16, 2015

To support UNICODE xblock.location #6167

To support UNICODE xblock.location #6167

Conversation

fmyzjs commented Dec 7, 2014

openedx-webhooks commented Dec 7, 2014

sarina commented Dec 8, 2014

sarina commented Dec 9, 2014

fmyzjs commented Dec 11, 2014

sarina commented Dec 11, 2014

sarina Dec 15, 2014

Choose a reason for hiding this comment

fmyzjs Dec 15, 2014

Choose a reason for hiding this comment

sarina commented Dec 16, 2014

singingwolfboy commented Dec 29, 2014

adampalay commented Jan 8, 2015

cpennington commented Jan 8, 2015

adampalay commented Jan 8, 2015

cpennington commented Jan 8, 2015

adampalay commented Feb 17, 2015

adampalay commented Feb 17, 2015

cpennington commented Feb 18, 2015

cpennington commented Feb 18, 2015

cpennington commented Feb 18, 2015

sarina commented Feb 18, 2015

sarina commented Feb 26, 2015

sarina commented Mar 3, 2015

sarina commented Mar 16, 2015

sarina commented Mar 16, 2015