New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

ability to change language via url redirect, added docstring #5342

Merged
merged 2 commits into from Nov 26, 2016

Conversation

Projects
None yet
3 participants
@needlestack
Contributor

needlestack commented Nov 24, 2016

Summary

(resubmitting against develop branch per request)

In multi-lingual environments, it is not user friendly for each user to change the default language manually with the drop-down box on the bottom of the page. This patch allows setting the language through a link, which then redirects the user to the requested content. We use this patch in RACHEL for our multi-lingual installations. You can see it in action here, where we link to English, French, and Spanish versions on the same server:

http://rachelfriends.org/previews/rachelplus-en/
http://rachelfriends.org/previews/rachelplus-es/
http://rachelfriends.org/previews/rachelplus-fr/

It would be nice if this functionality were included in the official release.

TODO

  • No tests existed for this module - I don't know how to write tests for python/django
  • I have added documentation (there was none before)
  • New dependencies in this file: from django.shortcuts import redirect
@benjaoming

This comment has been minimized.

Show comment
Hide comment
@benjaoming

benjaoming Nov 25, 2016

Member

This looks good, except we now have a new test failure :) This is reported in Circle CI...

======================================================================
FAIL: test_responses (kalite.distributed.tests.url_tests.AllUrlsTest)
----------------------------------------------------------------------
Traceback (most recent call last):
  File "/home/ubuntu/ka-lite/kalite/distributed/tests/url_tests.py", line 138, in test_responses
    check_urls(module.urlpatterns)
  File "/home/ubuntu/ka-lite/kalite/distributed/tests/url_tests.py", line 95, in check_urls
    check_urls(pattern.url_patterns, prefix=new_prefix)
  File "/home/ubuntu/ka-lite/kalite/distributed/tests/url_tests.py", line 95, in check_urls
    check_urls(pattern.url_patterns, prefix=new_prefix)
  File "/home/ubuntu/ka-lite/kalite/distributed/tests/url_tests.py", line 127, in check_urls
    url=url, status_code=response.status_code))
AssertionError: 500 not found in [200, 302, 400, 401, 404, 405] : /api/i18n/set_default_language/ gave status code 500

I believe the test may be caused by calling the page by GET without the lang param set. You should probably just fail gracefully or return a 302 redirect?

You can run the failing test like this: bin/kalite manage test distributed.AllUrlsTest

Member

benjaoming commented Nov 25, 2016

This looks good, except we now have a new test failure :) This is reported in Circle CI...

======================================================================
FAIL: test_responses (kalite.distributed.tests.url_tests.AllUrlsTest)
----------------------------------------------------------------------
Traceback (most recent call last):
  File "/home/ubuntu/ka-lite/kalite/distributed/tests/url_tests.py", line 138, in test_responses
    check_urls(module.urlpatterns)
  File "/home/ubuntu/ka-lite/kalite/distributed/tests/url_tests.py", line 95, in check_urls
    check_urls(pattern.url_patterns, prefix=new_prefix)
  File "/home/ubuntu/ka-lite/kalite/distributed/tests/url_tests.py", line 95, in check_urls
    check_urls(pattern.url_patterns, prefix=new_prefix)
  File "/home/ubuntu/ka-lite/kalite/distributed/tests/url_tests.py", line 127, in check_urls
    url=url, status_code=response.status_code))
AssertionError: 500 not found in [200, 302, 400, 401, 404, 405] : /api/i18n/set_default_language/ gave status code 500

I believe the test may be caused by calling the page by GET without the lang param set. You should probably just fail gracefully or return a 302 redirect?

You can run the failing test like this: bin/kalite manage test distributed.AllUrlsTest

@codecov-io

This comment has been minimized.

Show comment
Hide comment
@codecov-io

codecov-io Nov 25, 2016

Current coverage is 51.22% (diff: 4.00%)

Merging #5342 into develop will increase coverage by 0.03%

@@            develop      #5342   diff @@
==========================================
  Files           141        141          
  Lines          7447       7458    +11   
  Methods           0          0          
  Messages          0          0          
  Branches          0          0          
