From 52252875abf76a2143b0c333ee3b1cf0536c9387 Mon Sep 17 00:00:00 2001
From: Benjamin Dauvergne <bdauvergne@entrouvert.com>
Date: Mon, 13 Jul 2020 19:41:34 +0200
Subject: [PATCH] manager: show user import errors inline (#44803)

---
 src/authentic2/csv_import.py                  | 11 +++++
 .../manager/user_import_report.html           | 49 ++++++++++++++-----
 tests/test_user_manager.py                    | 14 ++++--
 3 files changed, 58 insertions(+), 16 deletions(-)

diff --git a/src/authentic2/csv_import.py b/src/authentic2/csv_import.py
index c8e514d8..be160516 100644
--- a/src/authentic2/csv_import.py
+++ b/src/authentic2/csv_import.py
@@ -312,9 +312,20 @@ class CsvRow(object):
     def action_display(self):
         return self.ACTIONS.get(self.action, self.action)
 
+    @property
+    def has_errors(self):
+        return self.errors or self.has_cell_errors
+
+    @property
+    def has_cell_errors(self):
+        return any(cell.errors for cell in self)
+
     def __iter__(self):
         return iter(self.cells)
 
+    def __len__(self):
+        return len(self.cells)
+
 
 @attrs
 class CsvCell(object):
diff --git a/src/authentic2/manager/templates/authentic2/manager/user_import_report.html b/src/authentic2/manager/templates/authentic2/manager/user_import_report.html
index 68940b91..ac75fdf0 100644
--- a/src/authentic2/manager/templates/authentic2/manager/user_import_report.html
+++ b/src/authentic2/manager/templates/authentic2/manager/user_import_report.html
@@ -108,24 +108,47 @@
           </thead>
           <tbody>
             {% for row in importer.rows %}
-              <tr
-                class="{% if row.is_valid %}row-valid{% else %}row-invalid{% endif %} row-action-{{ row.action }}"
-                {% if not row.is_valid %}title="{% for error in row.errors %}{% firstof error.description error.code %}
-                {% endfor %}{% for cell in row %}{% for error in cell.errors %}
-{{ cell.header.name }}: {% firstof error.description error.code %}{% endfor %}{% endfor %}"{% endif %}
-              >
+              <tr class="{% if row.is_valid %}row-valid{% else %}row-invalid{% endif %} row-action-{{ row.action }}">
                 <td class="row-line">{{ row.line }}</td>
                 {% for cell in row %}
-                  <td
-                    class="{% if cell.errors %}cell-errors{% endif %} {% if cell.action %}cell-action-{{ cell.action }}{% endif %}"
-                    {% if cell.errors %}title="{% for error in cell.errors %}{% firstof error.description error.code %}
-{% endfor %}"{% endif %}
-                  >
-                    {{ cell.value }}
-                  </td>
+                  <td class="{% if cell.errors %}cell-errors{% endif %} {% if cell.action %}cell-action-{{ cell.action }}{% endif %}">{{ cell.value }}</td>
                 {% endfor %}
                 <td class="row-action">{% firstof row.action_display "-" %}</td>
               </tr>
+              {% if not row.is_valid %}
+                {% if row.errors %}
+                  <tr class="row-invalid row-errors">
+                    <td></td>
+                    <td colspan="{{ row|length|add:"1" }}">
+                      <ul>
+                        {% for error in row.errors %}
+                          <li>{% firstof error.description error.code %}</li>
+                        {% endfor %}
+                      </ul>
+                    </td>
+                    <td></td>
+                  </tr>
+                {% endif %}
+                {% if row.has_cell_errors %}
+                  <tr class="row-invalid row-cells-errors">
+                    <td></td>
+                    {% for cell in row %}
+                      {% if cell.errors %}
+                        <td class="cell-errors cell-{{ cell.header.name }}">
+                          <ul>
+                            {% for error in cell.errors %}
+                              <li>{% firstof error.description error.code %}</li>
+                            {% endfor %}
+                          </ul>
+                        </td>
+                      {% else %}
+                        <td/>
+                      {% endif %}
+                    {% endfor %}
+                    <td></td>
+                  </tr>
+                {% endif %}
+              {% endif %}
             {% endfor %}
           </tbody>
         </table>
diff --git a/tests/test_user_manager.py b/tests/test_user_manager.py
index caf872e5..9f907f49 100644
--- a/tests/test_user_manager.py
+++ b/tests/test_user_manager.py
@@ -415,9 +415,15 @@ x,x,x,x'''.encode(encoding),
 
     response = response.click(href=simulate.uuid)
 
-    assert len(response.pyquery('table.main tbody tr')) == 3
+    assert len(response.pyquery('table.main tbody tr')) == 4
     assert len(response.pyquery('table.main tbody tr.row-valid')) == 2
-    assert len(response.pyquery('table.main tbody tr.row-invalid')) == 1
+    assert len(response.pyquery('table.main tbody tr.row-invalid')) == 2
+
+    assert len(response.pyquery('tr.row-errors')) == 0
+    assert len(response.pyquery('tr.row-cells-errors')) == 1
+    assert sum(bool(response.pyquery(td).text()) for td in response.pyquery('tr.row-cells-errors td li')) == 2
+    assert 'Enter a valid email address' in response.pyquery('tr.row-cells-errors td.cell-email li').text()
+    assert 'Phone number can start with' in response.pyquery('tr.row-cells-errors td.cell-phone li').text()
 
     assert User.objects.count() == user_count
 
@@ -582,7 +588,9 @@ Elliott,3'''
     login(app, admin, '/manage/users/')
     response = import_csv(content, app)
 
-    assert len(response.pyquery('table.main tbody tr.row-invalid')) == 1
+    assert len(response.pyquery('table.main tbody tr.row-invalid')) == 2
+    assert len(response.pyquery('table.main tbody tr.row-errors')) == 1
+    assert 'matches too many user' in response.pyquery('tr.row-errors').text()
 
 
 def test_manager_create_user_next(superuser_or_admin, app, ou1):
-- 
2.27.0