Author Archives: Azad Salahli

Meeting Minutes: November 16, 2014



  • m_conley (Mike Conley)
  • ChipX86 (Christian Hammond)
  • purple_cow (David Trowbridge)


  • brennie (Barret Rennie)
  • dkus (David Kus)
  • nicole_xin (Nicole Xin)
  • andrewhong (Andrew Hong)
  • rdone (Ryan Done)
  • asalahli (Azad Salahli)
  • justy777 (Justin Maillet)


We’ve only got a few weeks left in the term. Two more meetings and then it’s pencils down. So your projects should be approaching finish – you should have patches up and out of WIP before pencils down with enough time for a few review iterations.

Round Robin


  • m_conley: So you’re working on multiple commits per review request. Do you have a plan to move forward with, then?
  • brennie: Yeah. I just need to work out how the DB should be changed to reflect multiple diffs attached to a RR.
  • brennie: (the hackpad needs to be cleaned up, its mostly just a dump from IRC)
  • ChipX86: We know there’s only so much that can be done this semester.
  • m_conley: What’s the scope of this, considering the remaining time? Like, how much do you aim to get done before pencils down?
  • brennie: I want to add an extra API endpoint that will be able to handle taking multiple diffs and attaching them to a RR.
  • brennie: Modifying the DB to allow multiple commits per RR, and allowing RBtools to talk to that endpoint.
  • brennie: I don’t plan to start on the frontend until thats all done.
  • m_conley: Is there anything we can provide you? Anything that you need from us? I know you’re waiting on a review…
  • ChipX86: I plan to do a few rounds of reviews tonight. Been pretty unavailable the past few days
  • brennie: Where is the RR model? There is no reviewboard/reviews/
  • ChipX86: It’s in reviewboard/reviews/models/
  • purple_cow: There’s a models/ directory
  • m_conley: got too big so it got split up a while back


  • dkus: Not many questions from me, I still need reviews on my backend work.
  • m_conley: I took a look at your front-end patch too. Wouldn’t mind seeing some screenshots posted:
  • dkus: Yeah I’ll post ss of what I have.
  • dkus: And I’ll need some input soon (maybe from ChipX86) on deciding how those drop overlays should look.
  • ChipX86: Ok.
  • m_conley: I’m guessing we might want to echo what we do for attachment drag/drop upload for review requests, but yeah, let’s iron that out when we get there.
  • m_conley: Anything else blocking you, or anything you need from us?
  • dkus: Nope, I’m a little concerned about finishing this all in the next 2 weeks though.
  • m_conley: Ah, I see. What do you see is the biggest hurdle to get through?
  • dkus: Well, once I have the drop overlays all done and tested, I still need to look at showing progress on uploading files (unless we don’t need that).
  • ChipX86: I wouldn’t make that a priority.
  • m_conley: Considering the remaining time, along with time for review, yea: we might want to do that in a follow-up.
  • dkus: Alright. It’s something I wouldn’t mind doing after the semester if I don’t have time for it before the end.
  • m_conley: That’s music to our ears.
  • m_conley: Ok, so if we chop off the progress feedback, does that make getting something landable finished more feasible?
  • dkus: Yeah.
  • m_conley: Excellent.


  • nicole_xin: I just resolved reviewDialogViewTest this morning.
  • m_conley: That’s good. How about the second issue in your report? “One or more fields had errors”?
  • nicole_xin: So, about general comment reply, I was wondering any hints on that 400 Bad Request error?
  • nicole_xin: Still blocked by this one
  • ChipX86: What does the error payload say?
  • nicole_xin: I got this:

    {“fields”: {“include_text_types”: [“Field is not supported”]}, “stat”: “fail”, “err”: {“msg”: “One or more fields had errors”, “code”: 105}}

  • ChipX86: We made some big changes to how markdown support works in master. You’ll need to update some stuff in your API resource to match it.nicole_xin: But I check the request, everything seems normal, and include_text_types contains: raw
  • ChipX86: I’ll take a look at your change and try to point you in the right direction there.
  • nicole_xin: Alright thanks ChipX86!
  • m_conley: ChipX86: is there an example for nicole_xin to work off of? A diff of another resource that needed similar changing?
  • ChipX86: Well, there’s a series of changes.
  • ChipX86: Let’s dig into this after the meeting.
  • nicole_xin: Sounds good.
  • nicole_xin: I think general comments can be finished next week. I’m thinking about picking another short project.
  • m_conley: It’ll have to be pretty short
  • nicole_xin: Probably ‘rbt stamp’?
  • ChipX86: That might be more than a week or two’s worth of work, but you could make some progress toward it.
  • m_conley: Let’s give it a shot.
  • nicole_xin: Great. Oh I need some clarification here.
  • nicole_xin: rbt stamp just need to stamp the current commit with a review_request url?
  • ChipX86: Yes, but that requires making use of the same guessing ability that rbt post -u uses, which means moving that out into a utility function (I don’t think we landed a change for that yet?).
  • m_conley: A little light refactoring.
  • ChipX86: So that + optionally taking an ID (using rbt stamp -r ), and then augmenting the commit.
  • ChipX86: I think that part may require a bit more work. We have support for editing a new commit, but not augmenting one yet. Shouldn’t be too bad, if you just focus on git for now.
  • nicole_xin: I don’t have further questions.


  • m_conley: So, glad to see all the testing you’re doing on r/6333. Thanks for filling that data in. I’m assuming you’re just waiting for review on that patch now?
  • andrewhong_p: Yeah, but I found out that the matchContains flag doesn’t actually work so I’m looking into that, and then have to retest the change to make sure its implemented properly.
  • m_conley: Alright. Can you ping one of us when you’ve got that figured out so we can get that fix re-landed?
  • andrewhong_p: Sure!
  • m_conley: So, attachment ZIP downloading… Is that kinda mothballed while you deal with this autocomplete thing? I don’t see much about it in your status report.
  • andrewhong_p: I realize I probably wont finish by the end of the term but I am willing to finish it up after the term.
  • m_conley: What is remaining with the attachment zip stuff?
  • andrewhong_p: Implementing chunking, basically integrating ChipX86’s code snippet. I decided to keep focus on autocomplete this week.
  • m_conley: So can you give us a sense of what you think you’re going to have done in time for review before pencils down?
  • m_conley: What do you think you can realistically finish?
  • andrewhong_p: I’m hoping I can wrap it up entirely by the end of term.
  • m_conley: What are we talking about here? Autocomplete? Attachment downloading?
  • andrewhong_p: Both
  • m_conley: Wow, ok. Yes, please try to lean on it, push it through. We think you can do it with a good hard push. Just let us know when you’re blocked, and we’ll unblock you ASAP
  • m_conley: And you should have enough time to get these both ready to roll out. Sound good?
  • andrewhong_p: Sure thing, thanks. I should have lots more time after Wednesday this week.


  • m_conley: So sounds like you got unblocked, which is good
  • m_conley: Do you feel confident that you can get your project done by pencils down with time for review?
  • rdone_: I think I should get a significant chunk done: stuff working well, but maybe not 100% with tests and making sure it works flexibly with different file attachments
  • m_conley: What file attachments are you planning on having it work with?
  • rdone_: I think just image and text for now, but the fileAttachmentRevisionSlider could be generically used by any fileAttachment
  • ChipX86: Those are the only kinds we natively support in RB anyway. If they work for both, we’re in good shape
  • m_conley: Some documentation on how future developers (or extension writers) can support the slider for other attachments would be good
  • rdone_: Yeah i think that’s easier than trying to predictively test stuff
  • m_conley: Perhaps those should be your final deliverables – tested and working for images and text, and documentation for how to extend further. Does that sound realistic?
  • rdone_: Yeah for now
  • m_conley: Let us know if/when that changes ASAP, ok?
  • rdone_: For sure


  • asalahli: justy777: Sorry for delaying you 😦
  • m_conley: So the question you’re referring to in your status report – that’s regarding error handling?
  • asalahli: Yes. So, as heapify mentioned, one option is to pass ignore_errors=False and check return code, but merging and pushing uses 4-5 commands and checking the return code of every of them will complicate the code. And as I mentioned above, with that approach I cannot handle the errors in command main function, so thats why I added optional exception. And in the future with more scm support, I’m guessing it might scale beter
  • m_conley: In my experience, it’s strange for a function parameter to change the exception-throwing behaviour of that function.
  • ChipX86: Yeah, we should really be checking the return code instead of causing exceptions to occur. Exceptions would be fine if that was the only way of reporting errors in execute(), but it’s not.
  • m_conley: Do you feel like you can get this wrapped with time for review before pencils down?
  • asalahli: Actually, I think it is ready for full blown reviews by now (I’m hoping I haven’t missed any big detail in the implementation).
  • ChipX86: I’ll give it a review tonight
  • asalahli: The only thing I’d change is error handling. Oh, speaking of which: do we actually need special error handling in case of execute()? If execute fails, it prints the output and terminates the command. So, I’m asking would it be better to just stick to that behaviour?
  • ChipX86: What I’d suggest though is catching the errors and having a reasonable result from the function, so that we can standardize the results from any SCMClient.whatever() function
  • asalahli: I see. What should SCMClient.whatever() functions return when error occurs? error message itself? or just true false?
  • ChipX86: It can raise an exception, like a MergeError with a description
  • asalahli: Ok. One more question: should I define two seperate exceptions for merging and pushing to upstream, or a common exception is fine?
  • ChipX86: I’d say have a MergeError and a PushError


  • m_conley: So, you’ve got some hooks written – that’s great! With two remaining weeks, accounting for time for review, how many more hooks do you think you can polish off including the ones you’ve already put up?
  • justy777: There are 4, I would guess two this week could be polished off maybe 3 if im lucky. the 4th is up in the air.
  • m_conley: Wait – to be clear, those 4 include the ones you’ve already got up? Or are separate from the ones you’ve already put up?
  • justy777: Yes they do, 3 [WIP]s need to be polished, one of those that is up is iffy because of the way SCMTools are stored
  • m_conley: Ok – you said the 4th is iffy. Why is it iffy?
  • justy777: Because ChipX86 told me that SCMTools are stored in the database and the hook as of them could not be cleanly implemnted, and that I should work on the others first.
  • ChipX86: Yeah, I wouldn’t worry about the SCMTools. That’s going to be a much bigger, substantial project.
  • m_conley: Let’s get these three cleared out then, and some solid documentation.
  • ChipX86: Documentation-wise, I’d really like to see new docs for the hooks in the manual, like we have for all other hooks, complete with working examples
  • m_conley: Examples are the best – often those are what the eyes latch onto immediately
  • m_conley: Do you have any questions on any of that?
  • justy777: Just one. There are not a lot of tests for simple hooks like two of the ones I’ve been working on, should I be testing the register and unresgister work indivdually or together and should they be more thououghly tested?
  • ChipX86: Things should always be tested individually
  • purple_cow: Just because existing ones aren’t thoroughly tested doesn’t mean new ones shouldn’t be
  • m_conley: Yeah, let’s go for finer grain testing
  • ChipX86: When writing unit tests, I try to write a test for all generic use cases for that code, and try to think if there are edge cases that make sense to test
  • purple_cow: And we can blame the older ones on the people who built the extensions framework
  • m_conley: Any other questions?
  • justy777: Not at the moment

