-
Notifications
You must be signed in to change notification settings - Fork 405
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: Organisation can't have a new Github integration when had a prior one deleted #3874
fix: Organisation can't have a new Github integration when had a prior one deleted #3874
Conversation
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
🔍 Existing Issues For ReviewYour pull request is modifying functions with the following pre-existing issues: 📄 File: api/features/tasks.py
Did you find this useful? React with a 👍 or 👎 |
Uffizzi Preview |
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #3874 +/- ##
=======================================
Coverage 95.87% 95.87%
=======================================
Files 1129 1133 +4
Lines 35787 35858 +71
=======================================
+ Hits 34309 34378 +69
- Misses 1478 1480 +2 ☔ View full report in Codecov by Sentry. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Overall looks pretty good, just have a couple questions.
@pytest.mark.parametrize( | ||
"client", | ||
[lazy_fixture("admin_master_api_key_client"), lazy_fixture("admin_client")], | ||
) | ||
def test_cannot_create_github_configuration_due_to_unique_constraint( | ||
client: APIClient, | ||
organisation: Organisation, | ||
github_configuration: GithubConfiguration, | ||
) -> None: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@pytest.mark.parametrize( | |
"client", | |
[lazy_fixture("admin_master_api_key_client"), lazy_fixture("admin_client")], | |
) | |
def test_cannot_create_github_configuration_due_to_unique_constraint( | |
client: APIClient, | |
organisation: Organisation, | |
github_configuration: GithubConfiguration, | |
) -> None: | |
def test_cannot_create_github_configuration_due_to_unique_constraint( | |
admin_client_new: APIClient, | |
organisation: Organisation, | |
github_configuration: GithubConfiguration, | |
) -> None: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done.
@pytest.mark.parametrize( | ||
"client", | ||
[lazy_fixture("admin_master_api_key_client"), lazy_fixture("admin_client")], | ||
) | ||
def test_cannot_create_github_configuration_when_the_organization_already_has_an_integration( | ||
client: APIClient, | ||
organisation: Organisation, | ||
github_configuration: GithubConfiguration, | ||
) -> None: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@pytest.mark.parametrize( | |
"client", | |
[lazy_fixture("admin_master_api_key_client"), lazy_fixture("admin_client")], | |
) | |
def test_cannot_create_github_configuration_when_the_organization_already_has_an_integration( | |
client: APIClient, | |
organisation: Organisation, | |
github_configuration: GithubConfiguration, | |
) -> None: | |
def test_cannot_create_github_configuration_when_the_organization_already_has_an_integration( | |
admin_client_new: APIClient, | |
organisation: Organisation, | |
github_configuration: GithubConfiguration, | |
) -> None: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done.
api/integrations/github/views.py
Outdated
try: | ||
return super().create(request, *args, **kwargs) | ||
except IntegrityError: | ||
raise DuplicateGitHubIntegration |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is there a way of looking at the IntegrityError
message in order to raise DuplicateGitHubIntegration
only when the preconditions have been met?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good buddy :)
Thanks for submitting a PR! Please check the boxes below:
pre-commit
to check lintingdocs/
if required so people know about the feature!Changes
How did you test this code?