Author Archives: adrw.hong

Meeting Minutes: November 30, 2014


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


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



  •  <m_conley> Hello everybody! This is it – this is the last official meeting of the semester for UCOSP
    <m_conley> (yay – you made it! 🙂 )
    <m_conley> Before we go into the round-robin, I wanted to quickly talk about “what happens next”
    <m_conley> After Pencil’s Down, we write up your final evaluations
    <m_conley> your final evaluations from us are then sent to the UCOSP steering committee
    <m_conley> who then are responsible for sending those evaluations to your home faculty
    <m_conley> your final evaluations have more than just a numeric grade – we often write textual feedback in there
    <m_conley> stuff that we intend for you to see – so if you end up only getting a number from your home faculty
    <m_conley> pester them to get text bit too
    <m_conley> we’ve heard that some students in the past have only gotten the numeric grade
    <m_conley> so, just keep an eye out for that. I’m not sure what the timing is on the release of those grades – I think that’s up to your home faculty
    <m_conley> Once your evaluations are in, we hope you join us for the demos / post-mortem session, which I’ll send mail about sometime this week
    <m_conley> And then, well, after Pencil’s Down, your responsibilities for this course more or less cease
    <m_conley> HOWEVER
    <m_conley> we’d love…*love* for each and every one of you to continue contributing to Review Board
    <m_conley> you’ve all gained skills
    <m_conley> you all are now experienced in various sections of the codebase
    <m_conley> and some of you might have some final bits to do to push your project into a release state
    <m_conley> that’s totally fine, and we *encourage* you to continue hacking on Review Board
    <m_conley> and even if your project is put away, we can always hook you up with more work
    <m_conley> we’re going to have another wave (actually, waves) of students this upcoming semester
    <m_conley> and they’re going to be starting where you started from – from more or less zero
    <m_conley> and so giving guidance feedback to them is super useful, and something we encourage
    <m_conley> ChipX86 mentioned this last week, that we’re going to try an experiment
    <m_conley> where we transition from this IRC channel to Slack for communications
    <m_conley> so if you are interested in continuing to hack on RB, we can give you access to our Slack instance
    <m_conley> and you can hang out in the #alumni channel, and the #students channel to help out
    <m_conley> so if you’re interested in that, let us know, and we’ll hook that up
    <m_conley> I’ll also point out that you’re under no obligation to continue hacking on RB
    <m_conley> but if you like the project, we encourage it. 🙂
    <m_conley> I think that’s my shpeal
    <ChipX86> even if you’re not going to hack but want to keep in touch

Round Robin

brennie (Barret Rennie):

  • regarding open thread on commit history items:
    • <ChipX86> there’s a lot of feedback in there for many different scenarios
      <m_conley> nods
      <ChipX86> a v1.0 of this doesn’t need to implement it all, so long as we can move toward those things later
      <m_conley2> careful not to get bogged down by scope creep
      <m_conley2> something like this gets people excited
      <m_conley2> and they start asking for the moon
      <ChipX86> the majority of people will get benefit from just being able to post multiple commits
  • <brennie> right now I’m just trying to get the first parts that I mentioned in the last meeting working
    <m_conley2> brennie: ok. Anything we can help unblock you on?
    <brennie> But for some reason, the API endpoint doesn’t show up in the /api/ resource’s uri list
    <ChipX86> the uri_templates?
    <brennie> Yes
    <brennie> I *think* its supposed to show up there — but I’m not sure
    <ChipX86> I can help you debug that in a bit

dkus (David Kus):

  • <dkus_> Nothing new outside the status report. Continuing to look at reviews on my review requests. I’d also like access to the Slack instance so I can keep in touch & add any finishing touches to my project.
    <m_conley2> dkus_: thanks for responding to my feedback so quickly – those patches are looking pretty good
  • <ChipX86> if it’s a user-visible error, use alert() for now
    <ChipX86> it sucks, but it’s what we have
    <ChipX86> console.error will be silent on IE if the console wasn’t previously opened
  • <dkus_> If anyone else wants to review my changes go ahead. But I won’t have much time these next few days to work on this.

mloyzer (Mark Loyzer):

  • <mloyzer> cool, i don’t have anything new from my status report
  • <m_conley2> mloyzer: so you’re going to look at the browser devtools to see what might be eating your click event
    <mloyzer> besides that we talked briefly before the meeting, so i’ll try to debug the issue
  • <m_conley2> heapify / smacleod / purple_cow / ChipX86: Check out
  • <m_conley2> I wonder if perhaps that should be something that’s just included in the extension
    <ChipX86> I have thoughts
    <m_conley2> alright
    <m_conley2> I just feel a bit weird having something so specialized get added to the hook list
    <ChipX86> I think the handler for the action buttons is unintentinoally blocking any custom actions
    <ChipX86> we don’t want a custom hook for this though
    <m_conley2> ChipX86: oh, you’re talking about what’s eating the click event?
    <ChipX86> yeah
    <ChipX86> just realized this is different
    <mloyzer> has the ReviewRequestDropdownActionHook ever been used?
    <ChipX86> but no, we don’t want a custom hook. The reason we have get_actions() is so that subclasses can do things like this
    <m_conley2> ChipX86: so mloyzer should move his subclass into his extension?
    <ChipX86> mloyzer: once upon a time, but we haven’t done any real testing with it in a while
    <ChipX86> yes
    <m_conley2> boom
    <ChipX86> and should just return the entire result inline, rather than have an ‘actions =’ in the class
    <mloyzer> ah ok, so move it into my extension and debug then?
    <m_conley2> mloyzer: so no biggie, but just move the subclass into your extension somewhere so it ships with it instead of putting it into core.
    <ChipX86> yep
    <mloyzer> ya, makes sense
    <m_conley2> mloyzer: cool – then yeah, check out the event handlers on the dropdown to see if something’s eating it. I think purple_cow ran into something similar at the sprint with something he was working on…
    <m_conley2> I can’t remember how he resolved it
    <m_conley2> mloyzer: but he might get some feedback (he’s on the road right now, so his connection is spotty, but we can ask later)

nicole_xin (Nicole Xin):

  • <m_conley2> sounds like you’re ready for some real reviews o
    <m_conley2> nicole_xin: can do!
    <nicole_xin_> yes
    <m_conley2> nicole_xin: awesome – I’ll try to get some in tonight
    <nicole_xin_> everything is ready for reviews
    <nicole_xin_> oh thanks
    <m_conley2> oh awesome, yes
    <m_conley2> everything for you is out of WIP
    <m_conley2> so we’ll get reviews for you ASAP