Status Reports for October 18, 2014

Barret Rennie

What project are you working on?
I’ve been working on expanding the comment diff UI.

What you accomplished this week?
This week I’ve mostly been working on fixing up the expanding comment
diff UI. The UI is now animated and defaults to a collapsed state so as not
to use a lot of screen real estate and the collapse button now hovers over
the diff fragment instead of being a fixed header on the table. I also
removed some dead code from the diffviewer’s DiffReviewableView. All it
needs is some reviews — its a big change.

Links to anything you’ve done this week

What you plan to do next week?
I’ll be actually starting to work on the RBTools aliases functionality and I
think that they should function identically to git aliases for consistency’s

What, if anything, is blocking you from making progress?

Any other questions?

Andrew Hong

What project are you working on?
Downloading review request attachments as zip.

What you accomplished this week?
Finally figured out the cause of why the zips were corrupt when downloaded!
Feature is now out of WIP. Trying to rebase…
Fixing suggestions given in review request for feature

Links to anything you’ve done this week

What you plan to do next week?
Get started on a new project, haven’t decided on one yet; got a few on my mind though.

What, if anything, is blocking you from making progress?

Any other questions?

David Kus

What project are your working on?
Drag ‘n Drop inline images in the markdown editor

What you accomplished this week?

  • Completed the view that redirects to the file url given a file attachment id.
  • Cleaned up some code, added some unit tests around the changes I made to FileAttachment model.
  • Split my review request into two review requests (one for the backend, one for the front-end) to make this a bit more manageable. There were only a couple of reviews on the old one, and no outstanding issues.

Links to anything you’ve done this week:
New review request for backend:

What you plan to do next week?

  • Creating unit test for FileAttachment WebAPI resource. Do a bit more testing and then hopefully remove [WIP] tag for the backend and get some reviews on it.
  • Start working on wiring up the back-end to the front-end prototype

What, if anything, is blocking you from making progress?

  • I noticed that David T. (purple_cow) added some feature around file attachment history. How should user file attachments handle this? As it is right now, my code ignores this feature.
  • What should happen when using the view to try and view a file attachment before the file has been uploaded? e.g. View at /attachments/user-files// . This should redirect to the actual file for FileAttachment object with id (e.g. /uploads/etc/etc/). If there is no file associated with this FileAttachment object yet, what should it do?

Any other questions?
None at the moment!

Yanjia Xin

What project are your working on?
Same as last week, general comments

What you accomplished this week?

  • Implemented backend model test, all passed. I’m officially done on backend model.
  • Reorganized web api resource, merge a resource class
  • Add web api tests, still in progress

Links to anything you’ve done this week
Back-end model:

What you plan to do next week?

  • Refine web api and Pass all tests on web api
  • Start working on front end

What, if anything, is blocking you from making progress?

  • My database is crashed since last week, fixed just now! but it’s not blocking me from making progress since I have plenty of work to do regardless of the database.

