From dbee08c72178b74736bba2b248e9b4819e09cce3 Mon Sep 17 00:00:00 2001 From: David Ormsbee Date: Tue, 9 Feb 2016 17:40:54 -0500 Subject: [PATCH 1/2] Make Capa problems do initial load without AJAX. Before this commit, calling the student_view on a capa problem would cause it to render an empty placeholder
, wait for the DOMContentLoaded event to be fired, and then make AJAX requests to the the problem_get handlers to retrieve the HTML it needed to render the actual problems. This can significantly increase the end user load times for pages, particularly when there are many problems in a vertical. This commit takes a very conservative approach and has the server side add the rendered HTML into a new data-content attribute on the
enclosing the problem. When Capa's JS initialization runs, it grabs from that data-content attribute rather than reaching over the network for an AJAX request. I had attempted to make it somewhat smarter and push the rendered problem straight into the document instead of relying on the data-content attribute. This was faster, and should be our long term goal. However, it caused odd bugs, particularly around MathJAX rendering, and I never quite tracked the issue down. I'm still going forward with these changes because it's significantly better than the current situation that students have to deal with, and we can make the JS more performant in a future iteration. [PERF-261] --- common/lib/xmodule/xmodule/capa_base.py | 1 + common/lib/xmodule/xmodule/js/src/capa/display.coffee | 3 ++- lms/templates/problem_ajax.html | 2 +- 3 files changed, 4 insertions(+), 2 deletions(-) diff --git a/common/lib/xmodule/xmodule/capa_base.py b/common/lib/xmodule/xmodule/capa_base.py index 64e9729c2a..0eb1241de9 100644 --- a/common/lib/xmodule/xmodule/capa_base.py +++ b/common/lib/xmodule/xmodule/capa_base.py @@ -399,6 +399,7 @@ class CapaMixin(CapaFields): 'ajax_url': self.runtime.ajax_url, 'progress_status': Progress.to_js_status_str(progress), 'progress_detail': Progress.to_js_detail_str(progress), + 'content': self.get_problem_html(encapsulate=False) }) def check_button_name(self): diff --git a/common/lib/xmodule/xmodule/js/src/capa/display.coffee b/common/lib/xmodule/xmodule/js/src/capa/display.coffee index 7ea9ac17a9..211cdf8967 100644 --- a/common/lib/xmodule/xmodule/js/src/capa/display.coffee +++ b/common/lib/xmodule/xmodule/js/src/capa/display.coffee @@ -5,6 +5,7 @@ class @Problem @id = @el.data('problem-id') @element_id = @el.attr('id') @url = @el.data('url') + @content = @el.data('content') # has_timed_out and has_response are used to ensure that are used to # ensure that we wait a minimum of ~ 1s before transitioning the check @@ -12,7 +13,7 @@ class @Problem @has_timed_out = false @has_response = false - @render() + @render(@content) $: (selector) -> $(selector, @el) diff --git a/lms/templates/problem_ajax.html b/lms/templates/problem_ajax.html index 05c3c2ada8..664b7567c0 100644 --- a/lms/templates/problem_ajax.html +++ b/lms/templates/problem_ajax.html @@ -1 +1 @@ -
+
From dd8ab6eedeb3fb0085629c3af6bd0f504f38a168 Mon Sep 17 00:00:00 2001 From: Qubad786 Date: Thu, 17 Mar 2016 19:50:32 +0500 Subject: [PATCH 2/2] Keep track of updated problem states. An event is being fired on actions (Check/Save/Reset) to keep track of updated problems. On coming back to already visited sequence position, while putting sequence contents in container, only those problems that are found outdated, are going to be updated from earlier tracked problems. [SUST-40] --- common/lib/xmodule/xmodule/capa_base.py | 3 +- .../xmodule/js/src/capa/display.coffee | 4 + .../xmodule/js/src/sequence/display.coffee | 37 ++++ common/test/acceptance/pages/lms/problem.py | 34 +++- .../tests/lms/test_lms_courseware.py | 165 ++++++++++++++++++ 5 files changed, 241 insertions(+), 2 deletions(-) diff --git a/common/lib/xmodule/xmodule/capa_base.py b/common/lib/xmodule/xmodule/capa_base.py index 0eb1241de9..d8b01d575b 100644 --- a/common/lib/xmodule/xmodule/capa_base.py +++ b/common/lib/xmodule/xmodule/capa_base.py @@ -399,7 +399,7 @@ class CapaMixin(CapaFields): 'ajax_url': self.runtime.ajax_url, 'progress_status': Progress.to_js_status_str(progress), 'progress_detail': Progress.to_js_detail_str(progress), - 'content': self.get_problem_html(encapsulate=False) + 'content': self.get_problem_html(encapsulate=False), }) def check_button_name(self): @@ -1423,6 +1423,7 @@ class CapaMixin(CapaFields): return { 'success': True, 'msg': msg, + 'html': self.get_problem_html(encapsulate=False), } def reset_problem(self, _data): diff --git a/common/lib/xmodule/xmodule/js/src/capa/display.coffee b/common/lib/xmodule/xmodule/js/src/capa/display.coffee index 211cdf8967..07fd6a5929 100644 --- a/common/lib/xmodule/xmodule/js/src/capa/display.coffee +++ b/common/lib/xmodule/xmodule/js/src/capa/display.coffee @@ -317,6 +317,7 @@ class @Problem switch response.success when 'incorrect', 'correct' window.SR.readElts($(response.contents).find('.status')) + @el.trigger('contentChanged', [@id, response.contents]) @render(response.contents) @updateProgress response if @el.hasClass 'showed' @@ -332,6 +333,7 @@ class @Problem reset_internal: => Logger.log 'problem_reset', @answers $.postWithPrefix "#{@url}/problem_reset", id: @id, (response) => + @el.trigger('contentChanged', [@id, response.html]) @render(response.html) @updateProgress response @@ -420,6 +422,8 @@ class @Problem Logger.log 'problem_save', @answers $.postWithPrefix "#{@url}/problem_save", @answers, (response) => saveMessage = response.msg + if response.success + @el.trigger('contentChanged', [@id, response.html]) @gentle_alert saveMessage @updateProgress response diff --git a/common/lib/xmodule/xmodule/js/src/sequence/display.coffee b/common/lib/xmodule/xmodule/js/src/sequence/display.coffee index 94caf70e53..574b67d791 100644 --- a/common/lib/xmodule/xmodule/js/src/sequence/display.coffee +++ b/common/lib/xmodule/xmodule/js/src/sequence/display.coffee @@ -1,5 +1,6 @@ class @Sequence constructor: (element) -> + @updatedProblems = {} @requestToken = $(element).data('request-token') @el = $(element).find('.sequence') @contents = @$('.seq_contents') @@ -40,6 +41,33 @@ class @Sequence if position_link and position_link.data('page-title') document.title = position_link.data('page-title') + @base_page_title + hookUpContentStateChangeEvent: -> + $('.problems-wrapper').bind( + 'contentChanged', + (event, problem_id, new_content_state) => + @addToUpdatedProblems problem_id, new_content_state + ) + + addToUpdatedProblems: (problem_id, new_content_state) => + # Used to keep updated problem's state temporarily. + # params: + # 'problem_id' is problem id. + # 'new_content_state' is updated problem's state. + + # initialize for the current sequence if there isn't any updated problem + # for this position. + if not @anyUpdatedProblems @position + @updatedProblems[@position] = {} + + # Now, put problem content against problem id for current active sequence. + @updatedProblems[@position][problem_id] = new_content_state + + anyUpdatedProblems:(position) -> + # check for the updated problems for given sequence position. + # params: + # 'position' can be any sequence position. + return @updatedProblems[position] != undefined + hookUpProgressEvent: -> $('.problems-wrapper').bind 'progressChanged', @updateProgress @@ -129,12 +157,21 @@ class @Sequence bookmarked = if @el.find('.active .bookmark-icon').hasClass('bookmarked') then true else false @content_container.html(current_tab.text()).attr("aria-labelledby", current_tab.attr("aria-labelledby")).data('bookmarked', bookmarked) + + # update the data-attributes with latest contents only for updated problems. + if @anyUpdatedProblems new_position + $.each @updatedProblems[new_position], (problem_id, latest_content) => + @content_container + .find("[data-problem-id='#{ problem_id }']") + .data('content', latest_content) + XBlock.initializeBlocks(@content_container, @requestToken) window.update_schematics() # For embedded circuit simulator exercises in 6.002x @position = new_position @toggleArrows() + @hookUpContentStateChangeEvent() @hookUpProgressEvent() @updatePageTitle() diff --git a/common/test/acceptance/pages/lms/problem.py b/common/test/acceptance/pages/lms/problem.py index 988326b55c..284c4a76e8 100644 --- a/common/test/acceptance/pages/lms/problem.py +++ b/common/test/acceptance/pages/lms/problem.py @@ -29,6 +29,13 @@ class ProblemPage(PageObject): """ return self.q(css="div.problem p").text + @property + def problem_content(self): + """ + Return the content of the problem + """ + return self.q(css="div.problems-wrapper").text[0] + @property def message_text(self): """ @@ -103,17 +110,42 @@ class ProblemPage(PageObject): def click_check(self): """ - Click the Check button! + Click the Check button. """ self.q(css='div.problem button.check').click() self.wait_for_ajax() + def click_save(self): + """ + Click the Save button. + """ + self.q(css='div.problem button.save').click() + self.wait_for_ajax() + + def click_reset(self): + """ + Click the Reset button. + """ + self.q(css='div.problem button.reset').click() + self.wait_for_ajax() + def wait_for_status_icon(self): """ wait for status icon """ self.wait_for_element_visibility('div.problem section.inputtype div .status', 'wait for status icon') + def wait_for_expected_status(self, status_selector, message): + """ + Waits for the expected status indicator. + + Args: + status_selector(str): status selector string. + message(str): description of promise, to be logged. + """ + msg = "Wait for status to be {}".format(message) + self.wait_for_element_visibility(status_selector, msg) + def click_hint(self): """ Click the Hint button. diff --git a/common/test/acceptance/tests/lms/test_lms_courseware.py b/common/test/acceptance/tests/lms/test_lms_courseware.py index 0f1456fcbe..4f551f378f 100644 --- a/common/test/acceptance/tests/lms/test_lms_courseware.py +++ b/common/test/acceptance/tests/lms/test_lms_courseware.py @@ -5,6 +5,7 @@ End-to-end tests for the LMS. from nose.plugins.attrib import attr from ..helpers import UniqueCourseTest +from capa.tests.response_xml_factory import MultipleChoiceResponseXMLFactory from ...pages.studio.auto_auth import AutoAuthPage from ...pages.lms.create_mode import ModeCreationPage from ...pages.studio.overview import CourseOutlinePage @@ -542,3 +543,167 @@ class CoursewareMultipleVerticalsTest(UniqueCourseTest): self.courseware_page.a11y_audit.config.set_scope( include=['div.sequence-nav']) self.courseware_page.a11y_audit.check_for_accessibility_errors() + + +class ProblemStateOnNavigationTest(UniqueCourseTest): + """ + Test courseware with problems in multiple verticals + """ + USERNAME = "STUDENT_TESTER" + EMAIL = "student101@example.com" + + problem1_name = 'MULTIPLE CHOICE TEST PROBLEM 1' + problem2_name = 'MULTIPLE CHOICE TEST PROBLEM 2' + + def setUp(self): + super(ProblemStateOnNavigationTest, self).setUp() + + self.courseware_page = CoursewarePage(self.browser, self.course_id) + + # Install a course with section, tabs and multiple choice problems. + course_fix = CourseFixture( + self.course_info['org'], self.course_info['number'], + self.course_info['run'], self.course_info['display_name'] + ) + + course_fix.add_children( + XBlockFixtureDesc('chapter', 'Test Section 1').add_children( + XBlockFixtureDesc('sequential', 'Test Subsection 1,1').add_children( + self.create_multiple_choice_problem(self.problem1_name), + self.create_multiple_choice_problem(self.problem2_name), + ), + ), + ).install() + + # Auto-auth register for the course. + AutoAuthPage( + self.browser, username=self.USERNAME, email=self.EMAIL, + course_id=self.course_id, staff=False + ).visit() + + self.courseware_page.visit() + self.problem_page = ProblemPage(self.browser) + + def create_multiple_choice_problem(self, problem_name): + """ + Return the Multiple Choice Problem Descriptor, given the name of the problem. + """ + factory = MultipleChoiceResponseXMLFactory() + xml_data = factory.build_xml( + question_text='The correct answer is Choice 2', + choices=[False, False, True, False], + choice_names=['choice_0', 'choice_1', 'choice_2', 'choice_3'] + ) + + return XBlockFixtureDesc( + 'problem', + problem_name, + data=xml_data, + metadata={'rerandomize': 'always'} + ) + + def go_to_tab_and_assert_problem(self, position, problem_name): + """ + Go to sequential tab and assert that we are on problem whose name is given as a parameter. + Args: + position: Position of the sequential tab + problem_name: Name of the problem + """ + self.courseware_page.go_to_sequential_position(position) + self.problem_page.wait_for_element_presence( + self.problem_page.CSS_PROBLEM_HEADER, + 'wait for problem header' + ) + self.assertEqual(self.problem_page.problem_name, problem_name) + + def test_perform_problem_check_and_navigate(self): + """ + Scenario: + I go to sequential position 1 + Facing problem1, I select 'choice_1' + Then I click check button + Then I go to sequential position 2 + Then I came back to sequential position 1 again + Facing problem1, I observe the problem1 content is not + outdated before and after sequence navigation + """ + # Go to sequential position 1 and assert that we are on problem 1. + self.go_to_tab_and_assert_problem(1, self.problem1_name) + + # Update problem 1's content state by clicking check button. + self.problem_page.click_choice('choice_choice_1') + self.problem_page.click_check() + self.problem_page.wait_for_expected_status('label.choicegroup_incorrect', 'incorrect') + + # Save problem 1's content state as we're about to switch units in the sequence. + problem1_content_before_switch = self.problem_page.problem_content + + # Go to sequential position 2 and assert that we are on problem 2. + self.go_to_tab_and_assert_problem(2, self.problem2_name) + + # Come back to our original unit in the sequence and assert that the content hasn't changed. + self.go_to_tab_and_assert_problem(1, self.problem1_name) + problem1_content_after_coming_back = self.problem_page.problem_content + self.assertEqual(problem1_content_before_switch, problem1_content_after_coming_back) + + def test_perform_problem_save_and_navigate(self): + """ + Scenario: + I go to sequential position 1 + Facing problem1, I select 'choice_1' + Then I click save button + Then I go to sequential position 2 + Then I came back to sequential position 1 again + Facing problem1, I observe the problem1 content is not + outdated before and after sequence navigation + """ + # Go to sequential position 1 and assert that we are on problem 1. + self.go_to_tab_and_assert_problem(1, self.problem1_name) + + # Update problem 1's content state by clicking save button. + self.problem_page.click_choice('choice_choice_1') + self.problem_page.click_save() + self.problem_page.wait_for_expected_status('div.capa_alert', 'saved') + + # Save problem 1's content state as we're about to switch units in the sequence. + problem1_content_before_switch = self.problem_page.problem_content + + # Go to sequential position 2 and assert that we are on problem 2. + self.go_to_tab_and_assert_problem(2, self.problem2_name) + + # Come back to our original unit in the sequence and assert that the content hasn't changed. + self.go_to_tab_and_assert_problem(1, self.problem1_name) + problem1_content_after_coming_back = self.problem_page.problem_content + self.assertIn(problem1_content_after_coming_back, problem1_content_before_switch) + + def test_perform_problem_reset_and_navigate(self): + """ + Scenario: + I go to sequential position 1 + Facing problem1, I select 'choice_1' + Then perform the action – check and reset + Then I go to sequential position 2 + Then I came back to sequential position 1 again + Facing problem1, I observe the problem1 content is not + outdated before and after sequence navigation + """ + # Go to sequential position 1 and assert that we are on problem 1. + self.go_to_tab_and_assert_problem(1, self.problem1_name) + + # Update problem 1's content state – by performing reset operation. + self.problem_page.click_choice('choice_choice_1') + self.problem_page.click_check() + self.problem_page.wait_for_expected_status('label.choicegroup_incorrect', 'incorrect') + self.problem_page.click_reset() + self.problem_page.wait_for_expected_status('span.unanswered', 'unanswered') + + # Save problem 1's content state as we're about to switch units in the sequence. + problem1_content_before_switch = self.problem_page.problem_content + + # Go to sequential position 2 and assert that we are on problem 2. + self.go_to_tab_and_assert_problem(2, self.problem2_name) + + # Come back to our original unit in the sequence and assert that the content hasn't changed. + self.go_to_tab_and_assert_problem(1, self.problem1_name) + problem1_content_after_coming_back = self.problem_page.problem_content + self.assertEqual(problem1_content_before_switch, problem1_content_after_coming_back)