From 07d1dc182bcbfde9c12951bdea9cca07f0e2d414 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Laur=C3=A9line=20Gu=C3=A9rin?= <zebuline@entrouvert.com>
Date: Fri, 28 Jan 2022 14:54:39 +0100
Subject: [PATCH] wcs: explicit "id from URL" choice for card cell (#60957)

---
 combo/apps/wcs/forms.py |  22 ++++++++-
 tests/test_wcs.py       | 101 +++++++++++++++++++++++++++++++++-------
 2 files changed, 103 insertions(+), 20 deletions(-)

diff --git a/combo/apps/wcs/forms.py b/combo/apps/wcs/forms.py
index 4751fd86..380fd67d 100644
--- a/combo/apps/wcs/forms.py
+++ b/combo/apps/wcs/forms.py
@@ -98,8 +98,16 @@ class WcsCardInfoCellForm(forms.ModelForm):
             del self.fields['customize_display']
             del self.fields['custom_schema']
 
-        self.fields['related_card_path'].choices = [
-            ('', _('Other Card Identifiers'))
+        self.fields['related_card_path'].choices = []
+        with_sub_slug = any(p.sub_slug for p in self.instance.page.get_parents_and_self())
+        if with_sub_slug:
+            self.fields['related_card_path'].choices += [
+                ('--', _('Identifier from page URL')),
+            ]
+            if not self.instance.card_ids and not self.instance.related_card_path:
+                self.initial['related_card_path'] = '--'
+        self.fields['related_card_path'].choices += [
+            ('', _('Other Card Identifiers')),
         ] + self.instance.get_related_card_paths()
 
     def save(self, *args, **kwargs):
@@ -108,10 +116,20 @@ class WcsCardInfoCellForm(forms.ModelForm):
             self.instance.custom_schema = {}
         if self.instance.related_card_path:
             self.instance.card_ids = ''
+        if self.instance.related_card_path == '--':
+            self.instance.related_card_path = ''
         self.instance.without_user = not self.cleaned_data['with_user']
         self.instance.save()
         return self.instance
 
+    def clean(self):
+        cleaned_data = super().clean()
+
+        if not cleaned_data.get('related_card_path') and not cleaned_data.get('card_ids'):
+            self.add_error('card_ids', _('This field is required.'))
+
+        return cleaned_data
+
 
 class WcsCategoryCellForm(forms.ModelForm):
     class Meta:
diff --git a/tests/test_wcs.py b/tests/test_wcs.py
index f1736f55..94ae7a2c 100644
--- a/tests/test_wcs.py
+++ b/tests/test_wcs.py
@@ -1852,7 +1852,9 @@ def test_cards_cell_render_user(mock_send, context, nocache):
 
 @mock.patch('combo.apps.wcs.utils.requests.send', side_effect=mocked_requests_send)
 def test_card_cell_setup(mock_send, app, admin_user):
-    page = Page.objects.create(title='xxx', slug='test_card_cell_save_cache', template_name='standard')
+    page = Page.objects.create(
+        title='xxx', slug='test_card_cell_save_cache', template_name='standard', sub_slug='foobar'
+    )
     cell = WcsCardInfosCell(page=page, placeholder='content', order=0)
     form_class = cell.get_default_form_class()
     form = form_class(instance=cell)
@@ -1909,6 +1911,48 @@ def test_card_cell_setup(mock_send, app, admin_user):
     cell.refresh_from_db()
     assert cell.custom_schema == {}
 
+    assert cell.related_card_path == ''
+    assert cell.card_ids == ''
+    resp = app.get('/manage/pages/%s/' % page.pk)
+    assert resp.forms[0]['c%s-related_card_path' % cell.get_reference()].value == '--'
+    resp.forms[0]['c%s-card_ids' % cell.get_reference()].value = '42'
+    resp.forms[0].submit().follow()
+    cell.refresh_from_db()
+    assert cell.related_card_path == ''
+    assert cell.card_ids == ''
+    resp = app.get('/manage/pages/%s/' % page.pk)
+    assert resp.forms[0]['c%s-related_card_path' % cell.get_reference()].value == '--'
+    resp.forms[0]['c%s-related_card_path' % cell.get_reference()].value = ''
+    resp.forms[0]['c%s-card_ids' % cell.get_reference()].value = '42'
+    resp.forms[0].submit().follow()
+    cell.refresh_from_db()
+    assert cell.related_card_path == ''
+    assert cell.card_ids == '42'
+
+    # current page has a sub_slug, '--' option is present
+    resp = app.get('/manage/pages/%s/' % page.pk)
+    assert '--' in [o[0] for o in resp.forms[0]['c%s-related_card_path' % cell.get_reference()].options]
+
+    # current_page has no sub_slug, but parent page has one
+    parent_page = Page.objects.create(
+        title='parent', slug='parent', template_name='standard', sub_slug='foobar'
+    )
+    page.parent = parent_page
+    page.sub_slug = ''
+    page.save()
+    resp = app.get('/manage/pages/%s/' % page.pk)
+    assert '--' in [o[0] for o in resp.forms[0]['c%s-related_card_path' % cell.get_reference()].options]
+
+    # no sub_slug
+    parent_page.sub_slug = ''
+    parent_page.save()
+    resp = app.get('/manage/pages/%s/' % page.pk)
+    assert '--' not in [o[0] for o in resp.forms[0]['c%s-related_card_path' % cell.get_reference()].options]
+    assert resp.forms[0]['c%s-related_card_path' % cell.get_reference()].value == ''
+    resp.forms[0]['c%s-card_ids' % cell.get_reference()].value = ''
+    resp = resp.forms[0].submit()
+    assert resp.context['form'].errors == {'card_ids': ['This field is required.']}
+
 
 def test_card_cell_custom_schema_migration():
     cell = WcsCardInfosCell()
@@ -2092,7 +2136,7 @@ def test_card_cell_check_validity(mock_send):
 
 @mock.patch('combo.apps.wcs.utils.requests.send', side_effect=mocked_requests_send)
 def test_manager_card_cell(mock_send, app, admin_user):
-    page = Page.objects.create(title='xxx', slug='test_cards', template_name='standard')
+    page = Page.objects.create(title='xxx', slug='test_cards', template_name='standard', sub_slug='foobar')
     cell = WcsCardInfosCell.objects.create(page=page, placeholder='content', order=0)
 
     app = login(app)
@@ -2119,7 +2163,8 @@ def test_manager_card_cell(mock_send, app, admin_user):
     resp = app.get('/manage/pages/%s/' % page.pk)
     # but only one cell on the page, no relations to follow
     assert resp.forms[0]['c%s-related_card_path' % cell.get_reference()].options == [
-        ('', True, 'Other Card Identifiers')
+        ('--', True, 'Identifier from page URL'),
+        ('', False, 'Other Card Identifiers'),
     ]
 
     # add a second cell, related to the first card model
@@ -2129,11 +2174,13 @@ def test_manager_card_cell(mock_send, app, admin_user):
     resp = app.get('/manage/pages/%s/' % page.pk)
     # still no relation to follow
     assert resp.forms[0]['c%s-related_card_path' % cell.get_reference()].options == [
-        ('', True, 'Other Card Identifiers')
+        ('--', True, 'Identifier from page URL'),
+        ('', False, 'Other Card Identifiers'),
     ]
     # no cell with id and slug
     assert resp.forms[1]['c%s-related_card_path' % cell2.get_reference()].options == [
-        ('', True, 'Other Card Identifiers')
+        ('--', True, 'Identifier from page URL'),
+        ('', False, 'Other Card Identifiers'),
     ]
 
     # set a slug on first cell
@@ -2142,11 +2189,13 @@ def test_manager_card_cell(mock_send, app, admin_user):
     resp = app.get('/manage/pages/%s/' % page.pk)
     # still no relation to follow
     assert resp.forms[0]['c%s-related_card_path' % cell.get_reference()].options == [
-        ('', True, 'Other Card Identifiers')
+        ('--', True, 'Identifier from page URL'),
+        ('', False, 'Other Card Identifiers'),
     ]
     # multiple relations to follow
     assert resp.forms[1]['c%s-related_card_path' % cell2.get_reference()].options == [
-        ('', True, 'Other Card Identifiers'),
+        ('--', True, 'Identifier from page URL'),
+        ('', False, 'Other Card Identifiers'),
         ('sluga/cardb', False, 'sluga/cardb'),
         ('sluga/cardsb', False, 'sluga/cardsb'),
         ('sluga/blockb_cardb', False, 'sluga/blockb_cardb'),
@@ -2161,11 +2210,13 @@ def test_manager_card_cell(mock_send, app, admin_user):
     resp = app.get('/manage/pages/%s/' % page.pk)
     # still no relation to follow
     assert resp.forms[0]['c%s-related_card_path' % cell.get_reference()].options == [
-        ('', True, 'Other Card Identifiers')
+        ('--', False, 'Identifier from page URL'),
+        ('', True, 'Other Card Identifiers'),
     ]
     # can not user cell with multiple ids as reference
     assert resp.forms[1]['c%s-related_card_path' % cell2.get_reference()].options == [
-        ('', True, 'Other Card Identifiers')
+        ('--', True, 'Identifier from page URL'),
+        ('', False, 'Other Card Identifiers'),
     ]
 
     # define a slug on second cell
@@ -2176,14 +2227,16 @@ def test_manager_card_cell(mock_send, app, admin_user):
     resp = app.get('/manage/pages/%s/' % page.pk)
     # multiple relations to follow
     assert resp.forms[0]['c%s-related_card_path' % cell.get_reference()].options == [
-        ('', True, 'Other Card Identifiers'),
+        ('--', True, 'Identifier from page URL'),
+        ('', False, 'Other Card Identifiers'),
         ('slugb/reverse:cardb', False, 'slugb/cardb (reverse)'),
         ('slugb/reverse:cardsb', False, 'slugb/cardsb (reverse)'),
         ('slugb/reverse:blockb_cardb', False, 'slugb/blockb_cardb (reverse)'),
     ]
     # still multiple relations to follow
     assert resp.forms[1]['c%s-related_card_path' % cell2.get_reference()].options == [
-        ('', True, 'Other Card Identifiers'),
+        ('--', True, 'Identifier from page URL'),
+        ('', False, 'Other Card Identifiers'),
         ('sluga/cardb', False, 'sluga/cardb'),
         ('sluga/cardsb', False, 'sluga/cardsb'),
         ('sluga/blockb_cardb', False, 'sluga/blockb_cardb'),
@@ -2202,10 +2255,12 @@ def test_manager_card_cell(mock_send, app, admin_user):
     resp = app.get('/manage/pages/%s/' % page.pk)
     # no more relation to follow
     assert resp.forms[0]['c%s-related_card_path' % cell.get_reference()].options == [
-        ('', True, 'Other Card Identifiers')
+        ('--', True, 'Identifier from page URL'),
+        ('', False, 'Other Card Identifiers'),
     ]
     # still multiple relations to follow
     assert resp.forms[1]['c%s-related_card_path' % cell2.get_reference()].options == [
+        ('--', False, 'Identifier from page URL'),
         ('', False, 'Other Card Identifiers'),
         ('sluga/cardb', True, 'sluga/cardb'),
         ('sluga/cardsb', False, 'sluga/cardsb'),
@@ -2214,6 +2269,10 @@ def test_manager_card_cell(mock_send, app, admin_user):
         ('sluga/cardc/cardsb', False, 'sluga/cardc/cardsb'),
         ('sluga/cardc/blockb_cardb', False, 'sluga/cardc/blockb_cardb'),
     ]
+    resp.forms[1].submit()
+    cell2.refresh_from_db()
+    assert cell2.related_card_path == 'sluga/cardb'
+    assert cell2.card_ids == ''
 
     # check circular relations
     cell.slug = 'sluge'
@@ -2225,12 +2284,14 @@ def test_manager_card_cell(mock_send, app, admin_user):
     cell2.save()
     resp = app.get('/manage/pages/%s/' % page.pk)
     assert resp.forms[0]['c%s-related_card_path' % cell.get_reference()].options == [
-        ('', True, 'Other Card Identifiers'),
+        ('--', True, 'Identifier from page URL'),
+        ('', False, 'Other Card Identifiers'),
         ('slugd/cardd-foo/carde-foo', False, 'slugd/cardd-foo/carde-foo'),
         ('slugd/carde-foo', False, 'slugd/carde-foo'),
     ]
     assert resp.forms[1]['c%s-related_card_path' % cell2.get_reference()].options == [
-        ('', True, 'Other Card Identifiers'),
+        ('--', True, 'Identifier from page URL'),
+        ('', False, 'Other Card Identifiers'),
         ('sluge/cardd-bar', False, 'sluge/cardd-bar'),
         ('sluge/reverse:carde-foo', False, 'sluge/carde-foo (reverse)'),
     ]
@@ -2244,14 +2305,16 @@ def test_manager_card_cell(mock_send, app, admin_user):
     cell2.save()
     resp = app.get('/manage/pages/%s/' % page.pk)
     assert resp.forms[0]['c%s-related_card_path' % cell.get_reference()].options == [
-        ('', True, 'Other Card Identifiers'),
+        ('--', True, 'Identifier from page URL'),
+        ('', False, 'Other Card Identifiers'),
         ('slugd-bis/cardd-foo', False, 'slugd-bis/cardd-foo'),
         ('slugd-bis/reverse:cardd-foo', False, 'slugd-bis/cardd-foo (reverse)'),
         ('slugd-bis/carde-foo/cardd-bar', False, 'slugd-bis/carde-foo/cardd-bar'),
         ('slugd-bis/carde-foo/reverse:carde-foo', False, 'slugd-bis/carde-foo/carde-foo (reverse)'),
     ]
     assert resp.forms[1]['c%s-related_card_path' % cell2.get_reference()].options == [
-        ('', True, 'Other Card Identifiers'),
+        ('--', True, 'Identifier from page URL'),
+        ('', False, 'Other Card Identifiers'),
         ('slugd/cardd-foo', False, 'slugd/cardd-foo'),
         ('slugd/reverse:cardd-foo', False, 'slugd/cardd-foo (reverse)'),
         ('slugd/carde-foo/cardd-bar', False, 'slugd/carde-foo/cardd-bar'),
@@ -2267,11 +2330,13 @@ def test_manager_card_cell(mock_send, app, admin_user):
     cell2.save()
     resp = app.get('/manage/pages/%s/' % page.pk)
     assert resp.forms[0]['c%s-related_card_path' % cell.get_reference()].options == [
-        ('', True, 'Other Card Identifiers'),
+        ('--', True, 'Identifier from page URL'),
+        ('', False, 'Other Card Identifiers'),
         ('sluge-bis/cardd-bar/carde-foo', False, 'sluge-bis/cardd-bar/carde-foo'),
     ]
     assert resp.forms[1]['c%s-related_card_path' % cell2.get_reference()].options == [
-        ('', True, 'Other Card Identifiers'),
+        ('--', True, 'Identifier from page URL'),
+        ('', False, 'Other Card Identifiers'),
         ('sluge/cardd-bar/carde-foo', False, 'sluge/cardd-bar/carde-foo'),
     ]
 
-- 
2.34.1

