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

One thought on “Meeting Minutes: Oct 19, 2014

  1. Pingback: Status Report for October 25, 2014 | Review Board Student Blog

Leave a Reply

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

WordPress.com Logo

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

Twitter picture

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

Facebook photo

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

Google+ photo

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

Connecting to %s