Skip to content
This repository has been archived by the owner on Feb 8, 2018. It is now read-only.

Eating elephant, first step (events, db schema) #2006

Merged
merged 7 commits into from
Feb 27, 2014
Merged
Show file tree
Hide file tree
Changes from all 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
47 changes: 47 additions & 0 deletions branch.py
Original file line number Diff line number Diff line change
@@ -0,0 +1,47 @@
import heapq
import psycopg2
from gittip import wireup

def main():
db = wireup.db()
with db.get_cursor() as c:

claimed = c.all("""
SELECT claimed_time as ts, id, 'claim' as action
FROM participants
WHERE claimed_time IS NOT NULL
ORDER BY claimed_time
""")
usernames = c.all("""
SELECT claimed_time + interval '0.01 s' as ts, id, 'set' as action, username
FROM participants
WHERE claimed_time IS NOT NULL
ORDER BY claimed_time
""")
api_keys = c.all("""
SELECT a.mtime as ts, p.id as id, 'set' as action, a.api_key
FROM api_keys a
JOIN participants p
ON a.participant = p.username
ORDER BY ts
""")
goals = c.all("""
SELECT g.mtime as ts, p.id as id, 'set' as action, g.goal::text
FROM goals g
JOIN participants p
ON g.participant = p.username
ORDER BY ts
""")

for event in heapq.merge(claimed, usernames, api_keys, goals):
Copy link
Contributor

Choose a reason for hiding this comment

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

heapq.merge

Nice. I've never used that before. :-)

payload = dict(action=event.action, id=event.id)
if event.action == 'set':
payload['values'] = { event._fields[-1]: event[-1] }
c.run("""
INSERT INTO events (ts, type, payload)
VALUES (%s, %s, %s)
""", (event.ts, 'participant', psycopg2.extras.Json(payload)))


if __name__ == "__main__":
main()
21 changes: 21 additions & 0 deletions branch.sql
Original file line number Diff line number Diff line change
@@ -0,0 +1,21 @@
BEGIN;
DROP TABLE IF EXISTS events;
Copy link
Contributor

Choose a reason for hiding this comment

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

Really? Why would the events table ever exist already?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oh, well. Left over from continuous testing. Editing the file and rerunning it with \i is very convenient. But it is going to be merged to schema.sql anyway, right?

Copy link
Contributor

Choose a reason for hiding this comment

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

Sure, that's fine for development (I've been known to do the same). For a PR we should remove this line though, right?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That's a work with no real value because the file is going away anyway after the merge.

CREATE TABLE events
( id serial PRIMARY KEY
, ts timestamp NOT NULL DEFAULT CURRENT_TIMESTAMP
, type text NOT NULL
Copy link
Contributor

Choose a reason for hiding this comment

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

Would it be better to use kind instead of type, since (as we've mentioned somewhere) type is a builtin function in Python?

, payload json
);

CREATE INDEX events_ts ON events(ts ASC);
CREATE INDEX events_type ON events(type);

/* run branch.py before this
DROP RULE log_api_key_changes ON participants;
DROP RULE log_goal_changes ON participants;
Copy link
Contributor

Choose a reason for hiding this comment

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

We have four rules on the participants table right now:

  1. bitcoin_addresses logs into bitcoin_addresses
  2. log_api_key_changes logs into api_keys
  3. log_goal_changes logs into goals
  4. log_participant_number logs into log_participant_number

This PR replaces (2) and (3) with calls to the new add_event function, and it also uses add_event to start logging changes to username and claimed_time, which were not previously logged. (1) and (4) are unaffected by this PR.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That is correct. Now that we are somewhat unclogged I'll look into adding those over the weekend.

Copy link
Contributor

Choose a reason for hiding this comment

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

Cool. Be sure to give me bite-sized PRs, eh? :-)

DROP TABLE goals, api_keys;
DROP SEQUENCE api_keys_id_seq, goals_id_seq;
*/

END;

9 changes: 8 additions & 1 deletion gittip/models/__init__.py
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,7 @@

"""
from postgres import Postgres
import psycopg2.extras

class GittipDB(Postgres):

Expand Down Expand Up @@ -226,4 +227,10 @@ def _check_claimed_not_locked(self):
""")
assert len(locked) == 0

#

def add_event(c, type, payload):
SQL = """
INSERT INTO events (type, payload)
VALUES (%s, %s)
"""
c.run(SQL, (type, psycopg2.extras.Json(payload)))
48 changes: 29 additions & 19 deletions gittip/models/participant.py
Original file line number Diff line number Diff line change
Expand Up @@ -31,6 +31,8 @@
NoSelfTipping,
BadAmount,
)

