Meeting Minutes: Oct. 13, 2013


1. What are the rules for when a trophy should be awarded?

Currently, we have two rules: 1) The id for a review request that a user has submitted is a palindrome (palindrome >= 1001), and 2) The id for a review request is a “milestone” number, where a “milestone” number is any numbers >= 1000 that is a non-zero digit followed by all zeroes. So 2000, 20000, 30000, etc.

For more info, look at: reviews/templatetags/

2. Are those the only logic I should implement for now?


3. I was thinking something like awarding users for the # of reviews they do, or is that not something we’re interested in?

Yes, I’d start there. Might be fun to brainstorm some other trophy ideas though – maybe keep them in a Hackpad or something, so we can try implementing them later.

But let’s hold off on new additions until the framework is there. Brainstorming is fine though. The important part of this is to design trophies in a way where new ones can easily be added, either by us or through extensions. 

4. The TrophyManager has a computer_trophies function which assigns trophies to the right review request. Does it call the iftrophy function (which would be a rename of ifneatnumber funciton in reviewboard/reviews/templatetags/ to check for that?

It should be going through the list of registered trophy classes and seeing if any of them grant a trophy, given a review request. ifneatnumber won’t exist after this work, and there won’t be a single function that knows about every condition for a trophy. Keep this handy: 

5. In the hackpad article, it says that ifneatnumber should be renamed to iftrophy to check each review request to see if a trophy is associated with it.

So that’s basically just the way that the TrophyManager’s function for computing trophies will be called. It won’t actually do the milestone/palindrome checks itself. It should just iterate through all the registered trophy types (of which there will be 2 for now) and have each one check. It does a bit of both checking to see if a review request actually has a trophy associated with it, and that it should have one. So if it’s never ran the trophy checks before for that review request, it’ll do so then, through the TrophyManager.

6. Could someone validate my mockup please?

A datagrid isn’t really the right interface for this. A lot of space seems to be spent showing an image. A single row of X number of trophies, I think, is better, with a link taking you to a page showing every trophy (in a grid, say). 

(Behzad) That’s what I was thinking at first too, but I wasn’t sure how to show the date received and other info along with it in the initial view.

Alternatively, a grid of trophy icons, and when you click on one, details are displayed in a separate pane. Maybe just the trophy, then the “ID, date” below.


1. What does this mean: q = DiffSet.objects.filter(files__comments__review = review)?

Anytime you see a __ between fields, it’s an SQL JOIN. The double underscores traverse foreign key relationships. The more joins, the more expensive the query. You might find this useful:

2. Right now what my function is doing is getting relevant reviewer suggestions just by looking at past commenting/reviewing history. But if I also wanted to look at the history of changes made to a file, is that information I’d get from the source control tool or from reviewrequest history?

All this info gathering, regardless of the database or repository, gets very expensive. I think we need to figure out if the goals here are worth it, or if we should change a bit what we’re doing. I’m wondering if there’s a way to apply this that might be more useful to people. In most cases, people don’t assign to individual people. They assign to groups. I think groups are here we want to start with.

(David) Really? I think individuals is much more interesting. We care about who actually commented on something. 

(We should talk more about this after the meeting…) But to answer your question above, you’ll probably need to look at it from the database and not the repository.


1. When do we use single vs double quotes for string literals (assuming no quotes need to be escaped)? How to use blank lines with if/for/return statements?

Single quotes are preferable, unless you need single quotes within the string. We don’t have a hard rule on blank lines, but my preference (which I’m typically vocal about) is not to have statements right up against the opening/closing of an if/for statement, or any other statement that opens another indentation level. I think of statements like paragraphs. Each set of statements grouped together should be about the same thing. conditionals/loops are starting their own conversations.

2. What about return statements? Do we want a blank line before them?

I’d say generally yes. But again, this is a personal preference. 

(Mike) If you don’t know before hand, you’ll hear about it during the review. 🙂


1. I’m actually having troubles linking my static files. I had to copy them over to media/ext to work on my prototype, which I don’t think I’m supposed to do.

For development, create a symlink to your extension’s htdocs directory in the media/ext, in place of the directory that enabling gives you. 

(Mike) I seem to recall that ExtensionManager just automatically copies stuff over.

(David) I do the symlink.

So until furhter notice, sounds like the symlink’s the way forward.

2. My WIP for the prototype is up. Some feedback would be great:

(Mike) Yes, can do. I’ll have some feedback for you some time tomorrow.

3. Can post-review post diffs from two different repositories?

Nope. You’ll need to break them up into different review requests. (Btw, you should be using rbt post, not post-review. post-review is going away).

(Mike). You can (and probably should) mark the review request relationships using the ‘Depends On’ field. It’s one of the fields at the top of your review request, underneath “Bugs”. You can put the ID of a review request that the current one depends on in there. Helps keep track of what’s related to what.


(Behzad) I’ve seen you guys use this syntax: s/1.2/1.5 a few times. Does it mean swap 1.2 with 1.5?

Yeah, it’s a “regular expression”. 

(David) Kind of a riff on regular expression. 

(puiterwijk) It’s the sed substitute syntax. 

(Mike) It’s us being mega geeks.



Leave a Reply

Fill in your details below or click an icon to log in: Logo

You are commenting using your account. Log Out /  Change )

Google+ photo

You are commenting using your Google+ account. Log Out /  Change )

Twitter picture

You are commenting using your Twitter account. Log Out /  Change )

Facebook photo

You are commenting using your Facebook account. Log Out /  Change )

Connecting to %s