Any other questions?

Ryan Done

What project are you working on?
Finishing up the alphabetic paginator.
Started looking into next project.

What you accomplished this week?
Following meeting last Sunday I had some issues (Perforce and SSH) with my development environment that I had to sort out, all good now.
I addressed changes suggested in David Trowbridge’s review of my code: re-factored and simplified the template changes into two separate templates.
Decided to start on the spam filter extension as my next project, been mostly reading code for the last day or so looking into that so I don’t have much to show for it yet.

Links to anything you’ve done this week

What you plan to do next week?

  • Start writing code for the spam filter
  • Address any more reviews for the paginator
  • Do some more code reviews! (I’ve been reading a lot of the review requests but haven’t had much to say)

What, if anything, is blocking you from making progress?
Like I said I had some development environment issues that were causing unit tests to throw errors, but I’ve worked that out now.

Any other questions?
For the spam filter: should a newly registered user:

A) Not have any commenting privileges until their registration is approved by an admin? (A lot of discussion forums have implemented this behaviour)


B) Be shadowbanned or shadowmuted. Only they (and admins) can see comments and posts made to Reviewboard until an admin approves their registration. Of course we will still them know that their posts are not visible yet.

The latter option seems more difficult, but has its benefits.

Mark Andrew Loyzer

What project are you working on?
Adding an extension to Review Board that will allow people to export a review request as PDF or XML/JSON(pending).

What you accomplished this week?
Added a version 2 for a PDF mock up.
Began implementing the PDF report generation.

Links to anything you’ve done this week

What you plan to do next week?
Try to get everything at least up to the ‘Change Set’ summary implemented. This involves:
Adding headers to every page.
Finishing the Details section.
Adding Issue Summary section.
Adding File Summary section.

What, if anything, is blocking you from making progress?
I’m still waiting for a review on

Any other questions?
What is the difference between a review’s body_top, body_bottom, and comments? In order to add a ‘body_top’ I had to click the ‘add comment’ link which I assumed would generate and save the contents as a comment…but it was apparently the review’s body_top. So, in short, what is the difference between these?

Justin Maillet

What project are you working on?
Still working on sandboxing extensions.

What you accomplished this week?
Very sorry, but due to getting sick and midterms I only got a bit of research done.

