Author Archives: tomiaijo

Meeting Minutes: April 13, 2014

Announcement/General tips:

  • 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

Round Robin:

Bhushan

  • 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

Edwin

  • 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

Joonas

  • 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

Stephanie

  • 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

Audore

  • 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
    highlighting it
  • 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

Tomi

  • 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

Tami

  • 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

Mirai

  • Absent

 

Advertisements