rdone (Ryan Done):

  • <m_conley2> sounds like you’re also ready for some review on
    <m_conley2> awesome
    <rdone_> yeah
  • <rdone_> theres still a few changes I’d like to make
    <rdone_> but the core slider functionality is there
    <m_conley2> ” Is the appearance of the text attachment slider satisfactory (note the shadow)? If not, what can be done to improve it? CSS changes?”
    <m_conley2> right
    <m_conley2> yeah, I’m not the biggest fan of that shadow
    <ChipX86> agreed
    <m_conley2> can we just flat out remove it?
    <ChipX86> the slider needs to be toward the top too
    <m_conley2> right – above the diff?
    <ChipX86> yeah
    <m_conley2> I guess that makes sense – you don’t want to have to scroll down all the way to the bottom to change it
    <rdone_> right
    <ChipX86> I’d put it in the same visual box as the “test1.txt – test1”
    <ChipX86> maybe right above it, or below it, not sure
    <rdone_> I think there should also be a title to the right
    <rdone_> for the diff’d file
    <rdone_> “test2.txt – test2”
    <rdone_> but I haven’t gotten to that yet
    <ChipX86> ok
    <ChipX86> yeah, we should be as close as possible to the diff viewer for diffing
    <m_conley2> I’d put it above those two titles
    <rdone_> Ok
    <rdone_> at the moment its using this.$el.append(“”)
    <rdone_> and it sticks it at the bottom
    <rdone_> is there a a different call for above?
    <ChipX86> prepend
  • <m_conley2> ” I’m still unsure about how to manage swapping between “diff mode” and “view single file mode”. But I have a suggestion: Perhaps a button or control that allows the user to switch between 2 handles on the slider (for diffing) and 1 handle (for viewing each revision).”
    <m_conley2> I thought we resolved that last week, didn’t we?
    <rdone_> not really
    <rdone_> the response was unsure, so I figured an image or implementation would help
    <ChipX86> we should use what we have in the diff viewer
    <ChipX86> I thought we solved that with that but a different label suggested by purple_cow
    <ChipX86> (he’s on the road so he can’t weigh in)
    <ChipX86> it’s very important for users to be able to use this feature without learning something new
    <m_conley> nods
    <rdone_> what does the ‘orig’ tag in a file attachment history?
    <m_conley2> the more we can crib from the diff viewer, the better
    <rdone_> the context is sort of different from a code diff
    <ChipX86> it is different. purple_cow had a recommendation on it I think
    <rdone_> what does the orig tag *represent…
    <rdone_> sorry
    <ChipX86> maybe it was “new” or something
    <rdone_> purple_cow: I think “orig” is a really weird thing to say for file attachments, thought
    <ChipX86> we can figure out the label later. It’s the concept that matters
    <rdone_> Well I can add the label with a line or two
    <ChipX86> if a user goes in the diff viewer, uses hte slider, and learns how it works, then goes here and sees that the same slider for the same concept works differently, the user will grumble
    <m_conley> nods
    <m_conley2> same affordance should behave the same way
    <ChipX86> changing the label can be done by us down the road before release, if need be
    <ChipX86> so long as the behavior is identical
    <rdone_> What should happen when a file attachment review is first visited
    <rdone_> display 1 file?
    <ChipX86> yep
    <ChipX86> just as today
    <rdone_> and the slider is there with 2 handles, and they move on and get 2 files back
    <ChipX86> yep
    <rdone_> how do they ever return to single file mode
    <ChipX86> they click the label for the revision they want, or select a range from “orig” (or whatever) to the desired revision
    <ChipX86> identical to the diff viewer
    <rdone_> gotcha, so labels for selecting individual files
    <rdone_> and slider for selecting 2
    <ChipX86> it’s all part of the same slider
    <rdone_> yes
    <rdone_> in that case the ‘orig’ tag (or w/e its called) is purely cosmetic
    <ChipX86> yep
    <rdone_> to identify the first or last revision
    <m_conley2> cosmetic, but also familiar
    <ChipX86> just the one label on the left, before “1”
    <m_conley2> rdone_: make sense?
    <rdone_> would it still point to the first revision
    <rdone_> and then ‘1’ would point to the next one
    <ChipX86> “orig” doesn’t point to a revision
    <rdone_> err
    <ChipX86> it points to a state before revisions
    <rdone_> original file attachment
    <ChipX86> well, “orig” needs to be renamed later, but it’s the conept
    <rdone_> ok
    <ChipX86> it’s “none”
    <ChipX86> if I have a range from the “orig/none” to “5”, then I’m viewing revision 5
    <ChipX86> clicking the “5′ label puts it into that state as well
    <rdone_> ok
    <rdone_> yeah that makes sense
    <ChipX86> there are definitely things to iron out here, but it must be done along with changes to the diff viewer
    <ChipX86> and that’s on our plate 🙂
  • <m_conley2> “ChipX86 mentioned last meeting that we needed to make sure my code would not impact the diff viewer (image review ui in the diff viewer I believe), but I haven’t had a chance to look into this at all….”
    <m_conley2> rdone_: I guess that’ll get sorted out during review.
    <rdone_> ok

