Merge pull request #1078 from rdmorganiser/fix_import_element_sorting

Fix import element sorting when saving (for related items)

rdmo/core/constants.py

@@ -81,15 +81,15 @@
 }
 
 RDMO_MODELS = {
-  'catalog': 'questions.catalog',
-  'section': 'questions.section',
-  'page': 'questions.page',
-  'questionset': 'questions.questionset',
-  'question': 'questions.question',
   'attribute': 'domain.attribute',
-  'optionset': 'options.optionset',
-  'option': 'options.option',
   'condition': 'conditions.condition',
+  'option': 'options.option',
+  'optionset': 'options.optionset',
+  'question': 'questions.question',
+  'questionset': 'questions.questionset',
+  'page': 'questions.page',
+  'section': 'questions.section',
+  'catalog': 'questions.catalog',
   'task': 'tasks.task',
   'view': 'views.view'
 }

rdmo/core/import_helpers.py

@@ -33,6 +33,8 @@ def get_value(self, **kwargs):
 
     @staticmethod
     def get_value_from_callback(callback, kwargs):
+        if not callable(callback):
+            raise TypeError('callback must be callable')
         sig = signature(callback)
         kwargs = {k: val for k, val in kwargs.items() if k in sig.parameters}
         value = callback(**kwargs)

rdmo/core/imports.py

