fix: GitHub repos unique constraint and delete #4037

novakzaballa · 2024-05-28T16:27:22Z

Thanks for submitting a PR! Please check the boxes below:

I have run pre-commit to check linting
I have added information to docs/ if required so people know about the feature!
I have filled in the "Changes" section below?
I have filled in the "How did you test this code" section below?
I have used a Conventional Commit title for this Pull Request

Changes

Fix soft-deleted issues with GitHub repos/project uniqueness.
Fix when removing a GitHub repo from integration delete only the resources of that repo.
Improve unit test.

How did you test this code?

Run the GitHub view, and feature external resources unit tests

vercel · 2024-05-28T16:27:28Z

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name	Status	Preview	Comments	Updated (UTC)
docs	✅ Ready (Inspect)	Visit Preview	💬 Add feedback	May 28, 2024 8:04pm
flagsmith-frontend-preview	✅ Ready (Inspect)	Visit Preview	💬 Add feedback	May 28, 2024 8:04pm
flagsmith-frontend-staging	✅ Ready (Inspect)	Visit Preview	💬 Add feedback	May 28, 2024 8:04pm

github-actions · 2024-05-28T16:30:31Z

Uffizzi Preview deployment-52353 was deleted.

codecov · 2024-05-28T16:35:02Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 96.41%. Comparing base (50cc369) to head (525a7d8).
Report is 5 commits behind head on main.

Additional details and impacted files

@@           Coverage Diff           @@
##             main    #4037   +/-   ##
=======================================
  Coverage   96.41%   96.41%           
=======================================
  Files        1145     1146    +1     
  Lines       37295    37373   +78     
=======================================
+ Hits        35959    36035   +76     
- Misses       1336     1338    +2

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

zachaysan

Overall looks good, but left a number of nits. There is one change I think you should address which is the url__contains bit since it would bleed data.

zachaysan · 2024-05-28T17:43:23Z

api/conftest.py

+def post_request_mock(mocker: MockerFixture) -> MagicMock:
+    def mocked_request(*args, **kwargs):
+        class MockResponse:
+            def __init__(self, json_data, status_code):


Missing typing

zachaysan · 2024-05-28T17:43:48Z

api/conftest.py

@@ -85,6 +87,25 @@ def pytest_sessionstart(session: pytest.Session) -> None:
    fix_issue_3869()


+@pytest.fixture()
+def post_request_mock(mocker: MockerFixture) -> MagicMock:
+    def mocked_request(*args, **kwargs):


Missing typing

zachaysan · 2024-05-28T17:43:58Z

api/conftest.py

+            def raise_for_status(self) -> None:
+                pass
+
+            def json(self):


Missing typing

zachaysan · 2024-05-28T17:46:29Z

api/integrations/github/github.py

@@ -25,6 +26,21 @@
 logger = logging.getLogger(__name__)


+def handle_installation_deleted(payload: dict[str, typing.Any]) -> None:


nit: Any has already been imported from typing so this should be like the below

Suggested change

def handle_installation_deleted(payload: dict[str, typing.Any]) -> None:

def handle_installation_deleted(payload: dict[str, Any]) -> None:

zachaysan · 2024-05-28T17:48:19Z

api/integrations/github/github.py

+    installation_id = payload.get("installation", {}).get("id")
+    try:
+        GithubConfiguration.objects.get(installation_id=installation_id).delete()
+    except GithubConfiguration.DoesNotExist:


installation_id could be None which may match the queryset if there are installation_id matching records with None in them. I would add an explicit check for installation_id is None and handle that case separately.

zachaysan · 2024-05-28T17:48:40Z

api/integrations/github/github.py

+        GithubConfiguration.objects.get(installation_id=installation_id).delete()
+    except GithubConfiguration.DoesNotExist:
+        logger.error(
+            f"Github Configuration with installation_id {installation_id} does not exist"


nit:

Suggested change

f"Github Configuration with installation_id {installation_id} does not exist"

f"GitHub Configuration with installation_id {installation_id} does not exist"

zachaysan · 2024-05-28T17:51:44Z

api/integrations/github/models.py

            ],
+            # Filter by url containing the repository owner and name
+            url__contains=f"{self.repository_owner}/{self.repository_name}",


The match criteria isn't good enough. If a repository owner is named foo it would also match to examplefoo. you need to bound the region of the url__contains so that you're always matching the right object.

I'm using regex instead of contains

zachaysan · 2024-05-28T17:53:21Z

api/tests/unit/integrations/github/test_unit_github_views.py