asalahli (Azad Salahli):

  • <asalahli> So, I was going to ask for more reviews, but that got solved a few minutes ago 😀
    <m_conley2> asalahli: heya – yeah, I just saw that myself. 🙂
    <m_conley2> asalahli: any questions about what you’re seeing in that review?
    <asalahli> 2 things
    <asalahli> 1 is just a grammar issue
    <m_conley2> asalahli: go ahead
    <asalahli> don’t we “land something on something else”?
    <asalahli> in this case, land changes on a repository/branch
    <asalahli> or to a repository/branch
    <ChipX86> the first comment?
    <asalahli> I’m talking about the first review here
    <asalahli> here
    <asalahli> yeah
    <ChipX86> ”
    <ChipX86> may just be me
    <asalahli> oh I see the point now
    <asalahli> should I change it?
    <m_conley2> you could try “Land changes from a review request onto the remote respository”
    <ChipX86> there we go
    <m_conley2> best of both worlds
  • <asalahli> And second thing
    <m_conley2> asalahli: what’s the next question?
    <m_conley2> go ahead
    <asalahli> can we decide on option names for source and destination branches?
    <asalahli> thats the only thing blocking me right now
    <m_conley2> what’s wrong with –source-branch and –destination-branch ?
    <ChipX86> I don’t recall exactly, but I thought last week there was a suggestion to have the local branch be the first argument?
    <asalahli> ChipX86: yeah purple_cow suggested that
    <ChipX86> the destination branch will largely be in .reviewboardrc, so if the command line option is a little longer, not a big deal. It won’t be typed as often
    <asalahli> but I had a question and we couldn’t discuss it
    <ChipX86> ok
    <m_conley2> asalahli: what’s the question?
    <asalahli> m_conley2: –source-branch will be typed pretty often, so I thought its a bit long
    <m_conley2> well
    <m_conley2> often those arguments have single char counterparts
    <asalahli> Ok. so purple_cow’s suggestion:
    <m_conley2> –source-branch == -S
    <asalahli> ‘
    <asalahli> ‘
    <asalahli> two different types of usage
    <asalahli> but my concern is that, there are times when a user needs to specify both rr-id and src-branch
    <asalahli> btw, I like m_conley2’s suggestion
    <ChipX86> ‘
    <ChipX86> keeps it consistent with tools like git and with post-review
    <ChipX86> er
    <ChipX86> rbt post 😛
    <m_conley2> ah
    <m_conley2> yep
    <ChipX86> with rbt post, you specify an ID with -r
    <m_conley2> consistency is good – especially for grouped tools like this
    <ChipX86> and you can specify branches to post with a positional arg
    <ChipX86> git merge, etc. also use positional args
    <asalahli> and to land current branch just type ‘rbt land -r <id>’?
    <ChipX86> yeah
    <asalahli> Cool
  • <m_conley2> asalahli: any other questions
    <m_conley2> ?
    <asalahli> should I name the other one -D, –destination-branch ?
    <ChipX86> I don’t know that we need short-hand for this one
    <ChipX86> if we do, I think –dest or –dest-branch is fine
    <asalahli> I like –dest more
    <m_conley2> do it
  • <m_conley2> asalahli: anything else?
    <asalahli> Ok
    <asalahli> Thats it. I was supposed to implement more backends, but didnt have time this week. Apologies for that 😦
    <m_conley2> asalahli: that’s alright – perhaps you can do it after the course is over. 🙂
    <asalahli> that was my initial plan 🙂
    <asalahli> thats it from me today

andrewhong (Andrew Hong):

  • <andrewhong> Hello! No updates from me since the status update, still awaiting reviews though. I would like to get on the Slack list as well when it becomes available 🙂
    <m_conley2> ok
    <m_conley2> so you’re blocked on reviews
    <ChipX86> going through them today
    <m_conley2>> andrewhong: cool, thanks. 🙂
    <andrewhong> yeah, the tarball feature was still wip until this week
    <m_conley2> excellent
    <m_conley2> glad you got it out of WIP – good job.
    <andrewhong> so both should be good to go now 🙂
    <andrewhong> thanks!

justy777 (Justin Maillet):

  • <m_conley2> great job on all of those PEP8 issues
  • <m_conley2> you ask how to build the docs
    <justy777> indedd
    <m_conley2> smacleod / heapify / ChipX86: do any of you remember that command?
    <m_conley2> I’ve honestly never done it, but I know there is one.
    <ChipX86> ye
    <ChipX86> yep
    <ChipX86> just go into the docs and type “make html”
    <m_conley2> right – there seems to be a Makefile for each folder under docs
    <ChipX86> you’ll need Sphinx if you don’t have it
    <ChipX86> easy_install Sphinx
    <ChipX86> I think our Get Started guide has it
    <justy777> simple, I like it
    <ChipX86> er, has the list of things needed
  • <justy777> Does anything in reviewboard call the webAPI or is it something for third parties?
    <ChipX86> we do
    <ChipX86> we use it for most operations in the web UI
    <m_conley2> justy777: a bunch of the front-end uses it, and so do the client tools
    <m_conley2> it’s used pretty extensively
    <justy777> okay, does anything grab the server info?
    <ChipX86> RBTools does

