From bf67b1dc0ad3a34c38f8aabb8a5cfaf5f65863de Mon Sep 17 00:00:00 2001 From: Novak Zaballa <41410593+novakzaballa@users.noreply.github.com> Date: Wed, 15 May 2024 06:04:22 -0400 Subject: [PATCH] fix: Create GitHub comment as table (#3948) --- api/conftest.py | 23 +++++++ .../feature_external_resources/models.py | 11 +-- api/features/models.py | 2 +- api/integrations/github/constants.py | 11 ++- api/integrations/github/github.py | 69 ++++++++++++------- api/integrations/github/tasks.py | 6 +- ...t_unit_feature_external_resources_views.py | 38 +++++++--- 7 files changed, 117 insertions(+), 43 deletions(-) diff --git a/api/conftest.py b/api/conftest.py index e7537ea549f1..265a07ecb52d 100644 --- a/api/conftest.py +++ b/api/conftest.py @@ -553,6 +553,29 @@ def segment_featurestate(feature_segment, feature, environment): ) +@pytest.fixture() +def feature_with_value_segment( + feature_with_value: Feature, segment: Segment, environment: Environment +) -> FeatureSegment: + return FeatureSegment.objects.create( + feature=feature_with_value, segment=segment, environment=environment + ) + + +@pytest.fixture() +def segment_featurestate_and_feature_with_value( + feature_with_value_segment: FeatureSegment, + feature_with_value: Feature, + environment: Environment, +) -> FeatureState: + return FeatureState.objects.create( + feature_segment=feature_with_value_segment, + feature=feature_with_value, + environment=environment, + updated_at="2024-01-01 00:00:00", + ) + + @pytest.fixture() def environment_api_key(environment): return EnvironmentAPIKey.objects.create( diff --git a/api/features/feature_external_resources/models.py b/api/features/feature_external_resources/models.py index e8f307230914..29860fb5a173 100644 --- a/api/features/feature_external_resources/models.py +++ b/api/features/feature_external_resources/models.py @@ -61,11 +61,12 @@ def execute_after_save_actions(self): feature_id=self.feature_id, identity_id__isnull=True ) feature_data: GithubData = generate_data( - github_configuration, - self.feature_id, - self.feature.name, - WebhookEventType.FEATURE_EXTERNAL_RESOURCE_ADDED.value, - feature_states, + github_configuration=github_configuration, + feature_id=self.feature_id, + feature_name=self.feature.name, + 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( diff --git a/api/features/models.py b/api/features/models.py index 59014edac2c4..8c3db8b473f4 100644 --- a/api/features/models.py +++ b/api/features/models.py @@ -1014,7 +1014,6 @@ def create_github_comment(self) -> None: if ( not self.identity_id - and not self.feature_segment and self.feature.external_resources.exists() and self.environment.project.github_project.exists() and self.environment.project.organisation.github_config.exists() @@ -1032,6 +1031,7 @@ def create_github_comment(self) -> None: 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: diff --git a/api/integrations/github/constants.py b/api/integrations/github/constants.py index e5ca5be737ff..713e6e73a0e9 100644 --- a/api/integrations/github/constants.py +++ b/api/integrations/github/constants.py @@ -1,8 +1,13 @@ GITHUB_API_URL = "https://api.github.com/" GITHUB_API_VERSION = "2022-11-28" -LINK_FEATURE_TEXT = "### This pull request is linked to a Flagsmith Feature (`%s`):\n" +LINK_FEATURE_TITLE = """**Flagsmith feature linked:** `%s` +Default Values:\n""" +FEATURE_TABLE_HEADER = """| Environment | Enabled | Value | Last Updated (UTC) | +| :--- | :----- | :------ | :------ |\n""" +FEATURE_TABLE_ROW = "| [%s](%s) | %s | %s | %s |\n" +LINK_SEGMENT_TITLE = "Segment `%s` values:\n" UNLINKED_FEATURE_TEXT = "### The feature flag `%s` was unlinked from the issue/PR" -UPDATED_FEATURE_TEXT = "### The Flagsmith Feature `%s` was updated in the environment " -LAST_UPDATED_FEATURE_TEXT = "Last Updated %s" +UPDATED_FEATURE_TEXT = "Flagsmith Feature `%s` has been updated:\n" DELETED_FEATURE_TEXT = "### The Feature Flag `%s` was deleted" +FEATURE_ENVIRONMENT_URL = "%s/project/%s/environment/%s/features?feature=%s&tab=%s" diff --git a/api/integrations/github/github.py b/api/integrations/github/github.py index fad30c2cd6c4..5608a209bddb 100644 --- a/api/integrations/github/github.py +++ b/api/integrations/github/github.py @@ -1,18 +1,21 @@ -import datetime import logging import typing from dataclasses import dataclass import requests +from core.helpers import get_current_site_url from django.conf import settings from features.models import FeatureState, FeatureStateValue from integrations.github.client import generate_token from integrations.github.constants import ( DELETED_FEATURE_TEXT, + FEATURE_ENVIRONMENT_URL, + FEATURE_TABLE_HEADER, + FEATURE_TABLE_ROW, GITHUB_API_URL, - LAST_UPDATED_FEATURE_TEXT, - LINK_FEATURE_TEXT, + LINK_FEATURE_TITLE, + LINK_SEGMENT_TITLE, UNLINKED_FEATURE_TEXT, UPDATED_FEATURE_TEXT, ) @@ -28,8 +31,9 @@ class GithubData: feature_id: int feature_name: str type: str - feature_states: typing.List[dict[str, typing.Any]] = None - url: str = None + feature_states: typing.List[dict[str, typing.Any]] | None = None + url: str | None = None + project_id: int | None = None @classmethod def from_dict(cls, data_dict: dict) -> "GithubData": @@ -65,6 +69,8 @@ def post_comment_to_github( def generate_body_comment( name: str, event_type: str, + project_id: int, + feature_id: int, feature_states: typing.List[typing.Dict[str, typing.Any]], ) -> str: @@ -78,25 +84,34 @@ def generate_body_comment( if is_removed: return delete_text - last_updated_string = LAST_UPDATED_FEATURE_TEXT % ( - datetime.datetime.now().strftime("%dth %b %Y %I:%M%p") - ) - updated_text = UPDATED_FEATURE_TEXT % (name) - - result = updated_text if is_update else LINK_FEATURE_TEXT % (name) + result = UPDATED_FEATURE_TEXT % (name) if is_update else LINK_FEATURE_TITLE % (name) + last_segment_name = "" + if len(feature_states) > 0 and not feature_states[0].get("segment_name"): + result += FEATURE_TABLE_HEADER - # if feature_states is None: for v in feature_states: feature_value = v.get("feature_state_value") - feature_value_type = v.get("feature_state_value_type") - - feature_value_string = f"\n{feature_value_type}\n```{feature_value if feature_value else 'No value.'}```\n\n" - - result += f"**{v['environment_name']}{' - ' + v['segment_name'] if v.get('segment_name') else ''}**\n" - result += f"- [{'x' if v['feature_value'] else ' '}] {'Enabled' if v['feature_value'] else 'Disabled'}" - result += feature_value_string + tab = "segment-overrides" if v.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"), + 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"] + result += FEATURE_TABLE_HEADER + table_row = FEATURE_TABLE_ROW % ( + v["environment_name"], + environment_link_url, + "✅ Enabled" if v["enabled"] else "❌ Disabled", + f"`{feature_value}`" if feature_value else "", + v["last_updated"], + ) + result += table_row - result += last_updated_string return result @@ -109,8 +124,11 @@ def generate_data( feature_id: int, feature_name: str, type: str, - feature_states: typing.Union[list[FeatureState], list[FeatureStateValue]] = None, - url: str = None, + feature_states: ( + typing.Union[list[FeatureState], list[FeatureStateValue]] | None + ) = None, + url: str | None = None, + project_id: int | None = None, ) -> GithubData: if feature_states: @@ -126,7 +144,11 @@ def generate_data( 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["feature_value"] = feature_state.enabled + feature_env_data["enabled"] = feature_state.enabled + feature_env_data["last_updated"] = feature_state.updated_at + feature_env_data["environment_api_key"] = ( + feature_state.environment.api_key + ) if ( hasattr(feature_state, "feature_segment") and feature_state.feature_segment is not None @@ -147,4 +169,5 @@ def generate_data( else None ), feature_states=feature_states_list if feature_states else None, + project_id=project_id, ) diff --git a/api/integrations/github/tasks.py b/api/integrations/github/tasks.py index 89e301f95d0e..aba93e4e0621 100644 --- a/api/integrations/github/tasks.py +++ b/api/integrations/github/tasks.py @@ -24,12 +24,16 @@ class CallGithubData: def send_post_request(data: CallGithubData) -> None: feature_name = data.github_data.feature_name + feature_id = data.github_data.feature_id + project_id = data.github_data.project_id event_type = data.event_type feature_states = feature_states = ( data.github_data.feature_states if data.github_data.feature_states else None ) installation_id = data.github_data.installation_id - body = generate_body_comment(feature_name, event_type, feature_states) + body = generate_body_comment( + feature_name, event_type, project_id, feature_id, feature_states + ) if ( event_type == WebhookEventType.FLAG_UPDATED.value 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 6df4cc47393b..1cd5ea031a53 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,10 +1,8 @@ -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_mock import MockerFixture from rest_framework import status from rest_framework.test import APIClient @@ -37,24 +35,44 @@ def json(self): return MockResponse(json_data={"data": "data"}, status_code=200) -@freeze_time("2024-01-01") def test_create_feature_external_resource( admin_client_new: APIClient, feature_with_value: Feature, + segment_featurestate_and_feature_with_value: FeatureState, + environment: Environment, project: Project, github_configuration: GithubConfiguration, github_repository: GithubRepository, - mocker, + 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 ) - datetime_now = datetime.datetime.now() + + feature_state = FeatureState.objects.filter(feature=feature_with_value).first() + + 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" + ) feature_external_resource_data = { "type": "GITHUB_ISSUE", @@ -76,9 +94,7 @@ def test_create_feature_external_resource( # Then github_request_mock.assert_called_with( "https://api.github.com/repos/repoowner/repo-name/issues/35/comments", - json={ - "body": f"### This pull request is linked to a Flagsmith Feature (`feature_with_value`):\n**Test Environment**\n- [ ] Disabled\nunicode\n```value```\n\nLast Updated {datetime_now.strftime('%dth %b %Y %I:%M%p')}" # noqa E501 - }, + json={"body": f"{expected_comment_body}"}, headers={ "Accept": "application/vnd.github.v3+json", "Authorization": "Bearer mocked_token", @@ -320,9 +336,10 @@ def test_create_github_comment_on_feature_state_updated( # noqa: C901 with_environment_permissions: WithEnvironmentPermissionsCallable, feature_external_resource: FeatureExternalResource, feature: Feature, + project: Project, github_configuration: GithubConfiguration, github_repository: GithubRepository, - mocker, + mocker: MockerFixture, environment: Environment, event_type: str, ) -> None: @@ -412,4 +429,5 @@ def test_create_github_comment_on_feature_state_updated( # noqa: C901 feature_name=feature.name, type=WebhookEventType.FLAG_UPDATED.value, feature_states=[feature_state], + project_id=project.id, )