From d85cdcace22812770462b561b5aa2038f4c30ccd Mon Sep 17 00:00:00 2001
From: muhammad-ammar <mammar@gmail.com>
Date: Mon, 5 Sep 2016 19:38:22 +0500
Subject: [PATCH] update question tag extraction logic

---
 common/lib/capa/capa/capa_problem.py          | 14 +++-
 .../lib/capa/capa/tests/test_capa_problem.py  | 69 ++++++++++++++++++-
 2 files changed, 78 insertions(+), 5 deletions(-)

diff --git a/common/lib/capa/capa/capa_problem.py b/common/lib/capa/capa/capa_problem.py
index ae967438d7..6dd58fd2c5 100644
--- a/common/lib/capa/capa/capa_problem.py
+++ b/common/lib/capa/capa/capa_problem.py
@@ -942,11 +942,21 @@ class LoncapaProblem(object):
                 # store <label> tag containing question text to delete
                 # it later otherwise question will be rendered twice
                 element_to_be_deleted = responsetype_label_tag
-            elif 'label' not in inputfields[0].attrib:
+            elif 'label' in inputfields[0].attrib:
+                # in this case we have old problems with label attribute and p tag having question in it
+                # we will pick the first sibling of responsetype if its a p tag and match the text with
+                # the label attribute text. if they are equal then we will use this text as question.
+                # Get first <p> tag before responsetype, this <p> may contains the question text.
+                p_tag = response.xpath('preceding-sibling::*[1][self::p]')
+
+                if p_tag and p_tag[0].text == inputfields[0].attrib['label']:
+                    label = p_tag[0].text
+                    element_to_be_deleted = p_tag[0]
+            else:
                 # In this case the problems don't have tag or label attribute inside the responsetype
                 # so we will get the first preceding label tag w.r.t to this responsetype.
                 # This will take care of those multi-question problems that are not using --- in their markdown.
-                label_tag = response.xpath('preceding-sibling::label[1]')
+                label_tag = response.xpath('preceding-sibling::*[1][self::label]')
                 if label_tag:
                     label = label_tag[0].text
                     element_to_be_deleted = label_tag[0]
diff --git a/common/lib/capa/capa/tests/test_capa_problem.py b/common/lib/capa/capa/tests/test_capa_problem.py
index 352c00f302..bdfacca2e5 100644
--- a/common/lib/capa/capa/tests/test_capa_problem.py
+++ b/common/lib/capa/capa/tests/test_capa_problem.py
@@ -9,6 +9,7 @@ import unittest
 from . import new_loncapa_problem
 
 
+@ddt.ddt
 class CAPAProblemTest(unittest.TestCase):
     """ CAPA problem related tests"""
 
@@ -66,14 +67,14 @@ class CAPAProblemTest(unittest.TestCase):
             {
                 '1_2_1':
                 {
-                    'label': '',
+                    'label': question,
                     'descriptions': {}
                 }
             }
         )
         self.assertEqual(
             len(problem.tree.xpath("//*[normalize-space(text())='{}']".format(question))),
-            1
+            0
         )
 
     def test_neither_label_tag_nor_attribute(self):
@@ -176,7 +177,7 @@ class CAPAProblemTest(unittest.TestCase):
             {
                 '1_2_1':
                 {
-                    'label': '',
+                    'label': question,
                     'descriptions': {}
                 }
             }
@@ -361,6 +362,68 @@ class CAPAProblemTest(unittest.TestCase):
         question = problem_html.xpath("//*[normalize-space(text())='{}']".format(question))
         self.assertEqual(len(question), 1)
 
+    def assert_question_tag(self, question1, question2, tag, label_attr=False):
+        """
+        Verify question tag correctness.
+        """
+        question1_tag = '<{tag}>{}</{tag}>'.format(question1, tag=tag) if question1 else ''
+        question2_tag = '<{tag}>{}</{tag}>'.format(question2, tag=tag) if question2 else ''
+        question1_label_attr = 'label="{}"'.format(question1) if label_attr else ''
+        question2_label_attr = 'label="{}"'.format(question2) if label_attr else ''
+        xml = """
+        <problem>
+            {question1_tag}
+            <choiceresponse>
+                <checkboxgroup {question1_label_attr}>
+                    <choice correct="true">choice1</choice>
+                    <choice correct="false">choice2</choice>
+                </checkboxgroup>
+            </choiceresponse>
+            {question2_tag}
+            <multiplechoiceresponse>
+                <choicegroup type="MultipleChoice" {question2_label_attr}>
+                    <choice correct="false">choice1</choice>
+                    <choice correct="true">choice2</choice>
+                </choicegroup>
+            </multiplechoiceresponse>
+        </problem>
+        """.format(
+            question1_tag=question1_tag,
+            question2_tag=question2_tag,
+            question1_label_attr=question1_label_attr,
+            question2_label_attr=question2_label_attr,
+        )
+        problem = new_loncapa_problem(xml)
+        self.assertEqual(
+            problem.problem_data,
+            {
+                '1_2_1':
+                {
+                    'label': question1,
+                    'descriptions': {}
+                },
+                '1_3_1':
+                {
+                    'label': question2,
+                    'descriptions': {}
+                }
+            }
+        )
+        self.assertEqual(len(problem.tree.xpath('//{}'.format(tag))), 0)
+
+    @ddt.unpack
+    @ddt.data(
+        {'question1': 'question 1 label', 'question2': 'question 2 label'},
+        {'question1': '', 'question2': 'question 2 label'},
+        {'question1': 'question 1 label', 'question2': ''}
+    )
+    def test_correct_question_tag_is_picked(self, question1, question2):
+        """
+        For a problem with multiple questions verify that correct question tag is picked.
+        """
+        self.assert_question_tag(question1, question2, tag='label', label_attr=False)
+        self.assert_question_tag(question1, question2, tag='p', label_attr=True)
+
 
 @ddt.ddt
 class CAPAMultiInputProblemTest(unittest.TestCase):