From da1f81a5ae7ba5ef2334619ce908496d20b72f1f Mon Sep 17 00:00:00 2001 From: "John N. Milner" Date: Wed, 14 Jul 2021 05:39:43 -0400 Subject: [PATCH] Add minor improvements from review of #3057 --- kpi/serializers/v2/asset.py | 2 +- kpi/serializers/v2/paired_data.py | 2 +- kpi/tests/api/v2/test_api_paired_data.py | 3 +++ kpi/tests/test_utils.py | 1 + kpi/utils/query_parser/query_parser.py | 3 ++- kpi/utils/xml.py | 22 ++++++++++++---------- kpi/views/v2/asset.py | 8 ++++++-- kpi/views/v2/paired_data.py | 13 ++++++++++--- 8 files changed, 36 insertions(+), 18 deletions(-) diff --git a/kpi/serializers/v2/asset.py b/kpi/serializers/v2/asset.py index fb71034a76..96160baeb0 100644 --- a/kpi/serializers/v2/asset.py +++ b/kpi/serializers/v2/asset.py @@ -494,7 +494,7 @@ def validate_data_sharing(self, data_sharing: dict) -> dict: Only the type of each property is validated. No data is validated. It is consistent with partial permissions and REST services. - The responsibility of valid date is on users + The client bears the responsibility of providing valid data. """ errors = {} if not self.instance or not data_sharing: diff --git a/kpi/serializers/v2/paired_data.py b/kpi/serializers/v2/paired_data.py index bbeb9eb76e..bf8602ff26 100644 --- a/kpi/serializers/v2/paired_data.py +++ b/kpi/serializers/v2/paired_data.py @@ -193,7 +193,7 @@ def _validate_filename(self, attrs: dict): filename in media_filenames or ( filename in paired_data_filenames.values() - and (is_new or not is_new and pd_filename != filename) + and (is_new or (not is_new and pd_filename != filename)) ) ): raise serializers.ValidationError( diff --git a/kpi/tests/api/v2/test_api_paired_data.py b/kpi/tests/api/v2/test_api_paired_data.py index 1d2c1c5d5c..3f74042f29 100644 --- a/kpi/tests/api/v2/test_api_paired_data.py +++ b/kpi/tests/api/v2/test_api_paired_data.py @@ -239,6 +239,9 @@ def test_create_by_destination_editor(self): self.assertEqual(response.status_code, status.HTTP_403_FORBIDDEN) # Let's give 'change_asset' to user quidam. + # It should succeed now because quidam is allowed to modify the + # destination asset AND the owner of the destination asset + # (anotheruser) is allowed to view submissions of the source asset self.destination_asset.assign_perm(self.quidam, PERM_CHANGE_ASSET) response = self.paired_data(login_username='quidam', login_password='quidam') diff --git a/kpi/tests/test_utils.py b/kpi/tests/test_utils.py index 9aa8a371e5..998e9d713c 100644 --- a/kpi/tests/test_utils.py +++ b/kpi/tests/test_utils.py @@ -392,6 +392,7 @@ def test_strip_xml_nodes_by_xpaths_with_slashes(self): ) def __compare_xml(self, source: str, target: str) -> bool: + """ Attempts to standardize XML by removing whitespace between tags """ pattern = r'\s*(<[^>]+>)\s*' re_source = re.sub(pattern, r'\1', source) re_target = re.sub(pattern, r'\1', target) diff --git a/kpi/utils/query_parser/query_parser.py b/kpi/utils/query_parser/query_parser.py index c094cd6a60..fb4519fb57 100644 --- a/kpi/utils/query_parser/query_parser.py +++ b/kpi/utils/query_parser/query_parser.py @@ -58,7 +58,8 @@ def process_value(field, value): if value == 'null': return None - # Handle booleans + # Handle booleans - necessary when querying inside JSONBFields, and + # also some other contexts: see `get_parsed_parameters()` try: lower_value = value.lower() except AttributeError: diff --git a/kpi/utils/xml.py b/kpi/utils/xml.py index 3f7e398969..e9d98082c3 100644 --- a/kpi/utils/xml.py +++ b/kpi/utils/xml.py @@ -1,6 +1,6 @@ # coding: utf-8 import re -from typing import Union +from typing import Optional, Union from lxml import etree @@ -9,10 +9,13 @@ def strip_nodes( nodes_to_keep: list, use_xpath: bool = False, xml_declaration: bool = False, + rename_root_node_to: Optional[str] = None, ) -> str: """ Returns a stripped version of `source`. It keeps only nodes provided in - `nodes_to_keep` + `nodes_to_keep`. + If `rename_root_node_to` is provided, the root node will be renamed to the + value of that parameter in the returned XML string. """ # Force `source` to be bytes in case it contains an XML declaration # `etree` does not support strings with xml declarations. @@ -29,9 +32,7 @@ def get_xpath_matches(): if use_xpath: xpaths_ = [] for xpath_ in nodes_to_keep: - leading_slash = '' if xpath_.startswith('/') else '/' - trailing_slash = '' if xpath_.endswith('/') else '/' - xpaths_.append(f'{leading_slash}{xpath_}{trailing_slash}') + xpaths_.append(f"/{xpath_.strip('/')}/") return xpaths_ xpath_matches = [] @@ -42,8 +43,9 @@ def get_xpath_matches(): # To make a difference between XPaths with same beginning # string, we need to add a trailing slash for later comparison # in `process_node()`. - # For example, `subgroup1` and `subgroup11` both start with - # `subgroup1` but `subgroup11/` and `subgroup1/` do not. + # For example, `subgroup1` would match both `subgroup1/` and + # `subgroup11/`, but `subgroup1/` correctly excludes + # `subgroup11/` xpath_matches.append(f'{xpath_match}/') return xpath_matches @@ -61,7 +63,7 @@ def process_node(node_: etree._Element, xpath_matches_: list): parents must be kept. For example: - With `subset_fields = ['question_2', 'question_3']` and this XML: + With `nodes_to_keep = ['question_2', 'question_3']` and this XML: Value1 @@ -71,11 +73,11 @@ def process_node(node_: etree._Element, xpath_matches_: list): Nodes are processed in this order: - - ``: Removed because not in `subset_field` + - ``: Removed because not in `nodes_to_keep` - ``: Kept. Parent node `` is tagged `do_not_delete` - - ``: Kept even if it is not in `subset_field` because + - ``: Kept even if it is not in `nodes_to_keep` because it is tagged `do_not_delete` by its child `` - ``: Kept. diff --git a/kpi/views/v2/asset.py b/kpi/views/v2/asset.py index 866418c0b3..df4c1c0ce1 100644 --- a/kpi/views/v2/asset.py +++ b/kpi/views/v2/asset.py @@ -253,7 +253,8 @@ class AssetViewSet(NestedViewSetMixin, viewsets.ModelViewSet): > curl -X GET https://[kpi]/api/v2/assets/aSAvYreNzVEkrWg5Gdcvg/reports/ ### Data sharing - Enable data sharing for the current asset + + Control sharing of submission data from this project to other projects 
     PATCH /api/v2/assets/{uid}/
