Category Archives: Meeting Minutes

Meeting Minutes: November 30, 2014

Mentors

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

Students

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

Announcements

m_conley

  • ¬†<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 https://reviews.reviewboard.org/r/6633/
  • <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 https://reviews.reviewboard.org/r/6623/
    <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 https://reviews.reviewboard.org/r/6591/
    <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?”
    <rdone_> https://reviews.reviewboard.org/r/6591/file/703/
    <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 https://reviews.reviewboard.org/r/6509/#comment19122
    <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. ūüôā

Meeting Minutes: Nov 23, 2014

Mentors

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

Students

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

Announcements

m_conley

  • ¬†next weekend is technically our last meeting
  • So remember, you want to give us a few days to review your code
  • ¬†so you’re trying to get your patches out of WIP¬†and landed – aim to have that done a number of days – maybe 3 or 4 – before pencils down
  • to make sure we can get some review iterations in there
  • ¬†I think I mentioned this before – your patches, despite getting ship-its, or getting into a landable state¬†might not immediately land on master
  • ¬†they might go into a project branch
  • don’t worry about that
  • ChipX86:¬†important thing is that we get it into the codebase in some form
  • ChipX86:¬†a lot of that is largely about release scheduling
  • after Pencil’s Down, our job is to produce an evaluation for your work this semester
  • ¬†strictly speaking, after Pencil’s Down, the meetings are done
  • ¬†however;¬†we like to do this totally optional thing;
  • you’ve all been having your nose to the grindstone this semester
  • ¬†and you each have been working on some pretty neat stuff
  • so we like to do demo sessions, where you can show off what you’ve done
  • they’re not long – 2 minutes or so, just showing how RB has improved with your changes
  • we also use that as an opportunity, after the demos, for you to give us feedback about the course
  • We like to know where we should improve for the next set of students – so if you have suggestions for us, or feedback, that’s a great time to deliver it
  • we like to hold the demos / post-mortem (that’s what we call the feedback session) one right after the other, and usually do it not long after the evaluations go in
  • and that way, you’re free to say whatever you’d like, with no risk to your evaluation¬†(not that we’d shoot down your evaluation for giving us criticism)
  • ¬†but still, we feel that gives you students some ease-of-mind when giving us feedback.
  • ¬†Anyhow, I’ll send out mail over the next little while with a timeline
  • ¬†I should stress that the demos / post-mortem are totally optional,¬†¬†but they’re also totally cool.
  • Next week, I’ll likely launch into a talk about how to keep staying involved in the project even after the course ends (which is totally awesome to do and we encourage)
  • but I think I’ve taken up enough of this meeting time
  • have we mentioned that pencil’s down is 12/3?
  • andrewhong:¬†will we be able to work on the ‘student’ projects after the term?
  • potentially, yes – though we might have other work for you to do as well if you’d like to stay involved. The possibilities are endless. ūüôā
  • ChipX86: many of you are in a phase where you may be doing documentation work
  • ChipX86:¬†Python doc strings, API docs, actual documentation files
  • ChipX86:¬†and you may be wondering, how do I write good docs? Doc writing isn’t often something you think about when doing software development
  • ChipX86:¬†this came up last night, and I’ll repeat the advice. If you’re stuck on the doc writing, and aren’t sure how to actually write it, pick up a rubber duck or find someone and explain what you’re doing to them. Explain it a few times. Then write that down. You’ll have a great start

brennie

  • m_conley: brennie: your django-evolution problem – do we have a forward path on that?
  • brennie:¬†not really — i dont really know anything about how evolution works
  • ChipX86: brennie: was this the rel_field_name issue?¬†or did something else come up?
  • brennie:¬†yeah
  • ChipX86:¬†django_evolution doesn’t need to be modified. RelationCounterField in djblets should just be updated to have rel_field_name default to None
  • m_conley:¬†brennie: your other question was: “When it comes time to add support for rbtools should it require a flag (e.g. ‚Äďwith-history or -h or similar) to create the review? What should the flag be?”
  • brennie:¬†yeah im not quit sure what a good name would be that wouldnt be too long to type;¬†¬†–multi-commit-review-request is a bit long
  • m_conley:¬†brennie: it can be easily changed later, so I wouldn’t sweat about it too much
  • brennie:¬†I just want to make sure the api endpoint for /api/review-requests is reviewboard/webapi/resources/review_request.py
  • m_conley:¬†yep,¬†should be

dkus

  • m_conley:¬†dkus_: glad to see the backend land! congrats!¬†at least, the form bits, anyhow
  • dkus_:¬†ChipX86: I replied to some of your comments on https://reviews.reviewboard.org/r/6617/
  • ChipX86: I’ll take a look
  • m_conley:¬†dkus_: so, beyond reviews are you blocked on anything?
  • dkus_:¬†Nope
  • m_conley:¬†dkus_: ok, great – we’ll move along then. Thanks. ūüôā

mloyzer 

  • mloyzer_:¬†m_conley, i just have my 2 questions
  • mloyzer_:¬†as in the ones in the status report
  • m_conley:¬†mloyzer_: I can work with you on the ActionHook one after the meeting if you’d like – at least, I can explain how ActionHooks work
  • mloyzer_:¬†ok
  • m_conley:¬†and that should help you figure out how ChipX86’s solution applies
  • mloyzer_:¬†thats great thanks
  • m_conley:¬†‚ÄúWarning your working directory is not clean. Any changes which have not been commited to a branch will not be included in your review request.‚ÄĚ
  • m_conley:¬†hrm
  • m_conley:¬†mloyzer_: when you say it doesn’t upload the correct diffs… what diffs *does* it upload?
  • mloyzer_:¬†it uploads some docstring changeset
  • m_conley:¬†mloyzer_: do you recognize that changeset?
  • mloyzer_:¬†somethings i’ve never sen before
  • m_conley:¬†like, is it one you’ve made?
  • mloyzer_:¬†no
  • purple_cow:¬†it sounds like something is funky with your git branches
  • m_conley: I agree
  • mloyzer_:¬†so do you have any idea how to remedy it or?
  • m_conley:¬†mloyzer_: can you stick around after the meeting? We’ll sort out your branches

