From 4eb3c4b18edc6511f9bf8c98654490fd0dad2331 Mon Sep 17 00:00:00 2001 From: Chris Dodge Date: Fri, 19 Jun 2015 17:04:30 -0400 Subject: [PATCH 01/12] I18N needs to not include the string substitution --- .../instructor/instructor_dashboard_2/executive_summary.html | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/lms/templates/instructor/instructor_dashboard_2/executive_summary.html b/lms/templates/instructor/instructor_dashboard_2/executive_summary.html index 4312892a00..e118fb2377 100644 --- a/lms/templates/instructor/instructor_dashboard_2/executive_summary.html +++ b/lms/templates/instructor/instructor_dashboard_2/executive_summary.html @@ -28,7 +28,7 @@ h2 { -

${_("Executive Summary for {display_name}".format(display_name=display_name))}

+

${_("Executive Summary for {display_name}").format(display_name=display_name)}

From 42af5b8e35c39674164121b16ad5a6193fe7c1dd Mon Sep 17 00:00:00 2001 From: Chris Dodge Date: Fri, 19 Jun 2015 12:55:04 -0400 Subject: [PATCH 02/12] fix problems with multi-seat purchases when accessibility review was completed --- lms/templates/shoppingcart/shopping_cart.html | 32 +++++++++++-------- 1 file changed, 18 insertions(+), 14 deletions(-) diff --git a/lms/templates/shoppingcart/shopping_cart.html b/lms/templates/shoppingcart/shopping_cart.html index 2cbfac71f2..14ceab73c4 100644 --- a/lms/templates/shoppingcart/shopping_cart.html +++ b/lms/templates/shoppingcart/shopping_cart.html @@ -105,13 +105,13 @@ from django.utils.translation import ungettext
- +
- - @@ -310,15 +310,15 @@ from django.utils.translation import ungettext var wasBusinessType = false; var studentField = $(this).parent().find("input[type='text']"); var unit_cost = parseFloat(studentField.data('unit-cost')); - var ItemId = studentField.attr('id'); + var ItemId = studentField.data('item-id'); var $button = $(this); - var oldValue = $("#"+ItemId).data('qty'); + var oldValue = studentField.data('qty'); var newVal = 1; // initialize with 1. oldValue = parseFloat(oldValue); hideErrorMsg('students-'+ItemId); if ($.isNumeric(oldValue)){ - if ($button.text() == "+") { + if ($button.data("operation") == "inc") { if(oldValue > 0){ newVal = oldValue + 1; if(newVal > 1000){ @@ -338,7 +338,7 @@ from django.utils.translation import ungettext $button.parent().find("input").val(newVal); isBusinessType = getBusinessType(); update_user_cart(ItemId, newVal, oldValue, unit_cost, wasBusinessType, isBusinessType); - $("#"+ItemId).data('qty', newVal); + studentField.data('qty', newVal); } }); @@ -390,7 +390,7 @@ from django.utils.translation import ungettext typeChanged = true; $('html').css({'cursor':'wait'}); $(".button").css({'cursor':'wait'}); - $('.col-2.relative').find("input[type='submit']").attr('disabled', true); + $('.col-2.relative').find("button[type='submit']").attr('disabled', true); } @@ -406,11 +406,14 @@ from django.utils.translation import ungettext $(".button").css({'cursor':'default'}); $("#processor_form").html(data['form_html']); if(typeChanged){ - var submit_button = $('.col-2.relative').find("input[type='submit']") + var submit_button = $('.col-2.relative').find("button[type='submit']"); submit_button.removeAttr('disabled'); for (var i = 0; i< data['oldToNewIdMap'].length; i++) { - $('#'+data['oldToNewIdMap'][i]['oldId']+'').attr('id',data['oldToNewIdMap'][i]['newId']); - $('a.btn-remove[data-item-id]=' +data['oldToNewIdMap'][i]['oldId']+'').data('item-id', data['oldToNewIdMap'][i]['newId']); + var oldId = data['oldToNewIdMap'][i]['oldId']; + var newId = data['oldToNewIdMap'][i]['newId']; + + $('input.spin-counter[data-item-id]=' + oldId ).data('item-id', newId); + $('button.btn-remove[data-item-id]=' + oldId ).data('item-id', newId); } if(isbusinessType){ $( "div[name='payment']").addClass('hidden'); @@ -447,8 +450,9 @@ from django.utils.translation import ungettext function updateTextFieldQty(event){ if(isSpinnerBtnEnabled){ - var itemId = event.currentTarget.id; - var prevQty = $("#"+itemId).data('qty'); + var el_id = event.currentTarget.id; + var itemId = $("#"+el_id).data('item-id'); + var prevQty = $("#"+el_id).data('qty'); var newQty = event.currentTarget.value; var unitCost = event.currentTarget.dataset.unitCost; var isBusinessType = getBusinessType(); @@ -456,7 +460,7 @@ from django.utils.translation import ungettext var wasBusinessType = !isBusinessType; isSpinnerBtnEnabled = false; update_user_cart(itemId, newQty, prevQty, unitCost, wasBusinessType, isBusinessType); - $("#"+itemId).data('qty', newQty); + $("#"+el_id).data('qty', newQty); } } From 548d0b662ae5f08f28b3a13afa9aac495de0ccc2 Mon Sep 17 00:00:00 2001 From: Sarina Canelake Date: Mon, 22 Jun 2015 11:20:08 -0400 Subject: [PATCH 03/12] Remove hinting templates as 'tab:' definition fails on Split --- .../problem/checkboxes_response_hint.yaml | 70 ------------------- .../problem/multiplechoice_hint.yaml | 46 ------------ .../problem/numericalresponse_hint.yaml | 54 -------------- .../problem/optionresponse_hint.yaml | 51 -------------- .../problem/string_response_hint.yaml | 54 -------------- 5 files changed, 275 deletions(-) delete mode 100644 common/lib/xmodule/xmodule/templates/problem/checkboxes_response_hint.yaml delete mode 100644 common/lib/xmodule/xmodule/templates/problem/multiplechoice_hint.yaml delete mode 100644 common/lib/xmodule/xmodule/templates/problem/numericalresponse_hint.yaml delete mode 100644 common/lib/xmodule/xmodule/templates/problem/optionresponse_hint.yaml delete mode 100644 common/lib/xmodule/xmodule/templates/problem/string_response_hint.yaml diff --git a/common/lib/xmodule/xmodule/templates/problem/checkboxes_response_hint.yaml b/common/lib/xmodule/xmodule/templates/problem/checkboxes_response_hint.yaml deleted file mode 100644 index de88f5500e..0000000000 --- a/common/lib/xmodule/xmodule/templates/problem/checkboxes_response_hint.yaml +++ /dev/null @@ -1,70 +0,0 @@ ---- -metadata: - display_name: Checkboxes with Hints and Feedback - tab: hint - markdown: | - - You can provide feedback for each option in a checkbox problem, with distinct feedback depending on whether or not the learner selects that option. - - You can also provide compound feedback for a specific combination of answers. For example, if you have three possible answers in the problem, you can configure specific feedback for when a learner selects each combination of possible answers. - - You can also add hints for learners. - - Be sure to select Settings to specify a Display Name and other values that apply. - - Use the following example problem as a model. - - >>Which of the following is a fruit? Check all that apply.<< - - [x] apple {{ selected: You are correct that an apple is a fruit because it is the fertilized ovary that comes from an apple tree and contains seeds. }, { unselected: Remember that an apple is also a fruit.}} - [x] pumpkin {{ selected: You are correct that a pumpkin is a fruit because it is the fertilized ovary of a squash plant and contains seeds. }, { unselected: Remember that a pumpkin is also a fruit.}} - [ ] potato {{ U: You are correct that a potato is a vegetable because it is an edible part of a plant in tuber form.}, { S: A potato is a vegetable, not a fruit, because it does not come from a flower and does not contain seeds.}} - [x] tomato {{ S: You are correct that a tomato is a fruit because it is the fertilized ovary of a tomato plant and contains seeds. }, { U: Many people mistakenly think a tomato is a vegetable. However, because a tomato is the fertilized ovary of a tomato plant and contains seeds, it is a fruit.}} - - - {{ ((A B D)) An apple, pumpkin, and tomato are all fruits as they all are fertilized ovaries of a plant and contain seeds. }} - {{ ((A B C D)) You are correct that an apple, pumpkin, and tomato are all fruits as they all are fertilized ovaries of a plant and contain seeds. However, a potato is not a fruit as it is an edible part of a plant in tuber form and is a vegetable. }} - - ||A fruit is the fertilized ovary from a flower.|| - ||A fruit contains seeds of the plant.|| - - -data: | - - -

You can provide feedback for each option in a checkbox problem, with distinct feedback depending on whether or not the learner selects that option.

- -

You can also provide compound feedback for a specific combination of answers. For example, if you have three possible answers in the problem, you can configure specific feedback for when a learner selects each combination of possible answers.

- -

You can also add hints for learners.

- -

Use the following example problem as a model.

- -

Which of the following is a fruit? Check all that apply.

- - - apple - You are correct that an apple is a fruit because it is the fertilized ovary that comes from an apple tree and contains seeds. - Remember that an apple is also a fruit. - - pumpkin - You are correct that a pumpkin is a fruit because it is the fertilized ovary of a squash plant and contains seeds. - Remember that a pumpkin is also a fruit. - - potato - A potato is a vegetable, not a fruit, because it does not come from a flower and does not contain seeds. - You are correct that a potato is a vegetable because it is an edible part of a plant in tuber form. - - tomato - You are correct that a tomato is a fruit because it is the fertilized ovary of a tomato plant and contains seeds. - Many people mistakenly think a tomato is a vegetable. However, because a tomato is the fertilized ovary of a tomato plant and contains seeds, it a fruit. - - An apple, pumpkin, and tomato are all fruits as they all are fertilized ovaries of a plant and contain seeds. - You are correct that an apple, pumpkin, and tomato are all fruits as they all are fertilized ovaries of a plant and contain seeds. However, a potato is not a fruit as it is an edible part of a plant in tuber form and is classified as a vegetable. - - - - A fruit is the fertilized ovary from a flower. - A fruit contains seeds of the plant. - - diff --git a/common/lib/xmodule/xmodule/templates/problem/multiplechoice_hint.yaml b/common/lib/xmodule/xmodule/templates/problem/multiplechoice_hint.yaml deleted file mode 100644 index 99998e751a..0000000000 --- a/common/lib/xmodule/xmodule/templates/problem/multiplechoice_hint.yaml +++ /dev/null @@ -1,46 +0,0 @@ ---- -metadata: - display_name: Multiple Choice with Hints and Feedback - tab: hint - markdown: | - - You can provide feedback for each option in a multiple choice problem. - - You can also add hints for learners. - - Be sure to select Settings to specify a Display Name and other values that apply. - - Use the following example problem as a model. - - >>Which of the following is a vegetable?<< - ( ) apple {{An apple is the fertilized ovary that comes from an apple tree and contains seeds, meaning it is a fruit.}} - ( ) pumpkin {{A pumpkin is the fertilized ovary of a squash plant and contains seeds, meaning it is a fruit.}} - (x) potato {{A potato is an edible part of a plant in tuber form and is a vegetable.}} - ( ) tomato {{Many people mistakenly think a tomato is a vegetable. However, because a tomato is the fertilized ovary of a tomato plant and contains seeds, it is a fruit.}} - - ||A fruit is the fertilized ovary from a flower.|| - ||A fruit contains seeds of the plant.|| - -data: | - - -

You can provide feedback for each option in a multiple choice problem.

- -

You can also add hints for learners.

- -

Use the following example problem as a model.

- -

Which of the following is a vegetable?

- - - apple An apple is the fertilized ovary that comes from an apple tree and contains seeds, meaning it is a fruit. - pumpkin A pumpkin is the fertilized ovary of a squash plant and contains seeds, meaning it is a fruit. - potato A potato is an edible part of a plant in tuber form and is a vegetable. - tomato Many people mistakenly think a tomato is a vegetable. However, because a tomato is the fertilized ovary of a tomato plant and contains seeds, it is a fruit. - - - - A fruit is the fertilized ovary from a flower. - A fruit contains seeds of the plant. - - diff --git a/common/lib/xmodule/xmodule/templates/problem/numericalresponse_hint.yaml b/common/lib/xmodule/xmodule/templates/problem/numericalresponse_hint.yaml deleted file mode 100644 index d315ea291d..0000000000 --- a/common/lib/xmodule/xmodule/templates/problem/numericalresponse_hint.yaml +++ /dev/null @@ -1,54 +0,0 @@ ---- -metadata: - display_name: Numerical Input with Hints and Feedback - tab: hint - markdown: | - - You can provide feedback for correct answers in numerical input problems. You cannot provide feedback for incorrect answers. - - Use feedback for the correct answer to reinforce the process for arriving at the numerical value. - - You can also add hints for learners. - - Be sure to select Settings to specify a Display Name and other values that apply. - - Use the following example problem as a model. - - >>What is the arithmetic mean for the following set of numbers? (1, 5, 6, 3, 5)<< - - = 4 {{The mean for this set of numbers is 20 / 5, which equals 4.}} - - ||The mean is calculated by summing the set of numbers and dividing by n.|| - ||n is the count of items in the set.|| - - [explanation] - The mean is calculated by summing the set of numbers and dividing by n. In this case: (1 + 5 + 6 + 3 + 5) / 5 = 20 / 5 = 4. - [explanation] - -data: | - - -

You can provide feedback for correct answers in numerical input problems. You cannot provide feedback for incorrect answers.

- -

Use feedback for the correct answer to reinforce the process for arriving at the numerical value.

- -

Use the following example problem as a model.

- -

What is the arithmetic mean for the following set of numbers? (1, 5, 6, 3, 5)

- - - The mean for this set of numbers is 20 / 5, which equals 4. - - - -
-

Explanation

-

The mean is calculated by summing the set of numbers and dividing by n. In this case: (1 + 5 + 6 + 3 + 5) / 5 = 20 / 5 = 4.

-
- - - - The mean is calculated by summing the set of numbers and dividing by n. - n is the count of items in the set. - - \ No newline at end of file diff --git a/common/lib/xmodule/xmodule/templates/problem/optionresponse_hint.yaml b/common/lib/xmodule/xmodule/templates/problem/optionresponse_hint.yaml deleted file mode 100644 index c78933aa7a..0000000000 --- a/common/lib/xmodule/xmodule/templates/problem/optionresponse_hint.yaml +++ /dev/null @@ -1,51 +0,0 @@ ---- -metadata: - display_name: Dropdown with Hints and Feedback - tab: hint - markdown: | - - You can provide feedback for each available option in a dropdown problem. - - You can also add hints for learners. - - Be sure to select Settings to specify a Display Name and other values that apply. - - Use the following example problem as a model. - - >> A/an ________ is a vegetable.<< - - [[ - apple {{An apple is the fertilized ovary that comes from an apple tree and contains seeds, meaning it is a fruit.}} - pumpkin {{A pumpkin is the fertilized ovary of a squash plant and contains seeds, meaning it is a fruit.}} - (potato) {{A potato is an edible part of a plant in tuber form and is a vegetable.}} - tomato {{Many people mistakenly think a tomato is a vegetable. However, because a tomato is the fertilized ovary of a tomato plant and contains seeds, it is a fruit.}} - ]] - - ||A fruit is the fertilized ovary from a flower.|| - ||A fruit contains seeds of the plant.|| - -data: | - - -

You can provide feedback for each available option in a dropdown problem.

- -

You can also add hints for learners.

- -

Use the following example problem as a model.

- -

A/an ________ is a vegetable.

-
- - - - - - - - - - - A fruit is the fertilized ovary from a flower. - A fruit contains seeds of the plant. - - diff --git a/common/lib/xmodule/xmodule/templates/problem/string_response_hint.yaml b/common/lib/xmodule/xmodule/templates/problem/string_response_hint.yaml deleted file mode 100644 index e52bf9f8f3..0000000000 --- a/common/lib/xmodule/xmodule/templates/problem/string_response_hint.yaml +++ /dev/null @@ -1,54 +0,0 @@ ---- -metadata: - display_name: Text Input with Hints and Feedback - tab: hint - markdown: | - - You can provide feedback for the correct answer in text input problems, as well as for specific incorrect answers. - - Use feedback on expected incorrect answers to address common misconceptions and to provide guidance on how to arrive at the correct answer. - - Be sure to select Settings to specify a Display Name and other values that apply. - - Use the following example problem as a model. - - >>Which U.S. state has the largest land area?<< - - =Alaska {{Alaska is 576,400 square miles, more than double the land area - of the second largest state, Texas.}} - - not=Texas {{While many people think Texas is the largest state, it is actually the second largest, with 261,797 square miles.}} - - not=California {{California is the third largest state, with 155,959 square miles.}} - - ||Consider the square miles, not population.|| - ||Consider all 50 states, not just the continental United States.|| - -data: | - - -

You can provide feedback for the correct answer in text input problems, as well as for specific incorrect answers.

- -

Use feedback on expected incorrect answers to address common misconceptions and to provide guidance on how to arrive at the correct answer.

- -

Use the following example problem as a model.

- -

Which U.S. state has the largest land area?

- - - - Alaska is 576,400 square miles, more than double the land area of the second largest state, Texas. - - While many people think Texas is the largest state, it is actually the second largest, with 261,797 square miles. - - California is the third largest state, with 155,959 square miles. - - - - - - Consider the square miles, not population. - Consider all 50 states, not just the continental United States. - - - From 4c79a59b632fa899c17b974dacd13241ebd93fb7 Mon Sep 17 00:00:00 2001 From: Andy Armstrong Date: Mon, 22 Jun 2015 11:30:36 -0400 Subject: [PATCH 04/12] Use RequireJS Optimizer for content libraries --- cms/static/build.js | 1 + 1 file changed, 1 insertion(+) diff --git a/cms/static/build.js b/cms/static/build.js index bb890bee5c..b01b4a5720 100644 --- a/cms/static/build.js +++ b/cms/static/build.js @@ -39,6 +39,7 @@ 'js/certificates/factories/certificates_page_factory', 'js/factories/import', 'js/factories/index', + 'js/factories/library', 'js/factories/login', 'js/factories/manage_users', 'js/factories/outline', From c036aaf4d20c80dcd3e61402274ffb2d81530e8f Mon Sep 17 00:00:00 2001 From: Sarina Canelake Date: Mon, 22 Jun 2015 11:31:17 -0400 Subject: [PATCH 05/12] Remove Hints tab from Studio --- cms/templates/js/add-xblock-component-menu-problem.underscore | 3 --- 1 file changed, 3 deletions(-) diff --git a/cms/templates/js/add-xblock-component-menu-problem.underscore b/cms/templates/js/add-xblock-component-menu-problem.underscore index 301064935c..d3324d1b62 100644 --- a/cms/templates/js/add-xblock-component-menu-problem.underscore +++ b/cms/templates/js/add-xblock-component-menu-problem.underscore @@ -3,9 +3,6 @@
  • <%= gettext("Common Problem Types") %>
    • -
  • - <%= gettext("Common Problems with Hints and Feedback") %> -
  • <%= gettext("Advanced") %>
    • From 1b0246e2af561a75e7803f8ad258da5d95f26633 Mon Sep 17 00:00:00 2001 From: Awais Date: Mon, 22 Jun 2015 14:55:12 +0500 Subject: [PATCH 06/12] ECOM-1772 Due date issue test case fixed. --- lms/djangoapps/verify_student/tests/test_views.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/lms/djangoapps/verify_student/tests/test_views.py b/lms/djangoapps/verify_student/tests/test_views.py index 0027c312b7..833b8e4b95 100644 --- a/lms/djangoapps/verify_student/tests/test_views.py +++ b/lms/djangoapps/verify_student/tests/test_views.py @@ -1849,7 +1849,7 @@ class TestEmailMessageWithCustomICRVBlock(ModuleStoreTestCase): """ self.course_key = SlashSeparatedCourseKey("Robot", "999", "Test_Course") self.course = CourseFactory.create(org='Robot', number='999', display_name='Test Course') - self.due_date = datetime(2015, 6, 22, tzinfo=pytz.UTC) + self.due_date = datetime.now(pytz.UTC) + timedelta(days=20) self.allowed_attempts = 1 # Create the course modes From 1e21945e2223d17ec94c91c570ae7ffe811c62d1 Mon Sep 17 00:00:00 2001 From: Sarina Canelake Date: Mon, 22 Jun 2015 13:55:53 -0400 Subject: [PATCH 07/12] Revert "Merge pull request #8402 from edx/sarina/annotate-middleware" This reverts commit 09425c3d8f1eecf3400b76795441d75bc1b813bb, reversing changes made to 410b9282af9141a2f25eed918700eec53dfbb81a. --- cms/envs/common.py | 4 +- common/djangoapps/dark_lang/middleware.py | 35 ++-- common/djangoapps/dark_lang/models.py | 2 +- common/djangoapps/dark_lang/tests.py | 92 +--------- common/djangoapps/django_locale/__init__.py | 7 - common/djangoapps/django_locale/middleware.py | 83 --------- common/djangoapps/django_locale/tests.py | 157 ------------------ common/djangoapps/django_locale/trans_real.py | 131 --------------- lms/djangoapps/courseware/tests/test_i18n.py | 67 ++------ lms/envs/common.py | 4 +- 10 files changed, 31 insertions(+), 551 deletions(-) delete mode 100644 common/djangoapps/django_locale/__init__.py delete mode 100644 common/djangoapps/django_locale/middleware.py delete mode 100644 common/djangoapps/django_locale/tests.py delete mode 100644 common/djangoapps/django_locale/trans_real.py diff --git a/cms/envs/common.py b/cms/envs/common.py index 46a2dcc184..eb8ec6e78c 100644 --- a/cms/envs/common.py +++ b/cms/envs/common.py @@ -306,9 +306,7 @@ MIDDLEWARE_CLASSES = ( 'embargo.middleware.EmbargoMiddleware', # Detects user-requested locale from 'accept-language' header in http request - # TODO: Re-import the Django version once we upgrade to Django 1.8 [PLAT-671] - # 'django.middleware.locale.LocaleMiddleware', - 'django_locale.middleware.LocaleMiddleware', + 'django.middleware.locale.LocaleMiddleware', 'django.middleware.transaction.TransactionMiddleware', # needs to run after locale middleware (or anything that modifies the request context) diff --git a/common/djangoapps/dark_lang/middleware.py b/common/djangoapps/dark_lang/middleware.py index 7a7a6b8868..b18d064969 100644 --- a/common/djangoapps/dark_lang/middleware.py +++ b/common/djangoapps/dark_lang/middleware.py @@ -12,11 +12,9 @@ the SessionMiddleware. """ from django.conf import settings -from dark_lang.models import DarkLangConfig +from django.utils.translation.trans_real import parse_accept_lang_header -# TODO re-import this once we're on Django 1.5 or greater. [PLAT-671] -# from django.utils.translation.trans_real import parse_accept_lang_header -from django_locale.trans_real import parse_accept_lang_header +from dark_lang.models import DarkLangConfig def dark_parse_accept_lang_header(accept): @@ -83,17 +81,11 @@ class DarkLangMiddleware(object): self._clean_accept_headers(request) self._activate_preview_language(request) - def _fuzzy_match(self, lang_code): - """Returns a fuzzy match for lang_code""" - if lang_code in self.released_langs: - return lang_code - - lang_prefix = lang_code.partition('-')[0] - for released_lang in self.released_langs: - released_prefix = released_lang.partition('-')[0] - if lang_prefix == released_prefix: - return released_lang - return None + def _is_released(self, lang_code): + """ + ``True`` iff one of the values in ``self.released_langs`` is a prefix of ``lang_code``. + """ + return any(lang_code.lower().startswith(released_lang.lower()) for released_lang in self.released_langs) def _format_accept_value(self, lang, priority=1.0): """ @@ -110,13 +102,12 @@ class DarkLangMiddleware(object): if accept is None or accept == '*': return - new_accept = [] - for lang, priority in dark_parse_accept_lang_header(accept): - fuzzy_code = self._fuzzy_match(lang.lower()) - if fuzzy_code: - new_accept.append(self._format_accept_value(fuzzy_code, priority)) - - new_accept = ", ".join(new_accept) + new_accept = ", ".join( + self._format_accept_value(lang, priority) + for lang, priority + in dark_parse_accept_lang_header(accept) + if self._is_released(lang) + ) request.META['HTTP_ACCEPT_LANGUAGE'] = new_accept diff --git a/common/djangoapps/dark_lang/models.py b/common/djangoapps/dark_lang/models.py index e61ea41fb2..1daec994e8 100644 --- a/common/djangoapps/dark_lang/models.py +++ b/common/djangoapps/dark_lang/models.py @@ -25,7 +25,7 @@ class DarkLangConfig(ConfigurationModel): if not self.released_languages.strip(): # pylint: disable=no-member return [] - languages = [lang.lower().strip() for lang in self.released_languages.split(',')] # pylint: disable=no-member + languages = [lang.strip() for lang in self.released_languages.split(',')] # pylint: disable=no-member # Put in alphabetical order languages.sort() return languages diff --git a/common/djangoapps/dark_lang/tests.py b/common/djangoapps/dark_lang/tests.py index b7210088e9..6dd0b41882 100644 --- a/common/djangoapps/dark_lang/tests.py +++ b/common/djangoapps/dark_lang/tests.py @@ -4,10 +4,8 @@ Tests of DarkLangMiddleware from django.contrib.auth.models import User from django.http import HttpRequest -import ddt from django.test import TestCase from mock import Mock -import unittest from dark_lang.middleware import DarkLangMiddleware from dark_lang.models import DarkLangConfig @@ -25,7 +23,6 @@ def set_if_set(dct, key, value): dct[key] = value -@ddt.ddt class DarkLangMiddlewareTests(TestCase): """ Tests of DarkLangMiddleware @@ -85,10 +82,6 @@ class DarkLangMiddlewareTests(TestCase): def test_wildcard_accept(self): self.assertAcceptEquals('*', self.process_request(accept='*')) - def test_malformed_accept(self): - self.assertAcceptEquals('', self.process_request(accept='xxxxxxxxxxxx')) - self.assertAcceptEquals('', self.process_request(accept='en;q=1.0, es-419:q-0.8')) - def test_released_accept(self): self.assertAcceptEquals( 'rel;q=1.0', @@ -130,17 +123,14 @@ class DarkLangMiddlewareTests(TestCase): ) def test_accept_released_territory(self): - # We will munge 'rel-ter' to be 'rel', so the 'rel-ter' - # user will actually receive the released language 'rel' - # (Otherwise, the user will actually end up getting the server default) self.assertAcceptEquals( - 'rel;q=1.0, rel;q=0.5', + 'rel-ter;q=1.0, rel;q=0.5', self.process_request(accept='rel-ter;q=1.0, rel;q=0.5') ) def test_accept_mixed_case(self): self.assertAcceptEquals( - 'rel;q=1.0, rel;q=0.5', + 'rel-TER;q=1.0, REL;q=0.5', self.process_request(accept='rel-TER;q=1.0, REL;q=0.5') ) @@ -150,85 +140,11 @@ class DarkLangMiddlewareTests(TestCase): enabled=True ).save() - # Since we have only released "rel-ter", the requested code "rel" will - # fuzzy match to "rel-ter", in addition to "rel-ter" exact matching "rel-ter" self.assertAcceptEquals( - 'rel-ter;q=1.0, rel-ter;q=0.5', + 'rel-ter;q=1.0', self.process_request(accept='rel-ter;q=1.0, rel;q=0.5') ) - @ddt.data( - ('es;q=1.0, pt;q=0.5', 'es-419;q=1.0'), # 'es' should get 'es-419', not English - ('es-AR;q=1.0, pt;q=0.5', 'es-419;q=1.0'), # 'es-AR' should get 'es-419', not English - ) - @ddt.unpack - def test_partial_match_es419(self, accept_header, expected): - # Release es-419 - DarkLangConfig( - released_languages=('es-419, en'), - changed_by=self.user, - enabled=True - ).save() - - self.assertAcceptEquals( - expected, - self.process_request(accept=accept_header) - ) - - def test_partial_match_esar_es(self): - # If I release 'es', 'es-AR' should get 'es', not English - DarkLangConfig( - released_languages=('es, en'), - changed_by=self.user, - enabled=True - ).save() - - self.assertAcceptEquals( - 'es;q=1.0', - self.process_request(accept='es-AR;q=1.0, pt;q=0.5') - ) - - @ddt.data( - # Test condition: If I release 'es-419, es, es-es'... - ('es;q=1.0, pt;q=0.5', 'es;q=1.0'), # 1. es should get es - ('es-419;q=1.0, pt;q=0.5', 'es-419;q=1.0'), # 2. es-419 should get es-419 - ('es-es;q=1.0, pt;q=0.5', 'es-es;q=1.0'), # 3. es-es should get es-es - ) - @ddt.unpack - def test_exact_match_gets_priority(self, accept_header, expected): - # Release 'es-419, es, es-es' - DarkLangConfig( - released_languages=('es-419, es, es-es'), - changed_by=self.user, - enabled=True - ).save() - self.assertAcceptEquals( - expected, - self.process_request(accept=accept_header) - ) - - @unittest.skip("This won't work until fallback is implemented for LA country codes. See LOC-86") - @ddt.data( - 'es-AR', # Argentina - 'es-PY', # Paraguay - ) - def test_partial_match_es_la(self, latin_america_code): - # We need to figure out the best way to implement this. There are a ton of LA country - # codes that ought to fall back to 'es-419' rather than 'es-es'. - # http://unstats.un.org/unsd/methods/m49/m49regin.htm#americas - # If I release 'es, es-419' - # Latin American codes should get es-419 - DarkLangConfig( - released_languages=('es, es-419'), - changed_by=self.user, - enabled=True - ).save() - - self.assertAcceptEquals( - 'es-419;q=1.0', - self.process_request(accept='{};q=1.0, pt;q=0.5'.format(latin_america_code)) - ) - def assertSessionLangEquals(self, value, request): """ Assert that the 'django_language' set in request.session is equal to value @@ -308,6 +224,6 @@ class DarkLangMiddlewareTests(TestCase): ).save() self.assertAcceptEquals( - 'zh-cn;q=1.0, zh-tw;q=0.5, zh-hk;q=0.3', + 'zh-CN;q=1.0, zh-TW;q=0.5, zh-HK;q=0.3', self.process_request(accept='zh-Hans;q=1.0, zh-Hant-TW;q=0.5, zh-HK;q=0.3') ) diff --git a/common/djangoapps/django_locale/__init__.py b/common/djangoapps/django_locale/__init__.py deleted file mode 100644 index 655019022e..0000000000 --- a/common/djangoapps/django_locale/__init__.py +++ /dev/null @@ -1,7 +0,0 @@ -""" -TODO: This module is imported from the stable Django 1.8 branch, as a -copy of https://github.com/django/django/blob/stable/1.8.x/django/middleware/locale.py. - -Remove this file and re-import this middleware from Django once the -codebase is upgraded with a modern version of Django. [PLAT-671] -""" diff --git a/common/djangoapps/django_locale/middleware.py b/common/djangoapps/django_locale/middleware.py deleted file mode 100644 index b0601a807e..0000000000 --- a/common/djangoapps/django_locale/middleware.py +++ /dev/null @@ -1,83 +0,0 @@ -# TODO: This file is imported from the stable Django 1.8 branch. Remove this file -# and re-import this middleware from Django once the codebase is upgraded. [PLAT-671] -# pylint: disable=invalid-name, missing-docstring -"This is the locale selecting middleware that will look at accept headers" - -from django.conf import settings -from django.core.urlresolvers import ( - LocaleRegexURLResolver, get_resolver, get_script_prefix, is_valid_path, -) -from django.http import HttpResponseRedirect -from django.utils import translation -from django.utils.cache import patch_vary_headers -# Override the Django 1.4 implementation with the 1.8 implementation -from django_locale.trans_real import get_language_from_request - - -class LocaleMiddleware(object): - """ - This is a very simple middleware that parses a request - and decides what translation object to install in the current - thread context. This allows pages to be dynamically - translated to the language the user desires (if the language - is available, of course). - """ - response_redirect_class = HttpResponseRedirect - - def __init__(self): - self._is_language_prefix_patterns_used = False - for url_pattern in get_resolver(None).url_patterns: - if isinstance(url_pattern, LocaleRegexURLResolver): - self._is_language_prefix_patterns_used = True - break - - def process_request(self, request): - check_path = self.is_language_prefix_patterns_used() - # This call is broken in Django 1.4: - # https://github.com/django/django/blob/stable/1.4.x/django/utils/translation/trans_real.py#L399 - # (we override parse_accept_lang_header to a fixed version in dark_lang.middleware) - language = get_language_from_request( - request, check_path=check_path) - translation.activate(language) - request.LANGUAGE_CODE = translation.get_language() - - def process_response(self, request, response): - language = translation.get_language() - language_from_path = translation.get_language_from_path(request.path_info) - if (response.status_code == 404 and not language_from_path - and self.is_language_prefix_patterns_used()): - urlconf = getattr(request, 'urlconf', None) - language_path = '/%s%s' % (language, request.path_info) - path_valid = is_valid_path(language_path, urlconf) - if (not path_valid and settings.APPEND_SLASH - and not language_path.endswith('/')): - path_valid = is_valid_path("%s/" % language_path, urlconf) - - if path_valid: - script_prefix = get_script_prefix() - language_url = "%s://%s%s" % ( - request.scheme, - request.get_host(), - # insert language after the script prefix and before the - # rest of the URL - request.get_full_path().replace( - script_prefix, - '%s%s/' % (script_prefix, language), - 1 - ) - ) - return self.response_redirect_class(language_url) - - if not (self.is_language_prefix_patterns_used() - and language_from_path): - patch_vary_headers(response, ('Accept-Language',)) - if 'Content-Language' not in response: - response['Content-Language'] = language - return response - - def is_language_prefix_patterns_used(self): - """ - Returns `True` if the `LocaleRegexURLResolver` is used - at root level of the urlpatterns, else it returns `False`. - """ - return self._is_language_prefix_patterns_used diff --git a/common/djangoapps/django_locale/tests.py b/common/djangoapps/django_locale/tests.py deleted file mode 100644 index cc40ce9d4a..0000000000 --- a/common/djangoapps/django_locale/tests.py +++ /dev/null @@ -1,157 +0,0 @@ -# pylint: disable=invalid-name, line-too-long, super-method-not-called -""" -Tests taken from Django upstream: -https://github.com/django/django/blob/e6b34193c5c7d117ededdab04bb16caf8864f07c/tests/regressiontests/i18n/tests.py -""" -from django.conf import settings -from django.test import TestCase, RequestFactory -from django_locale.trans_real import ( - parse_accept_lang_header, get_language_from_request, LANGUAGE_SESSION_KEY -) - -# Added to test middleware around dark lang -from django.contrib.auth.models import User -from django.test.utils import override_settings -from dark_lang.models import DarkLangConfig - - -# Adding to support test differences between Django and our own settings -@override_settings(LANGUAGES=[ - ('pt', 'Portuguese'), - ('pt-br', 'Portuguese-Brasil'), - ('es', 'Spanish'), - ('es-ar', 'Spanish (Argentina)'), - ('de', 'Deutch'), - ('zh-cn', 'Chinese (China)'), - ('ar-sa', 'Arabic (Saudi Arabia)'), -]) -class MiscTests(TestCase): - """ - Tests taken from Django upstream: - https://github.com/django/django/blob/e6b34193c5c7d117ededdab04bb16caf8864f07c/tests/regressiontests/i18n/tests.py - """ - def setUp(self): - self.rf = RequestFactory() - # Added to test middleware around dark lang - user = User() - user.save() - DarkLangConfig( - released_languages='pt, pt-br, es, de, es-ar, zh-cn, ar-sa', - changed_by=user, - enabled=True - ).save() - - def test_parse_spec_http_header(self): - """ - Testing HTTP header parsing. First, we test that we can parse the - values according to the spec (and that we extract all the pieces in - the right order). - """ - p = parse_accept_lang_header - # Good headers. - self.assertEqual([('de', 1.0)], p('de')) - self.assertEqual([('en-AU', 1.0)], p('en-AU')) - self.assertEqual([('es-419', 1.0)], p('es-419')) - self.assertEqual([('*', 1.0)], p('*;q=1.00')) - self.assertEqual([('en-AU', 0.123)], p('en-AU;q=0.123')) - self.assertEqual([('en-au', 0.5)], p('en-au;q=0.5')) - self.assertEqual([('en-au', 1.0)], p('en-au;q=1.0')) - self.assertEqual([('da', 1.0), ('en', 0.5), ('en-gb', 0.25)], p('da, en-gb;q=0.25, en;q=0.5')) - self.assertEqual([('en-au-xx', 1.0)], p('en-au-xx')) - self.assertEqual([('de', 1.0), ('en-au', 0.75), ('en-us', 0.5), ('en', 0.25), ('es', 0.125), ('fa', 0.125)], p('de,en-au;q=0.75,en-us;q=0.5,en;q=0.25,es;q=0.125,fa;q=0.125')) - self.assertEqual([('*', 1.0)], p('*')) - self.assertEqual([('de', 1.0)], p('de;q=0.')) - self.assertEqual([('en', 1.0), ('*', 0.5)], p('en; q=1.0, * ; q=0.5')) - self.assertEqual([], p('')) - - # Bad headers; should always return []. - self.assertEqual([], p('en-gb;q=1.0000')) - self.assertEqual([], p('en;q=0.1234')) - self.assertEqual([], p('en;q=.2')) - self.assertEqual([], p('abcdefghi-au')) - self.assertEqual([], p('**')) - self.assertEqual([], p('en,,gb')) - self.assertEqual([], p('en-au;q=0.1.0')) - self.assertEqual([], p('XXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXZ,en')) - self.assertEqual([], p('da, en-gb;q=0.8, en;q=0.7,#')) - self.assertEqual([], p('de;q=2.0')) - self.assertEqual([], p('de;q=0.a')) - self.assertEqual([], p('12-345')) - self.assertEqual([], p('')) - - def test_parse_literal_http_header(self): - """ - Now test that we parse a literal HTTP header correctly. - """ - g = get_language_from_request - r = self.rf.get('/') - r.COOKIES = {} - r.META = {'HTTP_ACCEPT_LANGUAGE': 'pt-br'} - self.assertEqual('pt-br', g(r)) - - r.META = {'HTTP_ACCEPT_LANGUAGE': 'pt'} - self.assertEqual('pt', g(r)) - - r.META = {'HTTP_ACCEPT_LANGUAGE': 'es,de'} - self.assertEqual('es', g(r)) - - r.META = {'HTTP_ACCEPT_LANGUAGE': 'es-ar,de'} - self.assertEqual('es-ar', g(r)) - - # This test assumes there won't be a Django translation to a US - # variation of the Spanish language, a safe assumption. When the - # user sets it as the preferred language, the main 'es' - # translation should be selected instead. - r.META = {'HTTP_ACCEPT_LANGUAGE': 'es-us'} - self.assertEqual(g(r), 'es') - - # This tests the following scenario: there isn't a main language (zh) - # translation of Django but there is a translation to variation (zh_CN) - # the user sets zh-cn as the preferred language, it should be selected - # by Django without falling back nor ignoring it. - r.META = {'HTTP_ACCEPT_LANGUAGE': 'zh-cn,de'} - self.assertEqual(g(r), 'zh-cn') - - def test_logic_masked_by_darklang(self): - g = get_language_from_request - r = self.rf.get('/') - r.COOKIES = {} - r.META = {'HTTP_ACCEPT_LANGUAGE': 'ar-qa'} - self.assertEqual('ar-sa', g(r)) - - r.session = {LANGUAGE_SESSION_KEY: 'es'} - self.assertEqual('es', g(r)) - - def test_parse_language_cookie(self): - """ - Now test that we parse language preferences stored in a cookie correctly. - """ - g = get_language_from_request - r = self.rf.get('/') - r.COOKIES = {settings.LANGUAGE_COOKIE_NAME: 'pt-br'} - r.META = {} - self.assertEqual('pt-br', g(r)) - - r.COOKIES = {settings.LANGUAGE_COOKIE_NAME: 'pt'} - r.META = {} - self.assertEqual('pt', g(r)) - - r.COOKIES = {settings.LANGUAGE_COOKIE_NAME: 'es'} - r.META = {'HTTP_ACCEPT_LANGUAGE': 'de'} - self.assertEqual('es', g(r)) - - # This test assumes there won't be a Django translation to a US - # variation of the Spanish language, a safe assumption. When the - # user sets it as the preferred language, the main 'es' - # translation should be selected instead. - r.COOKIES = {settings.LANGUAGE_COOKIE_NAME: 'es-us'} - r.META = {} - self.assertEqual(g(r), 'es') - - # This tests the following scenario: there isn't a main language (zh) - # translation of Django but there is a translation to variation (zh_CN) - # the user sets zh-cn as the preferred language, it should be selected - # by Django without falling back nor ignoring it. - r.COOKIES = {settings.LANGUAGE_COOKIE_NAME: 'zh-cn'} - r.META = {'HTTP_ACCEPT_LANGUAGE': 'de'} - self.assertEqual(g(r), 'zh-cn') diff --git a/common/djangoapps/django_locale/trans_real.py b/common/djangoapps/django_locale/trans_real.py deleted file mode 100644 index 3ec6b6d026..0000000000 --- a/common/djangoapps/django_locale/trans_real.py +++ /dev/null @@ -1,131 +0,0 @@ -"""Translation helper functions.""" -# Imported from Django 1.8 -# pylint: disable=invalid-name -import re -from django.conf import settings -from django.conf.locale import LANG_INFO -from django.utils import translation - - -# Format of Accept-Language header values. From RFC 2616, section 14.4 and 3.9. -# and RFC 3066, section 2.1 -accept_language_re = re.compile(r''' - ([A-Za-z]{1,8}(?:-[A-Za-z0-9]{1,8})*|\*) # "en", "en-au", "x-y-z", "*" - (?:\s*;\s*q=(0(?:\.\d{,3})?|1(?:.0{,3})?))? # Optional "q=1.00", "q=0.8" - (?:\s*,\s*|$) # Multiple accepts per header. - ''', re.VERBOSE) - - -language_code_re = re.compile(r'^[a-z]{1,8}(?:-[a-z0-9]{1,8})*$', re.IGNORECASE) - - -LANGUAGE_SESSION_KEY = '_language' - - -def parse_accept_lang_header(lang_string): - """ - Parses the lang_string, which is the body of an HTTP Accept-Language - header, and returns a list of (lang, q-value), ordered by 'q' values. - - Any format errors in lang_string results in an empty list being returned. - """ - # parse_accept_lang_header is broken until we are on Django 1.5 or greater - # See https://code.djangoproject.com/ticket/19381 - result = [] - pieces = accept_language_re.split(lang_string) - if pieces[-1]: - return [] - for i in range(0, len(pieces) - 1, 3): - first, lang, priority = pieces[i: i + 3] - if first: - return [] - priority = priority and float(priority) or 1.0 - result.append((lang, priority)) - result.sort(key=lambda k: k[1], reverse=True) - return result - - -def get_supported_language_variant(lang_code, strict=False): - """ - Returns the language-code that's listed in supported languages, possibly - selecting a more generic variant. Raises LookupError if nothing found. - If `strict` is False (the default), the function will look for an alternative - country-specific variant when the currently checked is not found. - lru_cache should have a maxsize to prevent from memory exhaustion attacks, - as the provided language codes are taken from the HTTP request. See also - . - """ - if lang_code: - # If 'fr-ca' is not supported, try special fallback or language-only 'fr'. - possible_lang_codes = [lang_code] - try: - # TODO skip this, or import updated LANG_INFO format from __future__ - # (fallback option wasn't added until - # https://github.com/django/django/commit/5dcdbe95c749d36072f527e120a8cb463199ae0d) - possible_lang_codes.extend(LANG_INFO[lang_code]['fallback']) - except KeyError: - pass - generic_lang_code = lang_code.split('-')[0] - possible_lang_codes.append(generic_lang_code) - supported_lang_codes = dict(settings.LANGUAGES) - - for code in possible_lang_codes: - # Note: django 1.4 implementation of check_for_language is OK to use - if code in supported_lang_codes and translation.check_for_language(code): - return code - if not strict: - # if fr-fr is not supported, try fr-ca. - for supported_code in supported_lang_codes: - if supported_code.startswith(generic_lang_code + '-'): - return supported_code - raise LookupError(lang_code) - - -def get_language_from_request(request, check_path=False): - """ - Analyzes the request to find what language the user wants the system to - show. Only languages listed in settings.LANGUAGES are taken into account. - If the user requests a sublanguage where we have a main language, we send - out the main language. - If check_path is True, the URL path prefix will be checked for a language - code, otherwise this is skipped for backwards compatibility. - """ - if check_path: - # Note: django 1.4 implementation of get_language_from_path is OK to use - lang_code = translation.get_language_from_path(request.path_info) - if lang_code is not None: - return lang_code - - supported_lang_codes = dict(settings.LANGUAGES) - - if hasattr(request, 'session'): - lang_code = request.session.get(LANGUAGE_SESSION_KEY) - # Note: django 1.4 implementation of check_for_language is OK to use - if lang_code in supported_lang_codes and lang_code is not None and translation.check_for_language(lang_code): - return lang_code - - lang_code = request.COOKIES.get(settings.LANGUAGE_COOKIE_NAME) - - try: - return get_supported_language_variant(lang_code) - except LookupError: - pass - - accept = request.META.get('HTTP_ACCEPT_LANGUAGE', '') - # broken in 1.4, so defined above - for accept_lang, unused in parse_accept_lang_header(accept): - if accept_lang == '*': - break - - if not language_code_re.search(accept_lang): - continue - - try: - return get_supported_language_variant(accept_lang) - except LookupError: - continue - - try: - return get_supported_language_variant(settings.LANGUAGE_CODE) - except LookupError: - return settings.LANGUAGE_CODE diff --git a/lms/djangoapps/courseware/tests/test_i18n.py b/lms/djangoapps/courseware/tests/test_i18n.py index 4e4c147865..a67442e64b 100644 --- a/lms/djangoapps/courseware/tests/test_i18n.py +++ b/lms/djangoapps/courseware/tests/test_i18n.py @@ -4,59 +4,37 @@ Tests i18n in courseware import re from nose.plugins.attrib import attr -from django.contrib.auth.models import User from django.test import TestCase - -from dark_lang.models import DarkLangConfig - - -class BaseI18nTestCase(TestCase): - """ - Base utilities for i18n test classes to derive from - """ - def assert_tag_has_attr(self, content, tag, attname, value): - """Assert that a tag in `content` has a certain value in a certain attribute.""" - regex = r"""<{tag} [^>]*\b{attname}=['"]([\w\d\- ]+)['"][^>]*>""".format(tag=tag, attname=attname) - match = re.search(regex, content) - self.assertTrue(match, "Couldn't find desired tag '%s' with attr '%s' in %r" % (tag, attname, content)) - attvalues = match.group(1).split() - self.assertIn(value, attvalues) - - def release_languages(self, languages): - """ - Release a set of languages using the dark lang interface. - languages is a list of comma-separated lang codes, eg, 'ar, es-419' - """ - user = User() - user.save() - DarkLangConfig( - released_languages=languages, - changed_by=user, - enabled=True - ).save() +from django.test.utils import override_settings @attr('shard_1') -class I18nTestCase(BaseI18nTestCase): +@override_settings(LANGUAGES=[('eo', 'Esperanto'), ('ar', 'Arabic')]) +class I18nTestCase(TestCase): """ Tests for i18n """ + def assert_tag_has_attr(self, content, tag, attname, value): + """Assert that a tag in `content` has a certain value in a certain attribute.""" + regex = r"""<{tag} [^>]*\b{attname}=['"]([\w\d ]+)['"][^>]*>""".format(tag=tag, attname=attname) + match = re.search(regex, content) + self.assertTrue(match, "Couldn't find desired tag in %r" % content) + attvalues = match.group(1).split() + self.assertIn(value, attvalues) + def test_default_is_en(self): - self.release_languages('fr') response = self.client.get('/') self.assert_tag_has_attr(response.content, "html", "lang", "en") self.assertEqual(response['Content-Language'], 'en') self.assert_tag_has_attr(response.content, "body", "class", "lang_en") def test_esperanto(self): - self.release_languages('fr, eo') response = self.client.get('/', HTTP_ACCEPT_LANGUAGE='eo') self.assert_tag_has_attr(response.content, "html", "lang", "eo") self.assertEqual(response['Content-Language'], 'eo') self.assert_tag_has_attr(response.content, "body", "class", "lang_eo") def test_switching_languages_bidi(self): - self.release_languages('ar, eo') response = self.client.get('/') self.assert_tag_has_attr(response.content, "html", "lang", "en") self.assertEqual(response['Content-Language'], 'en') @@ -68,26 +46,3 @@ class I18nTestCase(BaseI18nTestCase): self.assertEqual(response['Content-Language'], 'ar') self.assert_tag_has_attr(response.content, "body", "class", "lang_ar") self.assert_tag_has_attr(response.content, "body", "class", "rtl") - - -@attr('shard_1') -class I18nRegressionTests(BaseI18nTestCase): - """ - Tests for i18n - """ - def test_es419_acceptance(self): - # Regression test; LOC-72, and an issue with Django - self.release_languages('es-419') - response = self.client.get('/', HTTP_ACCEPT_LANGUAGE='es-419') - self.assert_tag_has_attr(response.content, "html", "lang", "es-419") - - def test_unreleased_lang_resolution(self): - # Regression test; LOC-85 - self.release_languages('fa') - - # We've released 'fa', AND we have language files for 'fa-ir' but - # we want to keep 'fa-ir' as a dark language. Requesting 'fa-ir' - # in the http request (NOT with the ?preview-lang query param) should - # receive files for 'fa' - response = self.client.get('/', HTTP_ACCEPT_LANGUAGE='fa-ir') - self.assert_tag_has_attr(response.content, "html", "lang", "fa") diff --git a/lms/envs/common.py b/lms/envs/common.py index d2fedc6fed..dc3c1deeb5 100644 --- a/lms/envs/common.py +++ b/lms/envs/common.py @@ -1157,9 +1157,7 @@ MIDDLEWARE_CLASSES = ( 'lang_pref.middleware.LanguagePreferenceMiddleware', # Detects user-requested locale from 'accept-language' header in http request - # TODO: Re-import the Django version once we upgrade to Django 1.8 [PLAT-671] - # 'django.middleware.locale.LocaleMiddleware', - 'django_locale.middleware.LocaleMiddleware', + 'django.middleware.locale.LocaleMiddleware', 'django.middleware.transaction.TransactionMiddleware', # 'debug_toolbar.middleware.DebugToolbarMiddleware', From f0f3766443547bd7482fcb7187f79d2bea5bb114 Mon Sep 17 00:00:00 2001 From: Matt Drayer Date: Mon, 22 Jun 2015 14:01:05 -0400 Subject: [PATCH 08/12] mattdrayer/ECOM-1773: Fixed invalid URL reversal --- .../student/tests/test_certificates.py | 3 +- common/djangoapps/student/views.py | 9 ++- lms/djangoapps/certificates/api.py | 17 +++--- lms/djangoapps/certificates/tests/test_api.py | 4 +- .../certificates/tests/test_views.py | 57 ++++++++++++------- lms/djangoapps/certificates/views.py | 3 +- lms/djangoapps/courseware/tests/test_views.py | 9 ++- lms/djangoapps/courseware/views.py | 6 +- lms/envs/aws.py | 4 ++ lms/envs/common.py | 3 + lms/envs/test.py | 3 - lms/urls.py | 9 ++- 12 files changed, 83 insertions(+), 44 deletions(-) diff --git a/common/djangoapps/student/tests/test_certificates.py b/common/djangoapps/student/tests/test_certificates.py index dea4192a90..03d5b66e00 100644 --- a/common/djangoapps/student/tests/test_certificates.py +++ b/common/djangoapps/student/tests/test_certificates.py @@ -59,7 +59,8 @@ class CertificateDisplayTest(ModuleStoreTestCase): def test_linked_student_to_web_view_credential(self, enrollment_mode): test_url = get_certificate_url( user_id=self.user.id, - course_id=unicode(self.course.id) + course_id=unicode(self.course.id), + verify_uuid='abcdefg12345678' ) self._create_certificate(enrollment_mode) diff --git a/common/djangoapps/student/views.py b/common/djangoapps/student/views.py index 17a6bebf5d..b71f309248 100644 --- a/common/djangoapps/student/views.py +++ b/common/djangoapps/student/views.py @@ -307,11 +307,14 @@ def _cert_info(user, course, cert_status, course_mode): # showing the certificate web view button if certificate is ready state and feature flags are enabled. if settings.FEATURES.get('CERTIFICATES_HTML_VIEW', False): if get_active_web_certificate(course) is not None: + certificate_url = get_certificate_url( + user_id=user.id, + course_id=unicode(course.id), + verify_uuid=None + ) status_dict.update({ 'show_cert_web_view': True, - 'cert_web_view_url': u'{url}'.format( - url=get_certificate_url(user_id=user.id, course_id=unicode(course.id)) - ) + 'cert_web_view_url': u'{url}'.format(url=certificate_url) }) else: # don't show download certificate button if we don't have an active certificate for course diff --git a/lms/djangoapps/certificates/api.py b/lms/djangoapps/certificates/api.py index 311801dd0f..58b2b48a9f 100644 --- a/lms/djangoapps/certificates/api.py +++ b/lms/djangoapps/certificates/api.py @@ -253,15 +253,18 @@ def example_certificates_status(course_key): # pylint: disable=no-member -def get_certificate_url(user_id, course_id): +def get_certificate_url(user_id, course_id, verify_uuid): """ :return certificate url """ - url = u'{url}'.format(url=reverse('cert_html_view', - kwargs=dict( - user_id=str(user_id), - course_id=unicode(course_id)))) - return url + if settings.FEATURES.get('CERTIFICATES_HTML_VIEW', False): + return u'{url}'.format( + url=reverse( + 'cert_html_view', + kwargs=dict(user_id=str(user_id), course_id=unicode(course_id)) + ) + ) + return '{url}{uuid}'.format(url=settings.CERTIFICATES_STATIC_VERIFY_URL, uuid=verify_uuid) def get_active_web_certificate(course, is_preview_mode=None): @@ -290,7 +293,7 @@ def emit_certificate_event(event_name, user, course_id, course=None, event_data= data = { 'user_id': user.id, 'course_id': unicode(course_id), - 'certificate_url': get_certificate_url(user.id, course_id) + 'certificate_url': get_certificate_url(user.id, course_id, event_data['certificate_id']) } event_data = event_data or {} event_data.update(data) diff --git a/lms/djangoapps/certificates/tests/test_api.py b/lms/djangoapps/certificates/tests/test_api.py index bd77a2ff94..043bdf6639 100644 --- a/lms/djangoapps/certificates/tests/test_api.py +++ b/lms/djangoapps/certificates/tests/test_api.py @@ -148,7 +148,7 @@ class GenerateUserCertificatesTest(EventTestMixin, ModuleStoreTestCase): 'edx.certificate.created', user_id=self.student.id, course_id=unicode(self.course.id), - certificate_url=certs_api.get_certificate_url(self.student.id, self.course.id), + certificate_url=certs_api.get_certificate_url(self.student.id, self.course.id, cert.verify_uuid), certificate_id=cert.verify_uuid, enrollment_mode=cert.mode, generation_mode='batch' @@ -164,7 +164,7 @@ class GenerateUserCertificatesTest(EventTestMixin, ModuleStoreTestCase): self.assertEqual(cert.status, 'error') self.assertIn(self.ERROR_REASON, cert.error_reason) - @override_settings(FEATURES=FEATURES_WITH_CERTS_ENABLED) + @patch.dict(settings.FEATURES, {'CERTIFICATES_HTML_VIEW': True}) def test_new_cert_requests_returns_generating_for_html_certificate(self): """ Test no message sent to Xqueue if HTML certificate view is enabled diff --git a/lms/djangoapps/certificates/tests/test_views.py b/lms/djangoapps/certificates/tests/test_views.py index 979603a82a..e4f3eb7b80 100644 --- a/lms/djangoapps/certificates/tests/test_views.py +++ b/lms/djangoapps/certificates/tests/test_views.py @@ -308,7 +308,8 @@ class MicrositeCertificatesViewsTests(ModuleStoreTestCase): self.assertEquals(config.configuration, test_configuration_string) test_url = get_certificate_url( user_id=self.user.id, - course_id=self.course.id.to_deprecated_string() # pylint: disable=no-member + course_id=unicode(self.course.id), + verify_uuid=self.cert.verify_uuid ) self._add_course_certificates(count=1, signatory_count=2) response = self.client.get(test_url) @@ -341,7 +342,8 @@ class MicrositeCertificatesViewsTests(ModuleStoreTestCase): self.assertEquals(config.configuration, test_configuration_string) test_url = get_certificate_url( user_id=self.user.id, - course_id=self.course.id.to_deprecated_string() # pylint: disable=no-member + course_id=unicode(self.course.id), + verify_uuid=self.cert.verify_uuid ) self._add_course_certificates(count=1, signatory_count=2) response = self.client.get(test_url) @@ -427,7 +429,8 @@ class CertificatesViewsTests(ModuleStoreTestCase, EventTrackingTestCase): def test_render_html_view_valid_certificate(self): test_url = get_certificate_url( user_id=self.user.id, - course_id=unicode(self.course.id) # pylint: disable=no-member + course_id=unicode(self.course.id), + verify_uuid=self.cert.verify_uuid ) self._add_course_certificates(count=1, signatory_count=2) response = self.client.get(test_url) @@ -449,7 +452,8 @@ class CertificatesViewsTests(ModuleStoreTestCase, EventTrackingTestCase): def test_render_html_view_with_valid_signatories(self): test_url = get_certificate_url( user_id=self.user.id, - course_id=self.course.id.to_deprecated_string() # pylint: disable=no-member + course_id=unicode(self.course.id), + verify_uuid=self.cert.verify_uuid ) self._add_course_certificates(count=1, signatory_count=2) response = self.client.get(test_url) @@ -465,7 +469,8 @@ class CertificatesViewsTests(ModuleStoreTestCase, EventTrackingTestCase): # if certificate in descriptor has not course_title then course name should not be overridden with this title. test_url = get_certificate_url( user_id=self.user.id, - course_id=self.course.id.to_deprecated_string() # pylint: disable=no-member + course_id=unicode(self.course.id), + verify_uuid=self.cert.verify_uuid ) test_certificates = [ { @@ -488,7 +493,8 @@ class CertificatesViewsTests(ModuleStoreTestCase, EventTrackingTestCase): def test_certificate_view_without_org_logo(self): test_url = get_certificate_url( user_id=self.user.id, - course_id=self.course.id.to_deprecated_string() # pylint: disable=no-member + course_id=unicode(self.course.id), + verify_uuid=self.cert.verify_uuid ) test_certificates = [ { @@ -510,7 +516,8 @@ class CertificatesViewsTests(ModuleStoreTestCase, EventTrackingTestCase): def test_render_html_view_without_signatories(self): test_url = get_certificate_url( user_id=self.user.id, - course_id=self.course.id.to_deprecated_string() # pylint: disable=no-member + course_id=unicode(self.course), + verify_uuid=self.cert.verify_uuid ) self._add_course_certificates(count=1, signatory_count=0) response = self.client.get(test_url) @@ -518,19 +525,20 @@ class CertificatesViewsTests(ModuleStoreTestCase, EventTrackingTestCase): self.assertNotIn('Signatory_Title 0', response.content) @override_settings(FEATURES=FEATURES_WITH_CERTS_DISABLED) - def test_render_html_view_invalid_feature_flag(self): + def test_render_html_view_disabled_feature_flag_returns_static_url(self): test_url = get_certificate_url( user_id=self.user.id, - course_id=self.course.id.to_deprecated_string() # pylint: disable=no-member + course_id=unicode(self.course.id), + verify_uuid=self.cert.verify_uuid ) - response = self.client.get(test_url) - self.assertIn('invalid', response.content) + self.assertIn(str(self.cert.verify_uuid), test_url) @override_settings(FEATURES=FEATURES_WITH_CERTS_ENABLED) def test_render_html_view_invalid_course_id(self): test_url = get_certificate_url( user_id=self.user.id, - course_id='az/23423/4vs' + course_id='az/23423/4vs', + verify_uuid=self.cert.verify_uuid ) response = self.client.get(test_url) @@ -540,7 +548,8 @@ class CertificatesViewsTests(ModuleStoreTestCase, EventTrackingTestCase): def test_render_html_view_invalid_course(self): test_url = get_certificate_url( user_id=self.user.id, - course_id='missing/course/key' + course_id='missing/course/key', + verify_uuid=self.cert.verify_uuid ) response = self.client.get(test_url) self.assertIn('invalid', response.content) @@ -549,7 +558,8 @@ class CertificatesViewsTests(ModuleStoreTestCase, EventTrackingTestCase): def test_render_html_view_invalid_user(self): test_url = get_certificate_url( user_id=111, - course_id=self.course.id.to_deprecated_string() # pylint: disable=no-member + course_id=unicode(self.course.id), + verify_uuid=self.cert.verify_uuid ) response = self.client.get(test_url) self.assertIn('invalid', response.content) @@ -560,7 +570,8 @@ class CertificatesViewsTests(ModuleStoreTestCase, EventTrackingTestCase): self.assertEqual(len(GeneratedCertificate.objects.all()), 0) test_url = get_certificate_url( user_id=self.user.id, - course_id=self.course.id.to_deprecated_string() # pylint: disable=no-member + course_id=unicode(self.course.id), + verify_uuid=self.cert.verify_uuid ) response = self.client.get(test_url) self.assertIn('invalid', response.content) @@ -576,7 +587,8 @@ class CertificatesViewsTests(ModuleStoreTestCase, EventTrackingTestCase): self._add_course_certificates(count=1, signatory_count=2) test_url = get_certificate_url( user_id=self.user.id, - course_id=self.course.id.to_deprecated_string() # pylint: disable=no-member + course_id=unicode(self.course.id), + verify_uuid=self.cert.verify_uuid ) response = self.client.get(test_url + '?preview=honor') self.assertNotIn(self.course.display_name, response.content) @@ -594,7 +606,8 @@ class CertificatesViewsTests(ModuleStoreTestCase, EventTrackingTestCase): def test_render_html_view_invalid_certificate_configuration(self): test_url = get_certificate_url( user_id=self.user.id, - course_id=unicode(self.course.id) + course_id=unicode(self.course.id), + verify_uuid=self.cert.verify_uuid ) response = self.client.get(test_url) self.assertIn("Invalid Certificate", response.content) @@ -606,7 +619,8 @@ class CertificatesViewsTests(ModuleStoreTestCase, EventTrackingTestCase): self.recreate_tracker() test_url = get_certificate_url( user_id=self.user.id, - course_id=unicode(self.course.id) + course_id=unicode(self.course.id), + verify_uuid=self.cert.verify_uuid ) response = self.client.get(test_url) self.assertEqual(response.status_code, 200) @@ -626,7 +640,12 @@ class CertificatesViewsTests(ModuleStoreTestCase, EventTrackingTestCase): @override_settings(FEATURES=FEATURES_WITH_CERTS_ENABLED) def test_evidence_event_sent(self): - test_url = get_certificate_url(user_id=self.user.id, course_id=self.course_id) + '?evidence_visit=1' + cert_url = get_certificate_url( + user_id=self.user.id, + course_id=self.course_id, + verify_uuid=self.cert.verify_uuid + ) + test_url = '{}?evidence_visit=1'.format(cert_url) self.recreate_tracker() assertion = BadgeAssertion( user=self.user, course_id=self.course_id, mode='honor', diff --git a/lms/djangoapps/certificates/views.py b/lms/djangoapps/certificates/views.py index 963ce40304..7d0c7bf40a 100644 --- a/lms/djangoapps/certificates/views.py +++ b/lms/djangoapps/certificates/views.py @@ -439,7 +439,8 @@ def _update_certificate_context(context, course, user, user_certificate): user_certificate.mode, get_certificate_url( user_id=user.id, - course_id=course.id.to_deprecated_string() + course_id=unicode(course.id), + verify_uuid=user_certificate.verify_uuid ) ) diff --git a/lms/djangoapps/courseware/tests/test_views.py b/lms/djangoapps/courseware/tests/test_views.py index 5e511086af..217da31f53 100644 --- a/lms/djangoapps/courseware/tests/test_views.py +++ b/lms/djangoapps/courseware/tests/test_views.py @@ -824,7 +824,7 @@ class ProgressPageTests(ModuleStoreTestCase): If certificate web view is enabled then certificate web view button should appear for user who certificate is available/generated """ - GeneratedCertificateFactory.create( + certificate = GeneratedCertificateFactory.create( user=self.user, course_id=self.course.id, status=CertificateStatuses.downloadable, @@ -859,7 +859,12 @@ class ProgressPageTests(ModuleStoreTestCase): resp = views.progress(self.request, course_id=unicode(self.course.id)) self.assertContains(resp, u"View Certificate") self.assertContains(resp, u"You can now view your certificate") - self.assertContains(resp, certs_api.get_certificate_url(user_id=self.user.id, course_id=self.course.id)) + cert_url = certs_api.get_certificate_url( + user_id=self.user.id, + course_id=self.course.id, + verify_uuid=certificate.verify_uuid + ) + self.assertContains(resp, cert_url) # when course certificate is not active certificates[0]['is_active'] = False diff --git a/lms/djangoapps/courseware/views.py b/lms/djangoapps/courseware/views.py index f9e5fc3e0b..f9db7beb9e 100644 --- a/lms/djangoapps/courseware/views.py +++ b/lms/djangoapps/courseware/views.py @@ -1100,7 +1100,11 @@ def _progress(request, course_key, student_id): context.update({ 'show_cert_web_view': True, 'cert_web_view_url': u'{url}'.format( - url=certs_api.get_certificate_url(user_id=student.id, course_id=unicode(course.id)) + url=certs_api.get_certificate_url( + user_id=student.id, + course_id=unicode(course.id), + verify_uuid=None + ) ) }) else: diff --git a/lms/envs/aws.py b/lms/envs/aws.py index 1f113305a6..23163c3c96 100644 --- a/lms/envs/aws.py +++ b/lms/envs/aws.py @@ -640,3 +640,7 @@ EDXNOTES_INTERNAL_API = ENV_TOKENS.get('EDXNOTES_INTERNAL_API', EDXNOTES_INTERNA ##### Credit Provider Integration ##### CREDIT_PROVIDER_SECRET_KEYS = AUTH_TOKENS.get("CREDIT_PROVIDER_SECRET_KEYS", {}) + + +############ CERTIFICATE VERIFICATION URL (STATIC FILES) ########### +ENV_TOKENS.get('CERTIFICATES_STATIC_VERIFY_URL', CERTIFICATES_STATIC_VERIFY_URL) diff --git a/lms/envs/common.py b/lms/envs/common.py index d2fedc6fed..c8eb0f0f4c 100644 --- a/lms/envs/common.py +++ b/lms/envs/common.py @@ -2067,6 +2067,9 @@ REGISTRATION_EXTRA_FIELDS = { CERT_NAME_SHORT = "Certificate" CERT_NAME_LONG = "Certificate of Achievement" +############ CERTIFICATE VERIFICATION URL (STATIC FILES) ########### +CERTIFICATES_STATIC_VERIFY_URL = "https://verify-test.edx.org/cert/" + #################### Badgr OpenBadges generation ####################### # Be sure to set up images for course modes using the BadgeImageConfiguration model in the certificates app. BADGR_API_TOKEN = None diff --git a/lms/envs/test.py b/lms/envs/test.py index 5a81e3cca2..ffc2013c47 100644 --- a/lms/envs/test.py +++ b/lms/envs/test.py @@ -472,9 +472,6 @@ FACEBOOK_APP_SECRET = "Test" FACEBOOK_APP_ID = "Test" FACEBOOK_API_VERSION = "v2.2" -# Certificates Views -FEATURES['CERTIFICATES_HTML_VIEW'] = True - ######### custom courses ######### INSTALLED_APPS += ('ccx',) FEATURES['CUSTOM_COURSES_EDX'] = True diff --git a/lms/urls.py b/lms/urls.py index d1bf250754..ed6bfffe44 100644 --- a/lms/urls.py +++ b/lms/urls.py @@ -663,11 +663,10 @@ if settings.FEATURES.get('ENABLE_OAUTH2_PROVIDER'): ) # Certificates Web/HTML View -if settings.FEATURES.get('CERTIFICATES_HTML_VIEW', False): - urlpatterns += ( - url(r'^certificates/user/(?P[^/]*)/course/{course_id}'.format(course_id=settings.COURSE_ID_PATTERN), - 'certificates.views.render_html_view', name='cert_html_view'), - ) +urlpatterns += ( + url(r'^certificates/user/(?P[^/]*)/course/{course_id}'.format(course_id=settings.COURSE_ID_PATTERN), + 'certificates.views.render_html_view', name='cert_html_view'), +) BADGE_SHARE_TRACKER_URL = url( r'^certificates/badge_share_tracker/{}/(?P[^/]+)/(?P[^/]+)/$'.format( From 8718dc13ca2e7ca58d25dea4e453da2d001d272c Mon Sep 17 00:00:00 2001 From: David Ormsbee Date: Wed, 27 May 2015 18:35:03 -0400 Subject: [PATCH 09/12] Cache SplitMongo course structures in memcached. This is primarily to reduce load on MongoDB, where we've lately had performance problems that we suspect are caused by very large course structures being evicted from MongoDB's cache. This may potentially give us a path to better performance as well, but that's not the goal of this commit. Surprisingly, LZ4 seemed to actually run more slowly than zlib for this. Possibly because of some overhead in the Python bindings? GZip was also surprisingly slow given that it uses zlib underneath (something like 5x slower). Use separate cache backend for caching structures. Abstract out course structure cache. add datadog metrics for compressed course structure sizes Since we're using a different cache background, we don't need to have a cache prefix Use dummy cache backend for tests. Fallback to default cache if course_structure_cache doesn't exist. --- cms/envs/dev.py | 5 +- cms/envs/test.py | 4 +- .../split_mongo/mongo_connection.py | 60 +++++++++++++++++-- .../tests/test_mixed_modulestore.py | 2 +- lms/envs/dev.py | 4 ++ lms/envs/test.py | 4 +- 6 files changed, 69 insertions(+), 10 deletions(-) diff --git a/cms/envs/dev.py b/cms/envs/dev.py index 216817d66c..7ba5d46121 100644 --- a/cms/envs/dev.py +++ b/cms/envs/dev.py @@ -115,7 +115,10 @@ CACHES = { 'BACKEND': 'django.core.cache.backends.locmem.LocMemCache', 'LOCATION': 'edx_location_mem_cache', }, - + 'course_structure_cache': { + 'BACKEND': 'django.core.cache.backends.locmem.LocMemCache', + 'LOCATION': 'edx_course_structure_mem_cache', + }, } # Make the keyedcache startup warnings go away diff --git a/cms/envs/test.py b/cms/envs/test.py index b637368c9f..082960a804 100644 --- a/cms/envs/test.py +++ b/cms/envs/test.py @@ -166,7 +166,9 @@ CACHES = { 'BACKEND': 'django.core.cache.backends.locmem.LocMemCache', 'LOCATION': 'edx_location_mem_cache', }, - + 'course_structure_cache': { + 'BACKEND': 'django.core.cache.backends.dummy.DummyCache', + }, } # Add external_auth to Installed apps for testing diff --git a/common/lib/xmodule/xmodule/modulestore/split_mongo/mongo_connection.py b/common/lib/xmodule/xmodule/modulestore/split_mongo/mongo_connection.py index ab7f922df5..bcee23b7f0 100644 --- a/common/lib/xmodule/xmodule/modulestore/split_mongo/mongo_connection.py +++ b/common/lib/xmodule/xmodule/modulestore/split_mongo/mongo_connection.py @@ -2,7 +2,11 @@ Segregation of pymongo functions from the data modeling mechanisms for split modulestore. """ import datetime +import cPickle as pickle import math +import re +import zlib +from mongodb_proxy import autoretry_read, MongoProxy import pymongo import pytz import re @@ -11,6 +15,8 @@ from time import time # Import this just to export it from pymongo.errors import DuplicateKeyError # pylint: disable=unused-import +from django.core.cache import get_cache, InvalidCacheBackendError +import dogstats_wrapper as dog_stats_api from contracts import check, new_contract from mongodb_proxy import autoretry_read, MongoProxy @@ -203,6 +209,40 @@ def structure_to_mongo(structure, course_context=None): return new_structure +class CourseStructureCache(object): + """ + Wrapper around django cache object to cache course structure objects. + The course structures are pickled and compressed when cached. + """ + def __init__(self): + try: + self.cache = get_cache('course_structure_cache') + except InvalidCacheBackendError: + self.cache = get_cache('default') + + def get(self, key): + """Pull the compressed, pickled struct data from cache and deserialize.""" + compressed_pickled_data = self.cache.get(key) + if compressed_pickled_data is None: + return None + return pickle.loads(zlib.decompress(compressed_pickled_data)) + + def set(self, key, structure): + """Given a structure, will pickle, compress, and write to cache.""" + pickled_data = pickle.dumps(structure, pickle.HIGHEST_PROTOCOL) + # 1 = Fastest (slightly larger results) + compressed_pickled_data = zlib.compress(pickled_data, 1) + + # record compressed course structure sizes + dog_stats_api.histogram( + 'compressed_course_structure.size', + len(compressed_pickled_data), + tags=[key] + ) + # Stuctures are immutable, so we set a timeout of "never" + self.cache.set(key, compressed_pickled_data, None) + + class MongoConnection(object): """ Segregation of pymongo functions from the data modeling mechanisms for split modulestore. @@ -256,15 +296,23 @@ class MongoConnection(object): def get_structure(self, key, course_context=None): """ - Get the structure from the persistence mechanism whose id is the given key + Get the structure from the persistence mechanism whose id is the given key. + + This method will use a cached version of the structure if it is availble. """ with TIMER.timer("get_structure", course_context) as tagger_get_structure: - with TIMER.timer("get_structure.find_one", course_context) as tagger_find_one: - doc = self.structures.find_one({'_id': key}) - tagger_find_one.measure("blocks", len(doc['blocks'])) - tagger_get_structure.measure("blocks", len(doc['blocks'])) + cache = CourseStructureCache() - return structure_from_mongo(doc, course_context) + structure = cache.get(key) + tagger_get_structure.tag(from_cache='true' if structure else 'false') + if not structure: + with TIMER.timer("get_structure.find_one", course_context) as tagger_find_one: + doc = self.structures.find_one({'_id': key}) + tagger_find_one.measure("blocks", len(doc['blocks'])) + structure = structure_from_mongo(doc, course_context) + cache.set(key, structure) + + return structure @autoretry_read() def find_structures_by_id(self, ids, course_context=None): diff --git a/common/lib/xmodule/xmodule/modulestore/tests/test_mixed_modulestore.py b/common/lib/xmodule/xmodule/modulestore/tests/test_mixed_modulestore.py index b41bf268d6..e83c9be516 100644 --- a/common/lib/xmodule/xmodule/modulestore/tests/test_mixed_modulestore.py +++ b/common/lib/xmodule/xmodule/modulestore/tests/test_mixed_modulestore.py @@ -794,7 +794,7 @@ class TestMixedModuleStore(CommonMixedModuleStoreSetup): # find: find parent (definition.children) 2x, find draft item, get inheritance items # send: one delete query for specific item # Split: - # find: active_version & structure + # find: active_version & structure (cached) # send: update structure and active_versions @ddt.data(('draft', 4, 1), ('split', 2, 2)) @ddt.unpack diff --git a/lms/envs/dev.py b/lms/envs/dev.py index b8060f230d..a0e3dee83f 100644 --- a/lms/envs/dev.py +++ b/lms/envs/dev.py @@ -94,6 +94,10 @@ CACHES = { 'BACKEND': 'django.core.cache.backends.locmem.LocMemCache', 'LOCATION': 'edx_location_mem_cache', }, + 'course_structure_cache': { + 'BACKEND': 'django.core.cache.backends.locmem.LocMemCache', + 'LOCATION': 'edx_course_structure_mem_cache', + }, } diff --git a/lms/envs/test.py b/lms/envs/test.py index 38a30b944b..c8c1875f7f 100644 --- a/lms/envs/test.py +++ b/lms/envs/test.py @@ -207,7 +207,9 @@ CACHES = { 'BACKEND': 'django.core.cache.backends.locmem.LocMemCache', 'LOCATION': 'edx_location_mem_cache', }, - + 'course_structure_cache': { + 'BACKEND': 'django.core.cache.backends.dummy.DummyCache', + }, } # Dummy secret key for dev From 243ac0048151c57fe3815b0d5f1af8510cd307ce Mon Sep 17 00:00:00 2001 From: Awais Date: Mon, 22 Jun 2015 14:55:12 +0500 Subject: [PATCH 10/12] ECOM-1772 Due date issue test case fixed. --- lms/djangoapps/verify_student/tests/test_views.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/lms/djangoapps/verify_student/tests/test_views.py b/lms/djangoapps/verify_student/tests/test_views.py index 0027c312b7..833b8e4b95 100644 --- a/lms/djangoapps/verify_student/tests/test_views.py +++ b/lms/djangoapps/verify_student/tests/test_views.py @@ -1849,7 +1849,7 @@ class TestEmailMessageWithCustomICRVBlock(ModuleStoreTestCase): """ self.course_key = SlashSeparatedCourseKey("Robot", "999", "Test_Course") self.course = CourseFactory.create(org='Robot', number='999', display_name='Test Course') - self.due_date = datetime(2015, 6, 22, tzinfo=pytz.UTC) + self.due_date = datetime.now(pytz.UTC) + timedelta(days=20) self.allowed_attempts = 1 # Create the course modes From 39ab0f31dc475881d01c49ca783a597215316660 Mon Sep 17 00:00:00 2001 From: David Ormsbee Date: Wed, 27 May 2015 18:35:03 -0400 Subject: [PATCH 11/12] Cache SplitMongo course structures in memcached. This is primarily to reduce load on MongoDB, where we've lately had performance problems that we suspect are caused by very large course structures being evicted from MongoDB's cache. This may potentially give us a path to better performance as well, but that's not the goal of this commit. Surprisingly, LZ4 seemed to actually run more slowly than zlib for this. Possibly because of some overhead in the Python bindings? GZip was also surprisingly slow given that it uses zlib underneath (something like 5x slower). Use separate cache backend for caching structures. Abstract out course structure cache. add datadog metrics for compressed course structure sizes Since we're using a different cache background, we don't need to have a cache prefix Use dummy cache backend for tests. Fallback to default cache if course_structure_cache doesn't exist. --- cms/envs/dev.py | 5 +- cms/envs/test.py | 4 +- .../split_mongo/mongo_connection.py | 60 +++++++++++++++++-- .../tests/test_mixed_modulestore.py | 2 +- lms/envs/dev.py | 4 ++ lms/envs/test.py | 4 +- 6 files changed, 69 insertions(+), 10 deletions(-) diff --git a/cms/envs/dev.py b/cms/envs/dev.py index 216817d66c..7ba5d46121 100644 --- a/cms/envs/dev.py +++ b/cms/envs/dev.py @@ -115,7 +115,10 @@ CACHES = { 'BACKEND': 'django.core.cache.backends.locmem.LocMemCache', 'LOCATION': 'edx_location_mem_cache', }, - + 'course_structure_cache': { + 'BACKEND': 'django.core.cache.backends.locmem.LocMemCache', + 'LOCATION': 'edx_course_structure_mem_cache', + }, } # Make the keyedcache startup warnings go away diff --git a/cms/envs/test.py b/cms/envs/test.py index b637368c9f..082960a804 100644 --- a/cms/envs/test.py +++ b/cms/envs/test.py @@ -166,7 +166,9 @@ CACHES = { 'BACKEND': 'django.core.cache.backends.locmem.LocMemCache', 'LOCATION': 'edx_location_mem_cache', }, - + 'course_structure_cache': { + 'BACKEND': 'django.core.cache.backends.dummy.DummyCache', + }, } # Add external_auth to Installed apps for testing diff --git a/common/lib/xmodule/xmodule/modulestore/split_mongo/mongo_connection.py b/common/lib/xmodule/xmodule/modulestore/split_mongo/mongo_connection.py index ab7f922df5..bcee23b7f0 100644 --- a/common/lib/xmodule/xmodule/modulestore/split_mongo/mongo_connection.py +++ b/common/lib/xmodule/xmodule/modulestore/split_mongo/mongo_connection.py @@ -2,7 +2,11 @@ Segregation of pymongo functions from the data modeling mechanisms for split modulestore. """ import datetime +import cPickle as pickle import math +import re +import zlib +from mongodb_proxy import autoretry_read, MongoProxy import pymongo import pytz import re @@ -11,6 +15,8 @@ from time import time # Import this just to export it from pymongo.errors import DuplicateKeyError # pylint: disable=unused-import +from django.core.cache import get_cache, InvalidCacheBackendError +import dogstats_wrapper as dog_stats_api from contracts import check, new_contract from mongodb_proxy import autoretry_read, MongoProxy @@ -203,6 +209,40 @@ def structure_to_mongo(structure, course_context=None): return new_structure +class CourseStructureCache(object): + """ + Wrapper around django cache object to cache course structure objects. + The course structures are pickled and compressed when cached. + """ + def __init__(self): + try: + self.cache = get_cache('course_structure_cache') + except InvalidCacheBackendError: + self.cache = get_cache('default') + + def get(self, key): + """Pull the compressed, pickled struct data from cache and deserialize.""" + compressed_pickled_data = self.cache.get(key) + if compressed_pickled_data is None: + return None + return pickle.loads(zlib.decompress(compressed_pickled_data)) + + def set(self, key, structure): + """Given a structure, will pickle, compress, and write to cache.""" + pickled_data = pickle.dumps(structure, pickle.HIGHEST_PROTOCOL) + # 1 = Fastest (slightly larger results) + compressed_pickled_data = zlib.compress(pickled_data, 1) + + # record compressed course structure sizes + dog_stats_api.histogram( + 'compressed_course_structure.size', + len(compressed_pickled_data), + tags=[key] + ) + # Stuctures are immutable, so we set a timeout of "never" + self.cache.set(key, compressed_pickled_data, None) + + class MongoConnection(object): """ Segregation of pymongo functions from the data modeling mechanisms for split modulestore. @@ -256,15 +296,23 @@ class MongoConnection(object): def get_structure(self, key, course_context=None): """ - Get the structure from the persistence mechanism whose id is the given key + Get the structure from the persistence mechanism whose id is the given key. + + This method will use a cached version of the structure if it is availble. """ with TIMER.timer("get_structure", course_context) as tagger_get_structure: - with TIMER.timer("get_structure.find_one", course_context) as tagger_find_one: - doc = self.structures.find_one({'_id': key}) - tagger_find_one.measure("blocks", len(doc['blocks'])) - tagger_get_structure.measure("blocks", len(doc['blocks'])) + cache = CourseStructureCache() - return structure_from_mongo(doc, course_context) + structure = cache.get(key) + tagger_get_structure.tag(from_cache='true' if structure else 'false') + if not structure: + with TIMER.timer("get_structure.find_one", course_context) as tagger_find_one: + doc = self.structures.find_one({'_id': key}) + tagger_find_one.measure("blocks", len(doc['blocks'])) + structure = structure_from_mongo(doc, course_context) + cache.set(key, structure) + + return structure @autoretry_read() def find_structures_by_id(self, ids, course_context=None): diff --git a/common/lib/xmodule/xmodule/modulestore/tests/test_mixed_modulestore.py b/common/lib/xmodule/xmodule/modulestore/tests/test_mixed_modulestore.py index b41bf268d6..e83c9be516 100644 --- a/common/lib/xmodule/xmodule/modulestore/tests/test_mixed_modulestore.py +++ b/common/lib/xmodule/xmodule/modulestore/tests/test_mixed_modulestore.py @@ -794,7 +794,7 @@ class TestMixedModuleStore(CommonMixedModuleStoreSetup): # find: find parent (definition.children) 2x, find draft item, get inheritance items # send: one delete query for specific item # Split: - # find: active_version & structure + # find: active_version & structure (cached) # send: update structure and active_versions @ddt.data(('draft', 4, 1), ('split', 2, 2)) @ddt.unpack diff --git a/lms/envs/dev.py b/lms/envs/dev.py index b8060f230d..a0e3dee83f 100644 --- a/lms/envs/dev.py +++ b/lms/envs/dev.py @@ -94,6 +94,10 @@ CACHES = { 'BACKEND': 'django.core.cache.backends.locmem.LocMemCache', 'LOCATION': 'edx_location_mem_cache', }, + 'course_structure_cache': { + 'BACKEND': 'django.core.cache.backends.locmem.LocMemCache', + 'LOCATION': 'edx_course_structure_mem_cache', + }, } diff --git a/lms/envs/test.py b/lms/envs/test.py index ffc2013c47..9803879579 100644 --- a/lms/envs/test.py +++ b/lms/envs/test.py @@ -207,7 +207,9 @@ CACHES = { 'BACKEND': 'django.core.cache.backends.locmem.LocMemCache', 'LOCATION': 'edx_location_mem_cache', }, - + 'course_structure_cache': { + 'BACKEND': 'django.core.cache.backends.dummy.DummyCache', + }, } # Dummy secret key for dev From b1346fc7476268eeb29d9d139d5dc1cccba542e7 Mon Sep 17 00:00:00 2001 From: Adam Palay Date: Mon, 22 Jun 2015 23:14:07 -0400 Subject: [PATCH 12/12] add tests for structure cache use default cache for tests add test for when cache isn't configured add test for dummy cache noop if no cache found --- .../xmodule/xmodule/modulestore/__init__.py | 26 +++++++ .../split_mongo/mongo_connection.py | 14 +++- .../tests/test_mixed_modulestore.py | 2 +- .../tests/test_split_modulestore.py | 75 +++++++++++++++++++ 4 files changed, 113 insertions(+), 4 deletions(-) diff --git a/common/lib/xmodule/xmodule/modulestore/__init__.py b/common/lib/xmodule/xmodule/modulestore/__init__.py index c167c25422..f47c901b65 100644 --- a/common/lib/xmodule/xmodule/modulestore/__init__.py +++ b/common/lib/xmodule/xmodule/modulestore/__init__.py @@ -369,6 +369,19 @@ class EditInfo(object): source_version="UNSET" if self.source_version is None else self.source_version, ) # pylint: disable=bad-continuation + def __eq__(self, edit_info): + """ + Two EditInfo instances are equal iff their storable representations + are equal. + """ + return self.to_storable() == edit_info.to_storable() + + def __neq__(self, edit_info): + """ + Two EditInfo instances are not equal if they're not equal. + """ + return not self == edit_info + class BlockData(object): """ @@ -426,6 +439,19 @@ class BlockData(object): classname=self.__class__.__name__, ) # pylint: disable=bad-continuation + def __eq__(self, block_data): + """ + Two BlockData objects are equal iff all their attributes are equal. + """ + attrs = ['fields', 'block_type', 'definition', 'defaults', 'edit_info'] + return all(getattr(self, attr) == getattr(block_data, attr) for attr in attrs) + + def __neq__(self, block_data): + """ + Just define this as not self.__eq__(block_data) + """ + return not self == block_data + new_contract('BlockData', BlockData) diff --git a/common/lib/xmodule/xmodule/modulestore/split_mongo/mongo_connection.py b/common/lib/xmodule/xmodule/modulestore/split_mongo/mongo_connection.py index bcee23b7f0..fc179c81ba 100644 --- a/common/lib/xmodule/xmodule/modulestore/split_mongo/mongo_connection.py +++ b/common/lib/xmodule/xmodule/modulestore/split_mongo/mongo_connection.py @@ -4,9 +4,7 @@ Segregation of pymongo functions from the data modeling mechanisms for split mod import datetime import cPickle as pickle import math -import re import zlib -from mongodb_proxy import autoretry_read, MongoProxy import pymongo import pytz import re @@ -213,15 +211,22 @@ class CourseStructureCache(object): """ Wrapper around django cache object to cache course structure objects. The course structures are pickled and compressed when cached. + + If the 'course_structure_cache' doesn't exist, then don't do anything for + for set and get. """ def __init__(self): + self.no_cache_found = False try: self.cache = get_cache('course_structure_cache') except InvalidCacheBackendError: - self.cache = get_cache('default') + self.no_cache_found = True def get(self, key): """Pull the compressed, pickled struct data from cache and deserialize.""" + if self.no_cache_found: + return None + compressed_pickled_data = self.cache.get(key) if compressed_pickled_data is None: return None @@ -229,6 +234,9 @@ class CourseStructureCache(object): def set(self, key, structure): """Given a structure, will pickle, compress, and write to cache.""" + if self.no_cache_found: + return None + pickled_data = pickle.dumps(structure, pickle.HIGHEST_PROTOCOL) # 1 = Fastest (slightly larger results) compressed_pickled_data = zlib.compress(pickled_data, 1) diff --git a/common/lib/xmodule/xmodule/modulestore/tests/test_mixed_modulestore.py b/common/lib/xmodule/xmodule/modulestore/tests/test_mixed_modulestore.py index e83c9be516..b41bf268d6 100644 --- a/common/lib/xmodule/xmodule/modulestore/tests/test_mixed_modulestore.py +++ b/common/lib/xmodule/xmodule/modulestore/tests/test_mixed_modulestore.py @@ -794,7 +794,7 @@ class TestMixedModuleStore(CommonMixedModuleStoreSetup): # find: find parent (definition.children) 2x, find draft item, get inheritance items # send: one delete query for specific item # Split: - # find: active_version & structure (cached) + # find: active_version & structure # send: update structure and active_versions @ddt.data(('draft', 4, 1), ('split', 2, 2)) @ddt.unpack diff --git a/common/lib/xmodule/xmodule/modulestore/tests/test_split_modulestore.py b/common/lib/xmodule/xmodule/modulestore/tests/test_split_modulestore.py index a47655f1a7..06b5067223 100644 --- a/common/lib/xmodule/xmodule/modulestore/tests/test_split_modulestore.py +++ b/common/lib/xmodule/xmodule/modulestore/tests/test_split_modulestore.py @@ -12,6 +12,7 @@ import uuid from contracts import contract from nose.plugins.attrib import attr +from django.core.cache import get_cache, InvalidCacheBackendError from openedx.core.lib import tempdir from xblock.fields import Reference, ReferenceList, ReferenceValueDict @@ -29,6 +30,7 @@ from xmodule.fields import Date, Timedelta from xmodule.modulestore.split_mongo.split import SplitMongoModuleStore from xmodule.modulestore.tests.test_modulestore import check_has_course_method from xmodule.modulestore.split_mongo import BlockKey +from xmodule.modulestore.tests.factories import check_mongo_calls from xmodule.modulestore.tests.mongo_connection import MONGO_PORT_NUM, MONGO_HOST from xmodule.modulestore.tests.utils import mock_tab_from_json from xmodule.modulestore.edit_info import EditInfoMixin @@ -771,6 +773,79 @@ class SplitModuleCourseTests(SplitModuleTest): self.assertEqual(result.children[0].children[0].locator.version_guid, versions[0]) +class TestCourseStructureCache(SplitModuleTest): + """Tests for the CourseStructureCache""" + + def setUp(self): + # use the default cache, since the `course_structure_cache` + # is a dummy cache during testing + self.cache = get_cache('default') + + # make sure we clear the cache before every test... + self.cache.clear() + # ... and after + self.addCleanup(self.cache.clear) + + # make a new course: + self.user = random.getrandbits(32) + self.new_course = modulestore().create_course( + 'org', 'course', 'test_run', self.user, BRANCH_NAME_DRAFT, + ) + + super(TestCourseStructureCache, self).setUp() + + @patch('xmodule.modulestore.split_mongo.mongo_connection.get_cache') + def test_course_structure_cache(self, mock_get_cache): + # force get_cache to return the default cache so we can test + # its caching behavior + mock_get_cache.return_value = self.cache + + with check_mongo_calls(1): + not_cached_structure = self._get_structure(self.new_course) + + # when cache is warmed, we should have one fewer mongo call + with check_mongo_calls(0): + cached_structure = self._get_structure(self.new_course) + + # now make sure that you get the same structure + self.assertEqual(cached_structure, not_cached_structure) + + @patch('xmodule.modulestore.split_mongo.mongo_connection.get_cache') + def test_course_structure_cache_no_cache_configured(self, mock_get_cache): + mock_get_cache.side_effect = InvalidCacheBackendError + + with check_mongo_calls(1): + not_cached_structure = self._get_structure(self.new_course) + + # if the cache isn't configured, we expect to have to make + # another mongo call here if we want the same course structure + with check_mongo_calls(1): + cached_structure = self._get_structure(self.new_course) + + # now make sure that you get the same structure + self.assertEqual(cached_structure, not_cached_structure) + + def test_dummy_cache(self): + with check_mongo_calls(1): + not_cached_structure = self._get_structure(self.new_course) + + # Since the test is using the dummy cache, it's not actually caching + # anything + with check_mongo_calls(1): + cached_structure = self._get_structure(self.new_course) + + # now make sure that you get the same structure + self.assertEqual(cached_structure, not_cached_structure) + + def _get_structure(self, course): + """ + Helper function to get a structure from a course. + """ + return modulestore().db_connection.get_structure( + course.location.as_object_id(course.location.version_guid) + ) + + class SplitModuleItemTests(SplitModuleTest): ''' Item read tests including inheritance