From ea81a9133023cf2842d490fc20f3d10f6443173a Mon Sep 17 00:00:00 2001 From: "E. Kolpakov" Date: Tue, 7 Apr 2015 16:42:43 +0300 Subject: [PATCH 01/21] Removed custom deserialization code from questionnaire, tweaked tip deserialization to rely on field parser rather than custom parsing Fixed xml test fixtures --- problem_builder/questionnaire.py | 25 ------------------- .../tests/integration/test_dashboard.py | 16 ++++++------ .../tests/integration/xml/assessment_1.xml | 24 +++++++++--------- .../tests/integration/xml/assessment_2.xml | 14 +++++------ .../tests/integration/xml/mcq_1.xml | 16 ++++++------ .../integration/xml/mcq_with_comments_1.xml | 10 ++++---- .../integration/xml/mcq_with_html_choices.xml | 10 ++++---- .../integration/xml/mrq_with_html_choices.xml | 10 ++++---- .../tests/integration/xml/theme_1.xml | 8 +++--- problem_builder/tip.py | 2 +- problem_builder/v1/tests/test_upgrade.py | 3 +-- .../v1/tests/xml/v1_upgrade_a_new.xml | 24 +++++++++--------- .../v1/tests/xml/v1_upgrade_b_new.xml | 24 +++++++++--------- problem_builder/v1/xml_changes.py | 18 +++++++++++++ 14 files changed, 98 insertions(+), 106 deletions(-) diff --git a/problem_builder/questionnaire.py b/problem_builder/questionnaire.py index f4f36f84..513fa1b0 100644 --- a/problem_builder/questionnaire.py +++ b/problem_builder/questionnaire.py @@ -88,31 +88,6 @@ def _(self, text): """ translate text """ return self.runtime.service(self, "i18n").ugettext(text) - @classmethod - def parse_xml(cls, node, runtime, keys, id_generator): - """ - Custom XML parser that can handle list type fields properly, - as well as the old way of defining 'question' and 'message' field values via tags. - """ - block = runtime.construct_xblock_from_class(cls, keys) - - # Load XBlock properties from the XML attributes: - for name, value in node.items(): - field = block.fields[name] - if isinstance(field, List) and not value.startswith('['): - # This list attribute is just a string of comma separated strings: - setattr(block, name, [unicode(val).strip() for val in value.split(',')]) - elif isinstance(field, String): - setattr(block, name, value) - else: - setattr(block, name, field.from_json(value)) - - for xml_child in node: - if xml_child.tag is not etree.Comment: - block.runtime.add_node_as_child(block, xml_child, id_generator) - - return block - def student_view(self, context=None): name = getattr(self, "unmixed_class", self.__class__).__name__ diff --git a/problem_builder/tests/integration/test_dashboard.py b/problem_builder/tests/integration/test_dashboard.py index a1bc9044..704ebd88 100644 --- a/problem_builder/tests/integration/test_dashboard.py +++ b/problem_builder/tests/integration/test_dashboard.py @@ -60,19 +60,19 @@ def setUp(self): self.set_scenario_xml(""" - + Option 1 Option 2 Option 3 Option 4 - + Option 1 Option 2 Option 3 Option 4 - + Option 1 Option 2 Option 3 @@ -81,18 +81,18 @@ def setUp(self): This message here should be ignored. - + Option 4 Option 5 Option 6 - + Option 1 Option 2 Option 3 Option 4 - + Option 1 Option 2 Option 3 @@ -100,14 +100,14 @@ def setUp(self): - + Option 1 Option 2 Option 3 Option 4 + question="Which option?" correct_choices='["1","2","3","4"]'> Option A Option B Option C diff --git a/problem_builder/tests/integration/xml/assessment_1.xml b/problem_builder/tests/integration/xml/assessment_1.xml index e3806062..77bb7ec3 100644 --- a/problem_builder/tests/integration/xml/assessment_1.xml +++ b/problem_builder/tests/integration/xml/assessment_1.xml @@ -14,32 +14,32 @@ - + Yes Maybe not I don't understand - Great! - Ah, damn. -
Really?
 + Great! + Ah, damn. +
Really?
 - + I don't want to rate it - I love good grades. - Will do better next time... - Your loss! + I love good grades. + Will do better next time... + Your loss! - + Its elegance Its beauty Its gracefulness Its bugs - This MRQ is indeed very graceful - This is something everyone has to like about this MRQ - Nah, there isn't any! + This MRQ is indeed very graceful + This is something everyone has to like about this MRQ + Nah, there isn't any! diff --git a/problem_builder/tests/integration/xml/assessment_2.xml b/problem_builder/tests/integration/xml/assessment_2.xml index c7543e4e..dcffb26d 100644 --- a/problem_builder/tests/integration/xml/assessment_2.xml +++ b/problem_builder/tests/integration/xml/assessment_2.xml @@ -4,13 +4,13 @@

Please answer the questions below.

- - Yes - Maybe not - I don't understand + + Yes + Maybe not + I don't understand - Great! - Ah, damn. -
Really?
 + Great! + Ah, damn. +
Really?
 diff --git a/problem_builder/tests/integration/xml/mcq_1.xml b/problem_builder/tests/integration/xml/mcq_1.xml index 0d73fe4d..47293979 100644 --- a/problem_builder/tests/integration/xml/mcq_1.xml +++ b/problem_builder/tests/integration/xml/mcq_1.xml @@ -1,21 +1,21 @@ - + Yes Maybe not I don't understand - Great! - Ah, damn. -
Really?
 + Great! + Ah, damn. +
Really?
 - + I don't want to rate it - I love good grades. - Will do better next time... - Your loss! + I love good grades. + Will do better next time... + Your loss! diff --git a/problem_builder/tests/integration/xml/mcq_with_comments_1.xml b/problem_builder/tests/integration/xml/mcq_with_comments_1.xml index 32540f16..6f119c09 100644 --- a/problem_builder/tests/integration/xml/mcq_with_comments_1.xml +++ b/problem_builder/tests/integration/xml/mcq_with_comments_1.xml @@ -1,15 +1,15 @@ - + Its elegance Its beauty Its gracefulness Its bugs - This MRQ is indeed very graceful - This is something everyone has to like about this MRQ - This is something everyone has to like about beauty - Nah, there isn\'t any! + This MRQ is indeed very graceful + This is something everyone has to like about this MRQ + This is something everyone has to like about beauty + Nah, there isn\'t any! diff --git a/problem_builder/tests/integration/xml/mcq_with_html_choices.xml b/problem_builder/tests/integration/xml/mcq_with_html_choices.xml index 2a0c6112..f83de791 100644 --- a/problem_builder/tests/integration/xml/mcq_with_html_choices.xml +++ b/problem_builder/tests/integration/xml/mcq_with_html_choices.xml @@ -1,15 +1,15 @@ - + Its elegance Its beauty Its gracefulness Its bugs - This MCQ is indeed very graceful - This is something everyone has to like about this MCQ - This is something everyone has to like about beauty - Nah, there isn\'t any! + This MCQ is indeed very graceful + This is something everyone has to like about this MCQ + This is something everyone has to like about beauty + Nah, there isn\'t any! diff --git a/problem_builder/tests/integration/xml/mrq_with_html_choices.xml b/problem_builder/tests/integration/xml/mrq_with_html_choices.xml index ffcb95ee..4d234e45 100644 --- a/problem_builder/tests/integration/xml/mrq_with_html_choices.xml +++ b/problem_builder/tests/integration/xml/mrq_with_html_choices.xml @@ -1,15 +1,15 @@ - + Its elegance Its beauty Its gracefulness Its bugs - This MRQ is indeed very graceful - This is something everyone has to like about this MRQ - This is something everyone has to like about beauty - Nah, there aren\'t any! + This MRQ is indeed very graceful + This is something everyone has to like about this MRQ + This is something everyone has to like about beauty + Nah, there aren\'t any! diff --git a/problem_builder/tests/integration/xml/theme_1.xml b/problem_builder/tests/integration/xml/theme_1.xml index c7543e4e..2872d862 100644 --- a/problem_builder/tests/integration/xml/theme_1.xml +++ b/problem_builder/tests/integration/xml/theme_1.xml @@ -4,13 +4,13 @@

Please answer the questions below.

- + Yes Maybe not I don't understand - Great! - Ah, damn. -
Really?
 + Great! + Ah, damn. +