Last but not least… 😦

<m_conley2> alright folks
<m_conley2> well, that’s… that’s it I guess. Stay tuned for my email about the demos and post-mortem
<m_conley2> but officially speaking
<m_conley2> this is it, except for the interactions we have on IRC, mailing lists, or RB until Wednesday
<m_conley2> and well, any interactions we have with you on the project afterward. 🙂
<m_conley2> Thank you so so so much for all of your hard work this semester
<m_conley2> keep pushing through to this Wednesday, and make sure you get those reviews
<m_conley2> You were great! *applause*
<ChipX86> yay!
<heapify> yup, thanks everyone – great job this semester! 😀
<smacleod> Thanks everyone 😀
<ChipX86> seriously guys, this was a good semester
<mloyzer> Thanks guys
<andrewhong> thank you all! 🙂
<brennie> 🙂 thank you very much
<nicole_xin_> \o/
<asalahli> good? this was awesome 🙂
<ChipX86> 🙂
<asalahli> Thanks everybody 🙂
<ChipX86> (was telling my girlfriend yesterday how sad I am that it’s over already)
<justy777> Group Hug?
* asalahli hugs
<m_conley> hugs
* ChipX86 hugs
<m_conley2> woo!
<m_conley2> IRC hug
<justy777> \me hugs
* justy777 hugs
* smacleod strains to wrap his arms around the large group hug
<m_conley2> hehehe
<m_conley2> also
* brennie hug
<m_conley2> uh, I guess we’ll all see you in Slack too. 🙂
<m_conley2> Because basically everybody requested accounts
<m_conley2> so, see you there. 🙂


Status reports for October 31, 2014

Andrew Hong

What project are you working on?
Downloading Review Request Attachments as zip
Aggressive autocomplete in groups/people field.
Support for matching commits to review requests to more commands

What you accomplished this week? 
Been all over the place this week and had internet issues for the past few days… router is dead 😦 :
Downloading Review Request Attachments as zip: looking into how I can read chunks from files then write that chunk into the zip file. No obvious solution, probably need to write a wrapper for the zip file to make it work. Any direction this would be greatly appreciated! Googling didn’t turn up much for me.
Aggressive autocomplete in groups/people field: testing it again according to issues reported from users, it appears to working except in the case where the input matches in the middle of the string so it would not autocomplete. Looking into a solution that won’t break other cases.
Support for matching commits to review requests to more commands: got everything set up, like testing environment and how to test changes. Found appropriate section of code to change so looking into how to implement it best. Might be best to take the code that ‘guesses’ the review request into the Command class in RBTools.
Links to anything you’ve done this week
Sorry, it is mostly on scrap paper right now. Some things updated on my developement log: you plan to do next week? 

Focus on the zip feature and autocomplete; hopefully wrap them up, especially autocomplete.
What, if anything, is blocking you from making progress?
Anything regarding the zip feature that might help me would be great, or maybe an alternative approach that will also work?Any other questions?

Ryan Done
What project are you working on? 
Alphabetic paginator
File upload diff slider

What you accomplished this week? 
When doing some initial research for the diff slider, I happened upon a bug where users were unable to update a file upload in their review request, blocking any work I could do. After a bit of research into the bug, David Trowbridge recognized the problem and fixed it. His fix is here:
After submitting my review requests for the alphabetic paginator, Christian had some pretty helpful suggestions on how to improve the paginator . I spent most of my time this week addressing his review suggestions.
I created an AlphanumericDataGrid subclass that is now more generally usable by any other datagrid class that wants an alphanumeric datagrid. 
I also cleaned up the templates for the paginator a bit, and simplified the amount of context information being passed around from from the datagrid to paginator template tags (there is now just a simple, over-ridable render_paginator function, as opposed to custom template tags). 
Lastly, I moved this filtering of the queryset over to the AlphanumericDataGrid class in djblets to be performed dynamically. The “filter by” column is now a parameter passed in by the calling subclass.
Links to anything you’ve done this week 
What you plan to do next week?
If there is any more to do on the paginator update, I don’t foresee it being taking too much more time, so I’d like to really dig into the paginator stuff and get a proper review request up. 
What, if anything, is blocking you from making progress? 
Any other questions? 
All good for now. 

