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.

Sign up for GitHub

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

Jump to bottom

fix: GitHub integration tagging issues #4586

Open
wants to merge 8 commits into
base: main
Choose a base branch
from
Open

fix: GitHub integration tagging issues #4586

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
7da0d51
fix: GitHub integration tagging issues
novakzaballa Sep 4, 2024
8f865b5
fix: Address the comments
novakzaballa Sep 5, 2024
d8ab013
Add unit test for missing feature
zachaysan Oct 1, 2024
af18420
Check for feature and return if not present
zachaysan Oct 1, 2024
50dfb9f
Merge branch 'main' into fix/github-integration-tagging-issues
zachaysan Oct 1, 2024
2aebeee
Fix unnecessary check for event type
zachaysan Oct 9, 2024
0c4325b
Merge branch 'main' into fix/github-integration-tagging-issues
zachaysan Oct 9, 2024
f92e23e
Add dequeued and fix any typing
zachaysan Oct 9, 2024
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
30 changes: 22 additions & 8 deletions api/integrations/github/github.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,15 +14,17 @@
FEATURE_ENVIRONMENT_URL,
FEATURE_TABLE_HEADER,
FEATURE_TABLE_ROW,
GITHUB_TAG_COLOR,
LINK_FEATURE_TITLE,
LINK_SEGMENT_TITLE,
UNLINKED_FEATURE_TEXT,
UPDATED_FEATURE_TEXT,
GitHubEventType,
GitHubTag,
github_tag_description,
)
from integrations.github.dataclasses import GithubData
from integrations.github.models import GithubConfiguration
from integrations.github.models import GithubConfiguration, GitHubRepository
from integrations.github.tasks import call_github_app_webhook_for_feature_state
from projects.tags.models import Tag, TagType

Expand All @@ -46,7 +48,7 @@


def tag_feature_per_github_event(
event_type: str, action: str, metadata: dict[str, Any]
event_type: str, action: str, metadata: dict[str, Any], repo_full_name: str
) -> None:

# Get Feature with external resource of type GITHUB and url matching the resource URL
Expand All @@ -55,18 +57,28 @@ def tag_feature_per_github_event(
| Q(external_resources__type="GITHUB_ISSUE"),
external_resources__url=metadata.get("html_url"),
).first()
repo: list[str] = repo_full_name.split("/")
matthewelwell marked this conversation as resolved.
Show resolved Hide resolved
tagging_enabled = (
GitHubRepository.objects.filter(
project=feature.project, repository_owner=repo[0], repository_name=repo[1]
)
.first()
.tagging_enabled
)
zachaysan marked this conversation as resolved.
Show resolved Hide resolved

if feature:
if feature and tagging_enabled:
if (
event_type == "pull_request"
and action == "closed"
and metadata.get("merged")
):
action = "merged"
# Get corresponding project Tag to tag the feature
github_tag = Tag.objects.get(
# Get or create the corresponding project Tag to tag the feature
github_tag, _ = Tag.objects.get_or_create(
color=GITHUB_TAG_COLOR,
description=github_tag_description[tag_by_event_type[event_type][action]],
label=tag_by_event_type[event_type][action],
project=feature.project_id,
project=feature.project,
is_system_tag=True,
type=TagType.GITHUB.value,
)
Expand Down Expand Up @@ -100,8 +112,10 @@ def handle_github_webhook_event(event_type: str, payload: dict[str, Any]) -> Non
handle_installation_deleted(payload)
elif event_type in tag_by_event_type:
Copy link
Contributor

Choose a reason for hiding this comment

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

This is pointless, if event_type isn't in tag_by_event_type then we'll get a KeyError here. I assume based on that, there is no way for event_type to not exist in tag_by_event_type but we should check on this.

If that is the case, I think this can just be changed to (and we can remove one layer of nesting):

elif action in tag_by_event_type[event_type]

Copy link
Contributor

Choose a reason for hiding this comment

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

I checked and it was already filtered in the view code, so the check here is unnecessary. I've updated the relevant code.

action = str(payload.get("action"))
metadata = payload.get("issue", {}) or payload.get("pull_request", {})
tag_feature_per_github_event(event_type, action, metadata)
if action in tag_by_event_type[event_type]:
repo_full_name = payload.get("repository", {}).get("full_name")
metadata = payload.get("issue", {}) or payload.get("pull_request", {})
tag_feature_per_github_event(event_type, action, metadata, repo_full_name)
zachaysan marked this conversation as resolved.
Show resolved Hide resolved


def generate_body_comment(
Expand Down
2 changes: 2 additions & 0 deletions api/integrations/github/models.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@
from django.db import models
from django_lifecycle import (
AFTER_CREATE,
AFTER_UPDATE,
BEFORE_DELETE,
LifecycleModelMixin,
hook,
Expand Down Expand Up @@ -93,6 +94,7 @@ def delete_feature_external_resources(
).delete()

@hook(AFTER_CREATE)
@hook(AFTER_UPDATE, when="tagging_enabled", has_changed=True, was=False)
def create_github_tags(
self,
) -> None:
Expand Down
5 changes: 4 additions & 1 deletion api/tests/unit/integrations/github/test_unit_github_views.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -38,6 +38,9 @@
"html_url": "https://github.com/repositoryownertest/repositorynametest/issues/11",
"merged": True,
},
"repository": {
"full_name": "repositoryownertest/repositorynametest",
},
"action": "closed",
}
)
Expand All @@ -49,7 +52,7 @@
WEBHOOK_SIGNATURE_WITHOUT_INSTALLATION_ID = (
"sha1=f99796bd3cebb902864e87ed960c5cca8772ff67"
)
WEBHOOK_MERGED_ACTION_SIGNATURE = "sha1=712ec7a5db14aad99d900da40738ebb9508ecad2"
WEBHOOK_MERGED_ACTION_SIGNATURE = "sha1=f3f7e1e9b43448d570451317447d3b4f8f8142de"
WEBHOOK_SECRET = "secret-key"


Expand Down
Loading