Really?
 diff --git a/problem_builder/tip.py b/problem_builder/tip.py index 6d32671d..5c861991 100644 --- a/problem_builder/tip.py +++ b/problem_builder/tip.py @@ -132,7 +132,7 @@ def parse_xml(cls, node, runtime, keys, id_generator): """ block = runtime.construct_xblock_from_class(cls, keys) - block.values = [unicode(val).strip() for val in node.get('values', '').split(',')] + block.values = cls.values.from_string(node.get('values', '[]')) block.width = node.get('width', '') block.height = node.get('height', '') diff --git a/problem_builder/v1/tests/test_upgrade.py b/problem_builder/v1/tests/test_upgrade.py index bc2143f2..828a9f71 100644 --- a/problem_builder/v1/tests/test_upgrade.py +++ b/problem_builder/v1/tests/test_upgrade.py @@ -57,8 +57,7 @@ def setUp(self): @XBlock.register_temp_plugin(MentoringBlock, "mentoring") def test_xml_upgrade(self, file_name): """ - Convert a v1 mentoring block to v2 and then compare the resulting block to a - pre-converted one. + Convert a v1 mentoring block to v2 and then compare the resulting block to a pre-converted one. """ with open("{}/{}_old.xml".format(xml_path, file_name)) as xmlfile: temp_node = etree.parse(xmlfile).getroot() diff --git a/problem_builder/v1/tests/xml/v1_upgrade_a_new.xml b/problem_builder/v1/tests/xml/v1_upgrade_a_new.xml index a33eaa28..7dd3e2bf 100644 --- a/problem_builder/v1/tests/xml/v1_upgrade_a_new.xml +++ b/problem_builder/v1/tests/xml/v1_upgrade_a_new.xml @@ -6,30 +6,30 @@

Please answer the questions below.

- + Yes Maybe not I don't understand - Great! - Ah, damn. - + Great! + Ah, damn. +
Really?
 - + I don't want to rate it - I love good grades. - Will do better next time... - Your loss! + I love good grades. + Will do better next time... + Your loss! - + Its elegance Its beauty Its gracefulness Its bugs - This MRQ is indeed very graceful - This is something everyone has to like about this MRQ - Nah, there isn't any! + This MRQ is indeed very graceful + This is something everyone has to like about this MRQ + Nah, there isn't any!

Congratulations!

diff --git a/problem_builder/v1/tests/xml/v1_upgrade_b_new.xml b/problem_builder/v1/tests/xml/v1_upgrade_b_new.xml index 1c571895..2d3674ad 100644 --- a/problem_builder/v1/tests/xml/v1_upgrade_b_new.xml +++ b/problem_builder/v1/tests/xml/v1_upgrade_b_new.xml @@ -4,31 +4,31 @@

Now, let's make sure your frog meets the criteria for a strong column 1. Here is your frog:

- + Yes Maybe not I don't understand - Great. Your frog should be happy for you. - In the end, all the feedback you have gotten from others should not lead you to choose a frog that does not also feel happy and important to you. - + Great. Your frog should be happy for you. + In the end, all the feedback you have gotten from others should not lead you to choose a frog that does not also feel happy and important to you. +

If a frog is happy for you, that means it is a frog that you genuinely feel in your own heart to be something that you want to improve. What is in your heart?

 - + Yes Maybe not I don't understand - Great. Your frog should implicate you. - + Great. Your frog should implicate you. +

Since the Trial of Uruk-Shan focuses on your own growth and change, it is important to be clear about the ways you are hoping to change and improve.

 - Your frog implicates you if it is clear that you must get better at something. Your frog should focus on something you can control. + Your frog implicates you if it is clear that you must get better at something. Your frog should focus on something you can control. - + I don't understand - Great! - The Trial of Uruk-Shan helps you uncover some of the core beliefs and assumptions you have held that are preventing you from making change. - A frog is important if it is one that could make a big difference in helping you reach your frogs in your work life or your personal life (or both). + Great! + The Trial of Uruk-Shan helps you uncover some of the core beliefs and assumptions you have held that are preventing you from making change. + A frog is important if it is one that could make a big difference in helping you reach your frogs in your work life or your personal life (or both). Great! You have indicated that you have chosen a frog that is happy for you, implicates you, has room for improvement, and is important to you. You are now ready to move onto the next step. diff --git a/problem_builder/v1/xml_changes.py b/problem_builder/v1/xml_changes.py index 43c6e5e1..2427d08a 100644 --- a/problem_builder/v1/xml_changes.py +++ b/problem_builder/v1/xml_changes.py @@ -21,6 +21,7 @@ Each class in this file represents a change made to the XML schema between v1 and v2. """ from lxml import etree +import json import warnings @@ -292,6 +293,22 @@ def apply(self): self.node.tag = "html" +class CommaSeparatedListToJson(Change): + APPLY_TO_ATTRIBUTES = ("values", "correct_choices", "required_choices", "ignored_choices") + + def _convert_value(self, raw_value): + return json.dumps([unicode(val).strip() for val in raw_value.split(',')]) + + @staticmethod + def applies_to(node): + return node.tag in ("pb-tip", "pb-mrq", "pb-mcq", "pb-rating") + + def apply(self): + for attribute in self.APPLY_TO_ATTRIBUTES: + if attribute in self.node.attrib: + self.node.attrib[attribute] = self._convert_value(self.node.attrib[attribute]) + + # An *ordered* list of all XML schema changes: xml_changes = ( RenameMentoringTag, @@ -307,6 +324,7 @@ def apply(self): QuestionSubmitMessageToField, TipChanges, SharedHeaderToHTML, + CommaSeparatedListToJson, ) From 45ca8c774bb3240631d15c8761b395dd7da6be97 Mon Sep 17 00:00:00 2001 From: "E. Kolpakov" Date: Tue, 7 Apr 2015 16:42:58 +0300 Subject: [PATCH 02/21] Added explicit test_requirements and included them in Travis build --- .travis.yml | 1 + test_requirements.txt | 1 + 2 files changed, 2 insertions(+) create mode 100644 test_requirements.txt diff --git a/.travis.yml b/.travis.yml index 31de016f..df5fc10e 100644 --- a/.travis.yml +++ b/.travis.yml @@ -10,6 +10,7 @@ install: - "pip install -r $VIRTUAL_ENV/src/xblock-sdk/test-requirements.txt" - "pip install -r requirements.txt" - "pip uninstall -y xblock-problem-builder && python setup.py sdist && pip install dist/xblock-problem-builder-2.0.tar.gz" + - "pip install -r test_requirements.txt" script: - pep8 problem_builder --max-line-length=120 - pylint problem_builder --disable=all --enable=function-redefined,undefined-variable,unused-variable diff --git a/test_requirements.txt b/test_requirements.txt new file mode 100644 index 00000000..7b7a84e8 --- /dev/null +++ b/test_requirements.txt @@ -0,0 +1 @@ +-e git+https://github.com/edx/XBlock.git@496d3cb9aca1d9e0a18b0f5e73c7bede824e465f#egg=XBlock \ No newline at end of file From 552b5ba027a0481d352d22c4c169a60f199bf718 Mon Sep 17 00:00:00 2001 From: Jonathan Piacenti Date: Tue, 7 Apr 2015 20:54:10 +0000 Subject: [PATCH 03/21] Make travis work for all branches. --- .travis.yml | 3 --- 1 file changed, 3 deletions(-) diff --git a/.travis.yml b/.travis.yml index df5fc10e..bb9d34b7 100644 --- a/.travis.yml +++ b/.travis.yml @@ -17,6 +17,3 @@ script: - python run_tests.py --with-coverage --cover-package=problem_builder notifications: email: false -branches: - only: - - master From ea2875fb6be6f0d70a1a207307bc959f144dfc99 Mon Sep 17 00:00:00 2001 From: Braden MacDonald Date: Thu, 9 Apr 2015 15:32:58 -0700 Subject: [PATCH 04/21] Minor pylint fix --- .../tests/integration/test_dashboard.py | 16 ++++++++-------- 1 file changed, 8 insertions(+), 8 deletions(-) diff --git a/problem_builder/tests/integration/test_dashboard.py b/problem_builder/tests/integration/test_dashboard.py index 704ebd88..ffe8948d 100644 --- a/problem_builder/tests/integration/test_dashboard.py +++ b/problem_builder/tests/integration/test_dashboard.py @@ -60,19 +60,19 @@ def setUp(self): self.set_scenario_xml(""" - + Option 1 Option 2 Option 3 Option 4 - + Option 1 Option 2 Option 3 Option 4 - + Option 1 Option 2 Option 3 @@ -81,18 +81,18 @@ def setUp(self): This message here should be ignored. - + Option 4 Option 5 Option 6 - + Option 1 Option 2 Option 3 Option 4 - + Option 1 Option 2 Option 3 @@ -100,13 +100,13 @@ def setUp(self): - + Option 1 Option 2 Option 3 Option 4 - Option A Option B From 45072bbc661d9306fa751295e276827ab5fef3b7 Mon Sep 17 00:00:00 2001 From: Braden MacDonald Date: Thu, 9 Apr 2015 21:03:55 -0700 Subject: [PATCH 05/21] Fix: Question titles were not visible in the LMS --- problem_builder/questionnaire.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/problem_builder/questionnaire.py b/problem_builder/questionnaire.py index 513fa1b0..77bd7f97 100644 --- a/problem_builder/questionnaire.py +++ b/problem_builder/questionnaire.py @@ -93,7 +93,7 @@ def student_view(self, context=None): template_path = 'templates/html/{}.html'.format(name.lower()) - context = context or {} + context = context.copy() if context else {} context['self'] = self context['custom_choices'] = self.custom_choices context['hide_header'] = context.get('hide_header', False) or not self.show_title From 3d4bf295bd760526a97735102fb448ccb28889a9 Mon Sep 17 00:00:00 2001 From: Braden MacDonald Date: Thu, 9 Apr 2015 21:10:56 -0700 Subject: [PATCH 06/21] Fix: MCQ question text should be editable even if there are no choices defined yet --- problem_builder/mcq.py | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-) diff --git a/problem_builder/mcq.py b/problem_builder/mcq.py index 8782744a..9ba34775 100644 --- a/problem_builder/mcq.py +++ b/problem_builder/mcq.py @@ -133,9 +133,7 @@ def choice_name(choice_value): all_values = set(self.all_choice_values) correct = set(data.correct_choices) - if not all_values: - add_error(self._(u"No choices set yet.")) - elif not correct: + if all_values and not correct: add_error( self._(u"You must indicate the correct answer[s], or the student will always get this question wrong.") ) From 8bf362dd1edd7359393a812014213bbb8f079e95 Mon Sep 17 00:00:00 2001 From: Braden MacDonald Date: Thu, 9 Apr 2015 22:09:25 -0700 Subject: [PATCH 07/21] Fix: Can't retry if max_attempts set to zero after answering while it was nonzero --- problem_builder/public/js/mentoring_assessment_view.js | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-) diff --git a/problem_builder/public/js/mentoring_assessment_view.js b/problem_builder/public/js/mentoring_assessment_view.js index b6f9f0c6..7cc15ba7 100644 --- a/problem_builder/public/js/mentoring_assessment_view.js +++ b/problem_builder/public/js/mentoring_assessment_view.js @@ -33,10 +33,9 @@ function MentoringAssessmentView(runtime, element, mentoring) { tryAgainDOM.show(); var attempts_data = $('.attempts', element).data(); - if (attempts_data.num_attempts >= attempts_data.max_attempts) { + if (attempts_data.max_attempts > 0 && attempts_data.num_attempts >= attempts_data.max_attempts) { tryAgainDOM.attr("disabled", "disabled"); - } - else { + } else { tryAgainDOM.removeAttr("disabled"); } From 0d85a3fe174ab063aedeaa68b0d9a5b0e00e1ec4 Mon Sep 17 00:00:00 2001 From: Braden MacDonald Date: Thu, 9 Apr 2015 21:21:02 -0700 Subject: [PATCH 08/21] Fix: confusing behaviour of max_attempts when changing mode --- problem_builder/mentoring.py | 9 --------- 1 file changed, 9 deletions(-) diff --git a/problem_builder/mentoring.py b/problem_builder/mentoring.py index 5a72d0a1..78b1b103 100644 --- a/problem_builder/mentoring.py +++ b/problem_builder/mentoring.py @@ -505,15 +505,6 @@ def get_message_html(self, message_type): html += child.render('mentoring_view', {}).content # TODO: frament_text_rewriting ? return html - def clean_studio_edits(self, data): - """ - Given POST data dictionary 'data', clean the data before validating it. - e.g. fix capitalization, remove trailing spaces, etc. - """ - if data.get('mode') == 'assessment' and 'max_attempts' not in data: - # assessment has a default of 2 max_attempts - data['max_attempts'] = 2 - def validate(self): """ Validates the state of this XBlock except for individual field values. From dc4a43a71935fb727e0e001aebe656f1d1e5715e Mon Sep 17 00:00:00 2001 From: Braden MacDonald Date: Fri, 10 Apr 2015 13:00:38 -0700 Subject: [PATCH 09/21] Fix: spacing around question titles/text in Firefox --- problem_builder/public/css/mentoring.css | 13 +++++++++---- 1 file changed, 9 insertions(+), 4 deletions(-) diff --git a/problem_builder/public/css/mentoring.css b/problem_builder/public/css/mentoring.css index c8a63bba..4b97c44a 100644 --- a/problem_builder/public/css/mentoring.css +++ b/problem_builder/public/css/mentoring.css @@ -40,12 +40,13 @@ font-style: italic; } -.mentoring h4 { - margin-bottom: 20px; +.mentoring fieldset { + margin-top: 10px; } -.mentoring h4 { - margin-top: 25px; +.mentoring h3 { + margin-top: 0px; + margin-bottom: 7px; } .mentoring .submit { @@ -62,6 +63,10 @@ display: table; /* Enable line-wrapping in IE8 */ } +.mentoring .choices legend.question p:last-child { /* Selector must be more specific than 'div.course-wrapper section.course-content p' */ + margin-bottom: 0; +} + .mentoring .attempts { margin-left: 10px; display: inline-block; From 94cc894dfa237b4a1f2d0d1d37560edec3236fe0 Mon Sep 17 00:00:00 2001 From: Braden MacDonald Date: Thu, 23 Apr 2015 13:47:28 -0700 Subject: [PATCH 10/21] Hotfix: mentoring and problem_builder should not both use the same db table --- problem_builder/models.py | 3 +- .../south_migrations/0001_initial.py | 44 +++++++++++++++++++ problem_builder/south_migrations/__init__.py | 0 3 files changed, 46 insertions(+), 1 deletion(-) create mode 100644 problem_builder/south_migrations/0001_initial.py create mode 100644 problem_builder/south_migrations/__init__.py diff --git a/problem_builder/models.py b/problem_builder/models.py index 5b5e9fc0..5491995c 100644 --- a/problem_builder/models.py +++ b/problem_builder/models.py @@ -29,10 +29,11 @@ class Answer(models.Model): """ Django model used to store AnswerBlock data that need to be shared and queried accross XBlock instances (workaround). + + TODO: Deprecate this and move to edx-submissions """ class Meta: - db_table = 'mentoring_answer' unique_together = (('student_id', 'course_id', 'name'),) name = models.CharField(max_length=50, db_index=True) diff --git a/problem_builder/south_migrations/0001_initial.py b/problem_builder/south_migrations/0001_initial.py new file mode 100644 index 00000000..579aae31 --- /dev/null +++ b/problem_builder/south_migrations/0001_initial.py @@ -0,0 +1,44 @@ +# -*- coding: utf-8 -*- +from south.db import db +from south.v2 import SchemaMigration + + +class Migration(SchemaMigration): + + def forwards(self, orm): + # Adding model 'Answer' + db.create_table('problem_builder_answer', ( + ('id', self.gf('django.db.models.fields.AutoField')(primary_key=True)), + ('name', self.gf('django.db.models.fields.CharField')(max_length=50, db_index=True)), + ('student_id', self.gf('django.db.models.fields.CharField')(max_length=32, db_index=True)), + ('course_id', self.gf('django.db.models.fields.CharField')(max_length=50, db_index=True)), + ('student_input', self.gf('django.db.models.fields.TextField')(default='', blank=True)), + ('created_on', self.gf('django.db.models.fields.DateTimeField')(auto_now_add=True, blank=True)), + ('modified_on', self.gf('django.db.models.fields.DateTimeField')(auto_now=True, blank=True)), + )) + db.send_create_signal('problem_builder', ['Answer']) + + # Adding unique constraint on 'Answer', fields ['student_id', 'course_id', 'name'] + db.create_unique('problem_builder_answer', ['student_id', 'course_id', 'name']) + + def backwards(self, orm): + # Removing unique constraint on 'Answer', fields ['student_id', 'course_id', 'name'] + db.delete_unique('problem_builder_answer', ['student_id', 'course_id', 'name']) + + # Deleting model 'Answer' + db.delete_table('problem_builder_answer') + + models = { + 'problem_builder.answer': { + 'Meta': {'unique_together': "(('student_id', 'course_id', 'name'),)", 'object_name': 'Answer'}, + 'course_id': ('django.db.models.fields.CharField', [], {'max_length': '50', 'db_index': 'True'}), + 'created_on': ('django.db.models.fields.DateTimeField', [], {'auto_now_add': 'True', 'blank': 'True'}), + 'id': ('django.db.models.fields.AutoField', [], {'primary_key': 'True'}), + 'modified_on': ('django.db.models.fields.DateTimeField', [], {'auto_now': 'True', 'blank': 'True'}), + 'name': ('django.db.models.fields.CharField', [], {'max_length': '50', 'db_index': 'True'}), + 'student_id': ('django.db.models.fields.CharField', [], {'max_length': '32', 'db_index': 'True'}), + 'student_input': ('django.db.models.fields.TextField', [], {'default': "''", 'blank': 'True'}) + } + } + + complete_apps = ['problem_builder'] diff --git a/problem_builder/south_migrations/__init__.py b/problem_builder/south_migrations/__init__.py new file mode 100644 index 00000000..e69de29b From 7bb7d2b910a166594fd7bec4844cb4180f0d195a Mon Sep 17 00:00:00 2001 From: Braden MacDonald Date: Thu, 23 Apr 2015 15:01:28 -0700 Subject: [PATCH 11/21] Data migration --- .../0002_copy_from_mentoring.py | 46 +++++++++++++++++++ 1 file changed, 46 insertions(+) create mode 100644 problem_builder/south_migrations/0002_copy_from_mentoring.py diff --git a/problem_builder/south_migrations/0002_copy_from_mentoring.py b/problem_builder/south_migrations/0002_copy_from_mentoring.py new file mode 100644 index 00000000..014c73d1 --- /dev/null +++ b/problem_builder/south_migrations/0002_copy_from_mentoring.py @@ -0,0 +1,46 @@ +# -*- coding: utf-8 -*- +from django.db.utils import DatabaseError +from south.db import db +from south.v2 import DataMigration + + +class Migration(DataMigration): + + def forwards(self, orm): + """ + Copy student data from old table to the new one. + + Problem Builder stores student answers in 'problem_builder_answer'. + However earlier versions [may have] used 'mentoring_answer'. + If a 'mentoring' app is currently installed on this instance, copy the student data over + to the new table in case it is being used. + """ + try: + db.execute( + 'INSERT INTO problem_builder_answer (' + 'name, student_id, course_id, student_input, created_on, modified_on ' + ') SELECT ' + 'name, student_id, course_id, student_input, created_on, modified_on ' + 'FROM mentoring_answer' + ) + except DatabaseError: # Would like to just catch 'Table does not exist' but can't do that in a db-agnostic way + print(" - Seems like mentoring_answer does not exist. No data migration needed.") + + def backwards(self, orm): + raise RuntimeError("Cannot safely reverse this migration.") + + models = { + 'problem_builder.answer': { + 'Meta': {'unique_together': "(('student_id', 'course_id', 'name'),)", 'object_name': 'Answer'}, + 'course_id': ('django.db.models.fields.CharField', [], {'max_length': '50', 'db_index': 'True'}), + 'created_on': ('django.db.models.fields.DateTimeField', [], {'auto_now_add': 'True', 'blank': 'True'}), + 'id': ('django.db.models.fields.AutoField', [], {'primary_key': 'True'}), + 'modified_on': ('django.db.models.fields.DateTimeField', [], {'auto_now': 'True', 'blank': 'True'}), + 'name': ('django.db.models.fields.CharField', [], {'max_length': '50', 'db_index': 'True'}), + 'student_id': ('django.db.models.fields.CharField', [], {'max_length': '32', 'db_index': 'True'}), + 'student_input': ('django.db.models.fields.TextField', [], {'default': "''", 'blank': 'True'}) + } + } + + complete_apps = ['problem_builder'] + symmetrical = True From 0cf055d5463213b42da807c70084f13c06537b16 Mon Sep 17 00:00:00 2001 From: "E. Kolpakov" Date: Tue, 21 Apr 2015 16:34:49 +0300 Subject: [PATCH 12/21] Presentation updates: * Horizontally centering visual progress * Ability to override "Average" label * Added ability to hide numbers in dashboard * Ability to exclude specific questions from dashboard * Slightly improved a11y (aka accessibility) --- problem_builder/dashboard.py | 92 +++++++-- problem_builder/public/css/dashboard.css | 4 + problem_builder/templates/html/dashboard.html | 24 ++- .../tests/integration/test_dashboard.py | 192 ++++++++++++------ .../tests/integration/xml/dashboard.xml | 57 ++++++ 5 files changed, 281 insertions(+), 88 deletions(-) create mode 100644 problem_builder/tests/integration/xml/dashboard.xml diff --git a/problem_builder/dashboard.py b/problem_builder/dashboard.py index 57c36bbd..5526b250 100644 --- a/problem_builder/dashboard.py +++ b/problem_builder/dashboard.py @@ -36,7 +36,7 @@ from .sub_api import sub_api from lazy import lazy from xblock.core import XBlock -from xblock.fields import Scope, List, String +from xblock.fields import Scope, List, String, Boolean, Dict from xblock.fragment import Fragment from xblock.validation import ValidationMessage from xblockutils.helpers import child_isinstance @@ -172,6 +172,20 @@ class DashboardBlock(StudioEditableXBlockMixin, XBlock): ).format(example_here='["2754b8afc03a439693b9887b6f1d9e36", "215028f7df3d4c68b14fb5fea4da7053"]'), scope=Scope.settings, ) + exclude_questions = Dict( + display_name=_("Questions to be hidden"), + help=_( + "Optional rules to exclude specific questions both from displaying in dashboard and from the calculated " + "average. Rules must start with the url_name of a mentoring block, followed by list of question numbers " + "to exclude. Rule set must be in JSON format. Question numbers are one-based (the first question being " + "number 1). Must be in JSON format. Examples: {examples_here}" + ).format( + examples_here='{"2754b8afc03a439693b9887b6f1d9e36":[1,2], "215028f7df3d4c68b14fb5fea4da7053":[1,5]}' + ), + scope=Scope.content, + multiline_editor=True, + resettable_editor=False, + ) color_rules = String( display_name=_("Color Coding Rules"), help=_( @@ -207,8 +221,23 @@ class DashboardBlock(StudioEditableXBlockMixin, XBlock): ), scope=Scope.content, ) + average_label = String( + display_name=_("Label for average value"), + default=_("Average"), + help=_("Label to be shown for calculated average"), + scope=Scope.content, + ) + show_numbers = Boolean( + display_name=_("Display values"), + default=True, + help=_("Toggles if numeric values are displayed"), + scope=Scope.content + ) - editable_fields = ('display_name', 'mentoring_ids', 'color_rules', 'visual_rules', 'visual_title', 'visual_desc') + editable_fields = ( + 'display_name', 'mentoring_ids', 'exclude_questions', 'average_label', 'show_numbers', + 'color_rules', 'visual_rules', 'visual_title', 'visual_desc' + ) css_path = 'public/css/dashboard.css' js_path = 'public/js/dashboard.js' @@ -321,6 +350,12 @@ def _get_course_name(self): except Exception: return "" + def _get_problem_questions(self, mentoring_block): + """ Generator returning only children of specified block that are MCQs """ + for child_id in mentoring_block.children: + if child_isinstance(mentoring_block, child_id, MCQBlock): + yield child_id + def student_view(self, context=None): # pylint: disable=unused-argument """ Standard view of this XBlock. @@ -336,20 +371,35 @@ def student_view(self, context=None): # pylint: disable=unused-argument 'display_name': mentoring_block.display_name, 'mcqs': [] } - for child_id in mentoring_block.children: - if child_isinstance(mentoring_block, child_id, MCQBlock): - # Get the student's submitted answer to this MCQ from the submissions API: - mcq_block = self.runtime.get_block(child_id) - mcq_submission_key = self._get_submission_key(child_id) - try: - value = sub_api.get_submissions(mcq_submission_key, limit=1)[0]["answer"] - except IndexError: - value = None - block['mcqs'].append({ - "display_name": mcq_block.display_name_with_default, - "value": value, - "color": self.color_for_value(value) if value is not None else None, - }) + try: + hide_questions = self.exclude_questions.get(mentoring_block.url_name, []) + except Exception: # pylint: disable=broad-except-clause + log.exception("Cannot parse exclude_questions setting - probably malformed: %s", self.exclude_questions) + hide_questions = [] + + for question_number, child_id in enumerate(self._get_problem_questions(mentoring_block), 1): + try: + if question_number in hide_questions: + continue + except TypeError: + log.exception( + "Cannot check question number - expected list of ints got: %s", + hide_questions + ) + + # Get the student's submitted answer to this MCQ from the submissions API: + mcq_block = self.runtime.get_block(child_id) + mcq_submission_key = self._get_submission_key(child_id) + try: + value = sub_api.get_submissions(mcq_submission_key, limit=1)[0]["answer"] + except IndexError: + value = None + + block['mcqs'].append({ + "display_name": mcq_block.display_name_with_default, + "value": value, + "color": self.color_for_value(value) if value is not None else None, + }) # If the values are numeric, display an average: numeric_values = [ float(mcq['value']) for mcq in block['mcqs'] @@ -384,6 +434,8 @@ def student_view(self, context=None): # pylint: disable=unused-argument 'blocks': blocks, 'display_name': self.display_name, 'visual_repr': visual_repr, + 'average_label': self.average_label, + 'show_numbers': self.show_numbers, }) fragment = Fragment(html) @@ -406,6 +458,14 @@ def add_error(msg): except InvalidUrlName as e: add_error(_(u'Invalid block url_name given: "{bad_url_name}"').format(bad_url_name=unicode(e))) + if data.exclude_questions: + for key, value in data.exclude_questions.iteritems(): + if not isinstance(value, list): + add_error( + _(u"Exclude questions is malformed: value for key {key} is {value}, expected list of integers") + .format(key=key, value=value) + ) + if data.color_rules: try: self.parse_color_rules_str(data.color_rules, ignore_errors=False) diff --git a/problem_builder/public/css/dashboard.css b/problem_builder/public/css/dashboard.css index ba6b068e..249b5355 100644 --- a/problem_builder/public/css/dashboard.css +++ b/problem_builder/public/css/dashboard.css @@ -30,3 +30,7 @@ .pb-dashboard table .avg-row td.desc { font-style: italic; } + +.pb-dashboard-visual { + text-align: center; +} diff --git a/problem_builder/templates/html/dashboard.html b/problem_builder/templates/html/dashboard.html index 6640176c..bfee6b7d 100644 --- a/problem_builder/templates/html/dashboard.html +++ b/problem_builder/templates/html/dashboard.html @@ -42,16 +42,30 @@

