diff --git a/api/features/feature_external_resources/models.py b/api/features/feature_external_resources/models.py index ccb075caa194..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,12 +48,18 @@ 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 hasattr(self.feature.project.organisation, "github_config"): - github_configuration = self.feature.project.organisation.github_config - - feature_states = FeatureState.objects.filter(feature_id=self.feature_id) + 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 + ) feature_data: GithubData = generate_data( github_configuration, self.feature_id, @@ -68,8 +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 hasattr(self.feature.project.organisation, "github_config"): - github_configuration = self.feature.project.organisation.github_config + 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 d254d9b79252..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 @@ -6,6 +8,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 +32,15 @@ def create(self, request, *args, **kwargs): ), ) - if not hasattr(feature.project.organisation, "github_config") 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" @@ -42,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/features/tasks.py b/api/features/tasks.py index f5a45a20dba5..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 ( @@ -65,8 +66,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 = ( + Organisation.objects.prefetch_related("github_config") + .get(id=instance.environment.project.organisationn_id) + .github_config.first() + ) feature_state = { "environment_name": new_state["environment"]["name"], "feature_value": new_state["enabled"], @@ -78,11 +82,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_20240502_1949.py b/api/integrations/github/migrations/0002_auto_20240502_1949.py new file mode 100644 index 000000000000..4739e2825ea9 --- /dev/null +++ b/api/integrations/github/migrations/0002_auto_20240502_1949.py @@ -0,0 +1,24 @@ +# Generated by Django 3.2.25 on 2024-05-02 19:49 + +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(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 7aebde9ce0bc..22dcd6ee5333 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", + ], + name="githubconf_organisation_id_idx", + condition=models.Q(deleted_at__isnull=True), + ) + ] + class GithubRepository(LifecycleModelMixin, SoftDeleteExportableModel): github_configuration = models.ForeignKey( diff --git a/api/integrations/github/views.py b/api/integrations/github/views.py index 580b62aff629..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 @@ -12,6 +13,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 +63,13 @@ def get_queryset(self): organisation_id=self.kwargs["organisation_pk"] ) + def create(self, request, *args, **kwargs): + try: + return super().create(request, *args, **kwargs) + except IntegrityError as e: + if re.search(r"Key \(organisation_id\)=\(\d+\) already exists", str(e)): + raise DuplicateGitHubIntegration + class GithubRepositoryViewSet(viewsets.ModelViewSet): permission_classes = ( @@ -84,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"]) @@ -95,9 +109,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 +146,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/features/test_unit_feature_external_resources_views.py b/api/tests/unit/features/test_unit_feature_external_resources_views.py index 8b242e9464fc..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 @@ -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, +def test_cannot_create_feature_external_resource_when_doesnt_have_a_valid_github_integration( + 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, @@ -248,27 +228,37 @@ 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], ) # When - response = client.delete(url) + 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 ).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 +272,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 +293,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 5e3fcecfcf58..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,61 @@ 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( + admin_client_new: 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 = admin_client_new.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"] + ) + + +def test_cannot_create_github_configuration_when_the_organization_already_has_an_integration( + admin_client_new: 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 = admin_client_new.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"] + ) + + def test_delete_github_configuration( - client: APIClient, + admin_client_new: APIClient, organisation: Organisation, github_configuration: GithubConfiguration, github_repository: GithubRepository, @@ -71,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, ): @@ -91,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, @@ -119,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 @@ -151,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, @@ -176,22 +200,18 @@ 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 assert ( - "Duplication error. The Github repository already linked" in response.json()[0] + "Duplication error. The GitHub repository already linked" in response.json()[0] ) -@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, @@ -210,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(): @@ -232,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, @@ -255,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 @@ -273,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, @@ -293,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 @@ -311,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, @@ -332,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