Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Allow duplicate choices in snapshots #3757

Merged
merged 2 commits into from
Apr 5, 2022
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 2 additions & 2 deletions kpi/fixtures/test_data.json
Original file line number Diff line number Diff line change
Expand Up @@ -112,8 +112,8 @@
"kuid": "def"
}
]},
"date_created": "2015-02-12T19:52:14.406Z",
"date_modified": "2015-02-12T19:52:14.406Z",
"date_created": "2022-04-05T21:00:22.402Z",
"date_modified": "2022-04-05T21:00:33.946Z",
"owner": 2,
"uid": "aPEANgNgTH3xzhJGcctURu"
}
Expand Down
3 changes: 3 additions & 0 deletions kpi/models/asset_snapshot.py
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,7 @@
from kpi.utils.hash import calculate_hash
from kpi.utils.log import logging
from kpi.utils.models import DjangoModelABCMetaclass
from kpi.utils.pyxform_compatibility import allow_choice_duplicates


class AbstractFormList(
Expand Down Expand Up @@ -197,6 +198,8 @@ def generate_xml_from_source(self,
self._populate_fields_with_autofields(source_copy)
self._strip_kuids(source_copy)

allow_choice_duplicates(source_copy)

warnings = []
details = {}
try:
Expand Down
2 changes: 1 addition & 1 deletion kpi/tests/api/v1/test_api_imports.py
Original file line number Diff line number Diff line change
Expand Up @@ -23,7 +23,7 @@ class AssetImportTaskTest(BaseTestCase):
def setUp(self):
self.client.login(username='someuser', password='someuser')
self.user = User.objects.get(username='someuser')
self.asset = Asset.objects.first()
self.asset = Asset.objects.get(pk=1)

def _assert_assets_contents_equal(self, a1, a2, sheet='survey'):
def _prep_row_for_comparison(row):
Expand Down
4 changes: 2 additions & 2 deletions kpi/tests/api/v2/test_api_assets.py
Original file line number Diff line number Diff line change
Expand Up @@ -81,7 +81,7 @@ def test_asset_list_matches_detail(self):

def test_assets_hash(self):
another_user = User.objects.get(username="anotheruser")
user_asset = Asset.objects.first()
user_asset = Asset.objects.get(pk=1)
user_asset.save()
user_asset.assign_perm(another_user, "view_asset")

Expand Down Expand Up @@ -257,7 +257,7 @@ class AssetVersionApiTests(BaseTestCase):

def setUp(self):
self.client.login(username='someuser', password='someuser')
self.asset = Asset.objects.first()
self.asset = Asset.objects.get(pk=1)
self.asset.save()
self.version = self.asset.asset_versions.first()
self.version_list_url = reverse(self._get_endpoint('asset-version-list'),
Expand Down
2 changes: 1 addition & 1 deletion kpi/tests/api/v2/test_api_imports.py
Original file line number Diff line number Diff line change
Expand Up @@ -23,7 +23,7 @@ class AssetImportTaskTest(BaseTestCase):
def setUp(self):
self.client.login(username='someuser', password='someuser')
self.user = User.objects.get(username='someuser')
self.asset = Asset.objects.first()
self.asset = Asset.objects.get(pk=1)

def _assert_assets_contents_equal(self, a1, a2, sheet='survey'):
def _prep_row_for_comparison(row):
Expand Down
20 changes: 20 additions & 0 deletions kpi/tests/test_asset_snapshots.py
Original file line number Diff line number Diff line change
Expand Up @@ -55,3 +55,23 @@ def test_xml_export_auto_title(self):
asset = Asset.objects.create(asset_type='survey', content=content)
_snapshot = asset.snapshot
self.assertEqual(_snapshot.source.get('settings')['form_title'], 'no_title_asset')

def test_snapshots_allow_choice_duplicates(self):
"""
Choice duplicates should be allowed here but *not* when deploying
a survey
"""
content = {
'survey': [
{'type': 'select_multiple',
'select_from_list_name': 'xxx',
'label': 'pick one'},
],
'choices': [
{'list_name': 'xxx', 'label': 'ABC', 'name': 'ABC'},
{'list_name': 'xxx', 'label': 'Also ABC', 'name': 'ABC'},
],
'settings': {},
}
snap = AssetSnapshot.objects.create(source=content)
assert snap.xml.count('<value>ABC</value>') == 2
29 changes: 29 additions & 0 deletions kpi/tests/test_utils.py
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,7 @@
from kpi.exceptions import SearchQueryTooShortException
from kpi.utils.autoname import autoname_fields, autoname_fields_to_field
from kpi.utils.autoname import autovalue_choices_in_place
from kpi.utils.pyxform_compatibility import allow_choice_duplicates
from kpi.utils.query_parser import parse
from kpi.utils.sluggify import sluggify, sluggify_label
from kpi.utils.xml import strip_nodes, edit_submission_xml
Expand Down Expand Up @@ -224,6 +225,34 @@ def test_query_parser_default_search_too_short(self):
parse(query_string, default_field_lookups)
assert 'Your query is too short' in str(e.exception)

def test_allow_choice_duplicates(self):
surv = {
'survey': [
{'type': 'select_multiple',
'select_from_list_name': 'xxx'},
],
'choices': [
{'list_name': 'xxx', 'label': 'ABC', 'name': 'ABC'},
{'list_name': 'xxx', 'label': 'Also ABC', 'name': 'ABC'},
],
'settings': {},
}

# default should be 'yes'
allow_choice_duplicates(surv)
assert (
surv['settings']['allow_choice_duplicates']
== 'yes'
)

# 'no' should not be overwritten
surv['settings']['allow_choice_duplicates'] = 'no'
allow_choice_duplicates(surv)
assert (
surv['settings']['allow_choice_duplicates']
== 'no'
)


class XmlUtilsTestCase(TestCase):

Expand Down
15 changes: 15 additions & 0 deletions kpi/utils/pyxform_compatibility.py
Original file line number Diff line number Diff line change
@@ -0,0 +1,15 @@
from pyxform.constants import ALLOW_CHOICE_DUPLICATES

def allow_choice_duplicates(content: dict) -> None:
"""
Modify `content` to include `allow_choice_duplicates=Yes` in the settings
so long as that setting does not already exist.
This should *not* be done for content that's being newly updated by a user,
but rather *only* for internal conversions of existing content, e.g. when
generating XML for an `AssetVersion` that was created prior to pyxform
prohibiting duplicate choice values (aka choice names).
See https://github.com/kobotoolbox/kpi/issues/3751
"""
settings = content.setdefault('settings', {})
if ALLOW_CHOICE_DUPLICATES not in settings:
settings[ALLOW_CHOICE_DUPLICATES] = 'yes'