nicole_xin

  • nicole_xin:¬†hi,¬†i‚Äôm good i think
  • m_conley:¬†nicole_xin: ok, no blockers?
  • nicole_xin:¬†nope
  • m_conley:¬†awesome, thanks
  • nicole_xin:¬†thanks

rdone

  • m_conley:¬†heapify / purple_cow / ChipX86: any thoughts on that first one?
  • m_conley:¬†“How should I communicate back to the server when the slider is moved?”
  • purple_cow:¬†for the first one, I think just loading a new URL is okay for now
  • rdone_:¬†So all of that router logic would need to be added to the Text/Image view?
  • ChipX86:¬†I would say, make it so that the slider itself is just emitting events when you change it
  • ChipX86:¬†maybe the default is to reload the page with new query parameters or something
  • ChipX86:¬†but a review UI’s javascript code should be able to just handle that, fetch the file attachments over the API, and re-render without a reload
  • purple_cow:¬†I think given the time we have left, a reload is fine
  • rdone_:¬†The event callback for a Review UI’s code just calls the router to adjust the URL, but I don’t think it does a complete reload
  • rdone_:¬†Just the diff content,¬†errr for the revuew UI’s slider
  • m_conley:¬†Given the time left, what’s practical for rdone_ to get to?
  • ChipX86:¬†rdone_: can you make the slider itself just send events, then have the review UI list and set up handlers that can be overridden, and have the handler just do the reload?
  • ChipX86:¬†that’ll give us the flexibility we need
  • ChipX86:¬†you won’t use the Router for this case
  • ChipX86: we’ll figure out how to integrate that down the road
  • rdone_:¬†ok,¬†so it will do a hard reload,¬†in that case: I need to specify url parameters
  • m_conley:¬†rdone_: does that clear up that question?
  • rdone_:¬†yeah
  • m_conley:¬†next question was:
  • m_conley:¬†“How should image diffs be handled?”
  • m_conley:¬†ChipX86: is this the onion-skinning thing?
  • ChipX86:¬†yea
  • ChipX86: so the image diffs will just benefit from the reload currently
  • ChipX86:¬†it was originally written for the diff viewer, btu we didn’t get all the pieces in. Works, though
  • rdone_:¬†So I can use that for image diffs?
  • ChipX86: yep
  • m_conley:¬†ok, last question:¬†¬†“How might the user opt to turn off a file diff and just look at 1 instance of a file?”
  • rdone_:¬†More of a UI design question
  • ChipX86:¬†the diff viewer’s slider has a special value on the left: “orig
  • ChipX86:¬†viewing a single revision means having the slider point to that and the revision
  • purple_cow:¬†I think “orig” is a really weird thing to say for file attachments, thought
  • ChipX86:¬†it is,¬†but maybe the concept still works? I don’t know
    m_conley:¬†let’s get it there and see
  • ChipX86: I don’t have any great ideas for that
  • rdone_:¬†I figured you would never really need to do a single file view with an RR diff
  • purple_cow:¬†maybe we can label it “no diff” instead
  • rdone_:¬†but you might for a file attachment
  • ChipX86:¬†right
  • ChipX86:¬†btw, the slider shouldn’t appaer in the diff viewer’s view of a file attachment,¬†since that’s controlled by the diff viewer’s slider
  • rdone_¬†I’m not sure I follow
  • ChipX86:¬†the review UI for, say, images in the diff viewer (which is what image diffing was originally built for) should not have its own revision slider
  • ChipX86:¬†I need to dig into your code more to see how it’s used
  • purple_cow:¬†images in the diff viewer isn’t a publicly available feature yet
  • purple_cow:¬†but a lot of the code is there
  • m_conley:¬†rdone_: does that answer the question, or is this still unclear?
  • rdone_:¬†The imageReviewableView is a child class of FileAttachmentReviewableView
  • rdone_¬†so wouldn’t that be ok?
  • ChipX86:¬†that’s not the part that’s an issue. Basically, these diffable review UIs can be displayed now in two places:
  • ChipX86:¬†1) the diff viewer (currently requires some manual database editing)
  • ChipX86: 2) when viewing a file attachmetn on a review request
  • ChipX86: we only want the revision slider to appear on the review UI for #2
  • ChipX86¬†there’s a few easy ways we can tackle that
  • ChipX86: I’ll talk to you later about how to set up #1 to see it

asalahli

  • m_conley:¬†asalahli: you’re next
  • m_conley:¬†asalahli: great job on getting rbt land out of WIP
  • m_conley:¬†asalahli: what SCM were you thinking next?
  • asalahli:¬†So, nothing much from me, except please review it as much as possible ūüôā
  • m_conley:¬†right, yep
  • asalahli: I was thinking of either svn or mercurial, but I realized I need to write a documentation as well. So I’m thinking thats more important,¬†right?
  • m_conley¬†asalahli: documentation would be grand, yes – but I don’t see that taking up the rest of the course
  • m_conley: so yes, please write the documentation
  • asalahli: agreed
  • m_conley: asalahli: but I agree, svn or hg support would be nice – at least, to get the ball rolling on it
  • purple_cow:¬†in terms of demographics, I think svn is more popular than hg, but both are good
  • asalahli:¬†I think hg will take less time since its very similar to svn
  • ChipX86: Mozilla may strongly encourage hg ūüėČ
  • asalahli: but, I can try either one
  • m_conley:¬†it’s true, we’re biased
  • ChipX86:¬†I’d go with hg just because it’s a lot more similar to how git works than svn
  • m_conley:¬†dooooo it
  • m_conley¬†(there’s your bias right there)
  • ChipX86:¬†svn might be really easy though, so it’s possible that if you have more time, you could quickly knock that out
  • m_conley:¬†both would be incredible
  • asalahli: I’ll try to do both before pencils down ūüôā
  • m_conley:¬†asalahli: that’d be awesome
  • asalahli:¬†thats it from me ūüôā
  • m_conley:¬†asalahli: but prioritize the documentation, then the other two
  • ChipX86:¬†and of course, you’re more than welcome to stay on after the semester and tackle the rest ūüôā

