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

Check HEAD branch organization user #410

Merged
merged 7 commits into from Mar 4, 2019

Conversation

@tim-mc
Copy link
Member

tim-mc commented Feb 5, 2019

This PR adds functionality that was previously discussed with @KharitonOff.

Changes

  • Add optional feature flag: organization_override_enabled
  • With feature flag true, the PR webhook flow will now check to see whether the "Organization" user that is tied to the HEAD repo's branch is whitelisted (the whitelist being an existing functionality)

N.B.: The checkPullRequestSignatures method has been updated to use Promises/(async/await). This was necessary to avoid a rather messy change that would otherwise be required if the mix of Promise chains and callbacks were continued to be used. Without this, the code would've required some nasty nested Promise chains and/or duplicate calls to the getPR method. While I don't really like having to do so much refactoring to add this bit of functionality, I couldn't find a good way to make it work otherwise (and without breaking a bunch of tests). If these changes are not acceptable, I can try to find a different way, but the outcome may be worse overall.

@CLAassistant

This comment has been minimized.

Copy link
Member

CLAassistant commented Feb 5, 2019

CLA assistant check
All committers have signed the CLA.

@coveralls

This comment has been minimized.

Copy link

coveralls commented Feb 5, 2019

Coverage Status

Coverage remained the same at 90.013% when pulling 456a7ef on bloomberg:org-check into 6e93fe3 on cla-assistant:master.

@@ -244,9 +244,8 @@ module.exports = function () {
return deferred.promise;
};

let getLastSignatureOnMultiDates = function (user, userId, repoId, orgId, sharedGist, gist_url, gist_version, date) {
let getLastSignatureOnMultiDates = function (user, userId, repoId, orgId, sharedGist = false, gist_url, gist_version, date) {

This comment has been minimized.

Copy link
@KharitonOff

KharitonOff Feb 8, 2019

Member

Perfect!

src/server/services/cla.js Outdated Show resolved Hide resolved
done(er);
});

signees = _.uniqWith([...signees, ...committers], (object, other) => object.id == other.id);

This comment has been minimized.

Copy link
@KharitonOff

KharitonOff Feb 8, 2019

Member

496-504
this looks great! much easier to read!

signee && !(item.isUserWhitelisted !== undefined && item.isUserWhitelisted(signee.name))
);

return checkAll(

This comment has been minimized.

Copy link
@KharitonOff
@tim-mc

This comment has been minimized.

Copy link
Member Author

tim-mc commented Feb 8, 2019

Added the suggested change! Thanks @KharitonOff!

src/tests/server/api/cla.js Outdated Show resolved Hide resolved
Copy link
Member

KharitonOff left a comment

Great work! Thank you very much!
You are very welcome to do any further refactoring in case you'd like to ;)

@KharitonOff

This comment has been minimized.

Copy link
Member

KharitonOff commented Feb 8, 2019

It would be great to have an appropriate description on the 'white list' field.

screen shot 2019-02-08 at 18 19 56

Maybe we can have an explanation popup like this one

screen shot 2019-02-08 at 18 25 32

@tim-mc

This comment has been minimized.

Copy link
Member Author

tim-mc commented Feb 8, 2019

It would be great to have an appropriate description on the 'white list' field.

Agreed that would be useful. Is that something that you think should be included in this PR? The whitelist functionality itself wasn't something that was introduced as a part of my changes, so I didn't add anything related in the UI. If you think it's necessary, I can look into adding some messaging.

@KharitonOff

This comment has been minimized.

Copy link
Member

KharitonOff commented Feb 11, 2019

@tim-mc I think it is a great feature a lot of current and future users would benefit from! In order to make them aware of how it works, it would be great to have a good description. A lot of project owners could 'white list' their own organization (this would provide a nice solution for the issues #197 #309 #239 #61). I would see the UI changes as a part of this PR, if this is ok for you.

@BenLloydPearson

This comment has been minimized.

Copy link

BenLloydPearson commented Feb 12, 2019

@tim-mc, thanks for your work on this! It appears to solve a problem I have as well.

If I understand correctly, this feature allows you to add an organization to the existing whitelist field to make all members of that org whitelisted. Is this interpretation correct?

If so, how does CLA-Assistant access this membership information? Can it only do this for organizations the app has been installed to, or does it require org membership to be displayed publicly at the user account level?

@tim-mc

This comment has been minimized.

Copy link
Member Author

tim-mc commented Feb 12, 2019

@BenLloydPearson The way this works is that if a PR has been opened from an organization's fork (the HEAD is the organization's branch), and that organization itself has been whitelisted, the CLA is considered signed. No other checks are executed (i.e., there is no lookup of the organization's membership).

@tim-mc tim-mc force-pushed the bloomberg:org-check branch from b710d77 to 0f75364 Feb 20, 2019
@tim-mc

This comment has been minimized.

Copy link
Member Author

tim-mc commented Feb 20, 2019

@KharitonOff I've added some messaging in the UI regarding whitelisting that is similar to the example to which you pointed. Let me know if there are any changes you would like for me to make to that messaging.

@tim-mc

This comment has been minimized.

Copy link
Member Author

tim-mc commented Feb 25, 2019

@KharitonOff bump :)

Let me know if the UI changes are sufficient for the PR. Thanks!

@KharitonOff

This comment has been minimized.

Copy link
Member

KharitonOff commented Feb 26, 2019

@tim-mc sorry for late response. I haven't enough time to review your last changes and to make a proposal for the description. I will do my best to do it this week.

@KharitonOff

This comment has been minimized.

Copy link
Member

KharitonOff commented Mar 1, 2019

@tim-mc here is my proposal. Feel free to change the phrasing or adapt the text...

<div class="free-space">
        <h4>How does whitelisting work?</h4>
        <div>
            If a GitHub username is included in the whitelist, they will not be required to sign a CLA. This also applies to organization usernames.
        </div>
        <h4>Why whitelist usernames?</h4>
        <div>
            Since there's no way for bot users (such as Dependabot or Greenkeeper) to sign a CLA, you may want to whitelist them. You can do so by adding their names (in this case <i>dependabot[bot]</i> and <i>greenkeeper[bot]</i> separated by a comma) to the whitelist. You can also use wildcard symbol in case you want to whitlist all bot users <i>*[bot]</i>.
        </div>
        <h4>Why whitelist organizations?</h4>
        <div>
            We see at least two use cases you may want to do so:
            <ul>
                <li>
                    You develop in a team and commit your changes via feature branches. As soon you create a new pull request CLA assistant would ask you and your team fellows to sign a CLA even if you are part of the organization and doesn't need to do so. Now if you whitelist your own organization name all pull requests comming from the same repository will pass the CLA check.
                </li>
                <li>
                    You get contributions from another company which has already signed your corporate CLA. You may want to let CLA assistant accept all PRs comming from this company. Now you can whitelist the GitHub organization name of this company and all pull requests comming from this GitHub organization will pass the check.
                </li>
            </ul>
        </div>
    </div>
@tim-mc

This comment has been minimized.

Copy link
Member Author

tim-mc commented Mar 1, 2019

@KharitonOff Updated with your suggestion. I tweaked the second "Why whitelist organizations?" use case slightly just to be more specific about the functionality.

@KharitonOff KharitonOff merged commit 49fef1e into cla-assistant:master Mar 4, 2019
5 checks passed
5 checks passed
DeepScan 0 new and 0 fixed issues
Details
WIP Ready for review
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
coverage/coveralls Coverage remained the same at 90.013%
Details
license/cla Contributor License Agreement is signed.
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
5 participants
You can’t perform that action at this time.