Links to anything you’ve done this week

What you plan to do next week?
This next week I plan to get some serious work done.
Hopefully putting all the sandboxing up for review along with most if not all the tests.

What, if anything, is blocking you from making progress?
Nothing is blocking my progress.

Any other questions?
No other questions.

Azad Salahli

What project are you working on?
Add ‘rbt land’ for landing changes

What you accomplished this week?
I have been looking at how `rbt patch` works, and how new changes are applied to repositories right now.

Links to anything you’ve done this week
Nothing I can point out, unfortunately.

What you plan to do next week?
Get a prototype working for Git repositories.

What, if anything, is blocking you from making progress?
My understanding of the process is that, a review request is first patched to the local repository of the user, and then pushed to the remote server.

If that is how it works, wouldn’t it be redundant to create a new command that does the same thing as `rbt patch` except pushing it to remote repository?

Or maybe `rbt land` should use `rbt patch` command itself and build on top of it?

I apologize for a kind of empty status report. I wanted to discuss this today with mentors interactively, and send a more useful report, and perhaps a [WIP] review request. But if I can get unblocked on this (and if my understanding is not entirely wrong), then I can probably create a prototype very soon.

Also, I am still waiting on reviews for

Any other questions?
See above.

Meeting Minutes: September 14, 2014



  • ChipX86 (Christian Hammond)
  • heapify (Anselina Chia)
  • m_conley (Mike Conley)
  • purple_cow (David Trowbridge)
  • smacleod (Steven MacLeod)


  • andrewhong (Andrew Hong)
  • asalahli (Azad Salahli)
  • brennie (Barret Rennie)
  • dkus (David Kus)
  • justy777 (Justin Maillet)
  • mloyzer (Mark Loyzer)
  • nicole_xin (Nicole Xin)
  • rdone (Ryan Done)


  1. We (mentors) won’t always get to review requests immediately or even notice them, so if it’s been a day since you posted, point us to it directly
  2. Fill out your description and testing done thoroughly when doing RR. We need to know exactly what the change does and how you tested it in order to know what to focus on, and this information will also go into the commit message. This is also a good read:
  3. If you got a Fusion 6 license, and wanted Fusion 7, let ChipX86 know!

About the Sprint (m_conley):

I think there’s a start time of something like 9AM or so.

There’s no attendance or anything, but getting there on time means you’ll hear any starting announcements. Also, there might be donuts – Karen is known for bringing in donuts – and if you want a donut, well, you’d better hustle.

End times are around 5-ish, 5:30-ish? It’s really kinda fluid, and I think it depends on how dinner gets organized. I believe there’s a night where dinner is taken care of? I don’t know that for certain. But if so, that’ll likely involve a large reservation, so the timing will be organized more strictly. But 5-ish is about when the day ends.

I’m hedging because I’m going off past experience, and not from an official schedule or anything.

Even when there isn’t an organized dinner, we usually go out as a team. It’s a good idea, though not compulsory, to stick around in the evening and hang out, like, to get to know your fellow students, put names to faces in more concrete, for-reals-socially ways. I encourage you to participate – we’re all great people, I can tell.

If your inner introvert is freaking out, don’t worry about it – you’re surrounded by likeminded, interesting people who know how to speak your language. My inner introvert freaks out just a bit every time too.

ChipX86: when you get there, have m_conley tell you the horse head story

m_conley: I won’t be in town for most of the sprint, but if I have the strength after returning on Saturday night, I’ll show up for Sunday, and then tell the horse head story