@@ -138,11 +138,6 @@ def track_changes_on_element(element: dict,
         lookup_field = element_field if instance_field is None else instance_field
         original_value = getattr(original, lookup_field, '')
 
-    if isinstance(new_value,str) and isinstance(original_value,int):
-        # typecasting of original value to str, for comparison '0' == 0
-        # specific edge-case, maybe generalize later
-        original_value = str(original_value)
-
     element[ImportElementFields.DIFF][element_field][ImportElementFields.CURRENT] = original_value
     element[ImportElementFields.DIFF][element_field][ImportElementFields.NEW] = new_value
 
@@ -207,7 +202,7 @@ def track_changes_on_uri_of_foreign_field(element, field_name, foreign_uri, orig
     track_changes_on_element(element, field_name, new_value=foreign_uri, original_value=original_foreign_uri)
 
 
-def set_foreign_field(instance, field_name, element, uploaded_uris=None, original=None) -> None:
+def set_foreign_field(instance, field_name, element, original=None) -> None:
     if field_name not in element:
         return
 
@@ -278,45 +273,46 @@ def set_foreign_field(instance, field_name, element, uploaded_uris=None, origina
 
 
 def set_extra_field(instance, field_name, element,
-                    extra_field_helper: Optional[ExtraFieldHelper] = None, original=None) -> None:
+                    extra_field_helper: Optional[ExtraFieldHelper] = None,
+                    ) -> None:
 
-    extra_value = None
+    extra_field_value = None
     if field_name in element:
-        extra_value = element.get(field_name)
+        extra_field_value = element.get(field_name)
     else:
         # get the default field value from the instance
         instance_value = getattr(instance, field_name)
         element[field_name] = instance_value
-        extra_value = instance_value
+        extra_field_value = instance_value
 
     if extra_field_helper is not None:
         # default_value
         extra_value_from_helper = extra_field_helper.get_value(instance=instance,
                                                    key=field_name)
         # overwrite None or '' values by the get_value from the helper
-        if extra_value is None or extra_value == '':
-            extra_value = extra_value_from_helper
+        if extra_field_value is None or extra_field_value == '':
+            extra_field_value = extra_value_from_helper
 
         if extra_field_helper.overwrite_in_element:
-            element[field_name] = extra_value
-
-    if extra_value is not None:
-        setattr(instance, field_name, extra_value)
-        # track changes
-        track_changes_on_element(element, field_name, new_value=extra_value, original=original)
+            element[field_name] = extra_field_value
 
+    if extra_field_value is not None:
+        setattr(instance, field_name, extra_field_value)
 
 def track_changes_m2m_instances(element, field_name,
                                 foreign_instances, original=None):
     if original is None:
         return
     original_m2m_instance = getattr(original, field_name)
-    if original_m2m_instance is None:
-        return
-    original_m2m_uris = list(original_m2m_instance.values_list('uri', flat=True))
-    foreign_uris = [i.uri for i in foreign_instances]
-    track_changes_on_element(element, field_name, new_value=foreign_uris,
-                             original_value=original_m2m_uris)
+    original_m2m_instance = original_m2m_instance or []
+    # m2m instance fields are unordered so comparison by set
+    original_uris = set(original_m2m_instance.values_list('uri', flat=True))
+    foreign_uris = {i.uri for i in foreign_instances}
+    common_uris = list(original_uris & foreign_uris)
+    original_uris_list = common_uris + list(original_uris - foreign_uris)
+    foreign_uris_list = common_uris + list(foreign_uris - original_uris)
+    track_changes_on_element(element, field_name, new_value=foreign_uris_list,
+                             original_value=original_uris_list)
 
 
 def set_m2m_through_instances(instance, element, field_name=None, source_name=None,
@@ -354,6 +350,7 @@ def set_m2m_through_instances(instance, element, field_name=None, source_name=No
 
     for target_element in target_elements:
         target_uri = target_element.get('uri')
+        target_element['order'] = int(target_element['order']) # cast to int for ordering
         order = target_element.get('order')
         new_data.append(target_element)
 
@@ -497,6 +494,7 @@ def set_reverse_m2m_through_instance(instance, element, field_name=None, source_
 
     for target_element in target_elements:
         target_uri = target_element.get('uri')
+        target_element['order'] = int(target_element['order'])  # cast to int for ordering
         order = target_element.get('order')
         new_data.append(target_element)
 

rdmo/core/xml.py

@@ -16,7 +16,7 @@
 
 logger = logging.getLogger(__name__)
 
-DEFAULT_RDMO_XML_VERSION = '1.11.0'
+LEGACY_RDMO_XML_VERSION = '1.11.0'
 ELEMENTS_USING_KEY = {RDMO_MODELS['attribute']}
 
 
@@ -45,7 +45,7 @@ def validate_root(root: Optional[xmlElement]) -> Tuple[bool, Optional[str]]:
 
 
 def validate_and_get_xml_version_from_root(root: xmlElement) -> Tuple[Optional[Version], list]:
-    unparsed_root_version = root.attrib.get('version') or DEFAULT_RDMO_XML_VERSION
+    unparsed_root_version = root.attrib.get('version') or LEGACY_RDMO_XML_VERSION
     root_version, rdmo_version = parse(unparsed_root_version), parse(RDMO_INSTANCE_VERSION)
     if root_version > rdmo_version:
         logger.info('Import failed version validation (%s > %s)', root_version, rdmo_version)
@@ -117,13 +117,13 @@ def parse_xml_to_elements(xml_file=None) -> Tuple[OrderedDict, list]:
         return OrderedDict(), errors
 
     # step 3.1.1: validate the legacy elements
-    legacy_errors = validate_legacy_elements(elements, parse(root.attrib.get('version', DEFAULT_RDMO_XML_VERSION)))
+    legacy_errors = validate_legacy_elements(elements, parse(root.attrib.get('version', LEGACY_RDMO_XML_VERSION)))
     if legacy_errors:
         errors.extend(legacy_errors)
         return OrderedDict(), errors
 
     # step 4: convert elements from previous versions
-    elements = convert_elements(elements, parse(root.attrib.get('version', DEFAULT_RDMO_XML_VERSION)))
+    elements = convert_elements(elements, parse(root.attrib.get('version', LEGACY_RDMO_XML_VERSION)))
 
     # step 5: order the elements and return
     # ordering of elements is done in the import_elements function
@@ -257,6 +257,22 @@ def validate_pre_conversion_for_missing_key_in_legacy_elements(elements, version
             raise ValueError(f"Missing legacy elements, elements containing 'key' were expected for this XML with version {version} and elements {models_in_elements}.")   # noqa: E501
 
 
+def update_related_legacy_elements(elements: Dict,
+                                   target_uri: str, source_model: str,
+                                   legacy_element_field: str, element_field: str):
+    # search for the related elements that use the uri
+    related_elements = [
+        element for element in elements.values()
+        if element['model'] == source_model
+        and element.get(legacy_element_field, {}).get('uri') == target_uri
+    ]
+    # write the related elements back into the related element
+    elements[target_uri][element_field] = [
+        {k: v for k, v in element.items() if k in ('uri', 'model', 'order')}
+        for element in related_elements
+    ]
+
+
 def convert_legacy_elements(elements):
     # first pass: identify pages
     for uri, element in elements.items():
@@ -275,20 +291,30 @@ def convert_legacy_elements(elements):
 
         elif element['model'] == 'questions.catalog':
             element['uri_path'] = element.pop('key')
+            # Add sections to the catalog
+            update_related_legacy_elements(elements, uri, 'questions.section', 'catalog', 'sections')
 
         elif element['model'] == 'questions.section':
             del element['key']
             element['uri_path'] = element.pop('path')
-
-            if element.get('catalog') is not None:
-                element['catalog']['order'] = element.pop('order')
+            del element['catalog']  # sections do not have catalog anymore
+            # Add section_pages to the section
+            update_related_legacy_elements(elements, uri, 'questions.page', 'section', 'pages')
 
         elif element['model'] == 'questions.page':
             del element['key']
             element['uri_path'] = element.pop('path')
+            del element['section']  # pages do not have sections anymore
 
-            if element.get('section') is not None:
-                element['section']['order'] = element.pop('order')
+            # Add page_questionsets to the page
+            # Add questionsets to the page
+            update_related_legacy_elements(elements, uri, 'questions.questionset', 'questionset', 'questionsets')
+
+            # Add page_questions to the page
+            update_related_legacy_elements(elements, uri, 'questions.question', 'question', 'questions')
+
+            # Add page_conditions to the page
+            update_related_legacy_elements(elements, uri, 'conditions.condition', 'condition', 'conditions')
 
         elif element['model'] == 'questions.questionset':
             del element['key']
@@ -298,11 +324,15 @@ def convert_legacy_elements(elements):
             if parent is not None:
                 if elements[parent].get('model') == 'questions.page':
                     # this questionset belongs to a page now
-                    del element['questionset']
-                    element['page'] = {
-                        'uri': parent,
+                    parent_questionsets = elements[parent].get('questionset')
+                    parent_questionsets = parent_questionsets or []
+                    parent_questionsets.append({
+                        'uri': element['uri'],
+                        'model': element['model'],
                         'order': element.pop('order')
-                    }
+                    })
+                    elements[parent]['questionset'] = parent_questionsets
+                    del element['questionset']
                 else:
                     # this questionset still belongs to a questionset
                     element['questionset']['order'] = element.pop('order')
@@ -313,26 +343,26 @@ def convert_legacy_elements(elements):
 
             parent = element.get('questionset').get('uri')
             if parent is not None:
-                if elements[parent].get('model') == 'questions.page':
-                    # this question belongs to a page now
-                    del element['questionset']
-                    element['page'] = {
-                        'uri': parent,
-                        'order': element.pop('order')
-                    }
-                else:
-                    # this question still belongs to a questionset
-                    element['questionset']['order'] = element.pop('order')
+                parent_questionsets = elements[parent].get('questions', [])
+                parent_questionsets.append({
+                    'uri': element['uri'],
+                    'model': element['model'],
+                    'order': element.pop('order')
+                })
+                elements[parent]['questions'] = parent_questionsets
+                del element['questionset']
 
         elif element['model'] == 'options.optionset':
             element['uri_path'] = element.pop('key')
 
+            update_related_legacy_elements(elements, uri, 'options.option', 'optionset', 'options')
+
         elif element['model'] == 'options.option':
             del element['key']
             element['uri_path'] = element.pop('path')
 
-            if element.get('optionset') is not None:
-                element['optionset']['order'] = element.pop('order')
+            del element['optionset']  # options do not have optionsets anymore
+
 
         if element['model'] == 'tasks.task':
             element['uri_path'] = element.pop('key')
@@ -356,59 +386,21 @@ def convert_additional_input(elements):
 
     return elements
 
-def get_related_elements(element, ignored_keys=None):
-    ignored_keys = ignored_keys or list(ImportElementFields)
-    related_elements = {k: val for k, val in element.items() if
-                       k not in ignored_keys and (isinstance(val, (dict, list)))}
-    return related_elements
-
-
-def sort_by_relatives(elements, descendants_first=False, ancestors_first=False):
-    ancestors, descendants = [], []
 
-    if not descendants_first and not ancestors_first:
-        return elements
-
-    for uri, element in elements.items():
-        try:
-            has_descendants = get_related_elements(element)
-        except AttributeError:
-            has_descendants = False
-        if has_descendants:
-            ancestors.append((uri, element))
-        else:
-            descendants.append((uri, element))
-    if descendants_first:
-        sort_list = descendants + ancestors
-    elif ancestors_first:
-        sort_list = ancestors + descendants
-
-    sorted_elements = OrderedDict()
-    for uri,element in sort_list:
-        sorted_elements[uri] = element
-    return sorted_elements
-
-
-def order_elements(elements, order_sets_first=False, descendants_first=False) -> OrderedDict:
+def order_elements(elements: OrderedDict) -> OrderedDict:
     ordered_elements = OrderedDict()
-    if descendants_first:
-        elements = sort_by_relatives(elements, descendants_first=descendants_first)
-    for uri, element in elements.items():
-        append_element(ordered_elements, elements, uri, element, order_sets_first=order_sets_first)
+    for uri, element in reversed(elements.items()):
+        append_element(ordered_elements, elements, uri, element,)
     return ordered_elements
 
 
-def append_element(ordered_elements, unordered_elements, uri, element, order_sets_first=False) -> None:
+def append_element(ordered_elements, unordered_elements, uri, element) -> None:
     if element is None:
         return
+    for key, element_value in element.items():
+        if key in list(ImportElementFields):
+            continue
 
-    related_elements = get_related_elements(element)
-
-    if order_sets_first:
-        if related_elements and uri not in ordered_elements:
-            ordered_elements[uri] = element
-
-    for key, element_value in related_elements.items():
         if isinstance(element_value, dict):
             sub_uri = element_value.get('uri')
             sub_element = unordered_elements.get(sub_uri)
@@ -417,11 +409,10 @@ def append_element(ordered_elements, unordered_elements, uri, element, order_set
 
         elif isinstance(element_value, list):
             for value in element_value:
-                if isinstance(element_value, dict):
-                    sub_uri = value.get('uri')
-                    sub_element = unordered_elements.get(sub_uri)
-                    if sub_uri not in ordered_elements and sub_uri is not None:
-                        append_element(ordered_elements, unordered_elements, sub_uri, sub_element)
+                sub_uri = value.get('uri')
+                sub_element = unordered_elements.get(sub_uri)
+                if sub_uri not in ordered_elements and sub_uri is not None:
+                    append_element(ordered_elements, unordered_elements, sub_uri, sub_element)
 
     if uri not in ordered_elements:
         ordered_elements[uri] = element

rdmo/domain/imports.py

@@ -18,7 +18,6 @@ def build_attribute_uri(instance: Optional[Attribute]=None):
         return instance.build_uri(instance.uri_prefix, instance.path)
 
 
-
 import_helper_attribute = ElementImportHelper(
     model=Attribute,
     common_fields=('uri_prefix', 'key', 'comment'),

rdmo/management/assets/js/components/import/common/WarningsListGroup.js

@@ -37,8 +37,4 @@ WarningsListGroup.propTypes = {
   shouldShowURI: PropTypes.bool
 }
 
-WarningsListGroup.defaultProps = {
-  shouldShowURI: true,
-}
-
 export default WarningsListGroup