- You all have between 3 and 5 weeks left. If you post code and it needs review from the mentors, please bug them to make sure that they get to it
- Now is a good time to check out the review requests from your peers
- Hit a small roadblock with an assumption on how Beanstalk works, already resolved by ChipX86
- Question: The svn payload doesn’t contain a commit_id. It has revision number. Does that get stored as commit_id in the ReviewRequest object?
- purple_cow: Not usually, changes in SVN are not assigned a revision number until committed to the central repository. No revision number to store unless posting an already-committed change.
- Question: For svn repos, the function that the hook will always need the review request id in the commit message?
- purple_cow: Correct
- Thinks he can finish this hook by the end of this week and start working on google code post-receive hook
- Would like updates on #5653
- Week went pretty well, it took a little longer than expected for the first sandboxing
with the ReviewRequestApprovalHook. Mostly done now.
- Has been working on a couple other sandboxing things
- Question: For the sandboxing he is working on, he wasn’t quite sure how to test it since it seems like it gets called from an html file. How to simulate that with the unit tests? (NavigationBarHook get_entries in rb_extensions.py, called from navigation_bar_hooks)
- m_conley: That can be imported and called from our tests. Context is just a mapping of keys to values, which you can mock out. Example https://github.com/reviewboard/reviewboard/blob/master/reviewboard/extensions/tests.py#L60 and #L113
- Only has 1 or 2 more sandbox points to hit next week
- Did the menu icon with pure CSS, posted the code and screenshots
- m_conley: Why CSS instead of creating SVG icon in Inkscape?
- Felt more comfortable taking the CSS approach
- m_conley: All of our other icons are implemented via the SVG spritesheet.
- purple_cow: As long as the icon is well-factored into the code that can be addressed later. I’d rather make more progress on the actual responsiveness
make more progress on the actual responsiveness
- Mentors will follow-up on the discussion in /r/5655
- m_conley: How are you going to approach this next bit – the navigation bar?
- Mirai had some early mock ups of those, will take a look of those
- Also has to put the search bar to the top nav bar
- Implemented the removal with an already existing icon which will need to be replaced in the future
- It’s in the upper right hand corner, but there’s a confirm dialog, so it should be ok
- m_conley: How’s the backend coming along?
- It saves the state (which ones have been removed and added) but doesn’t show the differences on the front end quite yet
- Will be working on that next week
- No blockers, but eventually will need better images, and a new icon
- Part 1 is out of WIP, the code has been refactored and modified to follow the style
- Also created a WIP for the part 2
- Question: How would you like to the highlighted row to be shown to the user? (like different color background or bold frames etc.)
- m_conley: My instinct is a different coloured background
- Question: Any preference on the color?
- m_conley: Let’s just choose blue for now
- m_conley: What do you think is the timeline on this part 2 patch? How close are you to being in a non-WIP state?
- The early implementation is already scrolling to right line, but it’s not
- Highlight is the goal this week and also possible fixes on part 1 if needed
- Tries to have Part 2 out of WIP by the end-of-week
- Is worried if the change to add authentication for bug tracker HostingService is too large a change
- Splitted the review request (r/5531) into two parts: things that need the authentication and things that does not (r/5531)
- Ready to take r/5531 out of WIP, just one issue: There is no way to know whether any bug metadata is supported and the infobox is shown by default, has been trying two different ways:
- 1. I added a field to review request backbone model that tells whether the infobox should be shown. But bug_infobox is also used in dashboard view and common.js (document ready) and the review request is not at disposal
- 2. Catch the 404 received from RB in bug_infobox -function. The load function has second
parameter which equals to “error” in this case. But the infobox is shown briefly before
receiving the 404.
- purple_cow: I think we should avoid showing the infobox until we get the response back. It may take a little bit longer hovering over the link, but it’s better than flickering. That makes it act a bit more like a tooltip, which is a known interaction.
- Will discuss the bug tracker authentication with ChipX86
- Got the tests up
- Reviews on private review requests aren’t being filtered, so need to get that
fixed. The public field on a review only refers to if it is published or not.
- Question: Filtering for private review requests seems to be fairly complicated (line 294 of
datagrids/managers.py) – does filtering for private reviews have to be similarly complicated?
- purple_cow: You can just copy a lot of the logic there and make it use the relation to the review request
- Wasn’t sure if she would need to copy that logic into a method on the review manager
- purple_cow: Yes, I think so
- Question: If someone has multiple reviews on the same review request, they currently
look the same on the pag, the only thing that differs is the timestamp. Should I add another column that has maybe a summary of the review?
- purple_cow: I’d say maybe keep it as-is for now and we can revisit that later.
- purple_cow: One thing that could help filtering is only show top-level reviews and not replies
- Nearing the end of the course for everyone, CMU has 4 weeks left, TUT has 5 weeks left, MIT has 6 weeks left
- Think about where you are in the project, how fast you’re moving and the likelihood of you finishing your project given other commitments and adjust accordingly
- Please don’t stretch work out to fill out time, attack as hard as you can.
- tamijo: ok – i think the main things i want to test are: does a review show up, and are reviews on private requests hidden
- m_conley says those are fine things to test
- tamijo: I also havent thought about how multiple reviews on the same review request are shown on the grid
- m_conley: I think each individual review should be shown in the list, personally
- Tests seem pretty straight forward and tamijo is almost done
- Tests should have them up and project should be out of WIP by this week
- Should start browsing the project list for a new project, given how much time is left
[Excused: UCOSP Done]
- Put a patch up for sandboxing, it was pretty straightforward
- Patch is in WIP because he wanted to make sure was on the right path and the tests were written/done correctly
- Found the hook when m_conley showed when describing the project, also found it by searching through the code and having it on a list
- Expecting 2 patches per week, so we think you kinda need to up your speed a bit
- So should we see 3 patches this week
- m_conley: thanks for writing the regression test.
- Tests look good, but need to confirm my tests fails without my patch
[Excused: UCOSP Done]
- Shoutout to bro1, tomia_, and tamijo for giving feedback on the screenshots
- m_conley: I think your approach with the dropdown overlay is better than what you had before. It’s not a sexy slide-panel, but for a first version, in order to get something landed, I think we should go for it. I agree that an icon is better than “menu”.
- this is becoming the icon for “menu”: https://www.dropbox.com/s/1vbq4a3dcu672pi/Screenshot%202014-04-06%2014.21.08.png
- Screenshot is at: https://reviews.reviewboard.org/r/5620/file/518/
- The hit area for menu is not quite big enough yet, need to add more padding/make it bigger – those are next steps
- If we want to use the three-line “hamburger” icon for menu, to get the icon into RB, we have a master icons.svg and export that in normal and @2x versions says ChipX86
- To get that hamburger into spritesheet, need to use inkscape to edit the file, add the icon in the next available empty space, figure out the coordinates, and make a suitable entry in icons.less
- ChipX86: you can’t drop a png into the svg file, so it’ll need to be made from scratch
- m_conley: Joonas_: Inkscape isn’t really a thing that we expect people to be familiar with just yet, so please please please ask for help if you need it with it
- ChipX86: and if you’re getting stuck doing it, I can put an icon in there
- m_conley: Joonas_: or just copy it: http://css-tricks.com/three-line-menu-navicon/
- m_conley and purple_cow agree that the version number is really not necessary to display in the mobile UI, so it can be hidden
- About to take r/5531 out of WIP
- Realized today that needs_authorization flag does not work with hosting services that act as bug trackers, need to fix that, should be easy enough given it is already done for basic hosting services,
- tomia_: then I will start adding support for bug trackers, r/5531 is just the foundation. in other words, implement the bugtracker interface for those
- tomia_: And there are three identical issues open on r/5531, I have been thinking about those. They are about the comments being sent to bug trackers
- tomia_: I am leaning towards not making them templates like emails, just simple strings as they are. At least I would not like some outside system spamming bug tracker full of 10 line comments. And of course add the localization wrapper for those strings
- tomia_: https://reviews.reviewboard.org/r/5531/file/520/ the bug infobox is fully functional now, maybe need some css magic
- Got the issues from the previous week fixed, though i have to go the style thru
- Audore thinks she can have this first part out of WIP this week if it’s only about the style
- ChipX86: I need to do another pass through the actual logic. I still think it’s doing more than it needs.
- ChipX86: in the meantime, though, spend some time fixing up the style and get that to us ASAP
- Should be able to get cranked out in an hour
- Part 2 in the next task, Audore will implement it by showing the code line, which is in the URL
- m_conley: Audore: alright. I’m looking forward to seeing a WIP for that patch up this week – maybe the earlier the better. Don’t wait for the meeting to roll around.
- Also closed some of ChipX86′s issues without fixing them, so will need to reopen them
- m_conley: Audore: OK, so we’d like to see the style fixes up today, and a WIP for part 2 up by Tuesday. Sound good?
- Audore: well not today, i’m heading to sleep after the meeting, but tomorrow
- Put some more screenshots up as we speak, which shows the /r/:id screen for mobile
- m_conley: Joonas_: you might want to get in there and give some feedback to bro1
- Has some not so immediately-blocking problems though
- First thing: uses html meta tag viewport to disallow zoom on mobile devices for the *entire* site, which is rather undesirable because i am currently only working on the review details and was writing some code which wraps this around a django template if, which only inserts for certain urls, but that hasnt worked and was wondering what the correct syntax would be
- ChipX86: We want all-or-nothing. you won’t be able to really add that per-URL in the base template. you can add it in the more specific templates. but are we adding that just because the top bar and stuff aren’t mobile-friendly yet?
- bro1: that and perhaps the dashboard. i guess since we are eventually moving most of the stuff to mobile its not a big issue
- That was more of a curiosity question
- bro1: One more question, regarding code mirror
- because bro1 hasn’t been able to get the codemirror to work well on mobile, writing reviews has become much better on mobile (view screenshot), but the cursor does not focus automatically on mobile (probably due to codemirror)
- bro1: in fact, the user needs to click somewhere else (e.g. ship-it checkbox), and then hit the textarea in order to bring up the keyboard
- According to stackoverflow, apparently for iOS the browser ignores .focus()
- ChipX86: so I know they just released a new CodeMirror. Might be worth seeing if it works any better?
- bro1: ive tried hacking around by adding a click listener as some internet people suggested, but hasnt worked so far
- m_conley: bro1: maybe you can patch CodeMirror. http://codemirror.net/ says that the maintainer isn’t really targeting mobile, but accepts patches, but that may be getting distracted
- Should strive to have code mirror on mobile support
- ChipX86: we use codemirror in order to provide visual hints for markdown styles. without it, it’s too easy to accidentally write code that will turn into unwanted markdown-formatted stuff
- Backticks and things like *, _, etc.
- m_conley: maybe we can work around these bugs. bro1, can you stick around after the meeting to brainstorm?
- bro1: yeah i have another thing in half an hour but right after the meeting works
- bro1: regardless of this im going to start tackling the comments section this week
- bhushan89: Completed testing and development of Bitbucket post-receive hook rr#5653
- Also resolved the mercurial issue so testing on mercurial is complete
- bhushan89 just had to add some configurations in ~/.hgrc file
- Since it’s out of WIP, need to get it reviewed for real
- bhushan89 wants to get started on the next project this week
- bhushan89 had a question about Beanstalk testing for next project
- We have Beanstalk accounts, ChipX86 will give bhushan89 the account details
- bhushan89: so as David suggested that I can work on this project where the pre-receive hook checks for approval before closing the review request and then closes it automatically
- ChipX86: It doesn’t close, it just checks if you’re allowed to push. the post-receive hook still handles the actual closing. the pre-receive hook should block the push if you don’t have permission to push
- m_conley: to blocks pushing on pre-receive if not approved
- bhushan89: hmm…but won’t that mean taking two trips to the server ? or the pre-receive hook view just calls the post-receive hook view if it is approved ?
- ChipX86: it is two trips, btu it’s also two stages of a push. pre-receive happens before any code actually gets stored on the server there may be multiple things going on there. They may do things like queue off some tests, or whatever. post-receive happens after everything has been approved and changes are stored. those are the guidelines for repo hooks, and we need to respect them, even if it’s multiple trips to hte server (which aren’t expensive anyway)
- bhushan89: So I have around one month. I’m planning to get done with this and some more post-receive hooks towards the end.
- m_conley: bhushan89: awesome. You’ve got a good pace going. Keep up the good work.
- Finished the front end with dummy image (my own screenshots)
- Posted a picture of the UI on my WIP
- hellosjsu has changed the idea a little
- hellosjsu: originally would have the users click on an X button to remove a widget, but then i noticed the upper right hand corner, already had the minus sign for minimizing. so now add and remove happen in the same place. the user checks off widgets they want to add, and unchecks the ones they don’t want to remove
- m_conley: that’s slightly “dangerous”
- ChipX86: I think it’s okay to have the “X”, and it’s contextual, which is good. people can remove it without having to 1) open the menu, and 2) re-locate the widget they want to remove. we can always prompt if they’re sure, but I think it makes the usability nicer to have it alongside the “_”. plus, that’s consistent with how people use windows on basically any OS
- hellosjsu wondered if it should be in the same corner
- ChipX86 thinks it’s fine as long as they aren’t bumped up right next to each other
- hellosjsu: also right now, clicking anywhere on the header minimizes the widget, should we keep that if we do the X?
- ChipX86 and m_conley don’t get that, only get crosshairs
- m_conley: hellosjsu: in this case, maybe take some inspiration from Windows’ minimize and close buttons on Windows 7 in terms of clear separation
- m_conley: hellosjsu: alright – I look forward to seeing the back-end saving code.
Announcements / General tips
UCOSP is wrapping up – this is the _last_ required meeting for heapify and olessia! from what m_conley can see, the two of them are in the right spot with their projects, just solidifying the last bits
m_conley would like them to stick around after the semester ends – and adds that others should do as well
m_conley: As we get closer to the end, make sure you come to us during the week with anything and remind us to look at things if you go even a day without a review.
question regarding to an open issue here: https://reviews.reviewboard.org/r/5531/#comment14233 -
added new url to reviewboard (r/rr_id/bug/bug_id/) that redirects to the bug tracker url to get the infobox url similarly as the user infobox constructs it (just add infobox/ to the bug url) Asks if it sounds okay.
m_conley and purple_cow thinks its fine
no other questions or blocking factors
[Absent - MIA]
projects are done, just fixing bugs
m_conley points out she’s handling some smaller bugs as approaching final day
question for purple_cow: Bhushan told me you said it could be a good idea to add a check to see if the review request’s commit ID matches the revision that got pushed? (this is after the regex has matched a review request ID in the commit message) – should I add this?
purple_cow I think that would be something nice if the regex doesn’t match
oh, if the regex doesn’t match, which review request’s commit ID would we be checking the revision with?
purple_cow you’d use the commit ID to try to find the review request
m_conley heapify: awesome. Great work this semester!
[Absent - Travelling]
(m_conley looks at screenshots from review request)
Joonas_ – this point when resize the window the top navigation links move vertically aligned/one below another. now just hide them under a Menu button.
m_conley asks – idea is instead of sliding in a drawer from the side, to stretch down the titlebar?
Joonas_: yes – asks if it sounds good.
m_conley was personally more interested in the slide-y drawer thing, but thinks perhaps it is because he is more used to it. also curious to know what performance impact, if any, there is in increasing the vertical height of the page by stretching down that titlebar. claims sliding in the drawer over top with a CSS transform is nice because it offloads the work to the GPU and doesn’t require reflow whereas stretching an item like this will require reflowing the document
Joonas_ hadn’t thought about that.
m_conley (reflowing = making the document recalculate the heights, widths and locations of things, which is a computationally expensive thing – especially for complex documents)
m_conley asks how difficult it’d be to have it slide in over top. any sense of the scale of that work?
Joonas_ doesn’t know – just worked on the drop down menu – hasn’t done a slide out drawer before but can investigate in that
m_conley has done stuff like this before and suggests he talk after meeting, hash out what is required.
m_conley – regarding question from status update this week – suggests sketching out something after meeting
Joonas_ is okay with that.
purple_cow - probably also move the search box into the drop-down and hide the version number – everything that’s in the header takes space away from other stuff, so on mobile it should be super streamlined
* m_conley nods
m_conley – would like screenshots from mobile versions – to lower the barrier for the feedback-givers as much as possible – give us all of the materials we need to give you feedback quickly, and we’re more likely to give it to you quickly
bro1 can definitely do that
m_conley is really eager to see what this patch does. :)
bro1 will get screenshots up tonight
m_conley points out looks like Joonas_ asked you a question in your review request, that you’ll want to address
bro1 will take care of that as well
no other questions – m_conley suggests bro1 not wait for feedback but start ahead with other stuff
olessia is wrapping up, so really appreciate some answers to the questions on the review request
m_conley – alright, can provide those
ChipX86 meant to get the rest of that when he got back into town, but been knocked out of commission all week from a nasty cold – going through it now
olessia hopes ChipX86 is feeling better
m_conley asks if anything else you need from us before the term ends?
olessia – No – asks about performance review
m_conley according to Michelle, the evaluations go in on April 11 – so sometime after that
m_conley – olessia: great job this semester! – says we hope to see both you and heapify stick around – you’re both great contributors, and we’ve got plenty of work. :)
olessia thanks m_conley and claims she learned a lot.
two problems with the code in review request – if those figured out, can move forward – and also can’t get the template working
purple_cow asks to show the code for template
Audore not on review request currently
m_conley – :/
ChipX86 points out mentors want to see new code every week
Audore – because when I implemented I got an error right away, that the variable wasn’t declared – yea, there lot’s of other code
m_conley and purple_cow both want to see the errors that are generated – all of it
m_conley - even if it’s not polished, we need to see where you are and what you’re doing - impossible for us to help you if we can’t see what you’re doing. :/
m_conley requests to update review request with _all_ of current code and then include a pastie of the error
no other questions
m_conley claims edwingzg identified some places where we need sandboxing.
edwinzg has a list on hackpad of everywhere where i found the hooks being used – noticed a lot of them weren’t really used outside of being defined in hooks.py. asks if thats expected. e.g. DashboardColumnsHook
ChipX86 claims should be at a point now where, every single week, you should be able to get us at least 2 review requests, each one sandboxing something – follow the code and note what that one does
ChipX86: that’s an interface around registering columns – other code then looks up from that registry – you’ll need to sandbox where the columns are being looked up – many of the hooks register into central registries like that
edwinzg – that part is clear, but one more question
purple_cow told edwinzg that for testing i should create a new extension which misbehaves and activate an extension – was wondering where the best place in the could to do that would be and how exactly i create a new extension?
ChipX86: we have an extensive guide on how to do just that, plus a helper script: http://www.reviewboard.org/docs/manual/dev/extending/extensions/
m_conley thinks it’d be excellent if this testing could be part of our automated testing suites
edwinzg asks how to add it to the automated testing suites.
ChipX86: every directory has a tests.py file where the unit tests live
m_conley: believes we have tests that create and register extensions somewhere
ChipX86 – we already have tests for the hooks and the registries that some hooks call into you can just add new tests that simulate failures
m_conley: reviewboard/extensions/tests.py is where you’ll probably want to put your new tests
m_conley: so to not run the full suite each time – also https://reviewboardstudents.wordpress.com/2011/09/24/running-subsets-of-review-board-tests/