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 0.17-allow-new-players #242

Open
wants to merge 22 commits into
base: master
from

Conversation

Projects
None yet
3 participants
@tnelling
Copy link
Collaborator

tnelling commented Feb 15, 2019

@jeffkaufman's testing made me realize we probably need this.

Prevents fake users from joining.
Addresses an issue (see #243) that caused any new user PR to be rejected on the PR itself.
Adds a set of tests to validate the allow-new-players rule.

tnelling added some commits Feb 15, 2019

@tnelling tnelling added the reviewme label Feb 15, 2019

@tnelling

This comment has been minimized.

Copy link
Collaborator Author

tnelling commented Feb 15, 2019

I have tested that a fake user will return 404 from GitHub's API.

Better check for blocking existing players.
Also re-order to put cheaper checks first.
@tnelling

This comment has been minimized.

Copy link
Collaborator Author

tnelling commented Feb 15, 2019

I've also added a fix here per the discussion in #243. I think it makes sense to do it here so that we close the loophole allowing fake GitHub users before we open the gate we intended to for real users.

tnelling added some commits Feb 15, 2019

@jeffkaufman

This comment has been minimized.

Copy link
Owner

jeffkaufman commented Feb 15, 2019

I've also added a fix here per the discussion in #243.

Can you add that to the PR description?

tnelling added some commits Feb 15, 2019

@jeffkaufman

This comment has been minimized.

Copy link
Owner

jeffkaufman commented Feb 15, 2019

I made #244 for testing this

@jeffkaufman jeffkaufman referenced this pull request Feb 15, 2019

Closed

ban sockpuppets? #245

tnelling added some commits Feb 15, 2019

@tnelling tnelling added do-not-merge and removed reviewme labels Mar 5, 2019

@tnelling

This comment has been minimized.

Copy link
Collaborator Author

tnelling commented Mar 5, 2019

Relabelling until I have a chance to actually run these unit tests.

tnelling added some commits Mar 5, 2019

@tnelling tnelling referenced this pull request Mar 10, 2019

Closed

join game #255


if points_name != 'initial':
raise Exception('New player bonus value must be called "initial"')
raise Exception('New player bonus value is called "%s" instead of "initial"' % points_name)

This comment has been minimized.

@dnathe4th

dnathe4th Mar 11, 2019

Collaborator

What I'm not clear on is how we know we're looking at a PR for a new user at this point? If an existing user tries to add a new, non-'initial' bonus file won't this exception still throw?

This comment has been minimized.

@tnelling

tnelling Mar 11, 2019

Author Collaborator

Yes. And that's fine, because this is an allow rule. If it throws, we just move to the next rule. The general pattern for the allow rules is that we assume the PR in question is trying to pass that rule, and throw exceptions with wording to match.

This comment has been minimized.

@dnathe4th

dnathe4th Mar 11, 2019

Collaborator

Doesn't a throw still look like a failure in CI?

This comment has been minimized.

@tnelling

tnelling Mar 11, 2019

Author Collaborator

validate.py traps the exceptions and prints them.

@tnelling tnelling changed the title Don't allow fake users Fix allowing players to join without approval Mar 11, 2019

@tnelling tnelling changed the title Fix allowing players to join without approval Fix 0.17-allow-new-players Mar 11, 2019

tnelling added some commits Mar 11, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.