{{display_name}}

{% for mcq in block.mcqs %} {{ mcq.display_name }} - - {% if mcq.value %}{{ mcq.value }}{% endif %} + + {% if mcq.value and show_numbers %} + {{ mcq.value }} + {% endif %} {% endfor %} {% if block.has_average %} - {% trans "Average" %} - - {{ block.average|floatformat }} + {{ average_label }} + + {% if show_numbers %} + {{ block.average|floatformat }} + {% endif %} {% endif %} diff --git a/problem_builder/tests/integration/test_dashboard.py b/problem_builder/tests/integration/test_dashboard.py index ffe8948d..bd772011 100644 --- a/problem_builder/tests/integration/test_dashboard.py +++ b/problem_builder/tests/integration/test_dashboard.py @@ -17,8 +17,10 @@ # along with this program in a file in the toplevel directory called # "AGPLv3". If not, see . # +from textwrap import dedent from mock import Mock, patch from xblockutils.base_test import SeleniumXBlockTest +from xblockutils.resources import ResourceLoader class MockSubmissionsAPI(object): @@ -54,73 +56,28 @@ class TestDashboardBlock(SeleniumXBlockTest): """ Test the Student View of a dashboard XBlock linked to some problem builder blocks """ + SIMPLE_DASHBOARD = """""" + ALTERNATIVE_DASHBOARD = dedent(""" + + """) + HIDE_QUESTIONS_DASHBOARD = dedent(""" + + """) + MALFORMED_HIDE_QUESTIONS_DASHBOARD = dedent(""" + + """) + def setUp(self): super(TestDashboardBlock, self).setUp() - # Set up our scenario: - self.set_scenario_xml(""" - - - - Option 1 - Option 2 - Option 3 - Option 4 - - - Option 1 - Option 2 - Option 3 - Option 4 - - - Option 1 - Option 2 - Option 3 - Option 4 - - This message here should be ignored. - - - - Option 4 - Option 5 - Option 6 - - - Option 1 - Option 2 - Option 3 - Option 4 - - - Option 1 - Option 2 - Option 3 - Option 4 - - - - - Option 1 - Option 2 - Option 3 - Option 4 - - - Option A - Option B - Option C - - - - - - """) # Apply a whole bunch of patches that are needed in lieu of the LMS/CMS runtime and edx-submissions: def get_mentoring_blocks(dashboard_block, mentoring_ids, ignore_errors=True): return [dashboard_block.runtime.get_block(key) for key in dashboard_block.get_parent().children[:-1]] + mock_submisisons_api = MockSubmissionsAPI() patches = ( ( @@ -135,13 +92,21 @@ def get_mentoring_blocks(dashboard_block, mentoring_ids, ignore_errors=True): ), ("problem_builder.dashboard.DashboardBlock.get_mentoring_blocks", get_mentoring_blocks), ("problem_builder.dashboard.sub_api", mock_submisisons_api), - ("problem_builder.mcq.sub_api", mock_submisisons_api) + ("problem_builder.mcq.sub_api", mock_submisisons_api), + ( + "problem_builder.mentoring.MentoringBlock.url_name", + property(lambda block: block.display_name) + ) ) for p in patches: patcher = patch(*p) patcher.start() self.addCleanup(patcher.stop) - # All the patches are installed; now we can proceed with using the XBlocks for tests: + + def _install_fixture(self, dashboard_xml): + loader = ResourceLoader(self.__module__) + scenario = loader.render_template("xml/dashboard.xml", {'dashboard': dashboard_xml}) + self.set_scenario_xml(scenario) self.go_to_view("student_view") self.vertical = self.load_root_xblock() @@ -150,6 +115,7 @@ def test_empty_dashboard(self): Test that when the student has not submitted any question answers, we still see the dashboard, and its lists all the MCQ questions in the way we expect. """ + self._install_fixture(self.SIMPLE_DASHBOARD) dashboard = self.browser.find_element_by_css_selector('.pb-dashboard') step_headers = dashboard.find_elements_by_css_selector('thead') self.assertEqual(len(step_headers), 3) @@ -164,10 +130,7 @@ def test_empty_dashboard(self): value = mcq.find_element_by_css_selector('td:last-child') self.assertEqual(value.text, '') - def test_dashboard(self): - """ - Submit an answer to each MCQ, then check that the dashboard reflects those answers. - """ + def _set_mentoring_values(self): pbs = self.browser.find_elements_by_css_selector('.mentoring') for pb in pbs: mcqs = pb.find_elements_by_css_selector('fieldset.choices') @@ -178,6 +141,13 @@ def test_dashboard(self): submit.click() self.wait_until_disabled(submit) + def test_dashboard(self): + """ + Submit an answer to each MCQ, then check that the dashboard reflects those answers. + """ + self._install_fixture(self.SIMPLE_DASHBOARD) + self._set_mentoring_values() + # Reload the page: self.go_to_view("student_view") dashboard = self.browser.find_element_by_css_selector('.pb-dashboard') @@ -197,3 +167,91 @@ def test_dashboard(self): right_col = avg_row.find_element_by_css_selector('.value') expected_average = {0: "2", 1: "3", 2: "1"}[step_num] self.assertEqual(right_col.text, expected_average) + + def test_dashboard_alternative(self): + """ + Submit an answer to each MCQ, then check that the dashboard reflects those answers with alternative + configuration: + + * Average label is "Avg." instead of default "Average" + * Numerical values are not shown + """ + self._install_fixture(self.ALTERNATIVE_DASHBOARD) + self._set_mentoring_values() + + # Reload the page: + self.go_to_view("student_view") + dashboard = self.browser.find_element_by_css_selector('.pb-dashboard') + steps = dashboard.find_elements_by_css_selector('tbody') + self.assertEqual(len(steps), 3) + + for step in steps: + mcq_rows = step.find_elements_by_css_selector('tr:not(.avg-row)') + self.assertTrue(2 <= len(mcq_rows) <= 3) + for mcq in mcq_rows: + value = mcq.find_element_by_css_selector('td.value') + self.assertEqual(value.text, '') + # Check the average: + avg_row = step.find_element_by_css_selector('tr.avg-row') + left_col = avg_row.find_element_by_css_selector('.desc') + self.assertEqual(left_col.text, "Avg.") + right_col = avg_row.find_element_by_css_selector('.value') + self.assertEqual(right_col.text, "") + + def test_dashboard_exclude_questions(self): + """ + Submit an answer to each MCQ, then check that the dashboard ignores questions it is configured to ignore + """ + self._install_fixture(self.HIDE_QUESTIONS_DASHBOARD) + self._set_mentoring_values() + + # Reload the page: + self.go_to_view("student_view") + dashboard = self.browser.find_element_by_css_selector('.pb-dashboard') + steps = dashboard.find_elements_by_css_selector('tbody') + self.assertEqual(len(steps), 3) + + lengths = [1, 2, 1] + + for step_num, step in enumerate(steps): + mcq_rows = step.find_elements_by_css_selector('tr:not(.avg-row)') + self.assertEqual(len(mcq_rows), lengths[step_num]) + for mcq in mcq_rows: + value = mcq.find_element_by_css_selector('td.value') + self.assertIn(value.text, ('1', '2', '3', '4')) + # Check the average: + avg_row = step.find_element_by_css_selector('tr.avg-row') + left_col = avg_row.find_element_by_css_selector('.desc') + self.assertEqual(left_col.text, "Average") + right_col = avg_row.find_element_by_css_selector('.value') + expected_average = {0: "1", 1: "3", 2: "1"}[step_num] + self.assertEqual(right_col.text, expected_average) + + def test_dashboard_malformed_exclude_questions(self): + """ + Submit an answer to each MCQ, then check that the dashboard ignores questions it is configured to ignore + """ + self._install_fixture(self.MALFORMED_HIDE_QUESTIONS_DASHBOARD) + self._set_mentoring_values() + + # Reload the page: + self.go_to_view("student_view") + dashboard = self.browser.find_element_by_css_selector('.pb-dashboard') + steps = dashboard.find_elements_by_css_selector('tbody') + self.assertEqual(len(steps), 3) + + lengths = [3, 2, 1] + + for step_num, step in enumerate(steps): + mcq_rows = step.find_elements_by_css_selector('tr:not(.avg-row)') + self.assertEqual(len(mcq_rows), lengths[step_num]) + for mcq in mcq_rows: + value = mcq.find_element_by_css_selector('td.value') + self.assertIn(value.text, ('1', '2', '3', '4')) + # Check the average: + avg_row = step.find_element_by_css_selector('tr.avg-row') + left_col = avg_row.find_element_by_css_selector('.desc') + self.assertEqual(left_col.text, "Average") + right_col = avg_row.find_element_by_css_selector('.value') + expected_average = {0: "2", 1: "3", 2: "1"}[step_num] + self.assertEqual(right_col.text, expected_average) diff --git a/problem_builder/tests/integration/xml/dashboard.xml b/problem_builder/tests/integration/xml/dashboard.xml new file mode 100644 index 00000000..e4672a7d --- /dev/null +++ b/problem_builder/tests/integration/xml/dashboard.xml @@ -0,0 +1,57 @@ + + + + Option 1 + Option 2 + Option 3 + Option 4 + + + Option 1 + Option 2 + Option 3 + Option 4 + + + Option 1 + Option 2 + Option 3 + Option 4 + + This message here should be ignored. + + + + Option 4 + Option 5 + Option 6 + + + Option 1 + Option 2 + Option 3 + Option 4 + + + Option 1 + Option 2 + Option 3 + Option 4 + + + + + Option 1 + Option 2 + Option 3 + Option 4 + + + Option A + Option B + Option C + + + {{ dashboard|safe }} + From 51a820eeb3c138154d749e90bfe840383864a9cd Mon Sep 17 00:00:00 2001 From: Braden MacDonald Date: Fri, 24 Apr 2015 09:09:19 -0700 Subject: [PATCH 13/21] Fix travis builds following xblock-sdk PR 56 change --- .travis.yml | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/.travis.yml b/.travis.yml index bb9d34b7..3dfd81e6 100644 --- a/.travis.yml +++ b/.travis.yml @@ -6,9 +6,9 @@ before_install: - "sh -e /etc/init.d/xvfb start" install: - "pip install -e git://github.com/edx/xblock-sdk.git#egg=xblock-sdk" + - "pip install -r requirements.txt" - "pip install -r $VIRTUAL_ENV/src/xblock-sdk/requirements.txt" - "pip install -r $VIRTUAL_ENV/src/xblock-sdk/test-requirements.txt" - - "pip install -r requirements.txt" - "pip uninstall -y xblock-problem-builder && python setup.py sdist && pip install dist/xblock-problem-builder-2.0.tar.gz" - "pip install -r test_requirements.txt" script: From 5646645db866f1537eee1b3107a49b4ebdead2c8 Mon Sep 17 00:00:00 2001 From: "E. Kolpakov" Date: Fri, 24 Apr 2015 18:21:23 +0300 Subject: [PATCH 14/21] Allowed overriding average labels per mentoring --- problem_builder/dashboard.py | 39 ++++++++++++++++--- problem_builder/templates/html/dashboard.html | 2 +- .../tests/integration/test_dashboard.py | 12 ++++-- 3 files changed, 42 insertions(+), 11 deletions(-) diff --git a/problem_builder/dashboard.py b/problem_builder/dashboard.py index 5526b250..a4494303 100644 --- a/problem_builder/dashboard.py +++ b/problem_builder/dashboard.py @@ -221,10 +221,14 @@ class DashboardBlock(StudioEditableXBlockMixin, XBlock): ), scope=Scope.content, ) - average_label = String( + average_labels = Dict( display_name=_("Label for average value"), - default=_("Average"), - help=_("Label to be shown for calculated average"), + help=_( + "This settings allows overriding label for the calculated average per mentoring block. Must be in JSON " + "format. Examples: {examples_here}." + ).format( + examples_here='{"2754b8afc03a439693b9887b6f1d9e36": "Avg.", "215028f7df3d4c68b14fb5fea4da7053": "Mean"}' + ), scope=Scope.content, ) show_numbers = Boolean( @@ -235,7 +239,7 @@ class DashboardBlock(StudioEditableXBlockMixin, XBlock): ) editable_fields = ( - 'display_name', 'mentoring_ids', 'exclude_questions', 'average_label', 'show_numbers', + 'display_name', 'mentoring_ids', 'exclude_questions', 'average_labels', 'show_numbers', 'color_rules', 'visual_rules', 'visual_title', 'visual_desc' ) css_path = 'public/css/dashboard.css' @@ -408,6 +412,7 @@ def student_view(self, context=None): # pylint: disable=unused-argument if numeric_values: average_value = sum(numeric_values) / len(numeric_values) block['average'] = average_value + block['average_label'] = self.average_labels.get(mentoring_block.url_name, _("Average")) block['has_average'] = True block['average_color'] = self.color_for_value(average_value) blocks.append(block) @@ -434,7 +439,6 @@ def student_view(self, context=None): # pylint: disable=unused-argument 'blocks': blocks, 'display_name': self.display_name, 'visual_repr': visual_repr, - 'average_label': self.average_label, 'show_numbers': self.show_numbers, }) @@ -462,10 +466,33 @@ def add_error(msg): for key, value in data.exclude_questions.iteritems(): if not isinstance(value, list): add_error( - _(u"Exclude questions is malformed: value for key {key} is {value}, expected list of integers") + _(u"'Questions to be hidden' is malformed: value for key {key} is {value}, " + u"expected list of integers") .format(key=key, value=value) ) + if key not in data.mentoring_ids: + add_error( + _(u"'Questions to be hidden' is malformed: mentoring url_name {url_name} " + u"is not added to Dashboard") + .format(url_name=key) + ) + + if data.average_labels: + for key, value in data.average_labels.iteritems(): + if not isinstance(value, basestring): + add_error( + _(u"'Label for average value' is malformed: value for key {key} is {value}, expected string") + .format(key=key, value=value) + ) + + if key not in data.mentoring_ids: + add_error( + _(u"'Label for average value' is malformed: mentoring url_name {url_name} " + u"is not added to Dashboard") + .format(url_name=key) + ) + if data.color_rules: try: self.parse_color_rules_str(data.color_rules, ignore_errors=False) diff --git a/problem_builder/templates/html/dashboard.html b/problem_builder/templates/html/dashboard.html index bfee6b7d..e938b6dd 100644 --- a/problem_builder/templates/html/dashboard.html +++ b/problem_builder/templates/html/dashboard.html @@ -56,7 +56,7 @@

