Meeting Minutes: October 5, 2014


Mentors: m_conley (Mike Conley), ChipX86 (Christian Hammond), smacleod (Steven MacLeod), heapify (Anselina Chia), and purple_cow (David Trowbridge).

Students: rdone (Ryan Done), andrewhong (Andrew Hong), asalahli (Azad Salahli), dkus_ (David Kus), nicole_xin (Nicole Xin), justy777 (Justin Maillet), brennie (Barret Rennie), and mloyzer (Mark Loyzer).


  • (m_conley) we’re having your midterm evaluations handed in on the 24th of October
  • (heapify) it’d be great if everyone could include what they have left to do in the Description of the review request
  • (m_conley) if your patch is ready for a full-blown review, please remove the [WIP] and then we’ll give it some real attention. Like, we’re monitoring the WIPs right now, but we assume they’re not ready for full review since they’re still in flux. But please make sure you remove the [WIP] part of the summary once you’re ready for us to dig in and try to get it landed.\

Round Robin:

nicole_xin (Nicole Xin):

  • working on general comments, finished setting up the back-end model
  • question about opening a new review request for web API part of feature: “Sometimes there’s a good reason to split things up between different review requests. Like, if the changes are atomic enough (i.e., 1 can land without the other), it might make sense. Often times it’s easier to review (and therefore land) smaller chunks of work, rather than a large body of work in this case, I think it could either way. If you feel like you want to get the back-end reviewed and landed first, so be it – but make sure you create a new branch for the newer review request based on your first work. Like, you have slight added complication because now you’ve got two review requests to maintain. Which means having master -> Backend Branch -> WebAPI Branch. We might end up iterating on backend branch a little bit, which will require you to either merge in the changes from Backend Branch into WebAPI branch, or rebasing WebAPI Branch on top of your updated Backend Branch” — m_conley
  • On rebasing: “I hvae some really handy scripts to help work with series of branches rebased onto each other” — ChipX86
  • going with multiple review requests instead of having all web API work bunched into currently finished back end work.

andrewhong (Andrew Hong):

  • “:Remember to use our IRC nicks to prefix your messages with, that way, our IRC clients highlight your messages and we can pick them out at a glance when going through scrollback” — m_conley
  • Since the status report: cleaned up code for my feature, and submitted it for review. Fighting a peaceful war against ReviewBot. Did a few code reviews and probably some more later today.
  • can also get mentors attention by using “mentors” in chat.
  • Mailing lists (like reviewboard-dev) if can’t get in touch with anyone on IRC.
  • Questions as soon as work starts on having support for external files in downloading review request file attachments in zip format.

brennie (Barrett Rennie)

  • everything’s going well. was having some JS issues earlier in the week
  • on progress of diff expansion work: “All thats left is to make it behave like the diffviewer in that it will expand to either the whole file or up to the next method def and then make it pretty”
  • diff expansion WIP estimated to be out of WIP by Friday

dkus_ (David Kus)

  • In the project description, it recommends sub-classing BaseFileAttachmentResource. This resource is pretty heavily dependent on the file attachment being associated with a review request, which is not the case for user file attachments. Does it make more sense to just sub-class WebAPIResource instead?

    ChipX86: “1) make your new resource not based on this one. 2) rename BaseFileAttachmentResource to BaseReviewRequestFileAttachmentResource. 3) add a new BaseFileAttachmentResource that does contain the bare minimum (probbably ‘model’, ‘fields’ properties)”
    m_conley: “(3) in combination with (2), I would wager”
    ChipX86: “so I take it back, there’s a couple things to salvage from it, but the bulk is definitely tied to a review request”

  • question regarding permissions:

    m_conley: “For updating and deleting, we should make sure that it’s either the owner OR a superuser. Generally speaking, we give superusers powers over everything. So that’d be the only alteration I’d make. Viewing is a good question…”

  • ChipX86: something that’s non-obvious, because most people don’t work with it, is that you need to factor in the LocalSite. A LocalSite is a way to partition a Review Board instance into several virtual installs. It’s what we use for to give companies their own independent instances. You’ll want to ensure that, if a LocalSite is set on the attachment, the viewing user is a member of that LocalSite. A good source of inspiration for access controls is to look in the various is_accesible_by() and accessible() methods. But every model should really have an is_accessible_by() that would check these things, and a corresponding accessible() method on a Manager for that model. The API already has decorators applied to restrict access there though.

  • m_conley on LocalSite: basically, it’s possible to split up a single RB instance to seem like many RB instances. despite the fact that it’s still just a single RB instance. So, you can create many “RB sites” for a single installation, with different users in each site. They’re all partitioned off from one another but they all share the same database, RB code, etc. That’s not a thing that or your local instance really does but it’s something that rbcommons uses where rbcommons is the RB hosting service that ChipX86 and purple_cow run. A final thought – these attachments are inserted into comments which belong to reviews, which belong to review requests. So we probably want the logic of viewing the attachment to match the logic of viewing that review request.

  • “When deleting a file attachment resource from the database, should the actual file be deleted as well?” Yes.
  • “When testing the web API for creating the file attachment resource: After making a request with Basic Auth, and then removing the auth headers and making another request, the ‘request.user’ is still set to the last user that I made a request with, and so request.user.is_authorized() returns true even though I’m not providing any auth headers with the request. Is this supposed to be happening?” Cookie set for the session, must wipe it.

rdone (Ryan Done)

  • new paginator coming along nicely.
  • take another project off the student projects list when close to finishing or finished a previous project. No EasyFixes until end near end of term!
  • Check out general issues to tackle if no student projects are of interest. Contact mentors with any project ideas
  • a few issues overlooked for paginator: invalid input in url parameter and ensuring letter paginator still renders for querysets less than a single page size. No big show stoppers, might take a little longer to wrap up

mloyzer (Mark Loyzer)

  • PDF generator library recommandation from Django: ReportLab
  • put PDF mock document on hackpad or review request
  • “For testing, how do I add changes/diffs to Review Board on my localhost?”
  • m_conley: “one thing I like to do is add the Review Board (or, in your case, rb-extension-pack) Github repo to my local instance, and then use my current project as the patch that I test with, if that makes any sense. Alternatively, add the Review Board Github repo (or point it at your local git repo), and then apply a patch from with rbt patch and use that or add the repo, got to the New Review Request page, and just click a commit and it will post that (which should be fastest)” … “for fast results, add the Github repo for Review Board, then go to New Review Request as ChipX86 suggests, select the repo in the left-hand side, and choose an already-committed changeset to review.”

  • don’t write your own CSV library! plenty to choose from.

justy777 (Justin Maillet)

asalahi (Azad Salahi)

  • Hackpad, a useful artifact that mentors use for evaluation. Because even if the patches you end up producing don’t go anywhere, Hackpad is evidence that you tried things.
  • on creating various kinds of diffs to test changes: add rbtools repository to local devserver and then you can make branches with test changes and create diffs in your local tree, then try uploading those with rbtools

Pizza time!

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 )

Twitter picture

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

Facebook photo

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

Google+ photo

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

Connecting to %s