Add minor improvements from review of kobotoolbox#3057

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:

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(

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')

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)

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:

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:
         <root>
           <group>
               <question_1>Value1</question_1>
@@ -71,11 +73,11 @@ def process_node(node_: etree._Element, xpath_matches_: list):
         </root>
 
          Nodes are processed in this order:
-         - `<question_1>`: Removed because not in `subset_field`
+         - `<question_1>`: Removed because not in `nodes_to_keep`
 
          - `<question_2>`: Kept. Parent node `<group>` is tagged `do_not_delete`
 
-         - `<group>`: Kept even if it is not in `subset_field` because
+         - `<group>`: Kept even if it is not in `nodes_to_keep` because
                       it is tagged `do_not_delete` by its child `<question_2>`
 
          - `<question_3>`: Kept.

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
 
     <pre class="prettyprint">
     <b>PATCH</b> /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

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/<parent_lookup_asset>/paired-data/<paired_data_uid>/external/
+        - /api/v2/assets/<parent_lookup_asset>/paired-data/<paired_data_uid>/external.xml/
         """
         paired_data = self.get_object()