@@ -639,6 +650,32 @@ def test_github_webhook_delete_installation(
    assert not GithubConfiguration.objects.filter(installation_id=1234567).exists()


+def test_github_webhook_with_non_existing_installation(
+    github_configuration: GithubConfiguration,


nit:

Suggested change

github_configuration: GithubConfiguration,

github_configuration: GitHubConfiguration,

I think this shouldn't be changed as it's the model name.

zachaysan · 2024-05-28T17:53:58Z

api/tests/unit/integrations/github/test_unit_github_views.py

+    mocker_logger = mocker.patch("integrations.github.github.logger")
+
+    # When
+    client = APIClient()


nit: use api_client fixture

zachaysan · 2024-05-28T17:54:44Z

api/tests/unit/integrations/github/test_unit_github_views.py

+
+    # Then
+    mocker_logger.error.assert_called_once_with(
+        "Github Configuration with installation_id 765432 does not exist"


nit:

Suggested change

"Github Configuration with installation_id 765432 does not exist"

"GitHub Configuration with installation_id 765432 does not exist"

fix: GitHub repos unique constraint and delete

262bd84

github-actions bot added the api Issue related to the REST API label May 28, 2024

vercel bot deployed to Preview – flagsmith-frontend-staging May 28, 2024 16:28 View deployment

novakzaballa requested a review from zachaysan May 28, 2024 16:43

zachaysan approved these changes May 28, 2024

View reviewed changes

Add types and Update tests

1e13553

vercel bot deployed to Preview – flagsmith-frontend-preview May 28, 2024 19:10 View deployment

vercel bot deployed to Preview – flagsmith-frontend-staging May 28, 2024 19:12 View deployment

vercel bot deployed to Preview – docs May 28, 2024 19:12 View deployment

Use regex instead contains

525a7d8

vercel bot deployed to Preview – flagsmith-frontend-staging May 28, 2024 20:02 View deployment

vercel bot deployed to Preview – docs May 28, 2024 20:02 View deployment

vercel bot deployed to Preview – flagsmith-frontend-preview May 28, 2024 20:04 View deployment

novakzaballa added this pull request to the merge queue May 28, 2024

Merged via the queue into main with commit 7454e4a May 28, 2024
24 checks passed

novakzaballa deleted the fix/github-repos-unique-constraint-and-delete branch May 28, 2024 21:15

dabeeeenster mentioned this pull request May 28, 2024

chore(main): release 2.117.0 #4012

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix: GitHub repos unique constraint and delete #4037

fix: GitHub repos unique constraint and delete #4037

novakzaballa commented May 28, 2024

vercel bot commented May 28, 2024 •

edited

Loading

github-actions bot commented May 28, 2024 •

edited

Loading

codecov bot commented May 28, 2024 •

edited

Loading

zachaysan left a comment

zachaysan May 28, 2024

novakzaballa May 28, 2024

zachaysan May 28, 2024

novakzaballa May 28, 2024

zachaysan May 28, 2024

novakzaballa May 28, 2024

zachaysan May 28, 2024

novakzaballa May 28, 2024

zachaysan May 28, 2024

novakzaballa May 28, 2024

zachaysan May 28, 2024

novakzaballa May 28, 2024

zachaysan May 28, 2024

novakzaballa May 28, 2024

zachaysan May 28, 2024

novakzaballa May 28, 2024

zachaysan May 28, 2024

novakzaballa May 28, 2024

zachaysan May 28, 2024

novakzaballa May 28, 2024

		@@ -25,6 +26,21 @@
		logger = logging.getLogger(__name__)


		def handle_installation_deleted(payload: dict[str, typing.Any]) -> None:

	f"Github Configuration with installation_id {installation_id} does not exist"
	f"GitHub Configuration with installation_id {installation_id} does not exist"

	github_configuration: GithubConfiguration,
	github_configuration: GitHubConfiguration,

	"Github Configuration with installation_id 765432 does not exist"
	"GitHub Configuration with installation_id 765432 does not exist"

fix: GitHub repos unique constraint and delete #4037

fix: GitHub repos unique constraint and delete #4037

Conversation

novakzaballa commented May 28, 2024

Changes

How did you test this code?

vercel bot commented May 28, 2024 • edited Loading

github-actions bot commented May 28, 2024 • edited Loading

codecov bot commented May 28, 2024 • edited Loading

Codecov Report

zachaysan left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vercel bot commented May 28, 2024 •

edited

Loading

github-actions bot commented May 28, 2024 •

edited

Loading

codecov bot commented May 28, 2024 •

edited

Loading