@@ -273,7 +274,10 @@ class AssetViewSet(NestedViewSetMixin, viewsets.ModelViewSet):
     >        }
     >
 
-    * `fields`: Optional. List of questions of asset represented by their XPath. I.e., Hierarchy group must be kept.
+    * `fields`: Optional. List of questions whose responses will be shared. If
+        missing or empty, all responses will be shared. Questions must be
+        identified by full group path separated by slashes, e.g.
+        `group/subgroup/question_name`.
 
     >
     > Response
diff --git a/kpi/views/v2/paired_data.py b/kpi/views/v2/paired_data.py
index 109e293864..0f3da4cba1 100644
--- a/kpi/views/v2/paired_data.py
+++ b/kpi/views/v2/paired_data.py
@@ -56,7 +56,8 @@ class PairedDataViewset(AssetNestedObjectViewsetMixin,
     >       }
     >
 
-    The endpoint is paginated. Accept these parameters:
+    This endpoint is paginated and accepts these parameters:
+
     - `offset`: The initial index from which to return the results
     - `limit`: Number of results to return per page
 
@@ -90,8 +91,11 @@ class PairedDataViewset(AssetNestedObjectViewsetMixin,
     >       }
     >
 
-    * `fields`: Optional. List of questions of source asset represented by their XPath I.e., Hierarchy group must be kept.
-    * `filename`: Must be unique among all asset files. Only accept letters, numbers and '-'.
+    * `fields`: Optional. List of questions whose responses will be retrieved
+        from the source data. If missing or empty, all responses will be
+        retrieved. Questions must be identified by full group path separated by
+        slashes, e.g. `group/subgroup/question_name`.
+    * `filename`: Must be unique among all asset files. Only accepts letters, numbers and '-'.
 
     ### Retrieve a project
 
@@ -184,6 +188,9 @@ class PairedDataViewset(AssetNestedObjectViewsetMixin,
     def external(self, request, paired_data_uid, **kwargs):
         """
         Returns an XML which contains data submitted to paired asset
+        Creates the endpoints
+        - /api/v2/assets//paired-data//external/
+        - /api/v2/assets//paired-data//external.xml/
         """
         paired_data = self.get_object()