From d51568b5b468676176be9316521f45f4e25ccc61 Mon Sep 17 00:00:00 2001 From: Chatewgne Date: Tue, 15 Oct 2024 11:10:36 +0200 Subject: [PATCH] =?UTF-8?q?=F0=9F=90=9B=20[BUG]=20Fix=20errors=20in=20Aggr?= =?UTF-8?q?egator=20when=20retrieving=20unpublished=20Route=20Steps=20(ref?= =?UTF-8?q?s=20#3569)?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit --- docs/changelog.rst | 8 +++- geotrek/api/v2/views/trekking.py | 28 ++++++++++++ geotrek/common/tests/test_parsers.py | 2 + geotrek/trekking/parsers.py | 43 +++++++++++++++++-- .../data/geotrek_parser_v2/trek_children.json | 14 +++++- .../trek_children_do_not_exist.json | 21 +++++++-- .../data/geotrek_parser_v2/trek_network.json | 4 +- .../trek_published_step.json | 4 +- .../data/geotrek_parser_v2/trek_route.json | 6 +-- .../trek_unpublished_practice.json | 11 +++++ .../trek_unpublished_step.json | 14 +++--- geotrek/trekking/tests/test_parsers.py | 39 +++++++++++++---- 12 files changed, 161 insertions(+), 33 deletions(-) create mode 100644 geotrek/trekking/tests/data/geotrek_parser_v2/trek_unpublished_practice.json diff --git a/docs/changelog.rst b/docs/changelog.rst index 3f0ce22836..192c9fba23 100644 --- a/docs/changelog.rst +++ b/docs/changelog.rst @@ -20,6 +20,10 @@ CHANGELOG - Improve development quickstart documentation +**Bug fixes** + +- Fix missing unpublished related categories in Aggregator when retrieving unpublished Tour Steps (#3569) + 2.109.2 (2024-09-19) ---------------------------- @@ -46,8 +50,8 @@ CHANGELOG - ApidaeTrekParser duration import is fixed for multiple-days treks - Apidae tourism parser now handles missing contact properties - ApidaeTrekParser now handles missing source website -- Fix Aggregator does not retrieve unpublished Tour Steps (#3569)" -- Fix missing Annotation Categories in APIv2 for annotations other than Points (#4032)" +- Fix Aggregator does not retrieve unpublished Tour Steps (#3569) +- Fix missing Annotation Categories in APIv2 for annotations other than Points (#4032) - Change default CORS configuration to 'always' : see https://github.com/GeotrekCE/Geotrek-rando-v3/issues/1257 **Documentation** diff --git a/geotrek/api/v2/views/trekking.py b/geotrek/api/v2/views/trekking.py index 4df21783e0..a635fabade 100644 --- a/geotrek/api/v2/views/trekking.py +++ b/geotrek/api/v2/views/trekking.py @@ -124,6 +124,13 @@ class TrekRatingScaleViewSet(api_viewsets.GeotrekViewSet): queryset = trekking_models.RatingScale.objects \ .order_by('pk') # Required for reliable pagination + @cache_response_detail() + def retrieve(self, request, pk=None, format=None): + # Allow to retrieve objects even if not visible in list view + elem = get_object_or_404(trekking_models.RatingScale, pk=pk) + serializer = api_serializers.TrekRatingScaleSerializer(elem, many=False, context={'request': request}) + return Response(serializer.data) + class TrekRatingViewSet(api_viewsets.GeotrekViewSet): filter_backends = api_viewsets.GeotrekViewSet.filter_backends + ( @@ -134,6 +141,13 @@ class TrekRatingViewSet(api_viewsets.GeotrekViewSet): queryset = trekking_models.Rating.objects \ .order_by('order', 'name', 'pk') # Required for reliable pagination + @cache_response_detail() + def retrieve(self, request, pk=None, format=None): + # Allow to retrieve objects even if not visible in list view + elem = get_object_or_404(trekking_models.Rating, pk=pk) + serializer = api_serializers.TrekRatingSerializer(elem, many=False, context={'request': request}) + return Response(serializer.data) + class NetworkViewSet(api_viewsets.GeotrekViewSet): filter_backends = api_viewsets.GeotrekViewSet.filter_backends + (api_filters.TrekRelatedPortalFilter,) @@ -189,12 +203,26 @@ class AccessibilityViewSet(api_viewsets.GeotrekViewSet): serializer_class = api_serializers.AccessibilitySerializer queryset = trekking_models.Accessibility.objects.all() + @cache_response_detail() + def retrieve(self, request, pk=None, format=None): + # Allow to retrieve objects even if not visible in list view + elem = get_object_or_404(trekking_models.Accessibility, pk=pk) + serializer = api_serializers.AccessibilitySerializer(elem, many=False, context={'request': request}) + return Response(serializer.data) + class AccessibilityLevelViewSet(api_viewsets.GeotrekViewSet): filter_backends = api_viewsets.GeotrekViewSet.filter_backends + (api_filters.TrekRelatedPortalFilter,) serializer_class = api_serializers.AccessibilityLevelSerializer queryset = trekking_models.AccessibilityLevel.objects.all() + @cache_response_detail() + def retrieve(self, request, pk=None, format=None): + # Allow to retrieve objects even if not visible in list view + elem = get_object_or_404(trekking_models.AccessibilityLevel, pk=pk) + serializer = api_serializers.AccessibilityLevelSerializer(elem, many=False, context={'request': request}) + return Response(serializer.data) + class RouteViewSet(api_viewsets.GeotrekViewSet): filter_backends = api_viewsets.GeotrekViewSet.filter_backends + (api_filters.TrekRelatedPortalFilter,) diff --git a/geotrek/common/tests/test_parsers.py b/geotrek/common/tests/test_parsers.py index c255ec2c11..3832ce96cf 100644 --- a/geotrek/common/tests/test_parsers.py +++ b/geotrek/common/tests/test_parsers.py @@ -868,6 +868,7 @@ def test_geotrek_aggregator_parser(self, mocked_head, mocked_get): ('trekking', 'trek_children.json'), ('trekking', 'trek_published_step.json'), ('trekking', 'trek_unpublished_step.json'), + ('trekking', 'trek_unpublished_practice.json'), ('trekking', 'poi_ids.json'), ('trekking', 'poi.json'), ('tourism', 'informationdesk_ids.json'), @@ -895,6 +896,7 @@ def test_geotrek_aggregator_parser(self, mocked_head, mocked_get): ('trekking', 'trek_children.json'), ('trekking', 'trek_published_step.json'), ('trekking', 'trek_unpublished_step.json'), + ('trekking', 'trek_unpublished_practice.json'), ('trekking', 'poi_ids.json'), ('trekking', 'poi.json'), ('tourism', 'informationdesk_ids.json'), diff --git a/geotrek/trekking/parsers.py b/geotrek/trekking/parsers.py index 5a863e197e..fe8855d785 100644 --- a/geotrek/trekking/parsers.py +++ b/geotrek/trekking/parsers.py @@ -22,7 +22,7 @@ from geotrek.common.models import Label, Theme from geotrek.common.parsers import (ApidaeBaseParser, AttachmentParserMixin, GeotrekParser, GlobalImportError, Parser, - RowImportError, ShapeParser) + RowImportError, ShapeParser, DownloadImportError) from geotrek.core.models import Path, Topology from geotrek.trekking.models import (POI, Accessibility, DifficultyLevel, OrderedTrekChild, Service, Trek, @@ -195,19 +195,53 @@ def filter_points_reference(self, src, val): geom = GEOSGeometry(json.dumps(val)) return geom.transform(settings.SRID, clone=True) + def fetch_missing_categories_for_tour_steps(self, step): + # For all categories, search missing values in mapping + for category, route in self.url_categories.items(): + category_mapping = self.field_options.get(category, {}).get('mapping', {}) + category_values = step.get(category) + if not isinstance(category_values, list): + category_values = [category_values] + for value in category_values: + # If category PK is not found in mapping, fetch it from provider + if value not in list(category_mapping.keys()): + if self.categories_keys_api_v2.get(category): + try: + result = self.request_or_retry(f"{self.url}/api/v2/{route}/{int(value)}").json() + except DownloadImportError: + self.add_warning(f"Could not download {category} with id {value} from {self.provider}" + f" for Tour step {step.get('uuid')}") + continue + # Update mapping like we usually do + label = result[self.categories_keys_api_v2[category]] + if isinstance(result[self.categories_keys_api_v2[category]], dict): + if label[settings.MODELTRANSLATION_DEFAULT_LANGUAGE]: + self.field_options[category]["mapping"][value] = self.replace_mapping(label[settings.MODELTRANSLATION_DEFAULT_LANGUAGE], route) + else: + if label: + self.field_options[category]["mapping"][value] = self.replace_mapping(label, category) + def end(self): """Add children after all treks imported are created in database.""" self.next_url = f"{self.url}/api/v2/tour" + portals = self.portals_filter try: params = { 'in_bbox': ','.join([str(coord) for coord in self.bbox.extent]), - 'fields': 'steps,id' + 'fields': 'steps,id,uuid,published', + 'portals': ','.join(portals) if portals else '' } response = self.request_or_retry(f"{self.next_url}", params=params) results = response.json()['results'] + steps_to_download = 0 final_children = {} for result in results: - final_children[result['uuid']] = [step['id'] for step in result['steps']] + final_children[result['uuid']] = [] + for step in result['steps']: + final_children[result['uuid']].append(step['id']) + if not any(step['published'].values()): + steps_to_download += 1 + self.nb = steps_to_download for key, value in final_children.items(): if value: @@ -219,6 +253,9 @@ def end(self): for child_id in value: response = self.request_or_retry(f"{self.url}/api/v2/trek/{child_id}") child_trek = response.json() + # The Tour step might be linked to categories that are not published, + # we have to retrieve the missing ones first + self.fetch_missing_categories_for_tour_steps(child_trek) self.parse_row(child_trek) trek_child_instance = self.obj OrderedTrekChild.objects.update_or_create(parent=trek_parent_instance[0], diff --git a/geotrek/trekking/tests/data/geotrek_parser_v2/trek_children.json b/geotrek/trekking/tests/data/geotrek_parser_v2/trek_children.json index 84502ccaf9..8043e2f0d2 100644 --- a/geotrek/trekking/tests/data/geotrek_parser_v2/trek_children.json +++ b/geotrek/trekking/tests/data/geotrek_parser_v2/trek_children.json @@ -7,10 +7,20 @@ "uuid": "9e70b294-1134-4c50-9c56-d722720cacf1", "steps": [ { - "id": 10439 + "id": 10439, + "published": { + "fr": true, + "en": false, + "it": false + } }, { - "id": 10442 + "id": 10442, + "published": { + "fr": false, + "en": false, + "it": false + } } ] }, diff --git a/geotrek/trekking/tests/data/geotrek_parser_v2/trek_children_do_not_exist.json b/geotrek/trekking/tests/data/geotrek_parser_v2/trek_children_do_not_exist.json index c66237ec49..ca418ec52e 100644 --- a/geotrek/trekking/tests/data/geotrek_parser_v2/trek_children_do_not_exist.json +++ b/geotrek/trekking/tests/data/geotrek_parser_v2/trek_children_do_not_exist.json @@ -7,10 +7,20 @@ "uuid": "9e70b294-1134-4c50-9c56-d722720cacf1", "steps": [ { - "id": 1234 + "id": 1234, + "published": { + "fr": true, + "en": false, + "it": false + } }, { - "id": 1235 + "id": 1235, + "published": { + "fr": false, + "en": false, + "it": false + } } ] }, @@ -34,7 +44,12 @@ "uuid": "b2aea666-5e6e-4daa-a750-7d2ee52d3fe1", "steps": [ { - "id": 457 + "id": 457, + "published": { + "fr": false, + "en": false, + "it": false + } } ] } diff --git a/geotrek/trekking/tests/data/geotrek_parser_v2/trek_network.json b/geotrek/trekking/tests/data/geotrek_parser_v2/trek_network.json index cdff494575..801c52223f 100644 --- a/geotrek/trekking/tests/data/geotrek_parser_v2/trek_network.json +++ b/geotrek/trekking/tests/data/geotrek_parser_v2/trek_network.json @@ -7,7 +7,7 @@ "id": 2, "label": { "fr": "PR", - "en": null, + "en": "PR", "es": null, "it": null }, @@ -17,7 +17,7 @@ "id": 4, "label": { "fr": "VTT", - "en": null, + "en": "Bike", "es": null, "it": null }, diff --git a/geotrek/trekking/tests/data/geotrek_parser_v2/trek_published_step.json b/geotrek/trekking/tests/data/geotrek_parser_v2/trek_published_step.json index 6c7dc87b14..150f52a98e 100644 --- a/geotrek/trekking/tests/data/geotrek_parser_v2/trek_published_step.json +++ b/geotrek/trekking/tests/data/geotrek_parser_v2/trek_published_step.json @@ -174,7 +174,7 @@ }, "points_reference": null, "portal": [], - "practice": 4, + "practice": 1, "ratings": [], "ratings_description": { "fr": "", @@ -197,7 +197,7 @@ }, "reservation_system": null, "reservation_id": "", - "route": 3, + "route": 1, "second_external_id": null, "source": [], "structure": 3, diff --git a/geotrek/trekking/tests/data/geotrek_parser_v2/trek_route.json b/geotrek/trekking/tests/data/geotrek_parser_v2/trek_route.json index 17e5a389db..469796791d 100644 --- a/geotrek/trekking/tests/data/geotrek_parser_v2/trek_route.json +++ b/geotrek/trekking/tests/data/geotrek_parser_v2/trek_route.json @@ -8,7 +8,7 @@ "pictogram": "https://foo.fr/media/upload/route-loop.svg", "route": { "fr": "Boucle", - "en": null, + "en": "Loop", "es": null, "it": null } @@ -18,7 +18,7 @@ "pictogram": "https://foo.fr/media/upload/route-return.svg", "route": { "fr": "Aller-retour", - "en": null, + "en": "Return trip", "es": null, "it": null } @@ -28,7 +28,7 @@ "pictogram": "https://foo.fr/media/upload/route-cross.svg", "route": { "fr": "Traversée", - "en": null, + "en": "Crossing", "es": null, "it": null } diff --git a/geotrek/trekking/tests/data/geotrek_parser_v2/trek_unpublished_practice.json b/geotrek/trekking/tests/data/geotrek_parser_v2/trek_unpublished_practice.json new file mode 100644 index 0000000000..c061a4dd69 --- /dev/null +++ b/geotrek/trekking/tests/data/geotrek_parser_v2/trek_unpublished_practice.json @@ -0,0 +1,11 @@ + { + "id": 5, + "name": { + "fr": "Pratique non publiée", + "en": "Unpublished practice", + "es": null, + "it": null + }, + "order": 3, + "pictogram": "https://foo.fr/media/upload/practice-mountainbike.svg" + } \ No newline at end of file diff --git a/geotrek/trekking/tests/data/geotrek_parser_v2/trek_unpublished_step.json b/geotrek/trekking/tests/data/geotrek_parser_v2/trek_unpublished_step.json index f07733f15d..717cd655c5 100644 --- a/geotrek/trekking/tests/data/geotrek_parser_v2/trek_unpublished_step.json +++ b/geotrek/trekking/tests/data/geotrek_parser_v2/trek_unpublished_step.json @@ -5,9 +5,7 @@ "en": "", "it": "" }, - "accessibilities": [ - 1 - ], + "accessibilities": [], "accessibility_advice": { "fr": "", "en": "", @@ -219,7 +217,7 @@ ] }, "portal": [], - "practice": 3, + "practice": 5, "ratings": [], "ratings_description": { "fr": "", @@ -233,8 +231,8 @@ "it": "" }, "published": { - "fr": true, - "en": true, + "fr": false, + "en": false, "it": false }, "reservation_system": null, @@ -246,8 +244,8 @@ ], "structure": 1, "themes": [ - 8, - 4 + 1, + 2 ], "update_datetime": "2022-05-16T12:10:59.927409Z", "url": "https://foo.fr/api/v2/trek/2/", diff --git a/geotrek/trekking/tests/test_parsers.py b/geotrek/trekking/tests/test_parsers.py index 2ccf04bc79..c76fdfc1d9 100644 --- a/geotrek/trekking/tests/test_parsers.py +++ b/geotrek/trekking/tests/test_parsers.py @@ -16,6 +16,7 @@ from django.test import TestCase, SimpleTestCase from django.test.utils import override_settings +from geotrek.common.parsers import DownloadImportError from geotrek.common.utils import testdata from geotrek.common.utils.file_infos import get_encoding_file from geotrek.common.models import Theme, FileType, Attachment, Label @@ -265,7 +266,8 @@ def test_create(self, mocked_head, mocked_get): ('trekking', 'trek.json'), ('trekking', 'trek_children.json'), ('trekking', 'trek_published_step.json'), - ('trekking', 'trek_unpublished_step.json')] + ('trekking', 'trek_unpublished_step.json'), + ('trekking', 'trek_unpublished_practice.json')] # Mock GET mocked_get.return_value.status_code = 200 @@ -291,6 +293,7 @@ def test_create(self, mocked_head, mocked_get): self.assertAlmostEqual(trek.geom[0][1], 6190964.893167565, places=5) self.assertEqual(trek.children.first().name, "Foo") self.assertEqual(trek.children.last().name, "Etape non publiée") + self.assertEqual(trek.children.last().practice.name, "Pratique non publiée") self.assertEqual(trek.labels.count(), 3) self.assertEqual(trek.source.first().name, "Une source numero 2") self.assertEqual(trek.source.first().website, "https://www.ecrins-parcnational.fr") @@ -317,9 +320,11 @@ class MockResponse: ('trekking', 'trek_children.json'), ('trekking', 'trek_published_step.json'), ('trekking', 'trek_unpublished_step.json'), + ('trekking', 'trek_unpublished_practice.json'), ('trekking', 'trek_children.json'), ('trekking', 'trek_published_step.json'), - ('trekking', 'trek_unpublished_step.json')] + ('trekking', 'trek_unpublished_step.json'), + ('trekking', 'trek_unpublished_practice.json')] mock_time = 0 total_mock_response = 1 @@ -381,7 +386,8 @@ def test_create_attachment_max_size(self, mocked_head, mocked_get): ('trekking', 'trek.json'), ('trekking', 'trek_children.json'), ('trekking', 'trek_published_step.json'), - ('trekking', 'trek_unpublished_step.json')] + ('trekking', 'trek_unpublished_step.json'), + ('trekking', 'trek_unpublished_practice.json')] # Mock GET mocked_get.return_value.status_code = 200 @@ -412,7 +418,8 @@ class MockResponse: ('trekking', 'trek.json'), ('trekking', 'trek_children.json'), ('trekking', 'trek_published_step.json'), - ('trekking', 'trek_unpublished_step.json')] + ('trekking', 'trek_unpublished_step.json'), + ('trekking', 'trek_unpublished_practice.json')] mock_time = 0 a = 0 @@ -471,6 +478,7 @@ def test_create_multiple_fr(self, mocked_head, mocked_get): ('trekking', 'trek_children.json'), ('trekking', 'trek_published_step.json'), ('trekking', 'trek_unpublished_step.json'), + ('trekking', 'trek_unpublished_practice.json'), ('trekking', 'structure.json'), ('trekking', 'trek_difficulty.json'), ('trekking', 'trek_route.json'), @@ -562,6 +570,7 @@ def test_create_multiple_en(self, mocked_head, mocked_get): ('trekking', 'trek_children.json'), ('trekking', 'trek_published_step.json'), ('trekking', 'trek_unpublished_step.json'), + ('trekking', 'trek_unpublished_practice.json'), ('trekking', 'structure.json'), ('trekking', 'trek_difficulty.json'), ('trekking', 'trek_route.json'), @@ -652,7 +661,8 @@ def test_children_do_not_exist(self, mocked_head, mocked_get): ('trekking', 'trek.json'), ('trekking', 'trek_children_do_not_exist.json'), ('trekking', 'trek_published_step.json'), - ('trekking', 'trek_unpublished_step.json')] + ('trekking', 'trek_unpublished_step.json'), + ('trekking', 'trek_unpublished_practice.json')] # Mock GET mocked_get.return_value.status_code = 200 @@ -667,6 +677,17 @@ def test_children_do_not_exist(self, mocked_head, mocked_get): @mock.patch('requests.get') @mock.patch('requests.head') def test_wrong_children_error(self, mocked_head, mocked_get): + + def mock_json_with_404(): + filename = os.path.join('geotrek', self.mock_json_order[self.mock_time][0], 'tests', 'data', + 'geotrek_parser_v2', + self.mock_json_order[self.mock_time][1]) + self.mock_time += 1 + if "trek_not_found" in filename: + raise DownloadImportError("404 Trek does not exist") + with open(filename, 'r') as f: + return json.load(f) + self.mock_time = 0 self.mock_json_order = [('trekking', 'structure.json'), ('trekking', 'trek_difficulty.json'), @@ -685,14 +706,14 @@ def test_wrong_children_error(self, mocked_head, mocked_get): # Mock GET mocked_get.return_value.status_code = 200 - mocked_get.return_value.json = self.mock_json + mocked_get.return_value.json = mock_json_with_404 mocked_get.return_value.content = b'' mocked_head.return_value.status_code = 200 output = StringIO() call_command('import', 'geotrek.trekking.tests.test_parsers.TestGeotrekTrekParser', verbosity=2, stdout=output) - self.assertIn("An error occured in children generation : ValueImportError", output.getvalue()) + self.assertIn("An error occured in children generation : DownloadImportError", output.getvalue()) @mock.patch('requests.get') @mock.patch('requests.head') @@ -714,6 +735,7 @@ def test_updated(self, mocked_head, mocked_get): ('trekking', 'trek_children.json'), ('trekking', 'trek_published_step.json'), ('trekking', 'trek_unpublished_step.json'), + ('trekking', 'trek_unpublished_practice.json'), ('trekking', 'structure.json'), ('trekking', 'trek_difficulty.json'), ('trekking', 'trek_route.json'), @@ -728,7 +750,8 @@ def test_updated(self, mocked_head, mocked_get): ('trekking', 'trek_2.json'), ('trekking', 'trek_children.json'), ('trekking', 'trek_published_step.json'), - ('trekking', 'trek_unpublished_step.json')] + ('trekking', 'trek_unpublished_step.json'), + ('trekking', 'trek_unpublished_practice.json')] # Mock GET mocked_get.return_value.status_code = 200