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

Add trigger for user membership in a team #374

Open
wants to merge 5 commits into
base: master
Choose a base branch
from
Open

Conversation

gagoar
Copy link
Owner

@gagoar gagoar commented Feb 17, 2021

Issue Reference: fixes #346

Implements feature request #346

Motivation and Context

Useful to create custom rules like adding a label when a member is part of a team, among other things, to trigger a workflow.

it is a really useful filter when adding ppl to a review or commenting, for instance, checking if the person is on a team, might reduce adding a comment to that PR. etc.

How Has This Been Tested?

unit tests.

Checklist:

  • My code follows the code style of this project.
  • My change requires a change to the documentation.
  • I have updated the documentation accordingly.

@gagoar gagoar changed the title Gg/feature 346 Add trigger for user membership in a team Feb 17, 2021
@gagoar gagoar requested a review from cyamonide February 17, 2021 22:59
@cyamonide cyamonide mentioned this pull request Feb 19, 2021
cyamonide added a commit that referenced this pull request Feb 19, 2021
Merging into a non-master branch, so omitting most of the PR template. #374 has the relevant context.

This PR fixes a bad import and fixes some logic in the isMemberOf matcher.

Also adds 404s as an accepted response.
__tests__/index.spec.ts Outdated Show resolved Hide resolved
@@ -65,7 +65,7 @@ describe('rules', () => {
};
const rules = new Rules(rule);

expect(await rules.getMatchingRules(files, event)).toMatchInlineSnapshot('MatchingRules []');
expect(await rules.getMatchingRules(files, event, {} as OctokitClient)).toMatchInlineSnapshot('MatchingRules []');
Copy link
Owner Author

Choose a reason for hiding this comment

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

I should store the mocked object and then re-use it.

@codecov
Copy link

codecov bot commented Aug 19, 2021

Codecov Report

Merging #374 (bef9140) into master (832e036) will not change coverage.
The diff coverage is 100.00%.

@@            Coverage Diff            @@
##            master      #374   +/-   ##
=========================================
  Coverage   100.00%   100.00%           
=========================================
  Files            8         9    +1     
  Lines          344       373   +29     
  Branches        54        59    +5     
=========================================
+ Hits           344       373   +29     
Flag Coverage Δ
unittests 100.00% <100.00%> (ø)

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
src/index.ts 100.00% <100.00%> (ø)
src/isMemberOf.ts 100.00% <100.00%> (ø)
src/rules.ts 100.00% <100.00%> (ø)

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

cyamonide
cyamonide previously approved these changes Aug 24, 2021
@cyamonide cyamonide dismissed their stale review August 24, 2021 07:10

faulty test

Comment on lines +34 to +40
const results = await Promise.all(
membershipChecks.map((membership) => queue.add(() => client.teams.getMembershipForUserInOrg(membership)))
).catch(catchHandler(debug));

return (results as Record<string, unknown>[]).some((response: Record<string, unknown>) => {
return isGetMembershipForUserInOrg(response) && response.data.state == ACTIVE_STATE;
});
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think this bit of code is problematic, but you'll have to verify for me.

If I don't belong in one of the teams which we check for membership, then catchHandler will have to handle a 404 not found. This needs to be added to:

export enum AllowedHttpErrors {
UNPROCESSABLE_ENTITY = 422,
}

Furthermore, if any of the promises reject, I'm not sure that results is an array anymore, which is where I run into the error complaining that results.some is not a function. I think at that point, results is simply the value of the promise which rejected.

FYI even after I added 404 to the AllowedHttpErrors enum, I am still seeing the results.some is not a function error.

Copy link
Owner Author

Choose a reason for hiding this comment

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

mm, how is that not happening on the code?

src/isMemberOf.ts Outdated Show resolved Hide resolved
@sonarcloud
Copy link

sonarcloud bot commented Aug 30, 2022

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 0 Code Smells

No Coverage information No Coverage information
0.0% 0.0% Duplication

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Feature] Add trigger for user membership in a team.
2 participants