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

fix: Solve API GitHub integration issues #4502

Merged
merged 5 commits into from
Aug 19, 2024
Merged

fix: Solve API GitHub integration issues #4502

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
aa91e1c
fix: solve API GitHub integration issues
novakzaballa Aug 14, 2024
5fb7d30
Solve unit test
novakzaballa Aug 15, 2024
e4acf8b
Merge branch 'main' into fix/solve-api-github-integration-issues
novakzaballa Aug 15, 2024
1357116
Code cov
novakzaballa Aug 15, 2024
e57a7c4
Rename GithubRepository to GitHubRepository
novakzaballa Aug 19, 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
6 changes: 3 additions & 3 deletions api/conftest.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -40,7 +40,7 @@
from features.value_types import STRING
from features.versioning.tasks import enable_v2_versioning
from features.workflows.core.models import ChangeRequest
from integrations.github.models import GithubConfiguration, GithubRepository
from integrations.github.models import GithubConfiguration, GitHubRepository
from metadata.models import (
Metadata,
MetadataField,
Expand Down Expand Up @@ -1079,8 +1079,8 @@ def github_configuration(organisation: Organisation) -> GithubConfiguration:
def github_repository(
github_configuration: GithubConfiguration,
project: Project,
) -> GithubRepository:
return GithubRepository.objects.create(
) -> GitHubRepository:
return GitHubRepository.objects.create(
github_configuration=github_configuration,
repository_owner="repositoryownertest",
repository_name="repositorynametest",
Expand Down
15 changes: 13 additions & 2 deletions api/features/feature_external_resources/models.py
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,6 @@
import json
import logging
import re

from django.db import models
from django.db.models import Q
Expand All @@ -14,7 +15,7 @@
from features.models import Feature, FeatureState
from integrations.github.constants import GitHubEventType, GitHubTag
from integrations.github.github import call_github_task
from integrations.github.models import GithubRepository
from integrations.github.models import GitHubRepository
from organisations.models import Organisation
from projects.tags.models import Tag, TagType

Expand Down Expand Up @@ -79,9 +80,19 @@ def execute_after_save_actions(self):
.get(id=self.feature.project.organisation_id)
.github_config.first()
):
github_repo = GithubRepository.objects.get(
if self.type == "GITHUB_PR":
pattern = r"github.com/([^/]+)/([^/]+)/pull/\d+$"
elif self.type == "GITHUB_ISSUE":
pattern = r"github.com/([^/]+)/([^/]+)/issues/\d+$"
Comment on lines +83 to +86
Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe to future proof this it could be helpful to add an else keyword and raise an exception.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I don't think it's necessary to implement this, because I am already validating it before creating the feature external resource here:

flagsmith/api/features/feature_external_resources/views.py

Line 79 in 1357116

return Response(


url_match = re.search(pattern, self.url)
owner, repo = url_match.groups()

github_repo = GitHubRepository.objects.get(
github_configuration=github_configuration.id,
project=self.feature.project,
repository_owner=owner,
repository_name=repo,
)
if github_repo.tagging_enabled:
github_tag = Tag.objects.get(
Expand Down
4 changes: 2 additions & 2 deletions api/features/feature_external_resources/views.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,7 +10,7 @@
get_github_issue_pr_title_and_state,
label_github_issue_pr,
)
from integrations.github.models import GithubRepository
from integrations.github.models import GitHubRepository
from organisations.models import Organisation

from .models import FeatureExternalResource
Expand Down Expand Up @@ -85,7 +85,7 @@ def create(self, request, *args, **kwargs):
url_match = re.search(pattern, url)
if url_match:
owner, repo, issue = url_match.groups()
if GithubRepository.objects.get(
if GitHubRepository.objects.get(
github_configuration=github_configuration,
repository_owner=owner,
repository_name=repo,
Expand Down
20 changes: 17 additions & 3 deletions api/integrations/github/client.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -155,9 +155,23 @@ def fetch_search_github_resource(
headers: dict[str, str] = build_request_headers(
github_configuration.installation_id
)
response = requests.get(url, headers=headers, timeout=GITHUB_API_CALLS_TIMEOUT)
response.raise_for_status()
json_response = response.json()
try:
response = requests.get(url, headers=headers, timeout=GITHUB_API_CALLS_TIMEOUT)
response.raise_for_status()
json_response = response.json()

except HTTPError:
response_content = response.content.decode("utf-8")
error_message = (
"The resources do not exist or you do not have permission to view them"
)
error_data = json.loads(response_content)
if error_data.get("message", "") == "Validation Failed" and any(
error.get("code", "") == "invalid" for error in error_data.get("errors", [])
):
logger.warning(error_message)
raise ValueError(error_message)

results = [
{
"html_url": i["html_url"],
Expand Down
2 changes: 1 addition & 1 deletion api/integrations/github/models.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -41,7 +41,7 @@ class Meta:
ordering = ("id",)


class GithubRepository(LifecycleModelMixin, SoftDeleteExportableModel):
class GitHubRepository(LifecycleModelMixin, SoftDeleteExportableModel):
github_configuration = models.ForeignKey(
GithubConfiguration, related_name="repository_config", on_delete=models.CASCADE
)
Expand Down
4 changes: 2 additions & 2 deletions api/integrations/github/serializers.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,7 @@
PaginatedQueryParams,
RepoQueryParams,
)
from integrations.github.models import GithubConfiguration, GithubRepository
from integrations.github.models import GithubConfiguration, GitHubRepository


class GithubConfigurationSerializer(ModelSerializer):
Expand All @@ -19,7 +19,7 @@ class Meta:

class GithubRepositorySerializer(ModelSerializer):
class Meta:
model = GithubRepository
model = GitHubRepository
optional_fields = ("search_text", "page")
fields = (
"id",
Expand Down
6 changes: 3 additions & 3 deletions api/integrations/github/views.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -27,7 +27,7 @@
tag_by_event_type,
)
from integrations.github.helpers import github_webhook_payload_is_valid
from integrations.github.models import GithubConfiguration, GithubRepository
from integrations.github.models import GithubConfiguration, GitHubRepository
from integrations.github.permissions import HasPermissionToGithubConfiguration
from integrations.github.serializers import (
GithubConfigurationSerializer,
Expand Down Expand Up @@ -126,7 +126,7 @@ class GithubRepositoryViewSet(viewsets.ModelViewSet):
GithubIsAdminOrganisation,
)
serializer_class = GithubRepositorySerializer
model_class = GithubRepository
model_class = GitHubRepository

def perform_create(self, serializer):
github_configuration_id = self.kwargs["github_pk"]
Expand All @@ -136,7 +136,7 @@ def get_queryset(self):
try:
if github_pk := self.kwargs.get("github_pk"):
int(github_pk)
return GithubRepository.objects.filter(github_configuration=github_pk)
return GitHubRepository.objects.filter(github_configuration=github_pk)
except ValueError:
raise ValidationError({"github_pk": ["Must be an integer"]})

Expand Down
36 changes: 18 additions & 18 deletions api/tests/unit/features/test_unit_feature_external_resources_views.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,7 +21,7 @@
)
from features.versioning.models import EnvironmentFeatureVersion
from integrations.github.constants import GITHUB_API_URL, GITHUB_API_VERSION
from integrations.github.models import GithubConfiguration, GithubRepository
from integrations.github.models import GithubConfiguration, GitHubRepository
from projects.models import Project
from segments.models import Segment
from tests.types import WithEnvironmentPermissionsCallable
Expand Down Expand Up @@ -73,7 +73,7 @@ def test_create_feature_external_resource(
environment: Environment,
project: Project,
github_configuration: GithubConfiguration,
github_repository: GithubRepository,
github_repository: GitHubRepository,
post_request_mock: MagicMock,
mock_github_client_generate_token: MagicMock,
) -> None:
Expand Down Expand Up @@ -186,7 +186,7 @@ def test_cannot_create_feature_external_resource_with_an_invalid_gh_url(
feature: Feature,
project: Project,
github_configuration: GithubConfiguration,
github_repository: GithubRepository,
github_repository: GitHubRepository,
) -> None:
# Given
feature_external_resource_data = {
Expand Down Expand Up @@ -216,7 +216,7 @@ def test_cannot_create_feature_external_resource_with_an_incorrect_gh_type(
feature: Feature,
project: Project,
github_configuration: GithubConfiguration,
github_repository: GithubRepository,
github_repository: GitHubRepository,
) -> None:
# Given
feature_external_resource_data = {
Expand Down Expand Up @@ -316,7 +316,7 @@ def test_cannot_create_feature_external_resource_due_to_unique_constraint(
feature: Feature,
project: Project,
github_configuration: GithubConfiguration,
github_repository: GithubRepository,
github_repository: GitHubRepository,
feature_external_resource: FeatureExternalResource,
) -> None:
# Given
Expand Down Expand Up @@ -346,7 +346,7 @@ def test_update_feature_external_resource(
feature_external_resource: FeatureExternalResource,
project: Project,
github_configuration: GithubConfiguration,
github_repository: GithubRepository,
github_repository: GitHubRepository,
post_request_mock: MagicMock,
mocker: MockerFixture,
) -> None:
Expand All @@ -357,7 +357,7 @@ def test_update_feature_external_resource(
mock_generate_token.return_value = "mocked_token"
feature_external_resource_data = {
"type": "GITHUB_ISSUE",
"url": "https://github.com/userexample/example-project-repo/issues/12",
"url": f"https://github.com/{github_repository.repository_owner}/{github_repository.repository_name}/issues/12",
"feature": feature.id,
"metadata": '{"state": "open"}',
}
Expand All @@ -378,7 +378,7 @@ def test_delete_feature_external_resource(
feature: Feature,
project: Project,
github_configuration: GithubConfiguration,
github_repository: GithubRepository,
github_repository: GitHubRepository,
feature_external_resource: FeatureExternalResource,
post_request_mock: MagicMock,
mocker: MockerFixture,
Expand Down Expand Up @@ -417,7 +417,7 @@ def test_get_feature_external_resources(
feature: Feature,
project: Project,
github_configuration: GithubConfiguration,
github_repository: GithubRepository,
github_repository: GitHubRepository,
feature_external_resource: FeatureExternalResource,
mock_github_client_generate_token: MagicMock,
) -> None:
Expand Down Expand Up @@ -446,7 +446,7 @@ def test_get_feature_external_resource(
feature: Feature,
project: Project,
github_configuration: GithubConfiguration,
github_repository: GithubRepository,
github_repository: GitHubRepository,
feature_external_resource: FeatureExternalResource,
) -> None:
# Given
Expand All @@ -472,7 +472,7 @@ def test_create_github_comment_on_feature_state_updated(
feature: Feature,
project: Project,
github_configuration: GithubConfiguration,
github_repository: GithubRepository,
github_repository: GitHubRepository,
post_request_mock: MagicMock,
mocker: MockerFixture,
environment: Environment,
Expand Down Expand Up @@ -532,7 +532,7 @@ def test_create_github_comment_on_feature_was_deleted(
feature: Feature,
project: Project,
github_configuration: GithubConfiguration,
github_repository: GithubRepository,
github_repository: GitHubRepository,
feature_external_resource: FeatureExternalResource,
post_request_mock: MagicMock,
mock_github_client_generate_token: MagicMock,
Expand Down Expand Up @@ -566,7 +566,7 @@ def test_create_github_comment_on_segment_override_updated(
segment_override_for_feature_with_value: FeatureState,
project: Project,
github_configuration: GithubConfiguration,
github_repository: GithubRepository,
github_repository: GitHubRepository,
post_request_mock: MagicMock,
environment: Environment,
admin_client: APIClient,
Expand Down Expand Up @@ -624,7 +624,7 @@ def test_create_github_comment_on_segment_override_deleted(
segment_override_for_feature_with_value: FeatureState,
feature_with_value_segment: FeatureSegment,
github_configuration: GithubConfiguration,
github_repository: GithubRepository,
github_repository: GitHubRepository,
post_request_mock: MagicMock,
admin_client_new: APIClient,
feature_with_value_external_resource: FeatureExternalResource,
Expand Down Expand Up @@ -665,7 +665,7 @@ def test_create_github_comment_using_v2(
environment: Environment,
project: Project,
github_configuration: GithubConfiguration,
github_repository: GithubRepository,
github_repository: GitHubRepository,
feature_external_resource: FeatureExternalResource,
post_request_mock: MagicMock,
mocker: MockerFixture,
Expand Down Expand Up @@ -738,7 +738,7 @@ def test_create_github_comment_using_v2_fails_on_wrong_params(
environment: Environment,
project: Project,
github_configuration: GithubConfiguration,
github_repository: GithubRepository,
github_repository: GitHubRepository,
feature_external_resource: FeatureExternalResource,
post_request_mock: MagicMock,
mocker: MockerFixture,
Expand Down Expand Up @@ -781,7 +781,7 @@ def test_create_feature_external_resource_on_environment_with_v2(
admin_client_new: APIClient,
project: Project,
github_configuration: GithubConfiguration,
github_repository: GithubRepository,
github_repository: GitHubRepository,
segment_override_for_feature_with_value: FeatureState,
environment_v2_versioning: Environment,
post_request_mock: MagicMock,
Expand Down Expand Up @@ -858,7 +858,7 @@ def test_cannot_create_feature_external_resource_for_the_same_feature_and_resour
feature: Feature,
project: Project,
github_configuration: GithubConfiguration,
github_repository: GithubRepository,
github_repository: GitHubRepository,
feature_external_resource_gh_pr: FeatureExternalResource,
) -> None:
# Given
Expand Down
Loading
Loading