HDXDSYS-3031 Replace list fields such as groups in package_revise instead of merging by position

.github/workflows/run-python-tests.yaml

@@ -7,6 +7,9 @@ on:
   pull_request:
     branches-ignore: [gh-pages]
 
+concurrency:
+  group: ${{ github.workflow }}-${{ github.head_ref || github.ref_name }}
+
 jobs:
   build:
     runs-on: ubuntu-latest
@@ -40,13 +43,12 @@ jobs:
 
       - name: Publish Unit Test Results
         uses: EnricoMi/publish-unit-test-result-action@7622c5bf3d112a72ab4fa3951c2aa5723d8ab92d # v2.25.0
-        if: always()
+        if: (!cancelled())
         with:
           files: test-results.xml
 
       - name: Publish in Coveralls
         uses: coverallsapp/github-action@8d6379e14d29928660c4ba802d8e85393440b329 # v2.3.8
-        if: always()
         with:
           flag-name: tests
           format: lcov

pyproject.toml

@@ -38,8 +38,8 @@ dependencies = [
     "ckanapi>=4.11",
     "defopt>=7.0.0",
     "email_validator",
-    "hdx-python-country>=4.2.6",
-    "hdx-python-utilities>=4.2.6",
+    "hdx-python-country>=4.3.1",
+    "hdx-python-utilities>=4.2.7",
     "makefun",
     "requests",
 ]

src/hdx/data/dataset.py

@@ -667,68 +667,100 @@ def _prepare_hdx_call(self, data: dict, kwargs: Any) -> None:
             if "batch_mode" not in kwargs:
                 kwargs["batch_mode"] = "DONT_GROUP"
 
+    def _merge_list_fields(self, dataset_data_to_update: dict) -> list[str]:
+        """Merges each non-resource list in dataset_data_to_update with the
+        existing list of the same key (see _merge_list) in place.
+
+        Args:
+            dataset_data_to_update: Dataset data to be updated
+
+        Returns:
+            Keys whose merged list differs from the existing one
+        """
+        changed_keys = []
+        for key, value in dataset_data_to_update.items():
+            if (
+                key == "resources"
+                or not isinstance(value, list)
+                or key not in self._old_data
+            ):
+                continue
+            orig_list = self._old_data[key]
+            if not isinstance(orig_list, list):
+                orig_list = []
+            merged_list = self._merge_list(orig_list, value)
+            dataset_data_to_update[key] = merged_list
+            if merged_list != orig_list:
+                changed_keys.append(key)
+        return changed_keys
+
+    @staticmethod
+    def _merge_list(orig_list: list, new_list: list) -> list:
+        """Merges each dict in new_list that has an id or name with the dict in
+        orig_list with the same id (or failing that, name) so that fields not
+        given in new_list are kept. Other elements of new_list are kept as is.
+        Elements of orig_list that are not matched are dropped.
+
+        Args:
+            orig_list: Existing list
+            new_list: Updated list
+
+        Returns:
+            Merged list
+        """
+        merged_list = []
+        for new_value in new_list:
+            if not isinstance(new_value, dict):
+                merged_list.append(new_value)
+                continue
+            match = {}
+            for field in ("id", "name"):
+                field_value = new_value.get(field)
+                if not field_value:
+                    continue
+                match = next(
+                    (
+                        orig_value
+                        for orig_value in orig_list
+                        if isinstance(orig_value, dict)
+                        and orig_value.get(field) == field_value
+                    ),
+                    {},
+                )
+                if match:
+                    break
+            merged_list.append(match | new_value)
+        return merged_list
+
     def _revise_filter(
         self,
-        dataset_data_to_update: dict,
         keys_to_delete: Sequence[str],
         resources_to_delete: Sequence[int],
-    ):
+        changed_list_keys: Sequence[str],
+    ) -> list[str]:
         """Returns the revise filter parameter adding to it any keys in the
-        dataset metadata and resources that are specified to be deleted. Also compare
-        lists in original and updated metadata to see if any have had elements
-        removed in which case these should be added to the filter
+        dataset metadata and resources that are specified to be deleted. Keys
+        of changed lists are also added so that package_revise replaces those
+        lists rather than merging them by position (which keeps the existing
+        elements' ids eg. for groups).
 
         Args:
-            dataset_data_to_update: Dataset data to be updated
             keys_to_delete: List of top level metadata keys to delete
             resources_to_delete: List of indexes of resources to delete
+            changed_list_keys: Keys of lists that differ from the existing ones
+
+        Returns:
+            Revise filter
         """
         revise_filter = []
         for key in keys_to_delete:
             revise_filter.append(f"-{key}")
         if not self.is_requestable():
             for resource_index in resources_to_delete:
                 revise_filter.append(f"-resources__{resource_index}")
-        for key, value in dataset_data_to_update.items():
-            if not isinstance(value, list):
-                continue
-            if key not in self._old_data:
-                continue
-            orig_list = self._old_data[key]
-            elements_to_remove = []
-            for i, orig_value in enumerate(orig_list):
-                if isinstance(orig_value, dict) and any(
-                    x in orig_value for x in ("id", "name")
-                ):
-                    # Where lists have an id or name, we use one of those to
-                    # match elements and work out what needs to be deleted
-                    # ie. any elements in the original list that don't exist
-                    # in the updated list
-                    el_id = orig_value.get("id")
-                    el_name = orig_value.get("name")
-                    present = False
-                    for value_dict in value:
-                        if el_id and "id" in value_dict and value_dict["id"] == el_id:
-                            present = True
-                            break
-                        if (
-                            el_name
-                            and "name" in value_dict
-                            and value_dict["name"] == el_name
-                        ):
-                            present = True
-                            break
-                    if not present:
-                        elements_to_remove.append(i)
-                elif orig_value not in value:
-                    # Otherwise we do a simple exact match of list elements
-                    # and delete any elements that are in the original list but
-                    # not in the updated list
-                    elements_to_remove.append(i)
-            for element_index in reversed(elements_to_remove):
-                del orig_list[element_index]
-                if element_index == len(orig_list):
-                    revise_filter.append(f"-{key}__{element_index}")
+        for key in changed_list_keys:
+            if key not in keys_to_delete:
+                revise_filter.append(f"-{key}")
         return revise_filter
 
     def _revise_files_to_upload_resource_deletions(
@@ -806,8 +838,9 @@ def _revise_dataset(
         ):  # Whether or not CKAN should perform validation steps (checking fields present)
             dataset_data_to_update["skip_validation"] = kwargs["skip_validation"]
         dataset_data_to_update["state"] = "active"
+        changed_list_keys = self._merge_list_fields(dataset_data_to_update)
         revise_filter = self._revise_filter(
-            dataset_data_to_update, keys_to_delete, resources_to_delete
+            keys_to_delete, resources_to_delete, changed_list_keys
         )
         files_to_upload = self._revise_files_to_upload_resource_deletions(
             resources_to_update,
@@ -1048,8 +1081,8 @@ def _dataset_hdx_update(
             **kwargs,
         )
         keep_crisis_tags = kwargs.get("keep_crisis_tags", True)
-        if keep_crisis_tags:
-            old_tags = self._old_data.get("tags", [])
+        if keep_crisis_tags and "tags" in self._old_data:
+            old_tags = self._old_data["tags"]
             for tag in self.data.get("tags", []):
                 tag_name = tag["name"]
                 if tag_name[:7] != "crisis-":
@@ -1061,7 +1094,6 @@ def _dataset_hdx_update(
                         break
                 if not found:
                     old_tags.append(tag)
-            self._old_data["tags"] = old_tags
         self._prepare_hdx_call(self._old_data, kwargs)
         revise_call = self._revise_dataset(
             allow_no_resources,

tests/hdx/api/test_ckan.py

@@ -29,7 +29,8 @@
 
 class TestCKAN:
     @pytest.fixture(scope="class")
-    def configuration(self):
+    @classmethod
+    def configuration(cls):
         hdx_key = getenv("HDX_KEY_TEST")
         Configuration._create(
             hdx_site="demo",
@@ -47,7 +48,8 @@ def datasetmetadata(self, fixturesfolder):
         return fixturesfolder / "CKAN" / "hdx_dataset_static.yaml"
 
     @pytest.fixture(scope="class")
-    def params(self):
+    @classmethod
+    def params(cls):
         return {
             "corpora": "teamDrive",
             "teamDriveId": "0AKCBfHI3H-hcUk9PVA",

tests/hdx/api/test_configuration.py

@@ -11,39 +11,48 @@
 
 class TestConfiguration:
     @pytest.fixture(scope="class")
-    def hdx_base_config_yaml(self, configfolder):
+    @classmethod
+    def hdx_base_config_yaml(cls, configfolder):
         return configfolder / "hdx_base_config.yaml"
 
     @pytest.fixture(scope="class")
-    def hdx_base_config_json(self, configfolder):
+    @classmethod
+    def hdx_base_config_json(cls, configfolder):
         return configfolder / "hdx_base_config.json"
 
     @pytest.fixture(scope="class")
-    def hdx_missing_site_config_json(self, configfolder):
+    @classmethod
+    def hdx_missing_site_config_json(cls, configfolder):
         return configfolder / "hdx_missing_site_config.json"
 
     @pytest.fixture(scope="class")
-    def project_config_json(self, configfolder):
+    @classmethod
+    def project_config_json(cls, configfolder):
         return configfolder / "project_configuration.json"
 
     @pytest.fixture(scope="class")
-    def user_agent_config_yaml(self, configfolder):
+    @classmethod
+    def user_agent_config_yaml(cls, configfolder):
         return configfolder / "user_agent_config.yaml"
 
     @pytest.fixture(scope="class")
-    def user_agent_config2_yaml(self, configfolder):
+    @classmethod
+    def user_agent_config2_yaml(cls, configfolder):
         return configfolder / "user_agent_config2.yaml"
 
     @pytest.fixture(scope="class")
-    def user_agent_config3_yaml(self, configfolder):
+    @classmethod
+    def user_agent_config3_yaml(cls, configfolder):
         return configfolder / "user_agent_config3.yaml"
 
     @pytest.fixture(scope="class")
-    def empty_yaml(self, configfolder):
+    @classmethod
+    def empty_yaml(cls, configfolder):
         return configfolder / "empty.yaml"
 
     @pytest.fixture(scope="class")
-    def user_agent_config_wrong_yaml(self, configfolder):
+    @classmethod
+    def user_agent_config_wrong_yaml(cls, configfolder):
         return configfolder / "user_agent_config_wrong.yaml"
 
     def test_init(

tests/hdx/api/utilities/test_hdx_state.py

@@ -20,27 +20,33 @@
 
 class TestState:
     @pytest.fixture(scope="class")
-    def tempfolder(self):
+    @classmethod
+    def tempfolder(cls):
         return Path(gettempdir(), "test_state")
 
     @pytest.fixture(scope="class")
-    def statefolder(self, fixturesfolder):
+    @classmethod
+    def statefolder(cls, fixturesfolder):
         return fixturesfolder / "state"
 
     @pytest.fixture(scope="class")
-    def statefile(self):
+    @classmethod
+    def statefile(cls):
         return "last_build_date.txt"
 
     @pytest.fixture(scope="class")
-    def multidatestatefile(self):
+    @classmethod
+    def multidatestatefile(cls):
         return "analysis_dates.txt"
 
     @pytest.fixture(scope="class")
-    def date1(self):
+    @classmethod
+    def date1(cls):
         return datetime(2020, 9, 23, 0, 0, tzinfo=UTC)
 
     @pytest.fixture(scope="class")
-    def date2(self):
+    @classmethod
+    def date2(cls):
         return datetime(2022, 5, 12, 10, 15, tzinfo=UTC)
 
     @pytest.fixture(scope="function")

tests/hdx/data/test_dataset_core.py

@@ -171,11 +171,13 @@ def mockall(url, datadict):
 
 class TestDatasetCore:
     @pytest.fixture(scope="class")
-    def static_yaml(self, configfolder):
+    @classmethod
+    def static_yaml(cls, configfolder):
         return configfolder / "hdx_dataset_static.yaml"
 
     @pytest.fixture(scope="class")
-    def static_json(self, configfolder):
+    @classmethod
+    def static_json(cls, configfolder):
         return configfolder / "hdx_dataset_static.json"
 
     @pytest.fixture(scope="function")

tests/hdx/data/test_dataset_noncore.py

@@ -55,7 +55,8 @@ class TestDatasetNoncore:
     association = None
 
     @pytest.fixture(scope="class")
-    def static_resource_view_yaml(self, configfolder):
+    @classmethod
+    def static_resource_view_yaml(cls, configfolder):
         return configfolder / "hdx_resource_view_static.yaml"
 
     @pytest.fixture(scope="function")

tests/hdx/data/test_dataset_resource_generation.py

@@ -13,7 +13,7 @@
 
 
 class TestDatasetResourceGeneration:
-    url = "https://raw.githubusercontent.com/OCHA-DAP/hdx-python-api/main/tests/fixtures/test_data.csv"
+    url = "tests/fixtures/test_data.csv"
 
     def test_download_generate_resource(self, configuration, fixturesfolder):
         with temp_dir("test") as folder:
@@ -462,7 +462,7 @@ def process_year(row):
                     row_function=process_row,
                 )
                 assert success is True
-                url = "https://raw.githubusercontent.com/OCHA-DAP/hdx-python-api/main/tests/fixtures/empty.csv"
+                url = "tests/fixtures/empty.csv"
                 success, results = dataset.download_generate_resource(
                     downloader,
                     url,
@@ -474,7 +474,7 @@ def process_year(row):
                     yearcol="YEAR",
                 )
                 assert success is False
-                url = "https://raw.githubusercontent.com/OCHA-DAP/hdx-python-api/main/tests/fixtures/gen_resource/test_data_no_data.csv"
+                url = "tests/fixtures/gen_resource/test_data_no_data.csv"
                 success, results = dataset.download_generate_resource(
                     downloader,
                     url,
@@ -502,7 +502,7 @@ def process_year(row):
                     row_function=process_row,
                 )
                 assert success is False
-                url = "https://raw.githubusercontent.com/OCHA-DAP/hdx-python-api/main/tests/fixtures/gen_resource/test_data_no_years.csv"
+                url = "tests/fixtures/gen_resource/test_data_no_years.csv"
                 success, results = dataset.download_generate_resource(
                     downloader,
                     url,