andrewhong

  • andrewhong:¬†hello ūüôā
  • m_conley:¬†“I‚Äôm wondering how I can pull in file attributes like last modified, date created, etc. from externally (S3) stored files?”
  • m_conley:¬†so, I found this:¬†https://django-storages.readthedocs.org/en/latest/backends/amazon-S3.html
  • m_conley:¬†and I’ve kinda been flipping back and forth to it during this meeting
  • m_conley:¬†trying to find a way to stat a stored file
  • ChipX86:¬†so I can kind of answer this
  • ChipX86:¬†or part of it
  • ChipX86:¬†S3, while a file storage provider, doesn’t have a lot of filesystem concepts
  • ChipX86:¬†there are timestamps, btu I think just one. There are no attributes in the sense that we care about
  • ChipX86:¬†you can access the file’s storage through the File instanec’s storage attribute (iirc)
  • ChipX86:¬†that has some functions like created_time() and modified_time()
  • andrewhong: hmm.¬†i dont recall seeing that but ill dig at it again
  • andrewhong:¬†there is a lot to look through heh
  • m_conley:¬†yep
  • ChipX86:¬†for attributes, just hard-code to 644
  • andrewhong:¬†alright
  • m_conley:¬†andrewhong: other questions?
  • andrewhong:¬†are there any other attributes that are important?
  • ChipX86:¬†I don’t think S3 has created_time(), just modified_time()
  • ChipX86:¬†basically, you can’t assume a certain backend
  • ChipX86:¬†so you’ll need to try/except these
  • ChipX86:¬†they’ll raise a NotImplementedError if the backend doesn’t support them
  • andrewhong:¬†right
  • ChipX86:¬†I don’t think there are other attributes that matter
  • ChipX86:¬†size, probably:¬†(file.size)
  • andrewhong:¬†ya ill take a second look later on
  • m_conley:¬†andrewhong: anything else?
  • andrewhong:¬†i could use some reviews whenever possible ūüôā
  • m_conley:¬†andrewhong: will get to those

justy777

  • m_conley:¬†justy777: so you’re finishing off these hooks – that’s good
  • m_conley:¬†justy777: you’ve got a few weeks left – I was combing through the issue tracker for things you could tack on towards the end
  • ChipX86:¬†I had a thought for one thing
  • ChipX86:¬†wouldn’t take up all the remaining time
  • ChipX86:¬†m_conley: which did you find?
  • m_conley:¬†https://code.google.com/p/reviewboard/issues/detail?id=3613&sort=-id&colspec=ID%20Type%20Status%20Priority%20Component%20Owner%20Summary%20Milestone
  • m_conley:¬†justy777: thoughts on that?
  • ChipX86:¬†that looks to have a lot of complicatedness around it involving Python compatibility, from the Python bug report
  • m_conley:¬†hrm
  • ChipX86:¬†well, actually, the last post on the Python bug sounds like it’s a solution
  • ChipX86:¬†so sure, I’m for that
  • m_conley:¬†justy777: sound ok?
  • justy777:¬†sure, I‚Äôll do my best
  • justy777:¬†I‚Äôm also sandboxing those hook i just made
  • m_conley:¬†justy777: that all sounds good – thanks for leaning into that stuff.
  • m_conley:¬†justy777: any questions?
  • ChipX86:¬†another thought on possibly tasks
  • m_conley:¬†ChipX86: go ahead
  • ChipX86:¬†justy777: we talked about the HostingServiceHook sandboxing, and I mentioned that several of those functions are allowed to throw exceptions
  • ChipX86:¬†however, that’s super non-obvious without reading the code
  • justy777:¬†Where is the code for reviewbot? and/or what are the settings on pep8?
  • ChipX86:¬†maybe you could update the docstrings to indicate what exceptions are raised under what circumstances
  • ChipX86:¬†we have customized pep8 settings in our install
  • m_conley:¬†ReviewBot is here: https://github.com/reviewboard/ReviewBot
  • m_conley:¬†justy777: other questions?
  • justy777:¬†are those customized pep8 settings in a file I have access to?
  • m_conley:¬†justy777: I believe the ReviewBot extension on the r.rb.org instance communicates those settings to ReviewBot
  • m_conley: hrm,¬†I thought that was the case;¬†heapify: –^?
  • ChipX86: it’s stored in our DB;¬†we just pass a string to send along to the pep8 tool
  • ChipX86:¬†what did you need them for?
  • ChipX86:¬†actually, let’s finish the meeting and go into htat after ūüôā
  • justy777:¬†okay
  • m_conley:¬†awesome,¬†justy777: anything else?
  • heapify: (yeah, the PEP8 tool has some settings for which errors to ignore, etc.)
  • justy777:¬†that is what i was looking for. None right now

