Skip to content

Fix 0.17-allow-new-players #242

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.

Sign up for GitHub

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

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

Fix 0.17-allow-new-players #242

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
22 commits
Select commit Hold shift + click to select a range
c95d88d
Don't allow fake users
tnelling Feb 15, 2019
fd4aae9
No real reason to make the url its own method
tnelling Feb 15, 2019
b2e6d7c
Better check for blocking existing players.
tnelling Feb 15, 2019
9c44048
Oops
tnelling Feb 15, 2019
1e17c74
Missing import
tnelling Feb 15, 2019
32211a4
Validate that a new user created their own PR
tnelling Feb 15, 2019
6ef4d81
Turns out there's an author already
tnelling Feb 15, 2019
46ffd59
But it's a method
tnelling Feb 15, 2019
e5d86ec
Merge branch 'master' into no-fake-users
tnelling Feb 15, 2019
cb4af78
Merge branch 'master' into no-fake-users
tnelling Feb 21, 2019
f586275
Merge branch 'master' into no-fake-users
tnelling Feb 27, 2019
dc40949
Merge branch 'master' into no-fake-users
tnelling Mar 4, 2019
3412590
Add some unit tests. Could use more.
tnelling Mar 5, 2019
1835070
More unit tests
tnelling Mar 5, 2019
91d6fc2
Quotes around incorrect file name
tnelling Mar 5, 2019
e33014c
Placeholder for valid test case
tnelling Mar 5, 2019
6236973
255 enabled a test
tnelling Mar 10, 2019
6522b3d
More test cases, thanks Dom!
tnelling Mar 11, 2019
b199481
More tests
tnelling Mar 11, 2019
57bfdee
Merge branch 'master' into no-fake-users
tnelling Mar 12, 2019
03f2c4d
Merge branch 'master' into no-fake-users
tnelling Mar 19, 2019
9d4a79a
Merge branch 'master' into no-fake-users
tnelling Mar 19, 2019
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
14 changes: 11 additions & 3 deletions rules/0.17-allow-new-players.py
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,3 +1,4 @@
import os
import util

# This value set to 0 until we figure out a good way to prevent dummy account abuse
Expand All @@ -14,11 +15,12 @@ def should_allow(pr):

(points_user, points_name, points_change) = bonuses[0]

if points_user in util.users():
raise Exception('Cannot create an existing user')
if pr.author() != points_user:
raise Exception('New players should submit their own PRs, but %s submitted the PR to add %s' %
(pr.author(), points_user))

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)
Copy link
Copy Markdown
Collaborator

@dnathe4th dnathe4th Mar 11, 2019

Choose a reason for hiding this comment

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

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?

Copy link
Copy Markdown
Collaborator Author

@tnelling tnelling Mar 11, 2019

Choose a reason for hiding this comment

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

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.

Copy link
Copy Markdown
Collaborator

@dnathe4th dnathe4th Mar 11, 2019

Choose a reason for hiding this comment

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

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

Copy link
Copy Markdown
Collaborator Author

@tnelling tnelling Mar 11, 2019

Choose a reason for hiding this comment

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

validate.py traps the exceptions and prints them.


if points_change < 0:
raise Exception('Points cannot be negative')
Expand All @@ -27,4 +29,10 @@ def should_allow(pr):
raise Exception('%s initial points exceeds maximum starting value of %s points' %
(points_change, max_start_bonus))

bonus_directory = os.path.join('players', points_user, 'bonuses')
if os.path.isdir(bonus_directory):
for bonus in os.listdir(bonus_directory):
if bonus != 'initial':
raise Exception('%s already has bonuses' % points_user)

return True
13 changes: 13 additions & 0 deletions rules/0.25-block-test-failures.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,19 @@
import runpy

SIMPLE_TESTS = {
'0.17-allow-new-players.py': [
('247', 'All file changes must be additions'),
('250', 'not enough values to unpack (expected 4, got 2)'),
('260', 'Added file rules/tnelling/bonuses/initial is not a bonus file'),
('261', 'Added file players/tnelling/foo/initial is not a bonus file'),
('253', 'File should contain a single integer.'),
('254', 'Only one new player can be added in a PR'),
('236', 'New players should submit their own PRs, but jeffkaufman submitted the PR to add sockpupper1'),
('220', 'New player bonus value is called "lose-point" instead of "initial"'),
('255', 'Points cannot be negative'),
('225', '10 initial points exceeds maximum starting value of 0 points'),
('258', None),
],
'0.3-allow-points-transfer.py': [
('33', 'All file changes must be additions'),
('82', 'Points transfer PRs should not add users: got dchudz'),
Expand Down