From 7f8422fe2cfafb7dfa339f55cda50e4e0a921d2f Mon Sep 17 00:00:00 2001 From: Novak Zaballa Date: Wed, 1 May 2024 18:19:40 -0400 Subject: [PATCH 1/6] fix: Solve problems with Github integration when it was deleted --- .../feature_external_resources/models.py | 21 +++++-- .../feature_external_resources/views.py | 6 +- api/features/tasks.py | 11 ++-- api/integrations/github/exceptions.py | 6 ++ .../migrations/0002_auto_20240501_2137.py | 24 ++++++++ api/integrations/github/models.py | 13 ++++- api/integrations/github/views.py | 25 ++++++-- .../github/test_unit_github_views.py | 58 ++++++++++++++++++- 8 files changed, 146 insertions(+), 18 deletions(-) create mode 100644 api/integrations/github/exceptions.py create mode 100644 api/integrations/github/migrations/0002_auto_20240501_2137.py diff --git a/api/features/feature_external_resources/models.py b/api/features/feature_external_resources/models.py index ccb075caa194..e46ad4ec0d5a 100644 --- a/api/features/feature_external_resources/models.py +++ b/api/features/feature_external_resources/models.py @@ -12,6 +12,7 @@ from features.models import Feature, FeatureState from integrations.github.github import GithubData, generate_data +from integrations.github.models import GithubConfiguration from integrations.github.tasks import call_github_app_webhook_for_feature_state from webhooks.webhooks import WebhookEventType @@ -49,10 +50,16 @@ class Meta: @hook(AFTER_SAVE) def exectute_after_save_actions(self): # Add a comment to GitHub Issue/PR when feature is linked to the GH external resource - if hasattr(self.feature.project.organisation, "github_config"): - github_configuration = self.feature.project.organisation.github_config + if self.feature.project.organisation.github_config.filter( + deleted_at__isnull=True + ).exists(): + github_configuration = GithubConfiguration.objects.get( + organisation=self.feature.project.organisation, deleted_at__isnull=True + ) - feature_states = FeatureState.objects.filter(feature_id=self.feature_id) + feature_states = FeatureState.objects.filter( + feature_id=self.feature_id, identity_id__isnull=True + ) feature_data: GithubData = generate_data( github_configuration, self.feature_id, @@ -68,8 +75,12 @@ def exectute_after_save_actions(self): @hook(BEFORE_DELETE) def execute_before_save_actions(self) -> None: # Add a comment to GitHub Issue/PR when feature is unlinked to the GH external resource - if hasattr(self.feature.project.organisation, "github_config"): - github_configuration = self.feature.project.organisation.github_config + if self.feature.project.organisation.github_config.filter( + deleted_at__isnull=True + ).exists(): + github_configuration = GithubConfiguration.objects.get( + organisation=self.feature.project.organisation, deleted_at__isnull=True + ) feature_data: GithubData = generate_data( github_configuration=github_configuration, feature_id=self.feature_id, diff --git a/api/features/feature_external_resources/views.py b/api/features/feature_external_resources/views.py index d254d9b79252..8805bcdb9f2e 100644 --- a/api/features/feature_external_resources/views.py +++ b/api/features/feature_external_resources/views.py @@ -29,9 +29,9 @@ def create(self, request, *args, **kwargs): ), ) - if not hasattr(feature.project.organisation, "github_config") or not hasattr( - feature.project, "github_project" - ): + if not feature.project.organisation.github_config.filter( + deleted_at__isnull=True + ).exists() or not hasattr(feature.project, "github_project"): return Response( data={ "detail": "This Project doesn't have a valid GitHub integration configuration" diff --git a/api/features/tasks.py b/api/features/tasks.py index f5a45a20dba5..662212262e68 100644 --- a/api/features/tasks.py +++ b/api/features/tasks.py @@ -65,8 +65,11 @@ def trigger_feature_state_change_webhooks( and instance.environment.project.github_project.exists() and hasattr(instance.environment.project.organisation, "github_config") ): - github_configuration = instance.environment.project.organisation.github_config - + github_configuration = ( + instance.environment.project.organisation.github_config.get( + deleted_at__isnull=True + ) + ) feature_state = { "environment_name": new_state["environment"]["name"], "feature_value": new_state["enabled"], @@ -78,11 +81,11 @@ def trigger_feature_state_change_webhooks( github_configuration=github_configuration, feature_id=history_instance.feature.id, feature_name=history_instance.feature.name, - type=WebhookEventType.FLAG_UPDATED, + type=WebhookEventType.FLAG_UPDATED.value, feature_states=feature_states, ) - feature_data["feature_states"].append(feature_state) + feature_data.feature_states.append(feature_state) call_github_app_webhook_for_feature_state.delay( args=(asdict(feature_data),), diff --git a/api/integrations/github/exceptions.py b/api/integrations/github/exceptions.py new file mode 100644 index 000000000000..8d32b1ff099b --- /dev/null +++ b/api/integrations/github/exceptions.py @@ -0,0 +1,6 @@ +from rest_framework.exceptions import APIException + + +class DuplicateGitHubIntegration(APIException): + status_code = 400 + default_detail = "Duplication error. The GitHub integration already created" diff --git a/api/integrations/github/migrations/0002_auto_20240501_2137.py b/api/integrations/github/migrations/0002_auto_20240501_2137.py new file mode 100644 index 000000000000..53895ede0d34 --- /dev/null +++ b/api/integrations/github/migrations/0002_auto_20240501_2137.py @@ -0,0 +1,24 @@ +# Generated by Django 3.2.25 on 2024-05-01 21:37 + +from django.db import migrations, models +import django.db.models.deletion + + +class Migration(migrations.Migration): + + dependencies = [ + ('organisations', '0052_create_hubspot_organisation'), + ('github', '0001_initial'), + ] + + operations = [ + migrations.AlterField( + model_name='githubconfiguration', + name='organisation', + field=models.ForeignKey(on_delete=django.db.models.deletion.CASCADE, related_name='github_config', to='organisations.organisation'), + ), + migrations.AddConstraint( + model_name='githubconfiguration', + constraint=models.UniqueConstraint(fields=('organisation', 'installation_id'), name='unique_github_config_data'), + ), + ] diff --git a/api/integrations/github/models.py b/api/integrations/github/models.py index 7aebde9ce0bc..f94ba93a5583 100644 --- a/api/integrations/github/models.py +++ b/api/integrations/github/models.py @@ -10,7 +10,7 @@ class GithubConfiguration(SoftDeleteExportableModel): - organisation = models.OneToOneField( + organisation = models.ForeignKey( Organisation, on_delete=models.CASCADE, related_name="github_config" ) installation_id = models.CharField(max_length=100, blank=False, null=False) @@ -21,6 +21,17 @@ def has_github_configuration(organisation_id: int) -> bool: organisation_id=organisation_id ).exists() + class Meta: + constraints = [ + models.UniqueConstraint( + fields=[ + "organisation", + "installation_id", + ], + name="unique_github_config_data", + ) + ] + class GithubRepository(LifecycleModelMixin, SoftDeleteExportableModel): github_configuration = models.ForeignKey( diff --git a/api/integrations/github/views.py b/api/integrations/github/views.py index 580b62aff629..663507ae3157 100644 --- a/api/integrations/github/views.py +++ b/api/integrations/github/views.py @@ -12,6 +12,7 @@ from integrations.github.client import generate_token from integrations.github.constants import GITHUB_API_URL, GITHUB_API_VERSION +from integrations.github.exceptions import DuplicateGitHubIntegration from integrations.github.models import GithubConfiguration, GithubRepository from integrations.github.permissions import HasPermissionToGithubConfiguration from integrations.github.serializers import ( @@ -61,6 +62,17 @@ def get_queryset(self): organisation_id=self.kwargs["organisation_pk"] ) + def create(self, request, *args, **kwargs): + + organisation_id = self.kwargs["organisation_pk"] + if GithubConfiguration.has_github_configuration(organisation_id): + raise DuplicateGitHubIntegration + + try: + return super().create(request, *args, **kwargs) + except IntegrityError: + raise DuplicateGitHubIntegration + class GithubRepositoryViewSet(viewsets.ModelViewSet): permission_classes = ( @@ -86,7 +98,7 @@ def create(self, request, *args, **kwargs): return super().create(request, *args, **kwargs) except IntegrityError: raise ValidationError( - detail="Duplication error. The Github repository already linked" + detail="Duplication error. The GitHub repository already linked" ) @@ -95,9 +107,11 @@ def create(self, request, *args, **kwargs): @github_auth_required def fetch_pull_requests(request, organisation_pk): organisation = Organisation.objects.get(id=organisation_pk) - + github_configuration = GithubConfiguration.objects.get( + organisation=organisation, deleted_at__isnull=True + ) token = generate_token( - organisation.github_config.installation_id, + github_configuration.installation_id, settings.GITHUB_APP_ID, ) @@ -130,8 +144,11 @@ def fetch_pull_requests(request, organisation_pk): @github_auth_required def fetch_issues(request, organisation_pk): organisation = Organisation.objects.get(id=organisation_pk) + github_configuration = GithubConfiguration.objects.get( + organisation=organisation, deleted_at__isnull=True + ) token = generate_token( - organisation.github_config.installation_id, + github_configuration.installation_id, settings.GITHUB_APP_ID, ) diff --git a/api/tests/unit/integrations/github/test_unit_github_views.py b/api/tests/unit/integrations/github/test_unit_github_views.py index 5e3fcecfcf58..3e2edc48900f 100644 --- a/api/tests/unit/integrations/github/test_unit_github_views.py +++ b/api/tests/unit/integrations/github/test_unit_github_views.py @@ -52,6 +52,62 @@ def test_create_github_configuration( assert response.status_code == status.HTTP_201_CREATED +@pytest.mark.parametrize( + "client", + [lazy_fixture("admin_master_api_key_client"), lazy_fixture("admin_client")], +) +def test_cannot_create_github_configuration_due_to_unique_constraint( + client: APIClient, + organisation: Organisation, + github_configuration: GithubConfiguration, +) -> None: + # Given + data = { + "installation_id": 1234567, + } + url = reverse( + "api-v1:organisations:integrations-github-list", + kwargs={"organisation_pk": organisation.id}, + ) + # When + response = client.post(url, data) + # Then + assert response.status_code == status.HTTP_400_BAD_REQUEST + + assert ( + "Duplication error. The GitHub integration already created" + in response.json()["detail"] + ) + + +@pytest.mark.parametrize( + "client", + [lazy_fixture("admin_master_api_key_client"), lazy_fixture("admin_client")], +) +def test_cannot_create_github_configuration_when_the_organization_already_has_an_integration( + client: APIClient, + organisation: Organisation, + github_configuration: GithubConfiguration, +) -> None: + # Given + data = { + "installation_id": 7654321, + } + url = reverse( + "api-v1:organisations:integrations-github-list", + kwargs={"organisation_pk": organisation.id}, + ) + # When + response = client.post(url, data) + # Then + assert response.status_code == status.HTTP_400_BAD_REQUEST + + assert ( + "Duplication error. The GitHub integration already created" + in response.json()["detail"] + ) + + @pytest.mark.parametrize( "client", [lazy_fixture("admin_master_api_key_client"), lazy_fixture("admin_client")], @@ -182,7 +238,7 @@ def test_cannot_create_github_repository_due_to_unique_constraint( assert response.status_code == status.HTTP_400_BAD_REQUEST assert ( - "Duplication error. The Github repository already linked" in response.json()[0] + "Duplication error. The GitHub repository already linked" in response.json()[0] ) From 608c8810d985b06a4d5264778c9e421119d88931 Mon Sep 17 00:00:00 2001 From: Novak Zaballa Date: Thu, 2 May 2024 10:36:33 -0400 Subject: [PATCH 2/6] Add warlus operator --- .../feature_external_resources/models.py | 16 ++++------------ 1 file changed, 4 insertions(+), 12 deletions(-) diff --git a/api/features/feature_external_resources/models.py b/api/features/feature_external_resources/models.py index e46ad4ec0d5a..dd96cd90e420 100644 --- a/api/features/feature_external_resources/models.py +++ b/api/features/feature_external_resources/models.py @@ -12,7 +12,6 @@ from features.models import Feature, FeatureState from integrations.github.github import GithubData, generate_data -from integrations.github.models import GithubConfiguration from integrations.github.tasks import call_github_app_webhook_for_feature_state from webhooks.webhooks import WebhookEventType @@ -50,13 +49,9 @@ class Meta: @hook(AFTER_SAVE) def exectute_after_save_actions(self): # Add a comment to GitHub Issue/PR when feature is linked to the GH external resource - if self.feature.project.organisation.github_config.filter( + if github_configuration := self.feature.project.organisation.github_config.filter( deleted_at__isnull=True - ).exists(): - github_configuration = GithubConfiguration.objects.get( - organisation=self.feature.project.organisation, deleted_at__isnull=True - ) - + ).first(): feature_states = FeatureState.objects.filter( feature_id=self.feature_id, identity_id__isnull=True ) @@ -75,12 +70,9 @@ def exectute_after_save_actions(self): @hook(BEFORE_DELETE) def execute_before_save_actions(self) -> None: # Add a comment to GitHub Issue/PR when feature is unlinked to the GH external resource - if self.feature.project.organisation.github_config.filter( + if github_configuration := self.feature.project.organisation.github_config.filter( deleted_at__isnull=True - ).exists(): - github_configuration = GithubConfiguration.objects.get( - organisation=self.feature.project.organisation, deleted_at__isnull=True - ) + ).first(): feature_data: GithubData = generate_data( github_configuration=github_configuration, feature_id=self.feature_id, From d70c4a722710eb738e4680260b5ddf4d7a0176dd Mon Sep 17 00:00:00 2001 From: novakzaballa Date: Thu, 2 May 2024 15:57:28 -0400 Subject: [PATCH 3/6] Improve code --- .../feature_external_resources/models.py | 23 +++++++++++++------ .../feature_external_resources/views.py | 13 ++++++++--- api/features/tasks.py | 7 +++--- ...501_2137.py => 0002_auto_20240502_1949.py} | 4 ++-- api/integrations/github/models.py | 4 ++-- api/integrations/github/views.py | 5 ---- 6 files changed, 34 insertions(+), 22 deletions(-) rename api/integrations/github/migrations/{0002_auto_20240501_2137.py => 0002_auto_20240502_1949.py} (74%) diff --git a/api/features/feature_external_resources/models.py b/api/features/feature_external_resources/models.py index dd96cd90e420..e8f307230914 100644 --- a/api/features/feature_external_resources/models.py +++ b/api/features/feature_external_resources/models.py @@ -13,6 +13,7 @@ from features.models import Feature, FeatureState from integrations.github.github import GithubData, generate_data from integrations.github.tasks import call_github_app_webhook_for_feature_state +from organisations.models import Organisation from webhooks.webhooks import WebhookEventType logger = logging.getLogger(__name__) @@ -47,11 +48,15 @@ class Meta: ] @hook(AFTER_SAVE) - def exectute_after_save_actions(self): + def execute_after_save_actions(self): # Add a comment to GitHub Issue/PR when feature is linked to the GH external resource - if github_configuration := self.feature.project.organisation.github_config.filter( - deleted_at__isnull=True - ).first(): + if ( + github_configuration := Organisation.objects.prefetch_related( + "github_config" + ) + .get(id=self.feature.project.organisation_id) + .github_config.first() + ): feature_states = FeatureState.objects.filter( feature_id=self.feature_id, identity_id__isnull=True ) @@ -70,9 +75,13 @@ def exectute_after_save_actions(self): @hook(BEFORE_DELETE) def execute_before_save_actions(self) -> None: # Add a comment to GitHub Issue/PR when feature is unlinked to the GH external resource - if github_configuration := self.feature.project.organisation.github_config.filter( - deleted_at__isnull=True - ).first(): + if ( + github_configuration := Organisation.objects.prefetch_related( + "github_config" + ) + .get(id=self.feature.project.organisation_id) + .github_config.first() + ): feature_data: GithubData = generate_data( github_configuration=github_configuration, feature_id=self.feature_id, diff --git a/api/features/feature_external_resources/views.py b/api/features/feature_external_resources/views.py index 8805bcdb9f2e..b046c9dc5274 100644 --- a/api/features/feature_external_resources/views.py +++ b/api/features/feature_external_resources/views.py @@ -6,6 +6,7 @@ from features.models import Feature from features.permissions import FeatureExternalResourcePermissions +from organisations.models import Organisation from .models import FeatureExternalResource from .serializers import FeatureExternalResourceSerializer @@ -29,9 +30,15 @@ def create(self, request, *args, **kwargs): ), ) - if not feature.project.organisation.github_config.filter( - deleted_at__isnull=True - ).exists() or not hasattr(feature.project, "github_project"): + if not ( + ( + Organisation.objects.prefetch_related("github_config") + .get(id=feature.project.organisation_id) + .github_config.first() + ) + or not hasattr(feature.project, "github_project") + ): + return Response( data={ "detail": "This Project doesn't have a valid GitHub integration configuration" diff --git a/api/features/tasks.py b/api/features/tasks.py index 662212262e68..1d0c07feb904 100644 --- a/api/features/tasks.py +++ b/api/features/tasks.py @@ -5,6 +5,7 @@ from features.models import Feature, FeatureState from integrations.github.github import GithubData, generate_data from integrations.github.tasks import call_github_app_webhook_for_feature_state +from organisations.models import Organisation from task_processor.decorators import register_task_handler from webhooks.constants import WEBHOOK_DATETIME_FORMAT from webhooks.webhooks import ( @@ -66,9 +67,9 @@ def trigger_feature_state_change_webhooks( and hasattr(instance.environment.project.organisation, "github_config") ): github_configuration = ( - instance.environment.project.organisation.github_config.get( - deleted_at__isnull=True - ) + Organisation.objects.prefetch_related("github_config") + .get(id=instance.environment.project.organisationn_id) + .github_config.first() ) feature_state = { "environment_name": new_state["environment"]["name"], diff --git a/api/integrations/github/migrations/0002_auto_20240501_2137.py b/api/integrations/github/migrations/0002_auto_20240502_1949.py similarity index 74% rename from api/integrations/github/migrations/0002_auto_20240501_2137.py rename to api/integrations/github/migrations/0002_auto_20240502_1949.py index 53895ede0d34..4739e2825ea9 100644 --- a/api/integrations/github/migrations/0002_auto_20240501_2137.py +++ b/api/integrations/github/migrations/0002_auto_20240502_1949.py @@ -1,4 +1,4 @@ -# Generated by Django 3.2.25 on 2024-05-01 21:37 +# Generated by Django 3.2.25 on 2024-05-02 19:49 from django.db import migrations, models import django.db.models.deletion @@ -19,6 +19,6 @@ class Migration(migrations.Migration): ), migrations.AddConstraint( model_name='githubconfiguration', - constraint=models.UniqueConstraint(fields=('organisation', 'installation_id'), name='unique_github_config_data'), + constraint=models.UniqueConstraint(condition=models.Q(('deleted_at__isnull', True)), fields=('organisation',), name='githubconf_organisation_id_idx'), ), ] diff --git a/api/integrations/github/models.py b/api/integrations/github/models.py index f94ba93a5583..22dcd6ee5333 100644 --- a/api/integrations/github/models.py +++ b/api/integrations/github/models.py @@ -26,9 +26,9 @@ class Meta: models.UniqueConstraint( fields=[ "organisation", - "installation_id", ], - name="unique_github_config_data", + name="githubconf_organisation_id_idx", + condition=models.Q(deleted_at__isnull=True), ) ] diff --git a/api/integrations/github/views.py b/api/integrations/github/views.py index 663507ae3157..f661f5a809d9 100644 --- a/api/integrations/github/views.py +++ b/api/integrations/github/views.py @@ -63,11 +63,6 @@ def get_queryset(self): ) def create(self, request, *args, **kwargs): - - organisation_id = self.kwargs["organisation_pk"] - if GithubConfiguration.has_github_configuration(organisation_id): - raise DuplicateGitHubIntegration - try: return super().create(request, *args, **kwargs) except IntegrityError: From 044a2507ddf92d0346d0eea086b62c43e190b82b Mon Sep 17 00:00:00 2001 From: novakzaballa Date: Thu, 2 May 2024 16:15:43 -0400 Subject: [PATCH 4/6] Use admin_client_new instead pytest.mark.parametrize --- ...t_unit_feature_external_resources_views.py | 74 +++++--------- .../github/test_unit_github_views.py | 96 +++++-------------- 2 files changed, 47 insertions(+), 123 deletions(-) diff --git a/api/tests/unit/features/test_unit_feature_external_resources_views.py b/api/tests/unit/features/test_unit_feature_external_resources_views.py index 8b242e9464fc..91fbabb0a233 100644 --- a/api/tests/unit/features/test_unit_feature_external_resources_views.py +++ b/api/tests/unit/features/test_unit_feature_external_resources_views.py @@ -1,11 +1,9 @@ import datetime -import pytest import simplejson as json from django.core.serializers.json import DjangoJSONEncoder from django.urls import reverse from freezegun import freeze_time -from pytest_lazyfixture import lazy_fixture from rest_framework import status from rest_framework.test import APIClient @@ -32,13 +30,9 @@ def json(self): return MockResponse(json_data={"data": "data"}, status_code=200) -@pytest.mark.parametrize( - "client", - [lazy_fixture("admin_master_api_key_client"), lazy_fixture("admin_client")], -) @freeze_time("2024-01-01") def test_create_feature_external_resource( - client: APIClient, + admin_client_new: APIClient, feature_with_value: Feature, project: Project, github_configuration: GithubConfiguration, @@ -68,7 +62,9 @@ def test_create_feature_external_resource( ) # When - response = client.post(url, data=feature_external_resource_data, format="json") + response = admin_client_new.post( + url, data=feature_external_resource_data, format="json" + ) github_request_mock.assert_called_with( "https://api.github.com/repos/repoowner/repo-name/issues/35/comments", @@ -104,7 +100,7 @@ def test_create_feature_external_resource( kwargs={"project_pk": project.id, "feature_pk": feature_with_value.id}, ) - response = client.get(url) + response = admin_client_new.get(url) # Then assert response.status_code == status.HTTP_200_OK @@ -119,12 +115,8 @@ def test_create_feature_external_resource( ) -@pytest.mark.parametrize( - "client", - [lazy_fixture("admin_master_api_key_client"), lazy_fixture("admin_client")], -) def test_cannot_create_feature_external_resource_when_doesnt_have_a_valid_gitHub_integration( - client: APIClient, + admin_client_new: APIClient, feature: Feature, project: Project, ) -> None: @@ -140,18 +132,16 @@ def test_cannot_create_feature_external_resource_when_doesnt_have_a_valid_gitHub ) # When - response = client.post(url, data=feature_external_resource_data, format="json") + response = admin_client_new.post( + url, data=feature_external_resource_data, format="json" + ) # Then assert response.status_code == status.HTTP_400_BAD_REQUEST -@pytest.mark.parametrize( - "client", - [lazy_fixture("admin_master_api_key_client"), lazy_fixture("admin_client")], -) def test_cannot_create_feature_external_resource_when_doesnt_have_permissions( - client: APIClient, + admin_client_new: APIClient, feature: Feature, ) -> None: # Given @@ -166,18 +156,16 @@ def test_cannot_create_feature_external_resource_when_doesnt_have_permissions( ) # When - response = client.post(url, data=feature_external_resource_data, format="json") + response = admin_client_new.post( + url, data=feature_external_resource_data, format="json" + ) # Then assert response.status_code == status.HTTP_403_FORBIDDEN -@pytest.mark.parametrize( - "client", - [lazy_fixture("admin_master_api_key_client"), lazy_fixture("admin_client")], -) def test_cannot_create_feature_external_resource_when_the_type_is_incorrect( - client: APIClient, + admin_client_new: APIClient, feature: Feature, project: Project, ) -> None: @@ -192,17 +180,13 @@ def test_cannot_create_feature_external_resource_when_the_type_is_incorrect( ) # When - response = client.post(url, data=feature_external_resource_data) + response = admin_client_new.post(url, data=feature_external_resource_data) # Then assert response.status_code == status.HTTP_400_BAD_REQUEST -@pytest.mark.parametrize( - "client", - [lazy_fixture("admin_master_api_key_client"), lazy_fixture("admin_client")], -) def test_cannot_create_feature_external_resource_due_to_unique_constraint( - client: APIClient, + admin_client_new: APIClient, feature: Feature, feature_external_resource: FeatureExternalResource, project: Project, @@ -220,7 +204,7 @@ def test_cannot_create_feature_external_resource_due_to_unique_constraint( ) # When - response = client.post(url, data=feature_external_resource_data) + response = admin_client_new.post(url, data=feature_external_resource_data) # Then assert response.status_code == status.HTTP_400_BAD_REQUEST @@ -230,12 +214,8 @@ def test_cannot_create_feature_external_resource_due_to_unique_constraint( ) -@pytest.mark.parametrize( - "client", - [lazy_fixture("admin_master_api_key_client"), lazy_fixture("admin_client")], -) def test_delete_feature_external_resource( - client: APIClient, + admin_client_new: APIClient, feature_external_resource: FeatureExternalResource, feature: Feature, project: Project, @@ -254,7 +234,7 @@ def test_delete_feature_external_resource( ) # When - response = client.delete(url) + response = admin_client_new.delete(url) # Then assert response.status_code == status.HTTP_204_NO_CONTENT @@ -263,12 +243,8 @@ def test_delete_feature_external_resource( ).exists() -@pytest.mark.parametrize( - "client", - [lazy_fixture("admin_master_api_key_client"), lazy_fixture("admin_client")], -) def test_get_feature_external_resources( - client: APIClient, + admin_client_new: APIClient, feature_external_resource: FeatureExternalResource, feature: Feature, project: Project, @@ -282,18 +258,14 @@ def test_get_feature_external_resources( ) # When - response = client.get(url) + response = admin_client_new.get(url) # Then assert response.status_code == status.HTTP_200_OK -@pytest.mark.parametrize( - "client", - [lazy_fixture("admin_master_api_key_client"), lazy_fixture("admin_client")], -) def test_get_feature_external_resource( - client: APIClient, + admin_client_new: APIClient, feature_external_resource: FeatureExternalResource, feature: Feature, project: Project, @@ -307,7 +279,7 @@ def test_get_feature_external_resource( ) # When - response = client.get(url) + response = admin_client_new.get(url) # Then assert response.status_code == status.HTTP_200_OK diff --git a/api/tests/unit/integrations/github/test_unit_github_views.py b/api/tests/unit/integrations/github/test_unit_github_views.py index 3e2edc48900f..17a107bbd30c 100644 --- a/api/tests/unit/integrations/github/test_unit_github_views.py +++ b/api/tests/unit/integrations/github/test_unit_github_views.py @@ -11,12 +11,8 @@ from projects.models import Project -@pytest.mark.parametrize( - "client", - [lazy_fixture("admin_master_api_key_client"), lazy_fixture("admin_client")], -) def test_get_github_configuration( - client: APIClient, + admin_client_new: APIClient, organisation: Organisation, ) -> None: # Given @@ -25,17 +21,13 @@ def test_get_github_configuration( kwargs={"organisation_pk": organisation.id}, ) # When - response = client.get(url) + response = admin_client_new.get(url) # Then assert response.status_code == status.HTTP_200_OK -@pytest.mark.parametrize( - "client", - [lazy_fixture("admin_master_api_key_client"), lazy_fixture("admin_client")], -) def test_create_github_configuration( - client: APIClient, + admin_client_new: APIClient, organisation: Organisation, ) -> None: # Given @@ -47,17 +39,13 @@ def test_create_github_configuration( kwargs={"organisation_pk": organisation.id}, ) # When - response = client.post(url, data) + response = admin_client_new.post(url, data) # Then assert response.status_code == status.HTTP_201_CREATED -@pytest.mark.parametrize( - "client", - [lazy_fixture("admin_master_api_key_client"), lazy_fixture("admin_client")], -) def test_cannot_create_github_configuration_due_to_unique_constraint( - client: APIClient, + admin_client_new: APIClient, organisation: Organisation, github_configuration: GithubConfiguration, ) -> None: @@ -70,7 +58,7 @@ def test_cannot_create_github_configuration_due_to_unique_constraint( kwargs={"organisation_pk": organisation.id}, ) # When - response = client.post(url, data) + response = admin_client_new.post(url, data) # Then assert response.status_code == status.HTTP_400_BAD_REQUEST @@ -80,12 +68,8 @@ def test_cannot_create_github_configuration_due_to_unique_constraint( ) -@pytest.mark.parametrize( - "client", - [lazy_fixture("admin_master_api_key_client"), lazy_fixture("admin_client")], -) def test_cannot_create_github_configuration_when_the_organization_already_has_an_integration( - client: APIClient, + admin_client_new: APIClient, organisation: Organisation, github_configuration: GithubConfiguration, ) -> None: @@ -98,7 +82,7 @@ def test_cannot_create_github_configuration_when_the_organization_already_has_an kwargs={"organisation_pk": organisation.id}, ) # When - response = client.post(url, data) + response = admin_client_new.post(url, data) # Then assert response.status_code == status.HTTP_400_BAD_REQUEST @@ -108,12 +92,8 @@ def test_cannot_create_github_configuration_when_the_organization_already_has_an ) -@pytest.mark.parametrize( - "client", - [lazy_fixture("admin_master_api_key_client"), lazy_fixture("admin_client")], -) def test_delete_github_configuration( - client: APIClient, + admin_client_new: APIClient, organisation: Organisation, github_configuration: GithubConfiguration, github_repository: GithubRepository, @@ -127,17 +107,13 @@ def test_delete_github_configuration( ], ) # When - response = client.delete(url) + response = admin_client_new.delete(url) # Then assert response.status_code == status.HTTP_204_NO_CONTENT -@pytest.mark.parametrize( - "client", - [lazy_fixture("admin_master_api_key_client"), lazy_fixture("admin_client")], -) def test_get_github_repository( - client: APIClient, + admin_client_new: APIClient, organisation: Organisation, github_configuration: GithubConfiguration, ): @@ -147,17 +123,13 @@ def test_get_github_repository( args=[organisation.id, github_configuration.id], ) # When - response = client.get(url) + response = admin_client_new.get(url) # Then assert response.status_code == status.HTTP_200_OK -@pytest.mark.parametrize( - "client", - [lazy_fixture("admin_master_api_key_client"), lazy_fixture("admin_client")], -) def test_create_github_repository( - client: APIClient, + admin_client_new: APIClient, organisation: Organisation, github_configuration: GithubConfiguration, project: Project, @@ -175,7 +147,7 @@ def test_create_github_repository( args=[organisation.id, github_configuration.id], ) # When - response = client.post(url, data) + response = admin_client_new.post(url, data) # Then assert response.status_code == status.HTTP_201_CREATED @@ -207,12 +179,8 @@ def test_cannot_create_github_repository_when_does_not_have_permissions( assert response.status_code == status.HTTP_403_FORBIDDEN -@pytest.mark.parametrize( - "client", - [lazy_fixture("admin_master_api_key_client"), lazy_fixture("admin_client")], -) def test_cannot_create_github_repository_due_to_unique_constraint( - client: APIClient, + admin_client_new: APIClient, organisation: Organisation, github_configuration: GithubConfiguration, project: Project, @@ -232,7 +200,7 @@ def test_cannot_create_github_repository_due_to_unique_constraint( ) # When - response = client.post(url, data) + response = admin_client_new.post(url, data) # Then assert response.status_code == status.HTTP_400_BAD_REQUEST @@ -242,12 +210,8 @@ def test_cannot_create_github_repository_due_to_unique_constraint( ) -@pytest.mark.parametrize( - "client", - [lazy_fixture("admin_master_api_key_client"), lazy_fixture("admin_client")], -) def test_github_delete_repository( - client: APIClient, + admin_client_new: APIClient, organisation: Organisation, feature_external_resource: FeatureExternalResource, github_configuration: GithubConfiguration, @@ -266,7 +230,7 @@ def test_github_delete_repository( for feature in github_repository.project.features.all(): assert FeatureExternalResource.objects.filter(feature=feature).exists() # When - response = client.delete(url) + response = admin_client_new.delete(url) # Then assert response.status_code == status.HTTP_204_NO_CONTENT for feature in github_repository.project.features.all(): @@ -288,12 +252,8 @@ def json(self): return MockResponse(json_data={"data": "data"}, status_code=200) -@pytest.mark.parametrize( - "client", - [lazy_fixture("admin_master_api_key_client"), lazy_fixture("admin_client")], -) def test_fetch_pull_requests( - client: APIClient, + admin_client_new: APIClient, organisation: Organisation, github_configuration: GithubConfiguration, github_repository: GithubRepository, @@ -311,7 +271,7 @@ def test_fetch_pull_requests( data = {"repo_owner": "owner", "repo_name": "repo"} # When - response = client.get(url, data=data) + response = admin_client_new.get(url, data=data) response_json = response.json() # Then @@ -329,12 +289,8 @@ def test_fetch_pull_requests( ) -@pytest.mark.parametrize( - "client", - [lazy_fixture("admin_master_api_key_client"), lazy_fixture("admin_client")], -) def test_fetch_issue( - client: APIClient, + admin_client_new: APIClient, organisation: Organisation, github_configuration: GithubConfiguration, github_repository: GithubRepository, @@ -349,7 +305,7 @@ def test_fetch_issue( url = reverse("api-v1:organisations:get-github-issues", args=[organisation.id]) data = {"repo_owner": "owner", "repo_name": "repo"} # When - response = client.get(url, data=data) + response = admin_client_new.get(url, data=data) # Then assert response.status_code == 200 @@ -367,12 +323,8 @@ def test_fetch_issue( ) -@pytest.mark.parametrize( - "client", - [lazy_fixture("admin_master_api_key_client"), lazy_fixture("admin_client")], -) def test_fetch_repositories( - client: APIClient, + admin_client_new: APIClient, organisation: Organisation, github_configuration: GithubConfiguration, github_repository: GithubRepository, @@ -388,7 +340,7 @@ def test_fetch_repositories( "api-v1:organisations:get-github-installation-repos", args=[organisation.id] ) # When - response = client.get(url) + response = admin_client_new.get(url) # Then assert response.status_code == 200 From f4efe2c6bd0750092adaa494f89da964e5b77b96 Mon Sep 17 00:00:00 2001 From: novakzaballa Date: Thu, 2 May 2024 16:30:17 -0400 Subject: [PATCH 5/6] Solve external HTTP call when an feature external resources was deleted test --- .../test_unit_feature_external_resources_views.py | 14 ++++++++++++++ 1 file changed, 14 insertions(+) diff --git a/api/tests/unit/features/test_unit_feature_external_resources_views.py b/api/tests/unit/features/test_unit_feature_external_resources_views.py index 91fbabb0a233..63b247d16b81 100644 --- a/api/tests/unit/features/test_unit_feature_external_resources_views.py +++ b/api/tests/unit/features/test_unit_feature_external_resources_views.py @@ -228,6 +228,9 @@ def test_delete_feature_external_resource( "integrations.github.github.generate_token", ) mock_generate_token.return_value = "mocked_token" + github_request_mock = mocker.patch( + "requests.post", side_effect=mocked_requests_post + ) url = reverse( "api-v1:projects:feature-external-resources-detail", args=[project.id, feature.id, feature_external_resource.id], @@ -237,6 +240,17 @@ def test_delete_feature_external_resource( response = admin_client_new.delete(url) # Then + github_request_mock.assert_called_with( + "https://api.github.com/repos/userexample/example-project-repo/issues/11/comments", + json={ + "body": "### The feature flag Test Feature1 was unlinked from the issue/PR" + }, + headers={ + "Accept": "application/vnd.github.v3+json", + "Authorization": "Bearer mocked_token", + }, + timeout=10, + ) assert response.status_code == status.HTTP_204_NO_CONTENT assert not FeatureExternalResource.objects.filter( id=feature_external_resource.id From 1aee49f8744df0314f3d8a17d5b1cc825d630f59 Mon Sep 17 00:00:00 2001 From: Novak Zaballa Date: Thu, 2 May 2024 18:33:10 -0400 Subject: [PATCH 6/6] Added a condition to check the cause of the exception before raise --- .../feature_external_resources/views.py | 14 ++++++++++---- api/integrations/github/views.py | 19 +++++++++++++------ ...t_unit_feature_external_resources_views.py | 2 +- 3 files changed, 24 insertions(+), 11 deletions(-) diff --git a/api/features/feature_external_resources/views.py b/api/features/feature_external_resources/views.py index b046c9dc5274..822c3a07400e 100644 --- a/api/features/feature_external_resources/views.py +++ b/api/features/feature_external_resources/views.py @@ -1,3 +1,5 @@ +import re + from django.db.utils import IntegrityError from django.shortcuts import get_object_or_404 from rest_framework import status, viewsets @@ -49,10 +51,14 @@ def create(self, request, *args, **kwargs): try: return super().create(request, *args, **kwargs) - except IntegrityError: - raise ValidationError( - detail="Duplication error. The feature already has this resource URI" - ) + + except IntegrityError as e: + if re.search(r"Key \(feature_id, url\)", str(e)) and re.search( + r"already exists.$", str(e) + ): + raise ValidationError( + detail="Duplication error. The feature already has this resource URI" + ) def perform_update(self, serializer): external_resource_id = int(self.kwargs["id"]) diff --git a/api/integrations/github/views.py b/api/integrations/github/views.py index f661f5a809d9..ddbea03bae0e 100644 --- a/api/integrations/github/views.py +++ b/api/integrations/github/views.py @@ -1,3 +1,4 @@ +import re from functools import wraps import requests @@ -65,8 +66,9 @@ def get_queryset(self): def create(self, request, *args, **kwargs): try: return super().create(request, *args, **kwargs) - except IntegrityError: - raise DuplicateGitHubIntegration + except IntegrityError as e: + if re.search(r"Key \(organisation_id\)=\(\d+\) already exists", str(e)): + raise DuplicateGitHubIntegration class GithubRepositoryViewSet(viewsets.ModelViewSet): @@ -91,10 +93,15 @@ def create(self, request, *args, **kwargs): try: return super().create(request, *args, **kwargs) - except IntegrityError: - raise ValidationError( - detail="Duplication error. The GitHub repository already linked" - ) + + except IntegrityError as e: + if re.search( + r"Key \(github_configuration_id, project_id, repository_owner, repository_name\)", + str(e), + ) and re.search(r"already exists.$", str(e)): + raise ValidationError( + detail="Duplication error. The GitHub repository already linked" + ) @api_view(["GET"]) diff --git a/api/tests/unit/features/test_unit_feature_external_resources_views.py b/api/tests/unit/features/test_unit_feature_external_resources_views.py index 63b247d16b81..2f5330ac0dd4 100644 --- a/api/tests/unit/features/test_unit_feature_external_resources_views.py +++ b/api/tests/unit/features/test_unit_feature_external_resources_views.py @@ -115,7 +115,7 @@ def test_create_feature_external_resource( ) -def test_cannot_create_feature_external_resource_when_doesnt_have_a_valid_gitHub_integration( +def test_cannot_create_feature_external_resource_when_doesnt_have_a_valid_github_integration( admin_client_new: APIClient, feature: Feature, project: Project,