==========================================
+ Hits           3812       3820     +8   
- Misses         3635       3638     +3   
  Partials          0          0          

Powered by Codecov. Last update 5d5928f...9e2957b

codecov-io commented Nov 25, 2016

Current coverage is 51.22% (diff: 4.00%)

Merging #5342 into develop will increase coverage by 0.03%

@@            develop      #5342   diff @@
==========================================
  Files           141        141          
  Lines          7447       7458    +11   
  Methods           0          0          
  Messages          0          0          
  Branches          0          0          
==========================================
+ Hits           3812       3820     +8   
- Misses         3635       3638     +3   
  Partials          0          0          

Powered by Codecov. Last update 5d5928f...9e2957b

@needlestack

This comment has been minimized.

Show comment
Hide comment
@needlestack

needlestack Nov 25, 2016

Contributor

Ah, good idea. I have made this change and committed to this branch.

One problem, though -- I tested this change manually, but I was not able to run the test code as directed. When trying against my branch, I got "ImportError: No module named kalite" and when I tried under the master branch I got "ImportError: No module named dbbackup". Sorry, I'm not familiar enough with python and django :/

I guess we'll see if the automatic CircleCI tests turn anything up. Thanks for your help so far!

Contributor

needlestack commented Nov 25, 2016

Ah, good idea. I have made this change and committed to this branch.

One problem, though -- I tested this change manually, but I was not able to run the test code as directed. When trying against my branch, I got "ImportError: No module named kalite" and when I tried under the master branch I got "ImportError: No module named dbbackup". Sorry, I'm not familiar enough with python and django :/

I guess we'll see if the automatic CircleCI tests turn anything up. Thanks for your help so far!

@needlestack

This comment has been minimized.

Show comment
Hide comment
@needlestack

needlestack Nov 26, 2016

Contributor

Not sure what that codecov/patch error means - any suggestions appreciated...

Contributor

needlestack commented Nov 26, 2016

Not sure what that codecov/patch error means - any suggestions appreciated...

@benjaoming

This comment has been minimized.

Show comment
Hide comment
@benjaoming

benjaoming Nov 26, 2016

Member

@needlestack thanks that's great!! I would think that now we have the tests working, then it's safe to merge. Judging from the previous fail then the tests do reach part of the function.

codecov/patch is the metric for the code that's affected - I think it means that tests has only reached 4% of the changed lines.

Overall, it stacks up, but it means that you have contributed code where I have to rely on your assertion that it's working and my own ability to read code :)

But it's all good! I will write something for the release notes and possibly add a quick remark to ensure that future devs are able to understand why this code is there.

Member

benjaoming commented Nov 26, 2016

@needlestack thanks that's great!! I would think that now we have the tests working, then it's safe to merge. Judging from the previous fail then the tests do reach part of the function.

codecov/patch is the metric for the code that's affected - I think it means that tests has only reached 4% of the changed lines.

Overall, it stacks up, but it means that you have contributed code where I have to rely on your assertion that it's working and my own ability to read code :)

But it's all good! I will write something for the release notes and possibly add a quick remark to ensure that future devs are able to understand why this code is there.

@benjaoming benjaoming merged commit 884b26d into learningequality:develop Nov 26, 2016

2 of 3 checks passed

codecov/patch 4.00% of diff hit (target 51.18%)
Details
ci/circleci Your tests passed on CircleCI!
Details
codecov/project 51.22% (+0.03%) compared to 5d5928f
Details

benjaoming added a commit to benjaoming/ka-lite that referenced this pull request Nov 26, 2016

@needlestack needlestack deleted the rachelproject:change_language_via_url_redirect_2 branch Nov 26, 2016

@needlestack

This comment has been minimized.

Show comment
Hide comment
@needlestack

needlestack Nov 26, 2016

Contributor

Great - thanks for your help. Hope others find this useful!

Contributor

needlestack commented Nov 26, 2016

Great - thanks for your help. Hope others find this useful!

benjaoming added a commit that referenced this pull request Nov 28, 2016

Merge pull request #5344 from benjaoming/release-note
Comments and release notes about #5342
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment