Simplify and consolidate color coding

doc/Dashboard.md

@@ -13,26 +13,48 @@ And it will then look like this (after a student has submitted their answers):
 ![Screen shot of a Dashboard XBlock](img/dashboard-example.png)
 
 
+Color Coding Rules:
+-------------------
+
+Authors can add a list of rules (one per line) that apply colors to the various
+possible student answer values.
+
+Rules are entered into the dashboard configuration "Color Coding Rules", one
+rule per line. The first rule to match a given value will be used to color
+that value.
+
+The simplest rule looks like "3: red". With this line, if the student's answer
+is 3, it will be shown in red in the report. Colors can be specified using any
+valid CSS color value, e.g. "red", "#f00", "#ff0000", "rgb(255,0,0)", etc.
+
+For more advanced rules, you can use an expression in terms of x such as
+"x > 3: blue" or "0 <= x < 5: green" (green if x is greater than or equal to
+zero but less than five).
+
+You can also just specify a color on a line by itself, which will always match
+any value (usually this would be the last line, as a "default" color).
+
+
 Visual Representation
 ---------------------
 
 The Dashboard also supports an optional visual representation. This is a
-powerful feature, but setting it up is quite involved.
+powerful feature, but setting it up is a bit involved.
 
 The end result is shown below. You can see a diagram, in which a colored arrow
 appears as the student works through the various "Steps". Each "Step" is one
 mentoring block, which contains several multiple choice questions. Based on the
 average value of the student's choices, the step is given a color.
 
-In this example, steps that have not been attempted are transparent, steps that
-have been attempted but with an average less than two are grey, and steps with
-an average higher than two are green.
+In this example, steps that have not been attempted are transparent, and steps
+that have been attempted are colored in according to the color coding rules.
 
 ![Screen shot of visual representation](img/dashboard-visual.png)
 
 To achieve the result shown above requires a set of "stacked" image files (one
 for each step), as well as an overlay image (in this case, the overlay image
-contains all the text).
+contains all the text). For coloring to work, the images must be white where
+color is desired.
 
 To build this example, the images used look like this:  
 ![Images Used](img/dashboard-visual-instructions.png)
@@ -40,10 +62,6 @@ To build this example, the images used look like this:
 The block was configured to use these images as follows:
 ![Screen shot of visual representation rule configuration](img/dashboard-visual-config.png)
 
-Notice that step 2 has turned green due to the default rule "hueRotate 280" for
-any steps with an average above 2, and the first step has a blurred, grey
-background due to the first rule for average values less than 2.
-
 The **Visual Representation Settings** used to define the visual representation
 must be in JSON format. The supported entries are:
 
@@ -59,33 +77,6 @@ must be in JSON format. The supported entries are:
   layered images, with no effects applied.
 * **`"background"`**: (Optional) The URL of an image to be drawn behind the
   layered images, with no effects applied.
-* **`"colorRules"`**: (Optional) The rules used to determine how the student's
-  average choice values affect each layer's image. Each rule is evaluated in
-  order, and the first matching rule is used. Each rule is a JSON object
-  (`{ ... }`) that may consist of any of the following optional entries.
-  * `if`: An optional expression involving `x` (the student's average choice
-    value). If this is true, then this rule will be used. If this is false, the
-    rule will not be used. If no `if` clause is set, the rule will always be
-    used, unless an earlier rule matched first.  
-    Examples:
-      - `"if": "x > 3"`
-      - `"if": "0 < x < 1"`
-      - `"if": "(x > 3) and (x < 6 or x > 100)"`
-  * `hueRotate`: If this rule matches, adjust the color of all parts of the
-    image that have some saturation. In the above example, this is used to
-    change from a turquiose color to a light green. Valid values are between 0
-    (no change) and 360. Note that this effect will *not* affect any parts of
-    the image have no saturation (i.e. are white, black, or any shade of grey).  
-    Examples:
-      - `"hueRotate": "180"`
-      - `"hueRotate": "x * 30"`
-  * `blur`: If this rule matches, apply a gaussian blur to blur this layer's
-    image. The standard deviation of the blur must be specified.  
-    Examples:
-      - `"blur": "3"`
-      - `"blur": "x"`
-  * `saturate`: If this rule matches, adjust the saturation of the image. `1`
-    means no change, and `0` means fully desaturated. Valid values are between
-    zero and one.  
-    Examples:
-      - `"saturate": "0.2"`
+* **`"width"`**: (Important) The width of the images, in pixels (all images
+  should be the same size).
+* **`"height"`**: (Important) The height of the images, in pixels

mentoring/dashboard.py

@@ -26,16 +26,19 @@ will then display a table summarizing the values that the student chose for each
 """
 # Imports ###########################################################
 
+import ast
 import json
 import logging
+import operator as op
 
 from .dashboard_visual import DashboardVisualData
 from .mcq import MCQBlock
 from .sub_api import sub_api
+from lazy import lazy
 from webob import Response
 from xblock.core import XBlock
 from xblock.exceptions import XBlockNotFoundError
-from xblock.fields import Scope, Dict, List, String
+from xblock.fields import Scope, List, String
 from xblock.fragment import Fragment
 from xblock.validation import ValidationMessage
 from xblockutils.helpers import child_isinstance
@@ -56,6 +59,96 @@ def _(text):
 # Classes ###########################################################
 
 
+class ColorRule(object):
+    """
+    A rule used to conditionally set colors
+
+    >>> rule1 = ColorRule("3 < x <= 5", color_str="#ff0000")
+    >>> rule1.matches(2)
+    False
+    >>> rule1.matches(4)
+    True
+    """
+    def __init__(self, rule_str, color_str):
+        """
+        Instantiate a ColorRule with the given rule expression string and color value.
+        """
+        try:
+            self._rule_parsed = ast.parse(rule_str, mode='eval').body
+            # Once it's been parsed, also try evaluating it with a test value:
+            self._safe_eval_expression(self._rule_parsed, x=0)
+        except (TypeError, SyntaxError) as e:
+            raise ValueError("Invalid Expression: {}".format(e))
+        except ZeroDivisionError:
+            pass  # This may depend on the value of 'x' which we set to zero but don't know yet.
+        self.color_str = color_str
+
+    def matches(self, x):
+        """ Does this rule apply for the value x? """
+        try:
+            return bool(self._safe_eval_expression(self._rule_parsed, x))
+        except ZeroDivisionError:
+            return False
+
+    @staticmethod
+    def _safe_eval_expression(expr, x=0):
+        """
+        Safely evaluate a mathematical or boolean expression involving the value x
+
+        >>> _safe_eval_expression('2*x', x=3)
+        6
+        >>> _safe_eval_expression('x >= 0 and x < 2', x=3)
+        False
+
+        We use python syntax for the expressions, so the parsing and evaluation is mostly done
+        using Python's built-in asbstract syntax trees and operator implementations.
+
+        The expression can only contain: numbers, mathematical operators, boolean operators,
+        comparisons, and a placeholder variable called "x"
+        """
+        # supported operators:
+        operators = {
+            # Allow +, -, *, /, %, negative:
+            ast.Add: op.add, ast.Sub: op.sub, ast.Mult: op.mul, ast.Div: op.truediv, ast.Mod: op.mod, ast.USub: op.neg,
+            # Allow comparison:
+            ast.Eq: op.eq, ast.NotEq: op.ne, ast.Lt: op.lt, ast.LtE: op.le, ast.Gt: op.gt, ast.GtE: op.ge,
+        }
+
+        def eval_(node):
+            """ Recursive evaluation of syntax tree node 'node' """
+            if isinstance(node, ast.Num):  # <number>
+                return node.n
+            elif isinstance(node, ast.BinOp):  # <left> <operator> <right>
+                return operators[type(node.op)](eval_(node.left), eval_(node.right))
+            elif isinstance(node, ast.UnaryOp):  # <operator> <operand> e.g., -1
+                return operators[type(node.op)](eval_(node.operand))
+            elif isinstance(node, ast.Name) and node.id == "x":
+                return x
+            elif isinstance(node, ast.BoolOp):  # Boolean operator: either "and" or "or" with two or more values
+                if type(node.op) == ast.And:
+                    return all(eval_(val) for val in node.values)
+                else:  # Or:
+                    for val in node.values:
+                        result = eval_(val)
+                        if result:
+                            return result
+                    return result  # or returns the final value even if it's falsy
+            elif isinstance(node, ast.Compare):  # A comparison expression, e.g. "3 > 2" or "5 < x < 10"
+                left = eval_(node.left)
+                for comparison_op, right_expr in zip(node.ops, node.comparators):
+                    right = eval_(right_expr)
+                    if not operators[type(comparison_op)](left, right):
+                        return False
+                    left = right
+                return True
+            else:
+                raise TypeError(node)
+
+        if not isinstance(expr, ast.AST):
+            expr = ast.parse(expr, mode='eval').body
+        return eval_(expr)
+
+
 @XBlock.needs("i18n")
 class DashboardBlock(StudioEditableXBlockMixin, XBlock):
     """
@@ -75,12 +168,17 @@ class DashboardBlock(StudioEditableXBlockMixin, XBlock):
         ).format(example_here='["2754b8afc03a439693b9887b6f1d9e36", "215028f7df3d4c68b14fb5fea4da7053"]'),
         scope=Scope.settings,
     )
-    color_codes = Dict(
-        display_name=_("Color Coding"),
+    color_rules = String(
+        display_name=_("Color Coding Rules"),
         help=_(
-            "You can optionally set a color for each expected value. Example: {example_here}"
-        ).format(example_here='{ "1": "red", "2": "yellow", 3: "green", 4: "lightskyblue" }'),
+            "Optional rules to assign colors to possible answer values and average values. "
+            "One rule per line. First matching rule will be used. "
+            "Examples: {examples_here}"
+        ).format(examples_here='"1: red", "0 <= x < 5: blue", "green"'),
         scope=Scope.content,
+        default="",
+        multiline_editor=True,
+        resettable_editor=False,
     )
     visual_rules = String(
         display_name=_("Visual Representation"),
@@ -91,14 +189,14 @@ class DashboardBlock(StudioEditableXBlockMixin, XBlock):
         resettable_editor=False,
     )
 
-    editable_fields = ('display_name', 'mentoring_ids', 'color_codes', 'visual_rules')
+    editable_fields = ('display_name', 'mentoring_ids', 'color_rules', 'visual_rules')
     css_path = 'public/css/dashboard.css'
 
     def get_mentoring_blocks(self):
         """
         Generator returning the specified mentoring blocks, in order.
 
-        returns a list. Will insert None for every invalid mentoring block ID.
+        Returns a list. Will insert None for every invalid mentoring block ID.
         """
         for url_name in self.mentoring_ids:
             mentoring_id = self.scope_ids.usage_id.course_key.make_usage_key('problem-builder', url_name)
@@ -113,6 +211,41 @@ class DashboardBlock(StudioEditableXBlockMixin, XBlock):
                 except (XBlockNotFoundError, Exception):
                     yield None
 
+    def parse_color_rules_str(self, color_rules_str, ignore_errors=True):
+        """
+        Parse the color rules. Returns a list of ColorRule objects.
+
+        Color rules are like: "0 < x < 4: red" or "blue" (for a catch-all rule)
+        """
+        rules = []
+        for lineno, line in enumerate(color_rules_str.splitlines()):
+            line = line.strip()
+            if line:
+                try:
+                    if ":" in line:
+                        condition, value = line.split(':')
+                        value = value.strip()
+                        if condition.isnumeric():  # A condition just listed as an exact value
+                            condition = "x == " + condition
+                    else:
+                        condition = "1"  # Always true
+                        value = line
+                    rules.append(ColorRule(condition, value))
+                except ValueError:
+                    if ignore_errors:
+                        continue
+                    raise ValueError(
+                        _("Invalid color rule on line {line_number}").format(line_number=lineno + 1)
+                    )
+        return rules
+
+    @lazy
+    def color_rules_parsed(self):
+        """
+        Caching property to get parsed color rules. Returns a list of ColorRule objects.
+        """
+        return self.parse_color_rules_str(self.color_rules) if self.color_rules else []
+
     def _get_submission_key(self, usage_key):
         """
         Given the usage_key of an MCQ block, get the dict key needed to look it up with the
@@ -125,6 +258,18 @@ class DashboardBlock(StudioEditableXBlockMixin, XBlock):
             item_type=usage_key.block_type,
         )
 
+    def color_for_value(self, value):
+        """ Given a string value, get the color rule that matches, if any """
+        if isinstance(value, basestring):
+            if value.isnumeric():
+                value = float(value)
+            else:
+                return None
+        for rule in self.color_rules_parsed:
+            if rule.matches(value):
+                return rule.color_str
+        return None
+
     def generate_content(self, include_report_link=True):
         """
         Create the HTML for this block, by getting the data and inserting it into a template.
@@ -149,13 +294,15 @@ class DashboardBlock(StudioEditableXBlockMixin, XBlock):
                     block['mcqs'].append({
                         "display_name": mcq_block.question,
                         "value": value,
-                        "color": self.color_codes.get(value),
+                        "color": self.color_for_value(value),
                     })
             # If the values are numeric, display an average:
             numeric_values = [float(mcq['value']) for mcq in block['mcqs'] if mcq['value'] and mcq['value'].isnumeric()]
             if numeric_values:
-                block['average'] = sum(numeric_values) / len(numeric_values)
+                average_value = sum(numeric_values) / len(numeric_values)
+                block['average'] = average_value
                 block['has_average'] = True
+                block['average_color'] = self.color_for_value(average_value)
             blocks.append(block)
 
         visual_repr = None
@@ -165,7 +312,7 @@ class DashboardBlock(StudioEditableXBlockMixin, XBlock):
             except ValueError:
                 pass  # JSON errors should be shown as part of validation
             else:
-                visual_repr = DashboardVisualData(blocks, rules_parsed)
+                visual_repr = DashboardVisualData(blocks, rules_parsed, self.color_for_value)
 
         return loader.render_template('templates/html/dashboard.html', {
             'blocks': blocks,
@@ -210,6 +357,12 @@ class DashboardBlock(StudioEditableXBlockMixin, XBlock):
         def add_error(msg):
             validation.add(ValidationMessage(ValidationMessage.ERROR, msg))
 
+        if data.color_rules:
+            try:
+                self.parse_color_rules_str(data.color_rules, ignore_errors=False)
+            except ValueError as e:
+                add_error(unicode(e))
+
         if data.visual_rules:
             try:
                 rules = json.loads(data.visual_rules)

mentoring/dashboard_visual.py

@@ -27,15 +27,13 @@ block.
 For example, each layer can have its color turn green if the student's average value on MCQs in
 a specific Problem Builder block was at least 3.
 """
-import ast
-import operator as op
 
 
 class DashboardVisualData(object):
     """
     Data about the visual representation of a dashboard.
     """
-    def __init__(self, blocks, rules):
+    def __init__(self, blocks, rules, color_for_value):
         """
         Construct the data required for the optional visual representation of the dashboard.
 
@@ -59,6 +57,8 @@ class DashboardVisualData(object):
             "width": "500",
             "height": "500"
         }
+
+        color_for_value is a method that, given a value, returns a color string or None
         """
         # Images is a list of images, one per PB block, in the same order as 'blocks'
         # All images are rendered layered on top of each other, and can be hidden,
@@ -68,11 +68,6 @@ class DashboardVisualData(object):
         overlay = rules.get("overlay")
         # Background is an optional images drawn on the bottom, with no effects applied
         background = rules.get("background")
-        # Color rules specify how the average value of the PB block affects that block's image.
-        # Each rule is evaluated in order, and the first matching rule is used.
-        # Each rule can have an "if" expression that is evaluated and if true, that rule will
-        # be used.
-        colorRules = rules.get("colorRules", [])
         # Width and height of the image:
         self.width = int(rules.get("width", 400))
         self.height = int(rules.get("height", 400))
@@ -90,74 +85,8 @@ class DashboardVisualData(object):
                 break
 
             # Check if a color rule applies:
-            x = block["average"]
-            for rule in colorRules:
-                condition = rule.get("if")
-                if condition and not self._safe_eval_expression(condition, x):
-                    continue  # This rule does not apply
-                # Rule does apply:
-                layer_data["has_filter"] = True
-                for key in ("hueRotate", "blur", "saturate"):
-                    if key in rule:
-                        layer_data[key] = self._safe_eval_expression(rule[key], x)
-                break
+            layer_data["color"] = color_for_value(block["average"])
             self.layers.append(layer_data)
 
         if overlay:
             self.layers.append({"url": overlay})
-
-    @staticmethod
-    def _safe_eval_expression(expr, x=0):
-        """
-        Safely evaluate a mathematical or boolean expression involving the value x
-
-        >>> _safe_eval_expression('2*x', x=3)
-        6
-        >>> _safe_eval_expression('x >= 0 and x < 2', x=3)
-        False
-
-        We use python syntax for the expressions, so the parsing and evaluation is mostly done
-        using Python's built-in asbstract syntax trees and operator implementations.
-
-        The expression can only contain: numbers, mathematical operators, boolean operators,
-        comparisons, and a placeholder variable called "x"
-        """
-        # supported operators:
-        operators = {
-            # Allow +, -, *, /, %, negative:
-            ast.Add: op.add, ast.Sub: op.sub, ast.Mult: op.mul, ast.Div: op.truediv, ast.Mod: op.mod, ast.USub: op.neg,
-            # Allow comparison:
-            ast.Eq: op.eq, ast.NotEq: op.ne, ast.Lt: op.lt, ast.LtE: op.le, ast.Gt: op.gt, ast.GtE: op.ge,
-        }
-
-        def eval_(node):
-            """ Recursive evaluation of syntax tree node 'node' """
-            if isinstance(node, ast.Num):  # <number>
-                return node.n
-            elif isinstance(node, ast.BinOp):  # <left> <operator> <right>
-                return operators[type(node.op)](eval_(node.left), eval_(node.right))
-            elif isinstance(node, ast.UnaryOp):  # <operator> <operand> e.g., -1
-                return operators[type(node.op)](eval_(node.operand))
-            elif isinstance(node, ast.Name) and node.id == "x":
-                return x
-            elif isinstance(node, ast.BoolOp):  # Boolean operator: either "and" or "or" with two or more values
-                if type(node.op) == ast.And:
-                    return all(eval_(val) for val in node.values)
-                else:  # Or:
-                    for val in node.values:
-                        result = eval_(val)
-                        if result:
-                            return result
-                    return result  # or returns the final value even if it's falsy
-            elif isinstance(node, ast.Compare):  # A comparison expression, e.g. "3 > 2" or "5 < x < 10"
-                left = eval_(node.left)
-                for comparison_op, right_expr in zip(node.ops, node.comparators):
-                    right = eval_(right_expr)
-                    if not operators[type(comparison_op)](left, right):
-                        return False
-                    left = right
-                return True
-            else:
-                raise TypeError(node)
-
-        return eval_(ast.parse(expr, mode='eval').body)

mentoring/public/css/dashboard.css

@@ -16,11 +16,6 @@
 .pb-dashboard table td:last-child {
     min-width: 3em;
     text-align: right;
-}
-
-.pb-dashboard table td:last-child span {
-    position: relative;
-    background: transparent;
     color: black;
     /* Background color could be anything, so add a white outline to preserve readability */
     text-shadow:

mentoring/templates/html/dashboard.html

@@ -7,17 +7,23 @@
     <svg width="{{visual_repr.width}}" height="{{visual_repr.height}}">
       <!-- Filter definitions -->
       {% for layer in visual_repr.layers %}
-        {% if layer.has_filter %}
+        {% if layer.color %}
           <filter id="{{layer.id}}">
-            <feColorMatrix in="SourceGraphic" type="hueRotate" values="{% if layer.hueRotate %}{{layer.hueRotate}}{% else %}0{% endif %}" result="hued" />
-            <feColorMatrix in="hued" type="saturate" values="{% if 'saturate' in layer %}{{layer.saturate}}{% else %}1{% endif %}" result="sat" />
-            <feGaussianBlur stdDeviation="{% if layer.blur %}{{layer.blur}}{% else %}0{% endif %}" in="sat" />
+            <feFlood flood-color="{{layer.color}}" result="flood" />
+            <feBlend in="flood" in2="SourceGraphic" mode="multiply" />
           </filter>
+          <mask id="{{layer.id}}-mask" maskUnits="userSpaceOnUse" x="0" y="0" width="100%" height="100%">
+            <image xlink:href="{{layer.url}}" x="0" y="0" height="100%" width="100%" />
+          </mask>
         {% endif %}
       {% endfor %}
       <!-- Layer images -->
       {% for layer in visual_repr.layers %}
-        <image xlink:href="{{layer.url}}" x="0" y="0" height="100%" width="100%" {% if layer.has_filter %}style="filter: url(#{{layer.id}});"{% endif %} />
+        {% if layer.color %}
+          <rect x="0" y="0" height="100%" width="100%" fill="{{layer.color}}" mask="url(#{{layer.id}}-mask)" />
+        {% else %}
+          <image xlink:href="{{layer.url}}" x="0" y="0" height="100%" width="100%" />
+        {% endif %}
       {% endfor %}
     </svg>
   </div>
@@ -34,14 +40,14 @@
           <tr>
             <td>{{ mcq.display_name }}</td>
             <td {% if mcq.color %}style="background-color: {{mcq.color}};"{% endif %}>
-              <span>{% if mcq.value %}{{ mcq.value }}{% endif %}</span>
+              {% if mcq.value %}{{ mcq.value }}{% endif %}
             </td>
           </tr>
         {% endfor %}
         {% if block.has_average %}
          <tr class="avg-row">
             <td>{% trans "Average" %}</td>
-            <td>{{ block.average|floatformat }}</td>
+            <td {% if block.average_color %}style="background-color: {{block.average_color}};"{% endif %}>{{ block.average|floatformat }}</td>
           </tr>
         {% endif %}
       </tbody>

mentoring/tests/integration/test_dashboard.py

@@ -118,7 +118,7 @@ class TestDashboardBlock(SeleniumXBlockTest):
         """)
 
         # 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):
+        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 = (

mentoring/tests/unit/test_dashboard_visual.py

@@ -43,32 +43,26 @@ class TestDashboardVisualData(unittest.TestCase):
             ],
             "background": "background.png",
             "overlay": "overlay.png",
-            "colorRules": [
-                {"if": "x < 1", "hueRotate": "20"},
-                {"if": "x < 2", "hueRotate": "80", "blur": "1"},
-                {"blur": "x / 2", "saturate": "0.4"}
-            ],
             "width": "500",
             "height": "500"
         }
-        data = DashboardVisualData(blocks, rules)
+
+        def color_for_value(value):
+            """ Mock color_for_value """
+            return "red" if value > 1 else None
+
+        data = DashboardVisualData(blocks, rules, color_for_value)
         self.assertEqual(len(data.layers), 5)
         self.assertEqual(data.layers[0]["url"], "background.png")
         self.assertEqual(data.layers[4]["url"], "overlay.png")
         self.assertEqual(data.width, 500)
         self.assertEqual(data.height, 500)
         # Check the three middle layers built from the average values:
-        # Step 1, average is 0 - first colorRule should match
         self.assertEqual(data.layers[1]["url"], "step1.png")
-        self.assertEqual(data.layers[1]["has_filter"], True)
-        self.assertEqual(data.layers[1]["hueRotate"], 20)
-        # Step 2, average is 1.3 - second colorRule should match
+        self.assertEqual(data.layers[1].get("color"), None)
+
         self.assertEqual(data.layers[2]["url"], "step2.png")
-        self.assertEqual(data.layers[2]["has_filter"], True)
-        self.assertEqual(data.layers[2]["hueRotate"], 80)
-        self.assertEqual(data.layers[2]["blur"], 1)
-        # Step 3, average is 30.8 - final colorRule should match
+        self.assertEqual(data.layers[2]["color"], "red")
+
         self.assertEqual(data.layers[3]["url"], "step3.png")
-        self.assertEqual(data.layers[3]["has_filter"], True)
-        self.assertEqual(data.layers[3]["blur"], 30.8/2)
-        self.assertEqual(data.layers[3]["saturate"], 0.4)
+        self.assertEqual(data.layers[3]["color"], "red")