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

[WIP] brainstorming for /vote fast + prototype [WILL NOT BE UN-WIP-ED] #493

Closed
wants to merge 20 commits into from
Closed
Show file tree
Hide file tree
Changes from 5 commits
Commits
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
12 changes: 11 additions & 1 deletion cron/poll_pull_requests.py
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,7 @@
import os
import sys
from os.path import join, abspath, dirname
from lib.db.models import MeritocracyMentioned
from lib.db.models import MeritocracyMentioned, Issue

import settings
import github_api as gh
Expand Down Expand Up @@ -73,6 +73,16 @@ def poll_pull_requests(api):
# the PR is mitigated or the threshold is not reached ?
if variance >= threshold or not is_approved:
voting_window = gh.voting.get_extended_voting_window(api, settings.URN)

# read the db to see if this PR is expedited by /vote fast...
try:
issue = Issue.get(issue_id=pr["id"])
if issue.expedited: # expedite
voting_window /= 2
Copy link
Contributor

Choose a reason for hiding this comment

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

This code needs to be moved out of the if statement for the extended voting window. Currently this just halves the extended voting window.


except Issue.DoesNotExist:
pass # not expedited
Copy link
Contributor

Choose a reason for hiding this comment

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

Also, add an except: log.exception("Failed to get expedited status")

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@PlasmaPower This is not necessarily a failure, though... It just means nobody has posted /vote fast on this PR. In fact, we expect that to be the common case, so these would flood the logs.

Copy link
Contributor

Choose a reason for hiding this comment

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

I'm not sure you understand. I'm saying keep the current error handler, but add another general one in case a different exception happens.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ah, my bad... fixed


if (settings.IN_PRODUCTION and vote_total >= threshold / 2 and
seconds_since_updated > base_voting_window and not meritocracy_satisfied):
# check if we need to mention the meritocracy
Expand Down
41 changes: 37 additions & 4 deletions cron/poll_read_issue_comments.py
Original file line number Diff line number Diff line change
Expand Up @@ -21,7 +21,7 @@

# If no subcommands, map cmd: None
COMMAND_LIST = {
"/vote": ("close", "reopen")
"/vote": ("close", "reopen", "fast")
}

__log = logging.getLogger("read_issue_comments")
Expand All @@ -38,9 +38,12 @@ def get_seconds_remaining(api, comment_id):
def insert_or_update(api, cmd_obj):
# Find the comment, or create it if it doesn't exit
comment_id = cmd_obj["global_comment_id"]
issue, _ = Issue.get_or_create(issue_id=cmd_obj["issue_id"])
user, _ = User.get_or_create(login=cmd_obj["user"]["login"],
user_id=cmd_obj["user"]["id"])
issue, _ = Issue.get_or_create(issue_id=cmd_obj["issue_id"],
user=user, # TODO: I don't think this is the right one
created_at=cmd_obj["created_at"], # TODO: I don't think this is the right one
expedited=False)

comment, _ = Comment.get_or_create(comment_id=comment_id,
user=user, text=cmd_obj["comment_text"],
Expand Down Expand Up @@ -131,7 +134,10 @@ def get_command_votes(api, urn, comment_id):
return votes


def handle_vote_command(api, command, issue_id, comment_id, votes):
def handle_vote_command(api, command, cmdmeta, votes):
issue_id = cmdmeta.issue.issue_id
comment_id = cmdmeta.comment.comment_id

orig_command = command[:]
# Check for correct command syntax, ie, subcommands
log_warning = False
Expand All @@ -143,6 +149,33 @@ def handle_vote_command(api, command, issue_id, comment_id, votes):
elif sub_command == "reopen":
gh.issues.open_issue(api, settings.URN, issue_id)
gh.comments.leave_issue_reopened_comment(api, settings.URN, issue_id)
elif sub_command == "fast":
Copy link
Contributor

Choose a reason for hiding this comment

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

maybe pull this out into it's own function?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

comment_updated_at = cmdmeta.comment.updated_at
comment_created_at = cmdmeta.comment.created_at
comment_poster = cmdmeta.comment.user
issue_poster = cmdmeta.issue.user
issue_created_at = cmdmeta.issue.created_at

# The post should not have been updated
if updated_at != created_at:
Copy link
Contributor

Choose a reason for hiding this comment

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

maybe leave some kind of response indicating why the command didn't run? Makes debugging easier

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hmm... yes, I see your point... but would this be prone to inducing spam?

return

# The comment poster must be in the meritocracy
meritocracy = {}
with open('server/meritocracy.json', 'r') as mfp:
fs = fp.read()
if fs:
meritocracy = json.loads(fs)
Copy link
Contributor

Choose a reason for hiding this comment

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

This seems a bit flaky. How about defining a function get_meritocracy with the current code for generating the meritocracy?

Copy link
Contributor

Choose a reason for hiding this comment

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

server/meritocracy.json is temporary - it shouldn't ideally be read from because it's only there to fill the gap before the database is ready and we can use that instead.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@md678685 What should I do instead? Would it be ok to define get_meritocracy as @PlasmaPower said and simply re-implement it when the db is ready?

Copy link
Contributor

@PlasmaPower PlasmaPower Jun 9, 2017

Choose a reason for hiding this comment

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

I don't think it would need re-implemented when the DB is ready. Sure, the DB could be used, but there's no point in that since this code and the main loop (PRs) run in the same process (so they share the same memoization cache).


if comment_poster not in meritocracy:
return

# The comment must posted within 5 min of the issue
if (arrow.get(comment_created) - arrow.get(issue_created_at)).total_seconds() > 5*60:
return

# Leave a note on the issue that it is expedited
Issue.update(expedited=True).where(Issue.issue_id == cmdmeta.issue.issue_id).execute()
else:
# Implement other commands
pass
Expand Down Expand Up @@ -174,7 +207,7 @@ def handle_comment(api, cmd):
comment=comment_text))

if command == "/vote":
handle_vote_command(api, parsed_comment, issue_id, comment_id, votes)
handle_vote_command(api, parsed_comment, cmd, votes)

update_command_ran(api, comment_id, "Command Ran")

Expand Down
1 change: 0 additions & 1 deletion github_api/voting.py
Original file line number Diff line number Diff line change
Expand Up @@ -9,7 +9,6 @@

import settings


def get_votes(api, urn, pr, meritocracy):
""" return a mapping of username => -1 or 1 for the votes on the current
state of a pr. we consider comments and reactions, but only from users who
Expand Down
3 changes: 3 additions & 0 deletions lib/db/models.py
Original file line number Diff line number Diff line change
Expand Up @@ -28,6 +28,9 @@ class Comment(BaseModel):

class Issue(BaseModel):
issue_id = pw.IntegerField(primary_key=True)
created_at = pw.DateField()
user = pw.ForeignKeyField(User, related_name='poster')
expedited = pw.BooleanField()


class ActiveIssueCommands(BaseModel):
Expand Down