from gittip.models import add_event
from gittip.models._mixin_team import MixinTeam
from gittip.models.account_elsewhere import AccountElsewhere
from gittip.utils import canonicalize
Expand Down Expand Up @@ -185,7 +187,9 @@ def update_number(self, number):
def recreate_api_key(self):
api_key = str(uuid.uuid4())
SQL = "UPDATE participants SET api_key=%s WHERE username=%s"
self.db.run(SQL, (api_key, self.username))
with self.db.get_cursor() as c:
add_event(c, 'participant', dict(action='set', id=self.id, values=dict(api_key=api_key)))
c.run(SQL, (api_key, self.username))
return api_key


Expand All @@ -207,16 +211,18 @@ def resolve_unclaimed(self):
return '/on/%s/%s/' % (rec.platform, rec.user_name)

def set_as_claimed(self):
claimed_time = self.db.one("""\
with self.db.get_cursor() as c:
add_event(c, 'participant', dict(id=self.id, action='claim'))
claimed_time = c.one("""\

UPDATE participants
SET claimed_time=CURRENT_TIMESTAMP
WHERE username=%s
AND claimed_time IS NULL
RETURNING claimed_time
UPDATE participants
SET claimed_time=CURRENT_TIMESTAMP
WHERE username=%s
AND claimed_time IS NULL
RETURNING claimed_time

""", (self.username,))
self.set_attributes(claimed_time=claimed_time)
""", (self.username,))
self.set_attributes(claimed_time=claimed_time)



Expand Down Expand Up @@ -292,13 +298,14 @@ def change_username(self, suggested):
if suggested != self.username:
try:
# Will raise IntegrityError if the desired username is taken.
actual = self.db.one( "UPDATE participants "
"SET username=%s, username_lower=%s "
"WHERE username=%s "
"RETURNING username, username_lower"
, (suggested, lowercased, self.username)
, back_as=tuple
)
with self.db.get_cursor(back_as=tuple) as c:
add_event(c, 'participant', dict(id=self.id, action='set', values=dict(username=suggested)))
actual = c.one( "UPDATE participants "
"SET username=%s, username_lower=%s "
"WHERE username=%s "
"RETURNING username, username_lower"
, (suggested, lowercased, self.username)
)
Copy link
Contributor

Choose a reason for hiding this comment

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

Did you intentionally remove the back_as=tuple argument here? Why isn't it needed any more?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Not only is not needed, it is not allowed. The way it works is that back_as is a property of the cursor. Since now I create the cursor explicitly, I need to supply it as a param in the get_cursor call.

Copy link
Contributor

Choose a reason for hiding this comment

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

Ah, now I see that. Makes sense.

except IntegrityError:
raise UsernameAlreadyTaken(suggested)

Expand All @@ -310,9 +317,12 @@ def change_username(self, suggested):

def update_goal(self, goal):
typecheck(goal, (Decimal, None))
self.db.run( "UPDATE participants SET goal=%s WHERE username=%s"
, (goal, self.username)
)
with self.db.get_cursor() as c:
tmp = goal if goal is None else unicode(goal)
add_event(c, 'participant', dict(id=self.id, action='set', values=dict(goal=tmp)))
c.one( "UPDATE participants SET goal=%s WHERE username=%s RETURNING id"
, (goal, self.username)
)
self.set_attributes(goal=goal)


Expand Down
3 changes: 3 additions & 0 deletions templates/profile.html
Original file line number Diff line number Diff line change
Expand Up @@ -23,6 +23,9 @@ <h2>Navigation</h2>
{% if participant.show_as_team(user) %}
<a href="/{{ participant.username }}/members/"><button{% if 'members' == current_page %} class="selected"{% endif %}>Members</button></a>
{% endif %}
{% if user.ADMIN %}
<a href="/{{ participant.username }}/events/"><button{% if 'events' == current_page %} class="selected"{% endif %}>Events</button></a>
{% endif %}
Copy link
Contributor

Choose a reason for hiding this comment

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

Should the user themselves not be able to see this?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'd like to merge it and the events view is not ready yet for regular user consumption. In the future, yes, it is for the user to see.

</div>
{% elif participant.show_as_team(user) %}
<div class="nav level-2">
Expand Down
42 changes: 42 additions & 0 deletions www/%username/events/index.html.spt
Original file line number Diff line number Diff line change
@@ -0,0 +1,42 @@
from decimal import Decimal

from aspen import Response, log
from aspen.utils import to_age
from gittip.utils import get_participant, get_avatar_url

db = website.db

[-----------------------------------------------------------------------------]

participant = get_participant(request, restrict=True)
hero = "Events"
title = "%s - %s" % (participant.username, hero)
locked = False

SQL = """
SELECT * FROM events WHERE type = 'participant' AND payload->>'id' = %s ORDER BY ts ASC
"""

events = db.all(SQL, (unicode(participant.id),))

[-----------------------------------------------------------------------------]
{% extends "templates/profile.html" %}
{% block page %}

<style>
td { padding: 4px; }
</style>
<table id="events" class="centered">

{% for e in events %}
<tr>
<td>{{ e.ts }}</td>
<td>{{ e.type }}</td>
<td>{{ e.payload }}</td>

</tr>
{% endfor %}

</table>

{% endblock %}