Round Robin


  • I’ve got no questions
  • Waiting on a review for my review request (
    • m_conley: This is also an excellent time for you all to get some reviewing under your belts. I know what you’re thinking – “I’m new at this, how can I possibly review this code?”. Just take a look at the diff and see what you think. Maybe something is not clear, or maybe something looks really excellent, or something could be done more efficiently, etc. If so, comment on it. There are some tips for reviewing code in your welcome packet
  • Apparently ClearCase and Perforce are only paid for commercial use so I should be able to test them myself.
  • Q: do you know what you’re doing this week then?
    A: Yep. First off -X for hg (which should be easy because hg supports -X) then git and try the other SCMs


  • I am no longer working on issue 3452, mloyzer has taken over that, and I switched over to 2766
  • For the week I’ll just be working on that
  • In working on that found another bug, filed an issue ticket, and then ChipX86 resolved it
    • ChipX86: we already had it up for review by the time that was filed


  • Nothing outside the status report.
  • Saw that Chipx86‘s reply to my question. Haven’t had a chance to look over it yet, but this week i’ll be working on issue 3437


  • I do have some questions with bug 3532, which is what I’m working on.
  • There is a feature that when we have a link to a comment, the corresponding review box will be expanded. Somehow I have no clue where this part locates.
    • m_conley: That’ll almost certainly be in the static/rb/js
    • ChipX86: It seems the browser is actually handling the visibility of that box, and that maybe it doesn’t work for reviews because the <a> tag for that anchor is outside the box. So one thing to try is seeing what happens if that’s moved inside somewhere. If so, there’s a couple ways to fix this. Either move it inside, or we have some code that checks the anchor, matches it to a review box, and then opens it. Actually, the view function in Python (review_detail) computes whether boxes are open or closed by default, so that could actually match up the anchor and just force it open. If you have that code work for both reviews and comments, then it’ll fix the opening and the initial button state in one go
  • How to know a review id or comment id?
    • ChipX86: You’d have to parse the URL (there are Python functions for this) to see the anchor being used. We have a standard naming convention (not a good one, but a standard one) where we have prefixes of “review”, “comment”, “fcomment”, or “scomment”, along with the ID of the object.
  • I mean, how do I test this? To find my own review id
    • ChipX86: You can open the browser’s developer tools and inspect the review box and look for the nearest <a name=”review12345″>. Most of you are going to spend a lot of time in the browser’s developer tools. Good to get comfortable with it


  • I’ve been looking at issue 3300, and trying to get a feel for how things are organized.
  • Q: Do you have your hackpad set up?
    A: No, I will be doing that today after the meeting.
  • Q: You know how to move forward?
    A: Yes keep working on my bug in static/rb/js


  • Laptop been having issues since I went to Windows 8. Hoping everything is well once I go back to Windows 7. Setting it up now.. Shouldn’t take long to get back.
    • Mentors: Machine troubles are one of several excellent reasons why we’re going to be asking for WIP code drops every week. If you want to be extra careful, store everything in Dropbox or a Github fork of Review Board or RBTools, Djblets, whatever it is you’re hacking on.
  • In terms of RB: going to try and make more progress on the autocomplete bug (3444) this week. Still picking up Django as I go


  • I still cant reproduce the bug I was working on (
    • I’ve changed mail backend to console
    • Haven’t enabled registration emails. (This is probably the issue)
    • To enable, go to localhost:8080/admin/settings/email/ or click “E-Mail” in the System Settings group in the admin interface
  • Two Scoops of Django is a good book.


  • I pinged Ryan and we talked about it and he was willling to do another one so i took claim of the one we were both doing (
  • Submitted a RR last night (WIP) (
  • The bug says “a warning should be issued that the user did not actually change the RR”. As of right now, all my ‘fix’ does is check if the field was modified and if it was then save it, otherwise don’t. I don’t have any “warning”.
    • m_conley: There are a few ways of solving this problem:
      1. Do not allow the user to publish unless at least one field has changed. If any field changes, and then changes to its original value, revoke ability to publish.
      2. Or allow the user to publish if the change description is empty, but style the empty change description to make it more explicitly clear that nothing has actually changed. So instead of showing this we show some text in there saying, “Nothing was changed”, or something.
    • ChipX86: I kind of think this isn’t an EasyFix actually. There’s the JS side of things, but there’s also the API side. And the reality is, whether or not you’re posting something that looks like a change or not, it’s valid to create a draft with whatever you want even if that draft is empty.
    • purple_cow: I think when publishing we should check (on the API side) whether the draft is empty, and then return a failure, and on the client side, it can show a message when that happens, and that should be pretty easy to do. Just a new API error and then some additional handling on the client side. I wouldn’t try to make the client side figure out whether the draft is empty.
    • ChipX86: you’ll want to look into the ‘publish’ function in reviewboard/reviews/models/, and the one in