Barrett Rennie
What project are you working on?

This past week I’ve been working on the API caching and fixing up the alias support in rbtools.

What you accomplished this week?

I believe I’ve handled all the edge cases in rbtools alias support and API caching. Both are out of WIP now.

Links to anything you’ve done this week?

What you plan to do next week?

First off I want to get a issue with diff comment fragments fixed — they appear in the Edit This Review dialog but do not function. Then I’m going to be working on making ReviewBoard and rbtools support pushing/receiving DVCS history along with the patches.

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


Any other questions?

David Kus
What project are you working on?
Drag ‘n Drop inline images into the markdown editor
What you accomplished this week?
Worked on fixing the issues pointed out in the review request for the backend work I finished up last week.
Had to change quite a lot so I didn’t have time to work on the frontend.
Links to anything you’ve done this week.
What you plan to do next week?
Hopefully close off the backend work (once my changes are reviewed), and then get back to working on the frontend.
What, if anything, is blocking you from making progress?
Any other questions?

Azad Salahli
What project are you working on?
`rbt land` for landing changes.
What you accomplished this week?
I haven’t been able to do much this week, I’m sorry. I had midterms and an assignment. However, I’ve discussed how `rbt land` should be designed with Christian. There is a hackpad page related to that discussion (link in next section).
Links to anything you’ve done this week

What you plan to do next week?

Getting a working version ready with at least Git support.
What, if anything, is blocking you from making progress?

Any other questions?

Mark Loyzer
What project are you working on?
Adding an extension to Review Board that will allow people to export a review request as PDF and XML.
What you accomplished this week?
Figured out how to create ordered and unordered lists using reportlab which took too long (for ordered list) because of the lack of documentation.
Finished implementing most of the code for PDF (everything but inserting files, diff/change sets, and the full bodies of each review).
Links to anything you’ve done this week
What you plan to do next week?
Finish off PDF:
  Tidy up Header (still need to add the review request id to it).
  Get all files and change sets into the document.
Once this is done, I can start working on XML
What, if anything, is blocking you from making progress?
Do you have any tips on how to input the uploaded files or diff/change sets into the PDF?
What do the Mimetypes do?  Could I leverage that?
Any other questions?

Yanjia Xin
What project are you working on?
Same as last week, general comments
What you accomplished this week?
1. Create a new review request about some documentation changes on other comments (already pushed)
2. Improve general comment backend model
3. Create a general comment review box view
Links to anything you’ve done this week.
1. Documentation changes:
2. General comment backend model:
3. General comment front-end:
What you plan to do next week?
1. Improve general comment backend model and web-api (I already saw lots of comments on web-api)
2. Find a way to create a general comment (Currently my plan is using ReviewDialogView. If “open an issue”, we create a general comment; otherwise it’s a body-top.)
3. Modify the email content of the general comment (ChipX: I will put the email on the review request once I’ve done that)
4. Add tests, hopefully
What, if anything, is blocking you from making progress?
Not really, front-end is a little bit messy so I spend a lot of time poking around and play with that.
Any other questions?
I’d like to see if mentors have any ideas on how we let users to create a general comment. 🙂

Justin Maillet
What project are you working on?
I’m working on sandboxing extensions.
What you accomplished this week?
I’ve taken my patch for sandboxing Djblets Column subclasses out of WIP.
I Finished two more patches, one for sandboxing Reviewboard MimetypeHandler and the other for sandboxing Djblets TemplateHook applies_to() function.
I’ve started digging into the Djblets SignalHook class, Reviewboard ReviewUI class, and related files.
Additionally, I’ve made improvements to my patch sandboxing Reviewboard AuthBackend subclasses.
Links to anything you’ve done this week
What you plan to do next week?
This coming week, I plan to re-write the webapi tests for AuthBackend, finished the last two patches in the project, make improvements from comments in code reviews, and hopefully start on a new project.
What, if anything, is blocking you from making progress?
Nothing is blocking my progress.
Any other questions?
Is there documentation on the WebAPI testing framework in reviewboard? or can someone give me a small explanation to how a specific function gets called in WebAPI tests?

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!