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

chore(github): add open PR comments toggle and org option #54706

Closed
wants to merge 2 commits into from

Conversation

mifu67
Copy link
Contributor

@mifu67 mifu67 commented Aug 14, 2023

Add a toggle for open PR comments.

  • Org is not under feature flag: toggle is disabled
Screenshot 2023-08-14 at 10 58 44 AM - Org is under feature flag but doesn't have integration installed: toggle is disabled Screenshot 2023-08-14 at 10 58 16 AM - Org is under feature flag and has integration installed: toggle is enabled Toggle enabled

@mifu67 mifu67 requested review from cathteng and a team August 14, 2023 18:43
@github-actions github-actions bot added Scope: Frontend Automatically applied to PRs that change frontend components Scope: Backend Automatically applied to PRs that change backend components labels Aug 14, 2023
@github-actions
Copy link
Contributor

🚨 Warning: This pull request contains Frontend and Backend changes!

It's discouraged to make changes to Sentry's Frontend and Backend in a single pull request. The Frontend and Backend are not atomically deployed. If the changes are interdependent of each other, they must be separated into two pull requests and be made forward or backwards compatible, such that the Backend or Frontend can be safely deployed independently.

Have questions? Please ask in the #discuss-dev-infra channel.

@mifu67 mifu67 added the Do Not Merge Don't merge label Aug 14, 2023
Copy link
Member

@cathteng cathteng left a comment

Choose a reason for hiding this comment

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

looking good! i think you should separate the FE and BE though

Comment on lines +362 to +364
disabled:
!hasIntegration ||
!organization.features.includes('integrations-open-pr-comment'),
Copy link
Member

Choose a reason for hiding this comment

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

instead of checking for the feature here, you can add another field to this blob like this

visible: ({features}) => features.has('integrations-open-pr-comment')

this hides the toggle entirely if the org doesn't have the feature flag

then you can pass features into the JsonForm

<JsonForm features={organization.features} forms={forms} />

@@ -137,6 +137,12 @@
bool,
org_serializers.GITHUB_PR_BOT_DEFAULT,
),
(
"githubOpenPRComments",
Copy link
Member

Choose a reason for hiding this comment

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

hmm i feel like this name is pretty lengthy. maybe githubOpenPRBot would be better?

Comment on lines +334 to +339
let openPRDisabledReason = t(
'You must have a GitHub integration to enable this feature.'
);
if (!organization.features.includes('integrations-open-pr-comment')) {
openPRDisabledReason = t("This feature isn't available to you yet.");
}
Copy link
Member

Choose a reason for hiding this comment

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

if you hide the toggle when the org doesn't have the feature flag you won't need this extra reason

@codecov
Copy link

codecov bot commented Aug 14, 2023

Codecov Report

Merging #54706 (5a7aa98) into master (163b15f) will increase coverage by 0.01%.
Report is 19 commits behind head on master.
The diff coverage is 100.00%.

Additional details and impacted files
@@            Coverage Diff             @@
##           master   #54706      +/-   ##
==========================================
+ Coverage   79.77%   79.78%   +0.01%     
==========================================
  Files        5001     5000       -1     
  Lines      212264   212247      -17     
  Branches    36172    36159      -13     
==========================================
+ Hits       169327   169336       +9     
+ Misses      37721    37703      -18     
+ Partials     5216     5208       -8     
Files Changed Coverage Δ
static/app/views/alerts/rules/metric/types.tsx 100.00% <ø> (ø)
...ganizationIntegrations/integrationDetailedView.tsx 66.27% <ø> (ø)
src/sentry/api/endpoints/organization_details.py 86.21% <100.00%> (ø)
src/sentry/api/serializers/models/organization.py 95.14% <100.00%> (+0.83%) ⬆️

... and 48 files with indirect coverage changes

@mifu67 mifu67 closed this Aug 14, 2023
@github-actions github-actions bot locked and limited conversation to collaborators Aug 30, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Do Not Merge Don't merge Scope: Backend Automatically applied to PRs that change backend components Scope: Frontend Automatically applied to PRs that change frontend components
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants