Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix: Create GitHub comment as table #3948

Merged
merged 5 commits into from
May 15, 2024
Merged
Show file tree
Hide file tree
Changes from 2 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
11 changes: 6 additions & 5 deletions api/features/feature_external_resources/models.py
Original file line number Diff line number Diff line change
Expand Up @@ -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(
Expand Down
2 changes: 1 addition & 1 deletion api/features/models.py
Original file line number Diff line number Diff line change
Expand Up @@ -1005,7 +1005,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()
Expand All @@ -1023,6 +1022,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:
Expand Down
10 changes: 8 additions & 2 deletions api/integrations/github/constants.py
Original file line number Diff line number Diff line change
@@ -1,8 +1,14 @@
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 | Type | Updated (UTC) |
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
FEATURE_TABLE_HEADER = """| Environment | Enabled | Value | Type | Updated (UTC) |
FEATURE_TABLE_HEADER = """| Environment | Enabled | Value | Type | Last Updated (UTC) |

This way, we won't have to prepend "Last Updated" to every timestamp in this column.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done.

| :--- | :----- | :------ | :------- | :------ |\n"""
FEATURE_TABLE_ROW = "| [%s](%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 "
UPDATED_FEATURE_TEXT = "Flagsmith Feature `%s` has been updated:\n"
LAST_UPDATED_FEATURE_TEXT = "Last Updated %s"
DELETED_FEATURE_TEXT = "### The Feature Flag `%s` was deleted"
FEATURE_ENVIRONMENT_URL = "%s/project/%s/environment/%s/features?feature=%s&tab=%s"
60 changes: 45 additions & 15 deletions api/integrations/github/github.py
Original file line number Diff line number Diff line change
Expand Up @@ -4,15 +4,20 @@
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,
)
Expand All @@ -28,8 +33,9 @@
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":
Expand Down Expand Up @@ -65,6 +71,8 @@
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:

Expand All @@ -81,22 +89,37 @@
last_updated_string = LAST_UPDATED_FEATURE_TEXT % (
datetime.datetime.now().strftime("%dth %b %Y %I:%M%p")
)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not a fan of the custom datetime format.

Let's consider either sticking to ISO8601 or using Django locale, i.e.:

from django.utils.formats import get_format

datetime_format = get_format('DATETIME_INPUT_FORMATS')[0]

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done.

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

Check warning on line 96 in api/integrations/github/github.py

View check run for this annotation

Codecov / codecov/patch

api/integrations/github/github.py#L93-L96

Added lines #L93 - L96 were not covered by tests

# 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")
tab = "segment-overrides" if v.get("segment_name") is not None else "value"
environment_link_url = FEATURE_ENVIRONMENT_URL % (

Check warning on line 102 in api/integrations/github/github.py

View check run for this annotation

Codecov / codecov/patch

api/integrations/github/github.py#L101-L102

Added lines #L101 - L102 were not covered by tests
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
Comment on lines +102 to +105
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We should add a segment to the test or slap a # pragma: no cover on L108.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done.

table_row = FEATURE_TABLE_ROW % (

Check warning on line 113 in api/integrations/github/github.py

View check run for this annotation

Codecov / codecov/patch

api/integrations/github/github.py#L109-L113

Added lines #L109 - L113 were not covered by tests
v["environment_name"],
environment_link_url,
"✅ Enabled" if v["feature_value"] else "❌ Disabled",
feature_value if feature_value else "",
feature_value_type,
last_updated_string,
)
result += table_row

Check warning on line 121 in api/integrations/github/github.py

View check run for this annotation

Codecov / codecov/patch

api/integrations/github/github.py#L121

Added line #L121 was not covered by tests

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

result += last_updated_string
return result


Expand All @@ -109,8 +132,11 @@
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:
Expand All @@ -127,6 +153,9 @@
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["environment_api_key"] = (
feature_state.environment.api_key
)
if (
hasattr(feature_state, "feature_segment")
and feature_state.feature_segment is not None
Expand All @@ -147,4 +176,5 @@
else None
),
feature_states=feature_states_list if feature_states else None,
project_id=project_id,
)
6 changes: 5 additions & 1 deletion api/integrations/github/tasks.py
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
Original file line number Diff line number Diff line change
@@ -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

Expand Down Expand Up @@ -37,24 +35,28 @@ 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,
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()

mocker.patch(
"integrations.github.tasks.generate_body_comment",
return_value="Flag linked",
)

feature_external_resource_data = {
"type": "GITHUB_ISSUE",
Expand All @@ -76,9 +78,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": "Flag linked"},
headers={
"Accept": "application/vnd.github.v3+json",
"Authorization": "Bearer mocked_token",
Expand Down Expand Up @@ -320,9 +320,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:
Expand Down Expand Up @@ -412,4 +413,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,
)