Repository: datosgobar/pydatajson

Issue:
Agregar opción para copiar el `identifier` original, sin prependear el `catalog_id`
**Contexto**

Poder escribir un dataset con su identifier original es útil para utilizar la función no como federador, sino como forma de restaurar los metadatos de un dataset a partir de un backup anterior.

**Propuesta**

Agregar un argumento opcional `prepend_catalog_identifier = True` que indica si debe o no _prependearse_ el identificador de un catálogo al identificar del dataset que se desea escribir en CKAN.

Discussion:

Patch:
diff --git a/docs/MANUAL.md b/docs/MANUAL.md
index 39d9678..cd72809 100644
--- a/docs/MANUAL.md
+++ b/docs/MANUAL.md
@@ -87,13 +87,14 @@ Existen dos métodos, cuyos reportes se incluyen diariamente entre los archivos
 ### Métodos para federación de datasets
 
 - **pydatajson.DataJson.push_dataset_to_ckan()**: Copia la metadata de un dataset y la escribe en un portal de CKAN.
-Toma los siguientes parámetros:
-  - **catalog_id**: El prefijo que va a preceder el id del dataset en el portal destino. 
+Toma los siguientes parámetros: 
   - **owner_org**: La organización a la que pertence el dataset. Debe encontrarse en el portal de destino.
   - **dataset_origin_identifier**: Identificador del dataset en el catálogo de origen.
   - **portal_url**: URL del portal de CKAN de destino.
   - **apikey**: La apikey de un usuario del portal de destino con los permisos para crear el dataset bajo la
   organización pasada como parámetro.
+  - **catalog_id** (opcional, default: None): El prefijo que va a preceder el id del dataset en el portal destino,
+  separado por un guión bajo.
   - **demote_superThemes** (opcional, default: True):Si está en true, los ids de los themes del dataset, se escriben
   como groups de CKAN.
   - **demote_themes** (opcional, default: True): Si está en true, los labels de los themes del dataset, se escriben como
diff --git a/pydatajson/ckan_utils.py b/pydatajson/ckan_utils.py
index b915bde..f71570d 100644
--- a/pydatajson/ckan_utils.py
+++ b/pydatajson/ckan_utils.py
@@ -14,12 +14,12 @@ def append_attribute_to_extra(package, dataset, attribute, serialize=False):
         package['extras'].append({'key': attribute, 'value': value})
 
 