{{display_name}}

{% endfor %} {% if block.has_average %} - {{ average_label }} + {{ block.average_label }} """ ALTERNATIVE_DASHBOARD = dedent(""" - + """) HIDE_QUESTIONS_DASHBOARD = dedent(""" """) MALFORMED_HIDE_QUESTIONS_DASHBOARD = dedent(""" @@ -185,7 +187,9 @@ def test_dashboard_alternative(self): steps = dashboard.find_elements_by_css_selector('tbody') self.assertEqual(len(steps), 3) - for step in steps: + average_labels = ["Avg.", "Mean", "Second Quartile"] + + for step_num, step in enumerate(steps): mcq_rows = step.find_elements_by_css_selector('tr:not(.avg-row)') self.assertTrue(2 <= len(mcq_rows) <= 3) for mcq in mcq_rows: @@ -194,7 +198,7 @@ def test_dashboard_alternative(self): # Check the average: avg_row = step.find_element_by_css_selector('tr.avg-row') left_col = avg_row.find_element_by_css_selector('.desc') - self.assertEqual(left_col.text, "Avg.") + self.assertEqual(left_col.text, average_labels[step_num]) right_col = avg_row.find_element_by_css_selector('.value') self.assertEqual(right_col.text, "") From 7ea0c4ec02af579d5bdd372c65417ab1e66183da Mon Sep 17 00:00:00 2001 From: "E. Kolpakov" Date: Tue, 5 May 2015 14:06:37 +0300 Subject: [PATCH 15/21] Presentation updates, batch 2: * All dashboard tables are 700px wide and centered * Color codes made square * "Average" row label made semi-bold (font-weight: 600) --- problem_builder/public/css/dashboard.css | 10 +++++++++- 1 file changed, 9 insertions(+), 1 deletion(-) diff --git a/problem_builder/public/css/dashboard.css b/problem_builder/public/css/dashboard.css index 249b5355..71bec651 100644 --- a/problem_builder/public/css/dashboard.css +++ b/problem_builder/public/css/dashboard.css @@ -1,6 +1,10 @@ .pb-dashboard table { max-width: 800px; + width: 700px; + table-layout: auto; border-collapse: collapse; + margin-left: auto; + margin-right: auto; margin-bottom: 15px; } @@ -9,6 +13,10 @@ font-weight: bold; } +.pb-dashboard .dashboard-report .avg-row .desc { + font-weight: 600; +} + .pb-dashboard table td, .pb-dashboard table tbody th { border-top: 1px solid #ddd; border-bottom: 1px solid #ddd; @@ -24,7 +32,7 @@ min-width: 4em; text-align: right; padding-right: 5px; - border-right: 0.6em solid transparent; + border-right: 2em solid transparent; } .pb-dashboard table .avg-row td.desc { From 60713fddbb0db22ffccfd8706e1f005f6bc31795 Mon Sep 17 00:00:00 2001 From: "E. Kolpakov" Date: Tue, 5 May 2015 15:29:21 +0300 Subject: [PATCH 16/21] Accessibility updates - using sr and aria-hidden for better screen reader experience --- problem_builder/dashboard.py | 3 +++ problem_builder/templates/html/dashboard.html | 20 ++++++------------- 2 files changed, 9 insertions(+), 14 deletions(-) diff --git a/problem_builder/dashboard.py b/problem_builder/dashboard.py index a4494303..27ba8413 100644 --- a/problem_builder/dashboard.py +++ b/problem_builder/dashboard.py @@ -402,6 +402,7 @@ def student_view(self, context=None): # pylint: disable=unused-argument block['mcqs'].append({ "display_name": mcq_block.display_name_with_default, "value": value, + "accessible_value": _("Score: {value}").format(value=value) if value else _("No value yet"), "color": self.color_for_value(value) if value is not None else None, }) # If the values are numeric, display an average: @@ -412,6 +413,8 @@ def student_view(self, context=None): # pylint: disable=unused-argument if numeric_values: average_value = sum(numeric_values) / len(numeric_values) block['average'] = average_value + # average block is shown only if average value exists, so accessible text for no data is not required + block['accessible_average'] = _("Score: {average_value:.2f}").format(average_value=average_value) block['average_label'] = self.average_labels.get(mentoring_block.url_name, _("Average")) block['has_average'] = True block['average_color'] = self.color_for_value(average_value) diff --git a/problem_builder/templates/html/dashboard.html b/problem_builder/templates/html/dashboard.html index e938b6dd..857f9eee 100644 --- a/problem_builder/templates/html/dashboard.html +++ b/problem_builder/templates/html/dashboard.html @@ -42,30 +42,22 @@

{{display_name}}

{% for mcq in block.mcqs %} {{ mcq.display_name }} - + {% if mcq.value and show_numbers %} - {{ mcq.value }} + {% endif %} + {{ mcq.accessible_value }} {% endfor %} {% if block.has_average %} {{ block.average_label }} - + {% if show_numbers %} - {{ block.average|floatformat }} + {% endif %} + {{ block.accessible_average }} {% endif %} From a4703a2a0ab8eeffef463b67c0d8b392635d946b Mon Sep 17 00:00:00 2001 From: "E. Kolpakov" Date: Tue, 5 May 2015 17:14:24 +0300 Subject: [PATCH 17/21] Dashboard export HTML updates --- problem_builder/public/css/dashboard.css | 2 +- .../templates/html/dashboard_report.html | 14 ++++++++++++++ 2 files changed, 15 insertions(+), 1 deletion(-) diff --git a/problem_builder/public/css/dashboard.css b/problem_builder/public/css/dashboard.css index 71bec651..1e202d3c 100644 --- a/problem_builder/public/css/dashboard.css +++ b/problem_builder/public/css/dashboard.css @@ -13,7 +13,7 @@ font-weight: bold; } -.pb-dashboard .dashboard-report .avg-row .desc { +.pb-dashboard .avg-row .desc { font-weight: 600; } diff --git a/problem_builder/templates/html/dashboard_report.html b/problem_builder/templates/html/dashboard_report.html index cf29a6eb..fa1d6785 100644 --- a/problem_builder/templates/html/dashboard_report.html +++ b/problem_builder/templates/html/dashboard_report.html @@ -9,6 +9,20 @@ body { font-family: 'Open Sans', 'Helvetica Neue', Helvetica, Arial, sans-serif; } + .pb-dashboard table { + text-align: left; + } + /* screen reader class from edx-platform */ + .sr { + border: 0; + clip: rect(1px 1px 1px 1px); + height: 1px; + margin: -1px; + overflow: hidden; + padding: 0; + position: absolute; + width: 1px; + } {{css}} From e2f8c7ad881499946724e1229b8638c2e2d688d9 Mon Sep 17 00:00:00 2001 From: "E. Kolpakov" Date: Tue, 5 May 2015 17:48:45 +0300 Subject: [PATCH 18/21] Added ability to override feedback messages title --- problem_builder/mentoring.py | 8 +++++++- problem_builder/public/js/mentoring.js | 3 ++- problem_builder/public/js/mentoring_standard_view.js | 2 +- problem_builder/templates/html/mentoring.html | 2 +- 4 files changed, 11 insertions(+), 4 deletions(-) diff --git a/problem_builder/mentoring.py b/problem_builder/mentoring.py index 78b1b103..1d9d92ce 100644 --- a/problem_builder/mentoring.py +++ b/problem_builder/mentoring.py @@ -131,6 +131,12 @@ class MentoringBlock(XBlock, StepParentMixin, StudioEditableXBlockMixin, StudioC default=_("Mentoring Questions"), scope=Scope.settings ) + feedback_label = String( + display_name=_("Feedback Messages Title"), + help=_("Title for feedback messages"), + default=_("Feedback"), + scope=Scope.content + ) # User state attempted = Boolean( @@ -170,7 +176,7 @@ class MentoringBlock(XBlock, StepParentMixin, StudioEditableXBlockMixin, StudioC editable_fields = ( 'display_name', 'mode', 'followed_by', 'max_attempts', 'enforce_dependency', - 'display_submit', 'weight', + 'display_submit', 'feedback_label', 'weight', ) icon_class = 'problem' has_score = True diff --git a/problem_builder/public/js/mentoring.js b/problem_builder/public/js/mentoring.js index 760beb32..dbbd345e 100644 --- a/problem_builder/public/js/mentoring.js +++ b/problem_builder/public/js/mentoring.js @@ -22,7 +22,8 @@ function MentoringBlock(runtime, element) { hideAllSteps: hideAllSteps, step: step, steps: steps, - publish_event: publish_event + publish_event: publish_event, + data: data }; function publish_event(data) { diff --git a/problem_builder/public/js/mentoring_standard_view.js b/problem_builder/public/js/mentoring_standard_view.js index ac45f0c2..50452c61 100644 --- a/problem_builder/public/js/mentoring_standard_view.js +++ b/problem_builder/public/js/mentoring_standard_view.js @@ -25,7 +25,7 @@ function MentoringStandardView(runtime, element, mentoring) { // Messages should only be displayed upon hitting 'submit', not on page reload mentoring.setContent(messagesDOM, results.message); if (messagesDOM.html().trim()) { - messagesDOM.prepend('
' + gettext('Feedback') + '
'); + messagesDOM.prepend('
' + mentoring.data.feedback_label + '
'); messagesDOM.show(); } diff --git a/problem_builder/templates/html/mentoring.html b/problem_builder/templates/html/mentoring.html index df04e2b1..ac522305 100644 --- a/problem_builder/templates/html/mentoring.html +++ b/problem_builder/templates/html/mentoring.html @@ -1,5 +1,5 @@ {% load i18n %} -
+
{% with url=missing_dependency_url|safe %} {% blocktrans with link_start="" link_end="" %} From e889b9b8c2b32f161e83508d17f1d0a6f1c1d56b Mon Sep 17 00:00:00 2001 From: "E. Kolpakov" Date: Tue, 5 May 2015 18:30:36 +0300 Subject: [PATCH 19/21] Fixed tests and improved floaitng format output --- problem_builder/dashboard.py | 5 +- .../tests/integration/test_dashboard.py | 63 ++++++++++++++----- 2 files changed, 53 insertions(+), 15 deletions(-) diff --git a/problem_builder/dashboard.py b/problem_builder/dashboard.py index 27ba8413..7326c25b 100644 --- a/problem_builder/dashboard.py +++ b/problem_builder/dashboard.py @@ -30,6 +30,7 @@ import json import logging import operator as op +from django.template.defaultfilters import floatformat from .dashboard_visual import DashboardVisualData from .mcq import MCQBlock @@ -414,7 +415,9 @@ def student_view(self, context=None): # pylint: disable=unused-argument average_value = sum(numeric_values) / len(numeric_values) block['average'] = average_value # average block is shown only if average value exists, so accessible text for no data is not required - block['accessible_average'] = _("Score: {average_value:.2f}").format(average_value=average_value) + block['accessible_average'] = _("Score: {average_value}").format( + average_value=floatformat(average_value) + ) block['average_label'] = self.average_labels.get(mentoring_block.url_name, _("Average")) block['has_average'] = True block['average_color'] = self.color_for_value(average_value) diff --git a/problem_builder/tests/integration/test_dashboard.py b/problem_builder/tests/integration/test_dashboard.py index 856adbee..c016fc23 100644 --- a/problem_builder/tests/integration/test_dashboard.py +++ b/problem_builder/tests/integration/test_dashboard.py @@ -18,7 +18,9 @@ # "AGPLv3". If not, see . # from textwrap import dedent +from django.template.defaultfilters import floatformat from mock import Mock, patch +from selenium.common.exceptions import NoSuchElementException from xblockutils.base_test import SeleniumXBlockTest from xblockutils.resources import ResourceLoader @@ -112,6 +114,22 @@ def _install_fixture(self, dashboard_xml): self.go_to_view("student_view") self.vertical = self.load_root_xblock() + def _get_cell_contents(self, cell): + try: + visible_text = cell.find_element_by_css_selector('span:not(.sr)').text + except NoSuchElementException: + visible_text = "" + screen_reader_text = cell.find_element_by_css_selector('span.sr') + return visible_text, screen_reader_text.text + + def _assert_cell_contents(self, cell, expected_visible_text, expected_screen_reader_text): + visible_text, screen_reader_text = self._get_cell_contents(cell) + self.assertEqual(visible_text, expected_visible_text) + self.assertEqual(screen_reader_text, expected_screen_reader_text) + + def _format_sr_text(self, visible_text): + return "Score: {value}".format(value=visible_text) + def test_empty_dashboard(self): """ Test that when the student has not submitted any question answers, we still see @@ -129,8 +147,8 @@ def test_empty_dashboard(self): mcq_rows = step.find_elements_by_css_selector('tr') self.assertTrue(2 <= len(mcq_rows) <= 3) for mcq in mcq_rows: - value = mcq.find_element_by_css_selector('td:last-child') - self.assertEqual(value.text, '') + cell = mcq.find_element_by_css_selector('td:last-child') + self._assert_cell_contents(cell, '', 'No value yet') def _set_mentoring_values(self): pbs = self.browser.find_elements_by_css_selector('.mentoring') @@ -155,20 +173,23 @@ def test_dashboard(self): dashboard = self.browser.find_element_by_css_selector('.pb-dashboard') steps = dashboard.find_elements_by_css_selector('tbody') self.assertEqual(len(steps), 3) + expected_values = ('1', '2', '3', '4', 'B') for step_num, step in enumerate(steps): mcq_rows = step.find_elements_by_css_selector('tr:not(.avg-row)') self.assertTrue(2 <= len(mcq_rows) <= 3) for mcq in mcq_rows: - value = mcq.find_element_by_css_selector('td.value') - self.assertIn(value.text, ('1', '2', '3', '4', 'B')) + cell = mcq.find_element_by_css_selector('td.value') + visible_text, screen_reader_text = self._get_cell_contents(cell) + self.assertIn(visible_text, expected_values) + self.assertIn(screen_reader_text, map(self._format_sr_text, expected_values)) # Check the average: avg_row = step.find_element_by_css_selector('tr.avg-row') left_col = avg_row.find_element_by_css_selector('.desc') self.assertEqual(left_col.text, "Average") right_col = avg_row.find_element_by_css_selector('.value') expected_average = {0: "2", 1: "3", 2: "1"}[step_num] - self.assertEqual(right_col.text, expected_average) + self._assert_cell_contents(right_col, expected_average, self._format_sr_text(expected_average)) def test_dashboard_alternative(self): """ @@ -189,18 +210,25 @@ def test_dashboard_alternative(self): average_labels = ["Avg.", "Mean", "Second Quartile"] + expected_values = ('1', '2', '3', '4', 'B') + for step_num, step in enumerate(steps): mcq_rows = step.find_elements_by_css_selector('tr:not(.avg-row)') self.assertTrue(2 <= len(mcq_rows) <= 3) for mcq in mcq_rows: - value = mcq.find_element_by_css_selector('td.value') - self.assertEqual(value.text, '') + cell = mcq.find_element_by_css_selector('td.value') + visible_text, screen_reader_text = self._get_cell_contents(cell) + # this dashboard configured to not show numbers + self.assertEqual(visible_text, '') + # but screen reader content still added + self.assertIn(screen_reader_text, map(self._format_sr_text, expected_values)) # Check the average: avg_row = step.find_element_by_css_selector('tr.avg-row') left_col = avg_row.find_element_by_css_selector('.desc') self.assertEqual(left_col.text, average_labels[step_num]) right_col = avg_row.find_element_by_css_selector('.value') - self.assertEqual(right_col.text, "") + expected_average = {0: "2", 1: "3", 2: "1"}[step_num] + self._assert_cell_contents(right_col, '', self._format_sr_text(expected_average)) def test_dashboard_exclude_questions(self): """ @@ -214,6 +242,7 @@ def test_dashboard_exclude_questions(self): dashboard = self.browser.find_element_by_css_selector('.pb-dashboard') steps = dashboard.find_elements_by_css_selector('tbody') self.assertEqual(len(steps), 3) + expected_values = ('1', '2', '3', '4') lengths = [1, 2, 1] @@ -221,15 +250,17 @@ def test_dashboard_exclude_questions(self): mcq_rows = step.find_elements_by_css_selector('tr:not(.avg-row)') self.assertEqual(len(mcq_rows), lengths[step_num]) for mcq in mcq_rows: - value = mcq.find_element_by_css_selector('td.value') - self.assertIn(value.text, ('1', '2', '3', '4')) + cell = mcq.find_element_by_css_selector('td.value') + visible_text, screen_reader_text = self._get_cell_contents(cell) + self.assertIn(visible_text, expected_values) + self.assertIn(screen_reader_text, map(self._format_sr_text, expected_values)) # Check the average: avg_row = step.find_element_by_css_selector('tr.avg-row') left_col = avg_row.find_element_by_css_selector('.desc') self.assertEqual(left_col.text, "Average") right_col = avg_row.find_element_by_css_selector('.value') expected_average = {0: "1", 1: "3", 2: "1"}[step_num] - self.assertEqual(right_col.text, expected_average) + self._assert_cell_contents(right_col, expected_average, self._format_sr_text(expected_average)) def test_dashboard_malformed_exclude_questions(self): """ @@ -244,18 +275,22 @@ def test_dashboard_malformed_exclude_questions(self): steps = dashboard.find_elements_by_css_selector('tbody') self.assertEqual(len(steps), 3) + expected_values = ('1', '2', '3', '4') + lengths = [3, 2, 1] for step_num, step in enumerate(steps): mcq_rows = step.find_elements_by_css_selector('tr:not(.avg-row)') self.assertEqual(len(mcq_rows), lengths[step_num]) for mcq in mcq_rows: - value = mcq.find_element_by_css_selector('td.value') - self.assertIn(value.text, ('1', '2', '3', '4')) + cell = mcq.find_element_by_css_selector('td.value') + visible_text, screen_reader_text = self._get_cell_contents(cell) + self.assertIn(visible_text, expected_values) + self.assertIn(screen_reader_text, map(self._format_sr_text, expected_values)) # Check the average: avg_row = step.find_element_by_css_selector('tr.avg-row') left_col = avg_row.find_element_by_css_selector('.desc') self.assertEqual(left_col.text, "Average") right_col = avg_row.find_element_by_css_selector('.value') expected_average = {0: "2", 1: "3", 2: "1"}[step_num] - self.assertEqual(right_col.text, expected_average) + self._assert_cell_contents(right_col, expected_average, self._format_sr_text(expected_average)) From 3523e7502c2de4ee5c8ade8a91a18c628dca8988 Mon Sep 17 00:00:00 2001 From: "E. Kolpakov" Date: Tue, 5 May 2015 18:31:32 +0300 Subject: [PATCH 20/21] Suppressed logging in tests - selenium driver messages flood the console so it's hard to see which tests have failed and stacktraces --- problem_builder/dashboard.py | 6 +++--- problem_builder/mentoring.py | 4 ++-- run_tests.py | 4 ++++ 3 files changed, 9 insertions(+), 5 deletions(-) diff --git a/problem_builder/dashboard.py b/problem_builder/dashboard.py index 7326c25b..3c69e9db 100644 --- a/problem_builder/dashboard.py +++ b/problem_builder/dashboard.py @@ -403,7 +403,7 @@ def student_view(self, context=None): # pylint: disable=unused-argument block['mcqs'].append({ "display_name": mcq_block.display_name_with_default, "value": value, - "accessible_value": _("Score: {value}").format(value=value) if value else _("No value yet"), + "accessible_value": _("Score: {score}").format(score=value) if value else _("No value yet"), "color": self.color_for_value(value) if value is not None else None, }) # If the values are numeric, display an average: @@ -415,8 +415,8 @@ def student_view(self, context=None): # pylint: disable=unused-argument average_value = sum(numeric_values) / len(numeric_values) block['average'] = average_value # average block is shown only if average value exists, so accessible text for no data is not required - block['accessible_average'] = _("Score: {average_value}").format( - average_value=floatformat(average_value) + block['accessible_average'] = _("Score: {score}").format( + score=floatformat(average_value) ) block['average_label'] = self.average_labels.get(mentoring_block.url_name, _("Average")) block['has_average'] = True diff --git a/problem_builder/mentoring.py b/problem_builder/mentoring.py index 1d9d92ce..3b3022f6 100644 --- a/problem_builder/mentoring.py +++ b/problem_builder/mentoring.py @@ -132,8 +132,8 @@ class MentoringBlock(XBlock, StepParentMixin, StudioEditableXBlockMixin, StudioC scope=Scope.settings ) feedback_label = String( - display_name=_("Feedback Messages Title"), - help=_("Title for feedback messages"), + display_name=_("Feedback Header"), + help=_("Header for feedback messages"), default=_("Feedback"), scope=Scope.content ) diff --git a/run_tests.py b/run_tests.py index c4b17c38..8c7a8613 100755 --- a/run_tests.py +++ b/run_tests.py @@ -10,6 +10,8 @@ import os import sys +import logging + if __name__ == "__main__": # Use the workbench settings file: os.environ.setdefault("DJANGO_SETTINGS_MODULE", "workbench.settings") @@ -19,6 +21,8 @@ from django.conf import settings settings.INSTALLED_APPS += ("problem_builder", ) + logging.disable(logging.CRITICAL) + from django.core.management import execute_from_command_line args = sys.argv[1:] paths = [arg for arg in args if arg[0] != '-'] From 45b36094cace0d5ed0bce03c1b054a9594488327 Mon Sep 17 00:00:00 2001 From: Braden MacDonald Date: Tue, 28 Apr 2015 21:55:39 -0700 Subject: [PATCH 21/21] Fix Travis build --- .travis.yml | 2 ++ 1 file changed, 2 insertions(+) diff --git a/.travis.yml b/.travis.yml index 3dfd81e6..6c0587c4 100644 --- a/.travis.yml +++ b/.travis.yml @@ -17,3 +17,5 @@ script: - python run_tests.py --with-coverage --cover-package=problem_builder notifications: email: false +addons: + firefox: "36.0"