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: Organisation can't have a new Github integration when had a prior one deleted #3874

Conversation

novakzaballa
Copy link
Contributor

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

  • Add Unique constraints in the GitHub Configuration model.
  • Change OneToOneField to ForeignKey in the GitHub Configuration model.
  • Add a validation so that an organization does not have more than one valid installation.
  • Solve an issue when updating the feature value

How did you test this code?

  • Unit tests added.

Copy link

vercel bot commented May 2, 2024

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 2, 2024 10:38pm
flagsmith-frontend-preview ✅ Ready (Inspect) Visit Preview 💬 Add feedback May 2, 2024 10:38pm
flagsmith-frontend-staging ✅ Ready (Inspect) Visit Preview 💬 Add feedback May 2, 2024 10:38pm

Copy link

sentry-io bot commented May 2, 2024

🔍 Existing Issues For Review

Your pull request is modifying functions with the following pre-existing issues:

📄 File: api/features/tasks.py

Function Unhandled Issue
trigger_feature_state_change_webhooks TypeError: 'GithubData' object is not subscriptable /api/v1/environments/{environment_api_key}/featurestates/{p...
Event Count: 1

Did you find this useful? React with a 👍 or 👎

@github-actions github-actions bot added the api Issue related to the REST API label May 2, 2024
Copy link
Contributor

github-actions bot commented May 2, 2024

Uffizzi Preview deployment-51155 was deleted.

@codecov-commenter
Copy link

codecov-commenter commented May 2, 2024

Codecov Report

Attention: Patch coverage is 97.26027% with 2 lines in your changes are missing coverage. Please review.

Project coverage is 95.87%. Comparing base (b4e058a) to head (1aee49f).
Report is 13 commits behind head on main.

Files Patch % Lines
api/features/tasks.py 33.33% 2 Missing ⚠️
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.
📢 Have feedback on the report? Share it here.

@novakzaballa novakzaballa requested review from a team and gagantrivedi May 2, 2024 13:57
Copy link
Contributor

@zachaysan zachaysan left a 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.

Comment on lines 55 to 63
@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:
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
@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:

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.

Comment on lines 83 to 91
@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:
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
@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:

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.

Comment on lines 71 to 74
try:
return super().create(request, *args, **kwargs)
except IntegrityError:
raise DuplicateGitHubIntegration
Copy link
Contributor

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?

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

@novakzaballa novakzaballa changed the title fix: Solve problems with Github integration when it was deleted fix: Organisation can't have a new Github integration when had a prior one deleted May 2, 2024
@novakzaballa novakzaballa requested a review from zachaysan May 3, 2024 04:28
Copy link
Contributor

@zachaysan zachaysan left a comment

Choose a reason for hiding this comment

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

Looks good buddy :)

@novakzaballa novakzaballa added this pull request to the merge queue May 3, 2024
Merged via the queue into main with commit 53e728a May 3, 2024
22 checks passed
@novakzaballa novakzaballa deleted the fix/solve-problem-with-github-integration-when-it-was-deleted branch May 3, 2024 13:45
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
api Issue related to the REST API
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants