From e9246bc02cb856ab9e4dcf42f727a6e1c40437d0 Mon Sep 17 00:00:00 2001 From: Novak Zaballa <41410593+novakzaballa@users.noreply.github.com> Date: Wed, 15 May 2024 18:11:41 -0400 Subject: [PATCH] fix: segment overrides stale feature state value while creating GitHub comment (#3961) --- api/conftest.py | 11 + .../feature_external_resources/models.py | 7 +- api/features/models.py | 72 ++--- api/features/serializers.py | 32 +- api/integrations/github/github.py | 47 +-- api/integrations/github/tasks.py | 2 +- ...t_unit_feature_external_resources_views.py | 282 ++++++++++++------ 7 files changed, 295 insertions(+), 158 deletions(-) diff --git a/api/conftest.py b/api/conftest.py index 265a07ecb52d..583e60607713 100644 --- a/api/conftest.py +++ b/api/conftest.py @@ -974,6 +974,17 @@ def feature_external_resource(feature: Feature) -> FeatureExternalResource: ) +@pytest.fixture() +def feature_with_value_external_resource( + feature_with_value: Feature, +) -> FeatureExternalResource: + return FeatureExternalResource.objects.create( + url="https://github.com/userexample/example-project-repo/issues/11", + type="GITHUB_ISSUE", + feature=feature_with_value, + ) + + @pytest.fixture() def github_configuration(organisation: Organisation) -> GithubConfiguration: return GithubConfiguration.objects.create( diff --git a/api/features/feature_external_resources/models.py b/api/features/feature_external_resources/models.py index 29860fb5a173..14f44cb7260f 100644 --- a/api/features/feature_external_resources/models.py +++ b/api/features/feature_external_resources/models.py @@ -62,11 +62,9 @@ def execute_after_save_actions(self): ) feature_data: GithubData = generate_data( github_configuration=github_configuration, - feature_id=self.feature_id, - feature_name=self.feature.name, + feature=self.feature, type=WebhookEventType.FEATURE_EXTERNAL_RESOURCE_ADDED.value, feature_states=feature_states, - project_id=self.feature.project_id, ) call_github_app_webhook_for_feature_state.delay( @@ -85,8 +83,7 @@ def execute_before_save_actions(self) -> None: ): feature_data: GithubData = generate_data( github_configuration=github_configuration, - feature_id=self.feature_id, - feature_name=self.feature.name, + feature=self.feature, type=WebhookEventType.FEATURE_EXTERNAL_RESOURCE_REMOVED.value, url=self.url, ) diff --git a/api/features/models.py b/api/features/models.py index 8c3db8b473f4..5b0719d975cb 100644 --- a/api/features/models.py +++ b/api/features/models.py @@ -138,6 +138,35 @@ class Meta: # TODO: after upgrade to Django 4.0 use UniqueConstraint() ordering = ("id",) # explicit ordering to prevent pagination warnings + @hook(AFTER_SAVE) + def create_github_comment(self) -> None: + from integrations.github.github import GithubData, generate_data + from integrations.github.tasks import ( + call_github_app_webhook_for_feature_state, + ) + from webhooks.webhooks import WebhookEventType + + if ( + self.external_resources.exists() + and self.project.github_project.exists() + and self.project.organisation.github_config.exists() + and self.deleted_at + ): + github_configuration = GithubConfiguration.objects.get( + organisation_id=self.project.organisation_id + ) + + feature_data: GithubData = generate_data( + github_configuration=github_configuration, + feature=self, + type=WebhookEventType.FLAG_DELETED.value, + feature_states=[], + ) + + call_github_app_webhook_for_feature_state.delay( + args=(asdict(feature_data),), + ) + @hook(AFTER_CREATE) def create_feature_states(self): FeatureState.create_initial_feature_states_for_feature(feature=self) @@ -1004,49 +1033,6 @@ def copy_identity_feature_states( target_feature_state.save() - @hook(AFTER_SAVE) - def create_github_comment(self) -> None: - from integrations.github.github import GithubData, generate_data - from integrations.github.tasks import ( - call_github_app_webhook_for_feature_state, - ) - from webhooks.webhooks import WebhookEventType - - if ( - not self.identity_id - and self.feature.external_resources.exists() - and self.environment.project.github_project.exists() - and self.environment.project.organisation.github_config.exists() - ): - github_configuration = GithubConfiguration.objects.get( - organisation_id=self.environment.project.organisation_id - ) - feature_states = [] - feature_states.append(self) - - if self.deleted_at is None: - feature_data: GithubData = generate_data( - github_configuration=github_configuration, - feature_id=self.feature.id, - feature_name=self.feature.name, - type=WebhookEventType.FLAG_UPDATED.value, - feature_states=feature_states, - project_id=self.environment.project_id, - ) - - if self.deleted_at is not None: - feature_data: GithubData = generate_data( - github_configuration=github_configuration, - feature_id=self.feature.id, - feature_name=self.feature.name, - type=WebhookEventType.FLAG_DELETED.value, - feature_states=feature_states, - ) - - call_github_app_webhook_for_feature_state.delay( - args=(asdict(feature_data),), - ) - class FeatureStateValue( AbstractBaseFeatureValueModel, diff --git a/api/features/serializers.py b/api/features/serializers.py index 242710c65de9..f026e0562e80 100644 --- a/api/features/serializers.py +++ b/api/features/serializers.py @@ -1,4 +1,5 @@ import typing +from dataclasses import asdict from datetime import datetime import django.core.exceptions @@ -12,6 +13,9 @@ from environments.sdk.serializers_mixins import ( HideSensitiveFieldsSerializerMixin, ) +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 metadata.serializers import MetadataSerializer, SerializerWithMetadata from projects.models import Project from users.serializers import ( @@ -22,6 +26,7 @@ from util.drf_writable_nested.serializers import ( DeleteBeforeUpdateWritableNestedModelSerializer, ) +from webhooks.webhooks import WebhookEventType from .constants import INTERSECTION, UNION from .feature_segments.serializers import ( @@ -460,7 +465,32 @@ def get_feature_state_value(self, obj): def save(self, **kwargs): try: - return super().save(**kwargs) + response = super().save(**kwargs) + + feature_state = self.instance + if ( + not feature_state.identity_id + and feature_state.feature.external_resources.exists() + and feature_state.environment.project.github_project.exists() + and feature_state.environment.project.organisation.github_config.exists() + ): + github_configuration = GithubConfiguration.objects.get( + organisation_id=feature_state.environment.project.organisation_id + ) + feature_states = [] + feature_states.append(feature_state) + feature_data: GithubData = generate_data( + github_configuration=github_configuration, + feature=feature_state.feature, + type=WebhookEventType.FLAG_UPDATED.value, + feature_states=feature_states, + ) + + call_github_app_webhook_for_feature_state.delay( + args=(asdict(feature_data),), + ) + + return response except django.core.exceptions.ValidationError as e: raise serializers.ValidationError(str(e)) diff --git a/api/integrations/github/github.py b/api/integrations/github/github.py index 5608a209bddb..6e6d5f7758dc 100644 --- a/api/integrations/github/github.py +++ b/api/integrations/github/github.py @@ -5,8 +5,9 @@ import requests from core.helpers import get_current_site_url from django.conf import settings +from django.utils.formats import get_format -from features.models import FeatureState, FeatureStateValue +from features.models import Feature, FeatureState, FeatureStateValue from integrations.github.client import generate_token from integrations.github.constants import ( DELETED_FEATURE_TEXT, @@ -89,26 +90,29 @@ def generate_body_comment( if len(feature_states) > 0 and not feature_states[0].get("segment_name"): result += FEATURE_TABLE_HEADER - for v in feature_states: - feature_value = v.get("feature_state_value") - tab = "segment-overrides" if v.get("segment_name") is not None else "value" + for fs in feature_states: + feature_value = fs.get("feature_state_value") + tab = "segment-overrides" if fs.get("segment_name") is not None else "value" environment_link_url = FEATURE_ENVIRONMENT_URL % ( get_current_site_url(), project_id, - v.get("environment_api_key"), + fs.get("environment_api_key"), feature_id, tab, ) - if v.get("segment_name") is not None and v["segment_name"] != last_segment_name: - result += "\n" + LINK_SEGMENT_TITLE % (v["segment_name"]) - last_segment_name = v["segment_name"] + if ( + fs.get("segment_name") is not None + and fs["segment_name"] != last_segment_name + ): + result += "\n" + LINK_SEGMENT_TITLE % (fs["segment_name"]) + last_segment_name = fs["segment_name"] result += FEATURE_TABLE_HEADER table_row = FEATURE_TABLE_ROW % ( - v["environment_name"], + fs["environment_name"], environment_link_url, - "✅ Enabled" if v["enabled"] else "❌ Disabled", + "✅ Enabled" if fs["enabled"] else "❌ Disabled", f"`{feature_value}`" if feature_value else "", - v["last_updated"], + fs["last_updated"], ) result += table_row @@ -121,31 +125,28 @@ def check_not_none(value: any) -> bool: def generate_data( github_configuration: GithubConfiguration, - feature_id: int, - feature_name: str, + feature: Feature, type: str, feature_states: ( typing.Union[list[FeatureState], list[FeatureStateValue]] | None ) = None, url: str | None = None, - project_id: int | None = None, ) -> GithubData: - if feature_states: feature_states_list = [] for feature_state in feature_states: feature_state_value = feature_state.get_feature_state_value() - feature_state_value_type = feature_state.get_feature_state_value_type( - feature_state_value - ) feature_env_data = {} + if check_not_none(feature_state_value): feature_env_data["feature_state_value"] = feature_state_value - feature_env_data["feature_state_value_type"] = feature_state_value_type + if type is not WebhookEventType.FEATURE_EXTERNAL_RESOURCE_REMOVED.value: feature_env_data["environment_name"] = feature_state.environment.name feature_env_data["enabled"] = feature_state.enabled - feature_env_data["last_updated"] = feature_state.updated_at + feature_env_data["last_updated"] = feature_state.updated_at.strftime( + get_format("DATETIME_INPUT_FORMATS")[0] + ) feature_env_data["environment_api_key"] = ( feature_state.environment.api_key ) @@ -159,8 +160,8 @@ def generate_data( feature_states_list.append(feature_env_data) return GithubData( - feature_id=feature_id, - feature_name=feature_name, + feature_id=feature.id, + feature_name=feature.name, installation_id=github_configuration.installation_id, type=type, url=( @@ -169,5 +170,5 @@ def generate_data( else None ), feature_states=feature_states_list if feature_states else None, - project_id=project_id, + project_id=feature.project_id, ) diff --git a/api/integrations/github/tasks.py b/api/integrations/github/tasks.py index aba93e4e0621..e241af521475 100644 --- a/api/integrations/github/tasks.py +++ b/api/integrations/github/tasks.py @@ -27,7 +27,7 @@ def send_post_request(data: CallGithubData) -> None: feature_id = data.github_data.feature_id project_id = data.github_data.project_id event_type = data.event_type - feature_states = feature_states = ( + feature_states = ( data.github_data.feature_states if data.github_data.feature_states else None ) installation_id = data.github_data.installation_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 1cd5ea031a53..84d4b169018f 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,7 +1,7 @@ -import pytest import simplejson as json from django.core.serializers.json import DjangoJSONEncoder from django.urls import reverse +from django.utils.formats import get_format from pytest_mock import MockerFixture from rest_framework import status from rest_framework.test import APIClient @@ -10,16 +10,52 @@ from environments.permissions.constants import UPDATE_FEATURE_STATE from features.feature_external_resources.models import FeatureExternalResource from features.models import Feature, FeatureState -from features.serializers import FeatureStateSerializerBasic -from integrations.github.github import GithubData +from features.serializers import ( + FeatureStateSerializerBasic, + WritableNestedFeatureStateSerializer, +) from integrations.github.models import GithubConfiguration, GithubRepository from projects.models import Project from tests.types import WithEnvironmentPermissionsCallable -from webhooks.webhooks import WebhookEventType _django_json_encoder_default = DjangoJSONEncoder().default +def expected_default_body( + project_id: str, + environment_api_key: str, + feature_id: str, + feature_state_updated_at: str, + feature_state_enabled: str = "❌ Disabled", + feature_state_value: str = "`value`", +) -> str: + return ( + "| Environment | Enabled | Value | Last Updated (UTC) |\n" + "| :--- | :----- | :------ | :------ |\n" + f"| [Test Environment](https://example.com/project/{project_id}/" + f"environment/{environment_api_key}/features?feature={feature_id}&tab=value) " + f"| {feature_state_enabled} | {feature_state_value} | {feature_state_updated_at} |\n" + ) + + +def expected_segment_comment_body( + project_id: str, + environment_api_key: str, + feature_id: str, + segment_override_updated_at: str, + segment_override_enabled: str, + segment_override_value: str, +) -> str: + return ( + "Segment `segment` values:\n" + "| Environment | Enabled | Value | Last Updated (UTC) |\n" + "| :--- | :----- | :------ | :------ |\n" + f"| [Test Environment](https://example.com/project/{project_id}/" + f"environment/{environment_api_key}/features?feature={feature_id}&tab=segment-overrides) " + f"| {segment_override_enabled} | {segment_override_value} | {segment_override_updated_at} |\n" + ) + + def mocked_requests_post(*args, **kwargs): class MockResponse: def __init__(self, json_data, status_code): @@ -56,22 +92,33 @@ def test_create_feature_external_resource( ) feature_state = FeatureState.objects.filter(feature=feature_with_value).first() + feature_state_updated_at = feature_state.updated_at.strftime( + get_format("DATETIME_INPUT_FORMATS")[0] + ) + segment_override_updated_at = ( + segment_featurestate_and_feature_with_value.updated_at.strftime( + get_format("DATETIME_INPUT_FORMATS")[0] + ) + ) expected_comment_body = ( "**Flagsmith feature linked:** `feature_with_value`\n" - "Default Values:\n" - "| Environment | Enabled | Value | Last Updated (UTC) |\n" - "| :--- | :----- | :------ | :------ |\n" - f"| [Test Environment](https://example.com/project/{project.id}/" - f"environment/{environment.api_key}/features?feature={feature_with_value.id}&tab=value) " - f"| ❌ Disabled | `value` | {feature_state.updated_at} |\n" - "\n" - "Segment `segment` values:\n" - "| Environment | Enabled | Value | Last Updated (UTC) |\n" - "| :--- | :----- | :------ | :------ |\n" - f"| [Test Environment](https://example.com/project/{project.id}/" - f"environment/{environment.api_key}/features?feature={feature_with_value.id}&tab=segment-overrides) " - f"| ❌ Disabled | `value` | {segment_featurestate_and_feature_with_value.updated_at} |\n" + + "Default Values:\n" + + expected_default_body( + project.id, + environment.api_key, + feature_with_value.id, + feature_state_updated_at, + ) + + "\n" + + expected_segment_comment_body( + project.id, + environment.api_key, + feature_with_value.id, + segment_override_updated_at, + "❌ Disabled", + "`value`", + ) ) feature_external_resource_data = { @@ -324,14 +371,7 @@ def test_get_feature_external_resource( assert response.data["url"] == feature_external_resource.url -@pytest.mark.parametrize( - "event_type", - [ - ("update"), - ("delete"), - ], -) -def test_create_github_comment_on_feature_state_updated( # noqa: C901 +def test_create_github_comment_on_feature_state_updated( staff_client: APIClient, with_environment_permissions: WithEnvironmentPermissionsCallable, feature_external_resource: FeatureExternalResource, @@ -341,7 +381,6 @@ def test_create_github_comment_on_feature_state_updated( # noqa: C901 github_repository: GithubRepository, mocker: MockerFixture, environment: Environment, - event_type: str, ) -> None: # Given with_environment_permissions([UPDATE_FEATURE_STATE]) @@ -356,31 +395,21 @@ def test_create_github_comment_on_feature_state_updated( # noqa: C901 "requests.post", side_effect=mocked_requests_post ) - feature_state_value = feature_state.get_feature_state_value() - feature_env_data = {} - feature_env_data["feature_state_value"] = feature_state_value - feature_env_data["feature_state_value_type"] = ( - feature_state.get_feature_state_value_type(feature_state_value) - ) - feature_env_data["environment_name"] = environment.name - feature_env_data["feature_value"] = feature_state.enabled - if event_type == "update": - mock_generate_data = mocker.patch( - "integrations.github.github.generate_data", - return_value=GithubData( - installation_id=github_configuration.installation_id, - feature_id=feature.id, - feature_name=feature.name, - type=feature_external_resource.type, - feature_states=[feature_env_data], - url=feature_external_resource.url, - ), - ) + feature_state_updated_at = feature_state.updated_at.strftime( + get_format("DATETIME_INPUT_FORMATS")[0] + ) - mocker.patch( - "integrations.github.tasks.generate_body_comment", - return_value="Flag updated", + expected_body_comment = ( + "Flagsmith Feature `Test Feature1` has been updated:\n" + + expected_default_body( + project.id, + environment.api_key, + feature.id, + feature_state_updated_at, + "✅ Enabled", + "", ) + ) payload = dict(FeatureStateSerializerBasic(instance=feature_state).data) @@ -391,43 +420,126 @@ def test_create_github_comment_on_feature_state_updated( # noqa: C901 ) # When - if event_type == "update": - response = staff_client.put(path=url, data=payload, format="json") - elif event_type == "delete": - response = staff_client.delete(path=url) + response = staff_client.put(path=url, data=payload, format="json") # Then - if event_type == "update": - assert response.status_code == status.HTTP_200_OK - elif event_type == "delete": - assert response.status_code == status.HTTP_204_NO_CONTENT - - if event_type == "update": - github_request_mock.assert_called_with( - "https://api.github.com/repos/userexample/example-project-repo/issues/11/comments", - json={"body": "Flag updated"}, - headers={ - "Accept": "application/vnd.github.v3+json", - "Authorization": "Bearer mocked_token", - }, - timeout=10, - ) - elif event_type == "delete": - 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 deleted"}, - headers={ - "Accept": "application/vnd.github.v3+json", - "Authorization": "Bearer mocked_token", - }, - timeout=10, + assert response.status_code == status.HTTP_200_OK + + github_request_mock.assert_called_with( + "https://api.github.com/repos/userexample/example-project-repo/issues/11/comments", + json={"body": expected_body_comment}, + headers={ + "Accept": "application/vnd.github.v3+json", + "Authorization": "Bearer mocked_token", + }, + timeout=10, + ) + + +def test_create_github_comment_on_feature_was_deleted( + admin_client: APIClient, + with_environment_permissions: WithEnvironmentPermissionsCallable, + feature_external_resource: FeatureExternalResource, + feature: Feature, + project: Project, + github_configuration: GithubConfiguration, + github_repository: GithubRepository, + mocker: MockerFixture, +) -> None: + # Given + mock_generate_token = mocker.patch( + "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( + viewname="api-v1:projects:project-features-detail", + kwargs={"project_pk": project.id, "pk": feature.id}, + ) + + # When + response = admin_client.delete(path=url) + + # Then + assert response.status_code == status.HTTP_204_NO_CONTENT + + 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 deleted"}, + headers={ + "Accept": "application/vnd.github.v3+json", + "Authorization": "Bearer mocked_token", + }, + timeout=10, + ) + + +def test_create_github_comment_on_segment_override_updated( + feature_with_value: Feature, + segment_featurestate_and_feature_with_value: FeatureState, + feature_with_value_external_resource: FeatureExternalResource, + project: Project, + github_configuration: GithubConfiguration, + github_repository: GithubRepository, + mocker: MockerFixture, + environment: Environment, + admin_client: APIClient, +) -> None: + # Given + feature_state = segment_featurestate_and_feature_with_value + mock_generate_token = mocker.patch( + "integrations.github.github.generate_token", + ) + mock_generate_token.return_value = "mocked_token" + github_request_mock = mocker.patch( + "requests.post", side_effect=mocked_requests_post + ) + + payload = dict(WritableNestedFeatureStateSerializer(instance=feature_state).data) + + payload["enabled"] = not feature_state.enabled + payload["feature_state_value"]["string_value"] = "new value" + + url = reverse( + viewname="api-v1:features:featurestates-detail", + kwargs={"pk": feature_state.id}, + ) + + segment_override_updated_at = ( + segment_featurestate_and_feature_with_value.updated_at.strftime( + get_format("DATETIME_INPUT_FORMATS")[0] ) - if event_type == "update": - mock_generate_data.assert_called_with( - github_configuration=github_configuration, - feature_id=feature.id, - feature_name=feature.name, - type=WebhookEventType.FLAG_UPDATED.value, - feature_states=[feature_state], - project_id=project.id, + ) + + expected_comment_body = ( + "Flagsmith Feature `feature_with_value` has been updated:\n" + + "\n" + + expected_segment_comment_body( + project.id, + environment.api_key, + feature_with_value.id, + segment_override_updated_at, + "✅ Enabled", + "`new value`", ) + ) + + # When + response = admin_client.put(path=url, data=payload, format="json") + + # Then + assert response.status_code == status.HTTP_200_OK + + github_request_mock.assert_called_with( + "https://api.github.com/repos/userexample/example-project-repo/issues/11/comments", + json={"body": expected_comment_body}, + headers={ + "Accept": "application/vnd.github.v3+json", + "Authorization": "Bearer mocked_token", + }, + timeout=10, + )