-def map_dataset_to_package(dataset, catalog_id, owner_org, theme_taxonomy,
+def map_dataset_to_package(dataset, owner_org, theme_taxonomy, catalog_id=None,
                            demote_superThemes=True, demote_themes=True):
     package = dict()
     package['extras'] = []
 #   Obligatorios
-    package['id'] = catalog_id+'_'+dataset['identifier']
+    package['id'] = catalog_id+'_'+dataset['identifier'] if catalog_id else dataset['identifier']
     package['name'] = title_to_name(dataset['title'], decode=False)
     package['title'] = dataset['title']
     package['private'] = False
@@ -88,12 +88,12 @@ def convert_iso_string_to_utc(date_string):
     return utc_date_time.isoformat()
 
 
-def map_distributions_to_resources(distributions, catalog_id):
+def map_distributions_to_resources(distributions, catalog_id=None):
     resources = []
     for distribution in distributions:
         resource = dict()
 #       Obligatorios
-        resource['id'] = catalog_id + '_' + distribution['identifier']
+        resource['id'] = catalog_id + '_' + distribution['identifier'] if catalog_id else distribution['identifier']
         resource['name'] = distribution['title']
         resource['url'] = distribution['downloadURL']
         resource['created'] = convert_iso_string_to_utc(distribution['issued'])
diff --git a/pydatajson/federation.py b/pydatajson/federation.py
index 7807dfe..2c31623 100644
--- a/pydatajson/federation.py
+++ b/pydatajson/federation.py
@@ -9,17 +9,17 @@ from .ckan_utils import map_dataset_to_package
 from .search import get_datasets
 
 
-def push_dataset_to_ckan(catalog, catalog_id, owner_org, dataset_origin_identifier, portal_url, apikey,
-                         demote_superThemes=True, demote_themes=True):
+def push_dataset_to_ckan(catalog, owner_org, dataset_origin_identifier, portal_url, apikey,
+                         catalog_id=None, demote_superThemes=True, demote_themes=True):
     """Escribe la metadata de un dataset en el portal pasado por parámetro.
 
         Args:
             catalog (DataJson): El catálogo de origen que contiene el dataset.
-            catalog_id (str): El prefijo con el que va a preceder el id del dataset en catálogo destino.
             owner_org (str): La organización a la cual pertence el dataset.
             dataset_origin_identifier (str): El id del dataset que se va a federar.
             portal_url (str): La URL del portal CKAN de destino.
             apikey (str): La apikey de un usuario con los permisos que le permitan crear o actualizar el dataset.
+            catalog_id (str): El prefijo con el que va a preceder el id del dataset en catálogo destino.
             demote_superThemes(bool): Si está en true, los ids de los super themes del dataset, se propagan como grupo.
             demote_themes(bool): Si está en true, los labels de los themes del dataset, pasan a ser tags. Sino,
             se pasan como grupo.
@@ -31,7 +31,7 @@ def push_dataset_to_ckan(catalog, catalog_id, owner_org, dataset_origin_identifi
     ckan_portal = RemoteCKAN(portal_url, apikey=apikey)
     theme_taxonomy = catalog.themes
 
-    package = map_dataset_to_package(dataset, catalog_id, owner_org, theme_taxonomy,
+    package = map_dataset_to_package(dataset, owner_org, theme_taxonomy, catalog_id,
                                      demote_superThemes, demote_themes)
 
     # Get license id


Test patch:
diff --git a/tests/test_ckan_integration.py b/tests/test_ckan_integration.py
index 9b68543..9b76a24 100644
--- a/tests/test_ckan_integration.py
+++ b/tests/test_ckan_integration.py
@@ -57,8 +57,8 @@ class PushTestCase(unittest.TestCase):
         catalog_id = title_to_name(catalog['title'])
         dataset = catalog.datasets[0]
         dataset_id = dataset['identifier']
-        return_id = push_dataset_to_ckan(catalog, catalog_id, "oficina-de-muestra", dataset_id,
-                                         self.portal_url, self.apikey)
+        return_id = push_dataset_to_ckan(catalog, "oficina-de-muestra", dataset_id,
+                                         self.portal_url, self.apikey, catalog_id=catalog_id,)
         self.assertEqual(return_id, catalog_id + '_' + dataset_id)
 
     @CKAN_VCR.use_cassette()
@@ -66,12 +66,12 @@ class PushTestCase(unittest.TestCase):
         catalog = self.full_catalog
         catalog_id = title_to_name(catalog['title'])
         dataset_id = catalog.datasets[0]['identifier']
-        push_dataset_to_ckan(catalog, catalog_id, "oficina-de-muestra", dataset_id,
-                             self.portal_url, self.apikey)
+        push_dataset_to_ckan(catalog, "oficina-de-muestra", dataset_id,
+                             self.portal_url, self.apikey, catalog_id=catalog_id,)
 
         catalog.datasets[0]['description'] = 'updated description'
-        return_id = push_dataset_to_ckan(catalog, catalog_id, "oficina-de-muestra", dataset_id,
-                                         self.portal_url, self.apikey)
+        return_id = push_dataset_to_ckan(catalog, "oficina-de-muestra", dataset_id,
+                                         self.portal_url, self.apikey, catalog_id=catalog_id,)
 
         data_dict = {'id': catalog_id + '_' + dataset_id}
         package = self.portal.call_action('package_show', data_dict=data_dict)
@@ -83,21 +83,22 @@ class PushTestCase(unittest.TestCase):
         catalog_id = 'same-catalog-id'
         full_dataset = self.full_catalog.datasets[0]
         full_dataset_id = full_dataset['identifier']
-        push_dataset_to_ckan(self.full_catalog, catalog_id, 'oficina-de-muestra', full_dataset_id,
-                             self.portal_url, self.apikey)
+        push_dataset_to_ckan(self.full_catalog, 'oficina-de-muestra', full_dataset_id,
+                             self.portal_url, self.apikey, catalog_id=catalog_id,)
 
         justice_dataset = self.justice_catalog.datasets[0]
         justice_dataset_id = justice_dataset['identifier']
-        push_dataset_to_ckan(self.justice_catalog, catalog_id, 'oficina-de-muestra', justice_dataset_id,
-                             self.portal_url, self.apikey)
+        push_dataset_to_ckan(self.justice_catalog, 'oficina-de-muestra', justice_dataset_id,
+                             self.portal_url, self.apikey, catalog_id=catalog_id,)
         # Switch them and update
         full_dataset['distribution'], justice_dataset['distribution'] = \
             justice_dataset['distribution'], full_dataset['distribution']
 
-        full_package_id = push_dataset_to_ckan(self.full_catalog, catalog_id, 'oficina-de-muestra', full_dataset_id,
-                                               self.portal_url, self.apikey)
-        justice_package_id = push_dataset_to_ckan(self.justice_catalog, catalog_id, 'oficina-de-muestra',
-                                                  justice_dataset_id, self.portal_url, self.apikey)
+        full_package_id = push_dataset_to_ckan(self.full_catalog,'oficina-de-muestra', full_dataset_id,
+                                               self.portal_url, self.apikey, catalog_id=catalog_id,)
+        justice_package_id = push_dataset_to_ckan(self.justice_catalog, 'oficina-de-muestra',
+                                                  justice_dataset_id, self.portal_url,
+                                                  self.apikey, catalog_id=catalog_id,)
         # Switch them back
         full_dataset['distribution'], justice_dataset['distribution'] = \
             justice_dataset['distribution'], full_dataset['distribution']
diff --git a/tests/test_ckan_utils.py b/tests/test_ckan_utils.py
index dca112c..3f20984 100644
--- a/tests/test_ckan_utils.py
+++ b/tests/test_ckan_utils.py
@@ -22,18 +22,25 @@ class DatasetConversionTestCase(unittest.TestCase):
         cls.dataset_id = cls.dataset.get('identifier')
         cls.distributions = cls.dataset['distribution']
 
+    def test_catalog_id_is_prepended_to_dataset_id_if_passed(self):
+        package = map_dataset_to_package(self.dataset, 'owner', self.catalog.themes, catalog_id=self.catalog_id)
+        self.assertEqual(self.catalog_id + '_' + self.dataset_id, package['id'])
+
+    def test_dataset_id_is_preserved_if_catlog_id_is_not_passed(self):
+        package = map_dataset_to_package(self.dataset, 'owner', self.catalog.themes)
+        self.assertEqual(self.dataset_id, package['id'])
+
     def test_replicated_plain_attributes_are_corrext(self):
-        package = map_dataset_to_package(self.dataset, self.catalog_id, 'owner', self.catalog.themes)
+        package = map_dataset_to_package(self.dataset, 'owner', self.catalog.themes, catalog_id=self.catalog_id)
         plain_replicated_attributes = [('title', 'title'),
                                        ('notes', 'description'),
                                        ('url', 'landingPage')]
         for fst, snd in plain_replicated_attributes:
             self.assertEqual(self.dataset.get(snd), package.get(fst))
         self.assertEqual('owner', package['owner_org'])
-        self.assertEqual(self.catalog_id+'_'+self.dataset_id, package['id'])
 
     def test_dataset_nested_replicated_attributes_stay_the_same(self):
-        package = map_dataset_to_package(self.dataset, self.catalog_id, 'owner', self.catalog.themes)
+        package = map_dataset_to_package(self.dataset, 'owner', self.catalog.themes, catalog_id=self.catalog_id)
         contact_point_nested = [('maintainer', 'fn'),
                                 ('maintainer_email', 'hasEmail')]
         for fst, snd in contact_point_nested:
@@ -44,7 +51,7 @@ class DatasetConversionTestCase(unittest.TestCase):
             self.assertEqual(self.dataset.get('publisher').get(snd), package.get(fst))
 
     def test_dataset_array_attributes_are_correct(self):
-        package = map_dataset_to_package(self.dataset, self.catalog_id, 'owner', self.catalog.themes)
+        package = map_dataset_to_package(self.dataset, 'owner', self.catalog.themes, catalog_id=self.catalog_id)
         groups = [group['name'] for group in package.get('groups', [])]
         super_themes = [title_to_name(s_theme.lower()) for s_theme in self.dataset.get('superTheme')]
         try:
@@ -67,8 +74,8 @@ class DatasetConversionTestCase(unittest.TestCase):
             self.assertCountEqual(keywords + theme_labels, tags)
 
     def test_themes_are_preserved_if_not_demoted(self):
-        package = map_dataset_to_package(self.dataset, self.catalog_id, 'owner',
-                                         self.catalog.themes, demote_themes=False)
+        package = map_dataset_to_package(self.dataset, 'owner', self.catalog.themes,
+                                         catalog_id=self.catalog_id, demote_themes=False)
         groups = [group['name'] for group in package.get('groups', [])]
         super_themes = [title_to_name(s_theme.lower()) for s_theme in self.dataset.get('superTheme')]
         themes = self.dataset.get('theme', [])
@@ -85,8 +92,8 @@ class DatasetConversionTestCase(unittest.TestCase):
             self.assertCountEqual(keywords, tags)
 
     def test_superThemes_dont_impact_groups_if_not_demoted(self):
-        package = map_dataset_to_package(self.dataset, self.catalog_id, 'owner',
-                                         self.catalog.themes, demote_superThemes=False)
+        package = map_dataset_to_package(self.dataset, 'owner', self.catalog.themes,
+                                         catalog_id=self.catalog_id, demote_superThemes=False)
         groups = [group['name'] for group in package.get('groups', [])]
         tags = [tag['name'] for tag in package['tags']]
         keywords = self.dataset.get('keyword', [])
@@ -105,8 +112,8 @@ class DatasetConversionTestCase(unittest.TestCase):
             self.assertCountEqual(keywords + theme_labels, tags)
 
     def test_preserve_themes_and_superThemes(self):
-        package = map_dataset_to_package(self.dataset, self.catalog_id, 'owner',
-                                         self.catalog.themes, False, False)
+        package = map_dataset_to_package(self.dataset, 'owner', self.catalog.themes,
+                                         self.catalog_id, False, False)
         groups = [group['name'] for group in package.get('groups', [])]
         tags = [tag['name'] for tag in package['tags']]
         keywords = self.dataset.get('keyword', [])
@@ -121,7 +128,7 @@ class DatasetConversionTestCase(unittest.TestCase):
             self.assertCountEqual(keywords, tags)
 
     def test_dataset_extra_attributes_are_correct(self):
-        package = map_dataset_to_package(self.dataset, self.catalog_id, 'owner', self.catalog.themes)
+        package = map_dataset_to_package(self.dataset, 'owner', self.catalog.themes, catalog_id=self.catalog_id)
 #       extras are included in dataset
         if package['extras']:
             for extra in package['extras']:
@@ -137,7 +144,7 @@ class DatasetConversionTestCase(unittest.TestCase):
                     self.assertEqual(dataset_value, extra_value)
 
     def test_dataset_extra_attributes_are_complete(self):
-        package = map_dataset_to_package(self.dataset, self.catalog_id, 'owner', self.catalog.themes)
+        package = map_dataset_to_package(self.dataset, 'owner', self.catalog.themes, catalog_id=self.catalog_id)
 #       dataset attributes are included in extras
         extra_attrs = ['issued', 'modified', 'accrualPeriodicity', 'temporal', 'language', 'spatial', 'superTheme']
         for key in extra_attrs:
@@ -148,8 +155,20 @@ class DatasetConversionTestCase(unittest.TestCase):
                 resulting_dict = {'key': key, 'value': value}
                 self.assertTrue(resulting_dict in package['extras'])
 
+    def test_catalog_id_is_prefixed_in_resource_id_if_passed(self):
+        resources = map_distributions_to_resources(self.distributions, self.catalog_id)
+        for resource in resources:
+            distribution = next(x for x in self.dataset['distribution'] if x['title'] == resource['name'])
+            self.assertEqual(self.catalog_id + '_' + distribution['identifier'], resource['id'])
+
+    def test_resource_id_is_preserved_if_catalog_id_is_not_passed(self):
+        resources = map_distributions_to_resources(self.distributions)
+        for resource in resources:
+            distribution = next(x for x in self.dataset['distribution'] if x['title'] == resource['name'])
+            self.assertEqual(distribution['identifier'], resource['id'])
+
     def test_resources_replicated_attributes_stay_the_same(self):
-        resources = map_distributions_to_resources(self.distributions, self.catalog_id+'_'+self.dataset_id)
+        resources = map_distributions_to_resources(self.distributions, self.catalog_id)
         for resource in resources:
             distribution = next(x for x in self.dataset['distribution'] if x['title'] == resource['name'])
             replicated_attributes = [('url', 'downloadURL'),
@@ -163,7 +182,6 @@ class DatasetConversionTestCase(unittest.TestCase):
                     self.assertEqual(distribution.get(snd), resource.get(fst))
                 else:
                     self.assertIsNone(resource.get(fst))
-            self.assertEqual(self.catalog_id+'_'+self.dataset_id+'_'+distribution['identifier'], resource['id'])
 
     def test_resources_transformed_attributes_are_correct(self):
         resources = map_distributions_to_resources(self.distributions, self.catalog_id+'_'+self.dataset_id)
diff --git a/tests/test_federation.py b/tests/test_federation.py
index 0dc245d..e4a1d2e 100644
--- a/tests/test_federation.py
+++ b/tests/test_federation.py
@@ -47,8 +47,8 @@ class PushDatasetTestCase(unittest.TestCase):
             else:
                 return []
         mock_portal.return_value.call_action = mock_call_action
-        res_id = push_dataset_to_ckan(self.catalog, self.catalog_id, 'owner',
-                                      self.dataset['identifier'], 'portal', 'key')
+        res_id = push_dataset_to_ckan(self.catalog, 'owner', self.dataset['identifier'],
+                                      'portal', 'key', catalog_id=self.catalog_id)
         self.assertEqual(self.catalog_id + '_' + self.dataset_id, res_id)
 
     @patch('pydatajson.federation.RemoteCKAN', autospec=True)
@@ -61,10 +61,25 @@ class PushDatasetTestCase(unittest.TestCase):
             else:
                 return []
         mock_portal.return_value.call_action = mock_call_action
-        res_id = push_dataset_to_ckan(self.catalog, self.catalog_id, 'owner',
-                                      self.dataset['identifier'], 'portal', 'key')
+        res_id = push_dataset_to_ckan(self.catalog, 'owner', self.dataset['identifier'],
+                                      'portal', 'key', catalog_id=self.catalog_id)
         self.assertEqual(self.catalog_id + '_' + self.dataset_id, res_id)
 
+    @patch('pydatajson.federation.RemoteCKAN', autospec=True)
+    def test_dataset_id_is_preserved_if_catalog_id_is_not_passed(self, mock_portal):
+        def mock_call_action(action, data_dict=None):
+            if action == 'package_update':
+                return {'id': data_dict['id']}
+            if action == 'package_create':
+                self.fail('should not be called')
+            else:
+                return []
+
+        mock_portal.return_value.call_action = mock_call_action
+        res_id = push_dataset_to_ckan(self.catalog, 'owner', self.dataset['identifier'],
+                                      'portal', 'key')
+        self.assertEqual(self.dataset_id, res_id)
+
     @patch('pydatajson.federation.RemoteCKAN', autospec=True)
     def test_tags_are_passed_correctly(self, mock_portal):
         themes = self.dataset['theme']
@@ -87,8 +102,8 @@ class PushDatasetTestCase(unittest.TestCase):
                 return []
 
         mock_portal.return_value.call_action = mock_call_action
-        res_id = push_dataset_to_ckan(self.catalog, self.catalog_id, 'owner',
-                                      self.dataset['identifier'], 'portal', 'key')
+        res_id = push_dataset_to_ckan(self.catalog, 'owner', self.dataset['identifier'],
+                                      'portal', 'key', catalog_id=self.catalog_id)
         self.assertEqual(self.catalog_id + '_' + self.dataset_id, res_id)
 
     @patch('pydatajson.federation.RemoteCKAN', autospec=True)
@@ -103,8 +118,8 @@ class PushDatasetTestCase(unittest.TestCase):
             else:
                 return []
         mock_portal.return_value.call_action = mock_call_action
-        push_dataset_to_ckan(self.catalog, self.catalog_id, 'owner',
-                             self.dataset['identifier'], 'portal', 'key')
+        push_dataset_to_ckan(self.catalog, 'owner', self.dataset['identifier'],
+                             'portal', 'key', catalog_id=self.catalog_id)
 
     @patch('pydatajson.federation.RemoteCKAN', autospec=True)
     def test_dataset_without_license_sets_notspecified(self, mock_portal):
@@ -119,8 +134,8 @@ class PushDatasetTestCase(unittest.TestCase):
                 return []
 
         mock_portal.return_value.call_action = mock_call_action
-        push_dataset_to_ckan(self.minimum_catalog, self.minimum_catalog_id, 'owner',
-                             self.minimum_dataset['identifier'], 'portal', 'key')
+        push_dataset_to_ckan(self.minimum_catalog, 'owner', self.minimum_dataset['identifier'],
+                             'portal', 'key', catalog_id=self.minimum_catalog_id)
 
 
 class RemoveDatasetTestCase(unittest.TestCase):