Afterwards

  • ChipX86:¬†next weekend we’re going to talk more about what to do if you want to stay on the project past the semester
  • ChipX86:¬†for next semester, we’re going to be trying a replacement for IRC for the meetings
  • ChipX86:¬†if any of you are already thinking of staying on, or even just want an easy avenue to keep in touch with fellow students/us, we’d like to invite you to the new replacment
  • m_conley:¬†(It’s Slack btw
  • ChipX86:¬†yeah
  • ChipX86:¬†it’s like IRC but with a bunch of modern enhancements, easily accessible from mobile devices and desktop
  • ChipX86:¬†so let us know
  • ChipX86:¬†we have an #alumni channel in it
  • ChipX86:¬†that’s it!

Meeting Minutes: November 16, 2014

Attendees

Mentors

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

Students

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

Announcements

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

brennie

  • 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/models.py
  • ChipX86: It’s in reviewboard/reviews/models/
  • purple_cow: There’s a models/ directory
  • m_conley: models.py got too big so it got split up a while back

dkus

  • 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: https://reviews.reviewboard.org/r/6510/
  • 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

  • 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.

andrewhong

  • 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.

rdone

  • 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

  • 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

justy777

  • 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

Meeting Minutes: Nov 9, 2014

Mentors:

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

Students:

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

Announcements

heapify:
so there are 3 meetings left before pencils down, just keep that in mind and try to plan around that.

purple_cow:

as we head into the home stretch, if you need review on something, please let us know.¬†don’t wait until the meeting!

Round Robin

rdone (Ryan Done)

  • paginator review requests have been there for about a week,¬†wondering if theres anything more to do there.c
    • heapify:¬†we’ll be sure to get a review done this week

andrewhong (Andrew Hong)

  • I’m looking at the autocomplete for Groups and People again and wondering how it should behave if the user inputs something that is not the start of the string. When I type “Smith” it brings up results with all users that have the last name (in People) “Smith”. But in Groups, I have a group called “Cat Smith” and no results come up if I type “Smith” into the field. It would only show when “Cat” is typed. Should it be left the way it is or should we make the functionality in those two fields similar?
    • purple_cow: I think left the way it is,¬†people are a special case because they mostly have three¬†names.
    • ChipX86: Sounds like there were issues with the tarball generation. The secnod version of the snippet I posted was tested with a 860MB file, compressed. Can you put the integrated version up?
      • andrewhong:¬†yeah, i tried it with a pdf file of 8MB and it didnt fully write all the bytes.
      • ChipX86:¬†was this the version I had or the integrated version?
      • andrewhong: Revision 2.¬†I haven’t integrated it yet, i was testing the snippet and getting familiar with it. Need¬†to explore this more later.
  • other than that, i made a quick note about the rbtools project that I started and might not be able to go through with it all.¬†I have made notes on what i’ve seen in the code and how i planned on implementing it so i can put those up. I will look at it again if there is time.

mloyzer (Mark Loyzer)

  • For Change Descriptions, is there anyway to properly display the changes made?¬† Right now I‚Äôm using a dictionary to map that maps ‚Äėplus‚Äô to the attribute that represents what was added (either ‚Äėadded‚Äô or new‚Äô).¬† But even then sometimes the value is an array or an array of arrays.¬†I know that is extremely vague, so, in summary, I am trying to display the changes made to the Review Request through the ChangeDescription‚Äôs ‚Äėfields_changed‚Äô attribute,¬† Do you suggest any way to properly print the information in this dictionary?
    • ChipX86:¬†so the JSON blob can really hold anything, and it depends on the field (you’ll know that the ReviewRequestField instances may choose how to store data).¬†that said, there are some common patterns.¬†so changedescs/models.py, record_field_change,¬†you’ll see the default behavior for storing field data.¬†if the data naturally fits into a list (list of bugs, reviewers, etc), you’ll have a dictionary with ‘old’ (the complete old list), ‘new’ (complete new list), ‘added’ (which items were added), and ‘removed’ (which were removed).if it’s smoething that’s not a list, like the Description field, you’ll just get ‘old’ and ‘new’, with the value as 1 item in a tuple.¬†the trick is though that the value inside is an encoded version of the content, so it may be a tuple containing additional information¬†so it will depend on the field.
    • ChipX86:¬†you’ll need to handle it per-field.¬†every field may have a different way of storing the encoded content.¬†if there was a clean way of taking HTML adn converting to PDF, you could use existing APIs to do the rendering,¬†which would work with extensions as well.¬†that is, custom fields provided by extensions.
  • in regard to my review request,¬†when i finish PDF can i create a new one for XML,¬†just so i can somewhat develop in parallel (or at least more organized)?
    • approved by heapify and ChipX86

justy777 (Justin Maillet)

  • waiting for a bugfix to go in before finishing up sandboxing, a few questions¬†on the new hook extensions:
    • How does reviewboard currently find/get SCMTools?
      • ChipX86:¬†right now, they’re all registered in the database.¬†SCMTool.objects.all() will return them all.¬†that’s not ideal. I’d like to eventually make it more like mimetypes, review UIs, etc.
      • justify777: so for now, just pull the defaults out of the database or put third party SCMTools into the database?
      • ChipX86:¬†well, right now I don’t think we can cleanly do third-party SCMTools through extension hooks.¬†there is a mechanism in place today for third-party registration of SCMTools. It’s just not extension-based¬†so I’d skip that hook for now.
  • question about working on RB after the semester:
    • ChipX86:¬†Anybody who wants to work on RB is absolutely welcome to.¬†generally, we’d have you work toward things not on the Student Projects list after the semester, and of course there’s no status reports needed.
    • Heapify:¬†some students finished up loose ends of their projects after the semester, and you’re all free to do that, or pick entirely new projects!

nicole_x- (Nicole Xin)

  • What do those txt files in the reviewboard/templates/notifications for? I can see that they are different from those html emails, but how are they different and why?
    • purple_cow:¬†when we send an e-mail, it contains both text and html versions¬†because some people have their e-mail clients set to never show them HTML.

asalahli (Azad Salahli)

  • first of all, my WIP request is out.¬†It’d be great if I can get some reviews:¬†https://reviews.reviewboard.org/r/6509/
  • cannot call rbt patch inside rbt land
    • purple_cow:¬†you just need to create your own argv list with the arguments that you would be calling ‘rbt patch’ on the command-line with,¬†which would probably look something like argv = [‘rbt’, ‘patch’, ‘-c’, review_request]
  • I don’t know how should I check the outcome of merging/committing/pushing
    • purple_cow: for #2, the execute() method provides a variety of ways of getting that.¬†by default, if a sub-process fails (returns with an exit code other than 0), it will call die(),¬†but you can pass in various arguments to change that.
      ignore_errors=True, none_on_ignored_error=False means that execute() will return None if it failed¬†and there are other combinations too.¬†if you read through execute() you should see what’s available.

dkus_ (David Kus)

  • need more reviews on¬†https://reviews.reviewboard.org/r/6454/
  • Should we have some way for users to attached files to the RR when a markdown editor is open (since we need to turn off the DnD overlay)?
    • purple_cow:¬†maybe we can just have a small drop area over the attachment thumbnails?
    • ChipX86:¬†we don’t have an area for that if there aren’t any thumbnails yet,¬†we used to expand that area and make a drop zone.¬†the drop zone is created when we detect a drag over the window.¬†that code is entirely responsible for how that appears and what it does.¬†perhaps we can, for review requests, create several drop zones. One for the whole review request, and then one per field that’s open,¬†hide the bottom one when the drop is over the editor, to make it more clear where it’ll be dropped.
  • Should we keep track of file upload progress using a progress bar? I think it would be feasible to do, just not sure if it’s in scope or would be useful.
    • purple_cow:¬†have a spinner to show that something is still in progress

brennie (Barret Rennie)

  • all is well!

Meeting Minutes: Oct 26, 2014

Attendees

Mentors:

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

Students:

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

Announcements

m_conley (Mike Conley)
We’re pretty much at the half-point. Midterm evaluations got sent off last Friday, so you should be receiving some feedback sometime very soon. If you don’t get feedback from your faculty within the next week or so, make sure to let us know.

Round Robin

andrewhong (Andrew Hong)

  • Spent some time looking into this and I‚Äôm not sure if this is a bug, but when switching storage options in the admin panel, should I be needing to restart the server for the changes to apply? For example, when switching the local -> amazon s3, it would still upload files to the local file system. I had to restart the server for it to change over worth filing a bug report.
    • (purple_cow) At the very least we should warn people that they need to restart their web server.
    • (m_conley) hat might be a fine stopgap until we get to the bottom of this. It might be worth doing a preliminary investigation to see how tough of a problem that is to solve. If it’s non-trivial, to put such a warning in the UI while we sort it out.
  • Started digging around rbtools and haven‚Äôt jumped back into the autocomplete yet. I‚Äôll get back to the autocomplete first since it seems more important.

asalahli (Azad Salahli)

  • Going to post a WIP, but it didn’t go as expected. There is more work that needs to be done than I expected. The WIP has been posted, but it is nowhere close to what I wanted to get done.¬†Working: Taking the review request, check if its approved, create a new empty commit on top of current branch (with message “reviewed at:…”) and push it to remote. This bit works.¬†Having trouble calling rbt patch form rbt land, if the review request has to be applied first to local repo
    • (m_conley) I believe RBTools commands were designed to be composable, I seem to recall it being possible to call the main function of each command without assuming that main was being called from the rbt harness, and not some other command.
  • Can you point to an example code where main is being called from somewhere else, or any hint to how I can do it?
  • How to determine if I’ve already created an empty commit with just the message. I wanna look at the message of the last commit. There already is SCMTool.get_raw_commit_message(), but I need to pass 2 commit id’s to it, so that I can return everything inbetween, but SCMTool.parse_revision_spec() only returns the last commit id.
    • (purple_cow) Perhaps before we get too depp into the code, we should have a good plan for all of the different cases.
    • (heapify) I’m not sure if creating an empty commit with a message is the best way for rbt land to work.
    • (m_conley) Are you able to hand around after the meeting to see if we can flesh this out more?

brennie (Barret Rennie)

  • I’m wondering if I understand how the HTTP caching is supposed to work. So I made a flow chart that describes how I think its supposed to work (https://www.dropbox.com/s/s46rdynb33nbc9k/flowchart%20for%20caching.png?dl=0
    • (m_conley) This seems to match my understanding
  • I have a WIP that implements most of this behaviour. It just needs to be refactored to support testing. Allow it to specify the URL opener to use so that a fake opener can be used for testing.

dkus_ (David Kus)

  • How do you properly create a review request for a branch that is based off another branch
    • (purple_cow) If your branch structure is master >> backend >> frontend, you can do rbt post frontend for just the last commit on frontent, or rbt post backend..frontend
  • Code style problems. How to properly format https://reviews.reviewboard.org/r/6454/diff/7/?file=59973# (lines 77-82)?
    • (purple_cow) One thing to try: add a newline after the first (, and indent everything only four spaces.

justy777 (Justin Maillet)

  • I have a request from RequestFactory class and I need to insert something in it to get out with request.GET.get(). How do I do that?
    • (m_conley) Doesn’t kgb offer capabilities for preloading method calls with things like this
    • (justy777) Yes, but it caused over 100 tests to fail when I tried that approach.
    • (m_conley) Which tests are you attempting to do this RequestFactory insertion thing for?
    • (justy777) Testing search_users() for reviewboard.accounts.AuthBackend subclasses.
    • (m_conley) Can you stick around after the meeting to help you sort that out?
  • How would someone fix, a function like get_sort_field(self, ‚Ķ.) in Column class being called by StatefulColumn via __getattr__ since it gives an error that for self it returned an instance of StatefulColumn instead of A Column subclass instance?
    • (m_conley) These sort of technical questions are exactly why we like you to ask them in your status reports :). Because right off the top of our heads, I think we don’t know. Stick around after the meeting and we’ll try to sort that out, ok?
  • How well does git/reviewboard put together commits/patches on the same file as long as you’re just adding things? I want to have multiple review requests for the same file, and I want to know if they will interfere with each other.
    • (m_conley) Let’s address this after the meeting.

mloyzer (Mark Loyzer)

  • Nothing blocking from moving forward with the PDF stuff. The documentation on reportlab isn’t the best but I’m making progress, and I’ll be able to do alot more progress this week because 4 midterms done this week.

rdone (Ryan Done)

  • Anything blocking on the slider work? There was some initial ideas I wanted to confirm. It appears that some of this is partially implemented in different places. There’s a FileAttachmentHistory class, the REST API allows for ‘puts’ on an attachment etc. But there are gaps that need filling in. For example, the update button still calls POST to the web server, so an error is returned. (400 Bad Request)
    • (purple_cow) It should be a POST, it’s a new file attachment that’s associated with the same history object. It worked when I pushed all those changes but maybe something regressed
    • (ChipX86) If you check the network tab in the browser’s devtools, you should get more info about what is missing.
    • (m_conley) The server console might also hold some clues
    • (purple_cow) It looks like the javascript is using the name “attachment_history_id” but we renamed that parameter to “attachment_history”
  • Most of this stuff is in place, we just need the gui slider to retreive back the file history
    • (purple_cow / m_conley) Yes.

nicole_x- (Nicole Xin)

Meeting Minutes: Oct 19, 2014

Attendees:

Mentors:

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

Students:

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

Announcements:

m_conley (Mike Conley):

We’re pretty much here at the half-way point and¬†we’re submitting your midterm evaluations in the next few days.
Now, I’m not entirely sure how UCOSP / your home faculty is going to distribute these midterm evaluations,¬†but I did want to mention / remind you that whatever “grade” you receive as part of your midterm is what we (your mentors) think you would likely achieve at the end of the course if you were to maintain your current velocity.
This means that a not so great midterm evaluation does not guarantee a not so great final evaluation grade. Similarly, a fantastic midterm evaluation does not guarantee a fantastic final evaluation.¬†It’s really just a signal to say either “Keep it up”, or “You need to pick up the pace.”
The final evaluation is what your home faculty will use to generate the grade that you receive from your particular institution – not the midterm.¬†¬†At least, that’s what we (your mentors) have been told. It’s possible that your home faculty might do different things, which you can ask them about.
What I describe above is how things have been running with the universities so far.

Round Robin:

brennie (Barret Rennie)

  • ¬†<m_conley> brennie: so I took a look at your diff comment expansion patch. You’re right – it’s pretty substantial. ūüôā
  • <ChipX86> I plan on looking ati t soon
  • <m_conley> brennie: I know that purple_cow had some concerns with the UI – did that get sorted out?
  • <m_conley> I think I saw traffic in IRC about that… a conversation between brennie and purple_cow.
  • <brennie> m_conley: yeah. The controls are now hidden by default and appear when you mouseover, which solves the real estate problem. I also made it so the collapse button is a floating button like in the diffviewer, instead of a static button at the top/bottom of the diff.
  • <ChipX86> we were discussing stuff around I think the previous UI?
  • <ChipX86> brennie: you should link to the video you made in the status report
  • <m_conley> brennie: excellent, that sounds good. Oh! There’s a video! We love video. ūüôā
  • <brennie> heres the first one https://media-reviews-reviewboard-org.s3.amazonaws.com/uploaded/files/2014/10/16/b339ab91-b0bb-4052-8f32-df54284c81fc__control_toggle_demo.webm
  • <brennie> and the second:¬†¬†https://media-reviews-reviewboard-org.s3.amazonaws.com/uploaded/files/2014/10/16/2b43f7f0-2235-4fe9-bb4e-3c2bcd9a4e8c__expand_collapse_demo.webm
  • <m_conley> brennie: alright, excellent – well it sounds like you’re on the final phases of that patch, which is great. And all is well with rbt aliasing?
  • <brennie> I havent gotten a chance to start on it because fixing the floating comment button was not straightforward. I’ve decided that they should function as close as possible to git aliases¬†(for consistency’s sake)

andrewhong (Andrew Hong)

  • <andrewhong_p> Trying to clean up my git repo up. Got a little sick over the past few days but healing up now… Did a few of those quick fixes as suggested on the review request.
  • <m_conley> andrewhong_p: cool. glad to hear you figured out the zip file corruption thing
  • <m_conley> andrewhong_p: what ended up being the issue there? I tried to piece it together from the diff, but it wasn’t immediately obvious
  • <andrewhong_p> Yes! I think it would’ve been easier to find if the package was better documented
  • <andrewhong_p> I wasn’t passing the stringio object to the zipstream constructor
  • <m_conley> andrewhong_p: the solution might make a fine blog post, and maybe file a bug against the package then?
  • <andrewhong_p> Definitely, a short and useful one
  • <m_conley> andrewhong_p: well, glad you figured it out. Yes, a blog post please, would be very lovely there, to help future folk.
  • <m_conley> andrewhong_p: and so you’re looking for more work now?
  • <andrewhong_p> Yep I’ll be looking at student projects again next week
  • <ChipX86> let us know if nothing on the list is interesting. There are lots of things on the roadmap
  • <m_conley> andrewhong_p: and yes, please either select or alert us quickly – not having work to do is not a great state to be in.
  • <andrewhong_p> Sure, i picked out a few already

dkus_ (David Kus)

  • <m_conley> dkus_ wrote – “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.”
  • <ChipX86> you can ignore that;¬†<m_conley> I think ignoring it is the right idea
  • <dkus_> ok
  • <ChipX86> that’s a feature designed to allow you to replace file attachmetns on a review request;¬†like diffs
  • <m_conley> and see the history of those attachments over time
  • <m_conley> (actually the result of a student project – lyubinets’s, to be exact)
  • <m_conley> dkus_ also writes: “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?”
  • <purple_cow> I would think 404?
  • <m_conley> sounds like the webby thing to do
  • <ChipX86> yeah, probably 404
  • <dkus_> ok
  • <m_conley> dkus_: other questions or issues?
  • <dkus_> does that url make sense for the attachments?
  • <dkus_> /attachments/user-files
  • <purple_cow> that should be fine
  • <m_conley> dkus_: cool – all set to work on the front-end bits? Seems like the back-end is pretty much there.
  • <dkus_> yeah, thought i would try and add a few tests around the web api if i could
  • <m_conley> dkus_: that’s a great idea;¬†dkus_: there should already be some attachment tests lying around that you can use for inspiration
  • <dkus_> yeah I was going through them earlier
  • <ChipX86> yep, definitely add some tests;¬†everyone should strive to write tests for any code they write
  • <ChipX86> it’s the only way we know for sure that your feature won’t break down the road when something changes
  • <m_conley> they might suck to write, but maaaaaan can they save your butt in the future;¬†it’s basically a seat belt, but for your code.
  • <ChipX86> some of the most important code you’ll write are tests

nicole_xin_ (Nicole Xin)

  • <m_conley> nicole_xin_: looks like you’re out of WIP for the back-end stuff. Great job!
  • <nicole_xin_> i‚Äôm done with backend models!;¬†and i passed all the tests i wrote
  • <m_conley> great – we’ll have some reviews on that soon, but don’t be blocked by us (or if you are blocked by us, please alert us ASAP)
  • <nicole_xin_> i‚Äôm almost done on web api, just one new test to add
  • <m_conley> ah, tests. Music to our ears.
  • <nicole_xin_> yes sure
  • <m_conley> nicole_xin_: you said something about a database crash in your status report
    <nicole_xin_> yes; but i have no idea why; just got a new one solves everything
  • <m_conley> I just wanted to remind everybody that their development environments (unless you’re doing something freaky) are using SQLite as the database backend, and your database is a single file, called reviewboard.db, and it’s in your root directory
  • <m_conley> and you can copy and delete that file as you please
  • <ChipX86> and you should do that fairly often
  • <m_conley> I recommend backing it up just before doing anything interesting with the database – like running syncdb or an evolve
  • <nicole_xin_> great tips
  • <ChipX86> protip: append timestamps to the filenames when you back up
  • <nicole_xin_> i did spend a lot of time fixing it, just didn‚Äôt work
  • <ChipX86> if there’s something interesting about them, append to the filename (e.g., pre-evolve-for-branchname)
  • <m_conley> even pro-er tip, have a cronjob make the backups and filenames for you, maybe daily or every other day, or weekly.

rdone (Ryan Done)

  • <m_conley> I was in the middle of reviewing your screenshots when the meeting started – but I guess I can tell you here:¬†I wonder if we might somehow connect the selected letter with the page numbers a little more strongly…
  • <rdone> Any suggestions on how to do that?
  • <m_conley> like, surround them with the same background colour, to make them seem like the same unit
  • <m_conley> So:¬†suppose we make the selected letter in general stand out more, by not just bolding the letter, but giving it a background colour for more contrast,¬†or at least, to draw the eye more
  • <rdone> Ok
  • <m_conley> since it’s easy to get lost in that sea of letters, and not immediately see which one is selected;¬†colour can draw the eye
  • <rdone> Just the letter? Or the letter and number controls?
  • <m_conley> well, if there are number controls for a letter, yes – perhaps we can experiment with sharing the same background colour
  • <m_conley> also, for your spam fighting extension,¬†had anybody talked to you at all about how we would identify spam?
  • <purple_cow> I think the idea was that new users would be moderated for a time
  • <m_conley> purple_cow: oh, so this was more of an approval process, then something like Akismet?
  • <purple_cow> since the issue isn’t so much traditional “spam” as it is people who are just playing around
  • <ChipX86> some things that could be considered as well are reviews made on closed review requests by new users,¬†like, red flag that
  • <m_conley> we should come up with a list of heuristics
  • <rdone> The project description sounded like it was catching newly registered accounts¬†and giving them limited privleges until approval
  • <rdone> or should it be more retroactive: catching ‘spammy’ behaviour
  • <m_conley> rdone: in any event, the end result will be that some reviews / comments / replies are going to need to be mapped to some state of either hidden or not hidden.
  • <ChipX86> one thing I’d also like is a sort of Spam button that, with a prompt, lets me delete the review and optionally ban the user¬†on a review
  • <purple_cow> personally, I’d say that catching newly registered accounts is simpler¬†and it’s a rare enough problem that it wouldn’t be too onerous
  • <ChipX86> sure, but we do have a lot of accounts in our database right now
  • <purple_cow> ChipX86: we do, but we only get new users posting stuff maybe once a week
  • <ChipX86> it would also be super cool for projects like ours and apache to be able to look for registered spam accounts nad delete them
  • <ChipX86> I think there are blacklists to compare against somewhere out there
  • <ChipX86> btu anyway, jsut food for thought
  • <m_conley> rdone: are you able to stick around after the meeting to solidify this stuff?
  • <rdone> for sure it seems like theres a bunch of different requirements here
  • <m_conley> rdone: yeah, I think we need to figure out the scope of this thing

mloyzer (Mark Loyzer)

  • <m_conley> mloyzer: hey – so, I like your PDF formatting, and I think we should go with that
  • <mloyzer> m_conley, ya, i saw your review
  • <m_conley> purple_cow / ChipX86: not sure you saw from our last meeting minutes, but mloyzer, smacleod and I were pretty certain that CSV doesn’t make a whole lot of sense for review request export, due to the one-to-many relationships with various things (bugs, reviewers, reviews, etc)
  • <m_conley> so we thought instead of doing a CSV export, mloyzer could do XML instead.
  • <mloyzer> m_conley, i started with the more fundamental (harder to screw up) information anyway so I’ve done a fair amount of translating the review_request int a pdf
  • <m_conley> cool
  • <ChipX86> I have no problem with an XML export. I did wonder about CSV in that regard
  • <ChipX86> people can always take an XML dump and XSLT it to some preferred CSV format
  • <m_conley> enterprises love XML;¬†so, we’re a GO on XML
  • <mloyzer> m_conley, ok cool. I want to finish PDF first anyway now
  • <m_conley> mloyzer: “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?”
  • <mloyzer> m_conley, yup^. was just confused/curious about that
  • <m_conley> so, a review usually has comments on things like diffs or attachments, but not always. Like, sometimes there are general things to say that are not related to a particular line in a diff or a file
  • <m_conley> if you were to open a review on somebody’s review request right now,¬†like,¬†click on the “Review” button in an open review request, without having commented on anything
  • <m_conley> the dialog that comes up has a text input for your review. That’s the body_top.
  • <m_conley> It’s associated with a review, but not to a particular diff / line in a diff / screenshot / file.
  • <m_conley> mloyzer: now, if you were to comment on the diff, for example, when you went to edit your review
  • <m_conley> you’d see this:¬†https://www.dropbox.com/s/mnca2p6pcr19f8b/Screenshot%202014-10-19%2014.48.27.png?dl=0
  • <m_conley> that Add Header link exposes the same text input that you would use before if you hadn’t have commented on anything
  • <m_conley> and Add footer does something similar, but as kind of an “afterward” for all of your comments.
    <m_conley> so, in sum:  A review can have a body_top / body_bottom, which is text associated with a review request, but not associated with a particular diff / screenshot / file. The body_bottom ability is only exposed if comments on diffs / screenshots / files already exist.
  • <m_conley> as an ‘afterward’
  • <mloyzer> ah ok.. makes more sense now anyway ūüôā
  • <mloyzer> i’ll have to look into it more when I start transfering the information from reviews to PDF but that does help

justy777 (Justin Maillet)

  • <m_conley> justy777: do you have any questions to help you move forward?
  • <justy777> just a small question
  • <justy777> once i have tests and the actually sandboxing, should the test and sandboxing go onto the same review request for that extension?
  • <ChipX86> yep
  • <justy777> okay, that‚Äôs all i got for questions
  • <m_conley> justy777: I notice in your status report that it says you weren’t able to do much research this week – though that implies you did some. I would highly recommend you post those research notes in a Hackpad somewhere, so we can see them.
  • <m_conley> justy777: best if you could put those up immediately after this meeting.

salahli (Azad Salahli)

  • <m_conley> asalahli: so, your question is about whether or not we’re duplicating code by having rbt land do essentially what rbt patch does, but then doing a push?
  • <asalahli> yeah
  • <m_conley> asalahli: I think we should totally leverage rbt patch if we can.
  • <asalahli> thats what I thought
  • <m_conley> asalahli: I thought we discussed this with smacleod a few weeks back though – I’m pretty sure I brought up the fact that rbt push does much of what rbt patch already does…
  • <ChipX86> something that would also be good is to automatically add the “Reviewed at” on the commit message
  • <asalahli> rbt push?
  • <m_conley> asalahli: do you remember the result of our discussion with smacleod along those lines?
  • <purple_cow> ChipX86: well, we do that already in ‘rbt patch’
  • <asalahli> m_conley: yes, you said I should look at rbt patch, because it is very similar
  • <ChipX86> so, yes, but if I’m pushing my own change, Id on’t want to use rbt patch
  • <ChipX86> that’s a place where this differs
  • <asalahli> but it happened to be more similar than I expected;¬†thats why I wanted to confirm
    <ChipX86> I’d think rbt land would be more of a wrapper around rbt patch, the SCM’s merge/rebase and push
  • <ChipX86> I don’t know what the previous discussion is, btu I definitely have thoughts on this command based on talking to another company that wrote a similar tool
  • <ChipX86> so maybe we should sit down and discuss it more?
  • <m_conley> sounds good
  • <asalahli> ChipX86: That’d be awesome
  • <m_conley> asalahli: can you stick around after the meeting?
  • <m_conley> let’s get this thing ironed out
  • <asalahli> m_conley: yeah
  • <asalahli> mentors: Can I haz more reviews to my RR please?
  • <m_conley> heh,¬†yes, you can haz. Sorry, we’ve all been a bit backlogged with a few other things.
  • <asalahli> https://reviews.reviewboard.org/r/6407/
  • <m_conley> ^– other students – this is a great time to do some reviews too…
  • <asalahli> dkus_: I added students group to the RR
  • <m_conley> asalahli: yes, we’ll get in there. Thanks for hounding us. ūüôā
  • <asalahli> how do I build documentation locally?
  • <m_conley> ChipX86 / purple_cow / heapify: is that a manage.py command?
  • <asalahli> I still haven’t fixed WEB API docs, but I think I know how to do it
  • <ChipX86> go into docs/manual/;¬†run:¬†make html
  • <ChipX86> you will need sphinx and maybe a couple other things installed
  • <ChipX86> (make html will build docs, but they will look different than what’s on the website)

Meeting Minutes: October 5, 2014

Attendees

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).

Announcements:

  • (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 rbcommons.com 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 r.rb.org 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¬†https://docs.djangoproject.com/en/1.7/howto/outputting-pdf/
  • 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 r.rb.org 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!