Author Archives: davidkus

Status Reports – November 22, 2014

Barret Rennie

What project are you working on?
Multiple commits per review request

What did you accomplish this week?
Got the initial schema done for multi-commit RRs and started looking into API endpoints.  I also cleaned up

Links to anything done this week?
https://reviews.reviewboard.org/r/6504/
https://reviews.reviewboard.org/r/6618/

What do you plan to do next week?
I plan to get modify the review-requests/ api endpoint to allow creating/updating a RR draft with multiple commits and appending a commit to the current draft. I also want to start working on the RBTools end of things.

What, if anything, is blocking you from making progress?
Technically its not blocking me but a bug in django-evolution is preventing my evolution from applying cleanly.
My API caching RR — https://reviews.reviewboard.org/r/6504/ — could use a review.

Any other questions?
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?


David Kus

What project are you working on?
Drag ’n Drop Images into the  Markdown editor.

What you accomplished this week?
Split my backend review request into 4 smaller ones.
Updated the API documentation for the end-point that I’m adding, as per ChipX86’s comments on my review request.
Finished up the bulk of the front-end work.
Added quite a few tests for the front-end changes.

Links to anything you’ve done this week
https://reviews.reviewboard.org/r/6510/ (front-end)
https://reviews.reviewboard.org/r/6616/ (backend view)
https://reviews.reviewboard.org/r/6617/ (backend WebAPI)
https://reviewboard.hackpad.com/dkus-Development-Log-LpIrKtA6wfI (Hackpad)

What you plan to do next week?
Final touches and any changes from code review suggestions. Possibly some more testing (I’d like to test it in Internet Explorer at least)

What, if anything, is blocking you from making progress?
Reviews and feedback on how the Drag ’n Drop overlays look (images on the rr).

Any other questions?
None


Mark Loyzer

Project:
Working on exporting Review Request as PDF and XML.

Accomplished:
I finished adding a Data Extraction object for Reviews, tidied up some of the code then started the XML stuff.
I wasn’t able to get the URLs working (I’m not sure how to create unique urls for Action Hooks) otherwise I would have removed the WIP tags.

Links:
https://reviews.reviewboard.org/r/6393/
https://reviewboard.hackpad.com/mloyzers-Development-Log-3ERurrwGH78
(there should be another one but I can’t get rbt post to work properly)

Next week:
Sunday I want to resolve the URL issue and then remove the [WIP] tags from 6393.
Early in the week I should be able to finish XML export and submit that.

Questions:
 I tried posting to review board and got the error:
“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.”
Then no diffs were uploaded.
I have since cleaned my branch and when I try to post it doesn’t upload the correct diffs.
Secondly, how do you create unique urls for Action Hooks?  ChipX86 recommended overriding get_actions but I didn’t see anything in the documentation of how it is used/when it is called.


Nicole Xin

What project are you working on?
‘rbt stamp’: Adding the review request url to the commit message.

What you accomplished this week?
1. Fix general comment reply.
2. Fix some issues about front-end of general comments.
3. Move ‘guessing ability’ of ‘rbt post -u’ into utility functions. (rbt post -u works correctly)
4. Start working on ‘rbt stamp’.

Links to anything you’ve done this week
General comments front-end: https://reviews.reviewboard.org/r/6506/
‘rbt stamp’: https://reviews.reviewboard.org/r/6623/ (Sorry it ooks a little bit messy, still a WIP)

What you plan to do next week?
Try my best to get ‘rbt stamp’ work for git.

What, if anything, is blocking you from making progress?
Nope, all good.

Any other questions?
Nope.


Ryan Done

What project are you working on?
File Attachment revision slider

What you accomplished this week?
Addressed some initial style and clean up from Chip’s review.
I’ve got a rendering slider for both image and text attachments; (although any aesthetics may not be ideal by pencils down).
I’ve gotten all the callbacks to respond to slider interaction, and they get the correct values back (revision selected value that I can use) (_revisionSelected)
I understand how to do side by side display within the text file template (templates/reviews/ui/text.html) using the table/column/row setup
Then I’ve got a bunch of questions to clarify and then I should be able to move ahead with everything.

Links to anything you’ve done this week
https://reviewboard.hackpad.com/rdone-development-log-LK0GSpfQxxN
https://reviews.reviewboard.org/r/6591

What you plan to do next week?
Finish up the and the image diff and text attachment diff, may not get to all the aesthetic stuff like dynamic labels and positioning .

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

Any other questions?
1. How should I communicate back to the server when the slider is moved?
I noticed the diff viewer slider adjusts the URL arguments using using the Router, forcing the page element to request new diff content to be passed back in response.
This is at least a little tricky right now with the text attachments: the html template actually places the lines of the file into the page on the server end. (IE the text from the file is there on the page even before the JS does any of its rendering work), so it would require a completely new response.
I think all that will get stripped and the file contents will get sent to the JS model, which will push the contents onto the page?
This is a little less of a problem for images because you can always just request the resource again.

2. How should image diffs be handled?
There appears to be stuff in place to select a variety of different image diffing modes that are enabled when a diffAgainstFileAttachmentID is provided by the server. Presumably once 1) is worked out, the slider will get moved and the server will provide back a new JS model with that value set and the diffing modes will be turned on.
Or do I need to do a side by side UI setup similar to a text review? (I think this is actually the ImageTwoUpDiffView option anyways).

3. How might the user opt to turn off a file diff and just look at 1 instance of a file?
By default there is 2 handles that can choose 2 revisions of the file, but unlike a Review Request Diff slider there should be an option to switch back to a 1 file view (or even a 1 handled slider?) ? Perhaps the label click events?


Azad Salahli

What project are you working on?
`rbt land` for landing changes in upstream

What you accomplished this week?
Fixed issues pointed out by Christian.
Refactored duplicate code into utility functions

Links to anything you’ve done this week
RR: https://reviews.reviewboard.org/r/6509/
Meeting minutes: https://reviewboardstudents.wordpress.com/2014/11/23/meeting-minutes-november-16-2014/

What you plan to do next week?
Get my RR ready for landing. Maybe attempt another SCM support

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

Any other questions?
None


Andrew Hong

What project are you working on?
Download review request attachments as tarball
Aggressive autcomplete bug

What you accomplished this week?
Implemented “contains” for users/groups/search resource query but ChipX86 pointed out that it will need a full rewrite for this feature to be implemented properly.
Tested autocomplete again to be sure it’s working as expected with “contains” feature removed.
Wrapped up downloading review request attachments as tarball; tested and all, seems to be working great 🙂
Awaiting reviews on both features.

Links to anything you’ve done this week
https://reviews.reviewboard.org/r/6402/
https://reviews.reviewboard.org/r/6333/

What you plan to do next week?
Go over both features to see if there are any other improvements to make.
Implement suggestions as they come from reviews.

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

Any other questions?
I’m wondering how I can pull in file attributes like last modified, date created, etc. from externally (S3) stored files?


Justin Maillet

What project are you working on?
I’m currently working on creating new extension hooks.

What you accomplished this week?
I’ve finished work on two of the three new hooks and their documentation.

Links to anything you’ve done this week
https://reviews.reviewboard.org/r/6573/
https://reviews.reviewboard.org/r/6597/
https://reviews.reviewboard.org/r/6572/
https://reviews.reviewboard.org/r/6600/
https://reviews.reviewboard.org/r/6613/

What you plan to do next week?
This coming week I plan to finish the last extension hook and it’s documentation, then get started on sandboxing those new extension hooks.
At some point I’ll also be fixing formatting errors that pep8 picks up in the different modules.

What, if anything, is blocking you from making progress?
Nothing is blocking my progress.

Any other questions?
None for now.

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)

Status Reports: Sept 14, 2014

Questions:

  1. If you didn’t have your development environment set up last meeting, do you have it set up now? If not, where are you blocked? (If you did have your development environment set up last week, just say “N/A”)
  2. What did you accomplish this week on Review Board in terms of EasyFix bugs? Please provide links to the bugs you’re working on, and any review requests you may have opened, or Hackpads you may have started.
  3. What do you plan to do next week?
  4. What is blocking you from making progress?
  5. Any other questions – anything at all!

Barret Rennie

  1. N/A.
  2. I’ve been working on two things:
    1. The student project idea for allowing `rbt post` and `rbt diff` to exclude local files from the command line. I have an idea on how to proceed, mainly by creating a filterdiff[1]-esque tool for each SCM. It is just a matter of getting diff output for cvs, svn (which I can set up this weekend to check), and clearcase. While working on this I stumbled across (ii).
    2. Bug 3565 [2], where rbt crashes when hg upstream is empty. I know why it is crashing, it is just a matter of determing what the correct behaviour should be when the upstream is empty. This bug may also apply to other SCMs (e.g., in git you /can’t/ diff against an empty upstream) so perhaps a overall fix for all SCMs is in order.
    3. My hackpad is at https://reviewboard.hackpad.com/Barrets-Devlog-0FyARzq47OT
  3. I plan on getting a rough sketch of the filterdiff working for at least hg and git this weekend, hopefully the other SCMs, too. I also want to see if bug 3565 applies to other SCMs and attempt a fix for it.
  4. I don’t know if I will be able to test (i.) on all SCMs (Perforce and Plastic appear to be commercial products).

Ryan Done

  1. N/A
  2. https://code.google.com/p/reviewboard/issues/detail?id=3452
    Picked out an easy fix bug to work on. I’ve sifted through some of the JS code and have *almost* narrowed down where I should be fixing this behaviour. I’ve also got a pretty good idea of how to fix it, or at the very least the behaviour that causes the bug. Also polished up on my git a bit. Will at the very least have a WIP review for Sunday going.
  3. Fixing the bug, and if I haven’t already get a review for that going. Then looking into project ideas, and packing for code sprint.
  4. Nothing quite yet.
  5. IRC has been pretty good about any immediate questions that come up.

David Kus

  1. Got my development environment set up using vmware instead of virtualbox.
  2. Fixed Issue 3542 Review requests allow drag-and-drop of attachments when not logged in.
    The fix has been merged.
    For reference:
    – Issue: https://code.google.com/p/reviewboard/issues/detail?id=3542
    – CodeReview: https://reviews.reviewboard.org/r/6312/Started working on Issue 3437 Double-clicking to Add a Comment in the Diff View Discards Any Comments Made
    – Issue: https://code.google.com/p/reviewboard/issues/detail?id=3437
  3. Continue working on Issue 3437. Once I get that done, I want to start looking deeper into the Student Projects and decide on which one I’d like to do.
  4. I have an idea for solving issue 3437, not sure if its the best way to go about it though. The solution I had in mind involves making it so that clicking on a comment flag for a comment that’s already open does nothing (instead of closing and reopening it). It’s a bit different from the way Christian(chipx86) was looking at it I think (from what I’ve gathered from his comment on the issue). I’d like to know what he thinks about it.
  5. None

Nicole Xin

  1. Last week I set up the dev environment with virtualbox since vagrant is unable to install the plugin for vmware_fusion. This week, it was solved by upgrading vagrant to v1.6.4, seems like a bug with the previous version.
  2. I’m working on issue 3532: https://code.google.com/p/reviewboard/issues/detail?id=3532 I’ve narrowed down where I should be fixing but I still struggling around here. And I’m also getting familiar with the review model. There’s also another bug found by ChipX86 during our talk on IRC, but I’m unable to file it as a new issue on google code. It’s basically a link to a comment with corresponding box expanded with ‘+’ button (while it should be a ‘-‘). The underlying behaviour is correct. Somehow I have no clue where this part locates. Too bad.
  3. I plan to work on these two bugs next week, get familiar with the code base, then spend some time checking out the project ideas.
  4. Issue 3532 “a link to a particular review” seems to have a similar behaviour to “a link to a particular comment”. As I mentioned above, what is blocking me is that I cannot find where this parts goes. Or, fundamentally, how do I know it’s a link to a comment or review, or review request? (Just so I can deal with them differently.) Uh, since I’m fairly new to web dev, lots of questions I guess.
  5. It just seems to me that issue 3444(confirmation when discarding a pending review) & 3300 (Autocomplete is too aggressive) is already fixed before the term start. Can someone confirm?

Justin Maillet

  1. N/A
  2. I have started looking into Issue 3300: Add a confirmation when discarding a pending review<https://code.google.com/p/reviewboard/issues/detail?id=3300&q=label:EasyFix&colspec=ID%20Type%20Status%20Priority%20Component%20Owner%20Summary%20Milestone> and I’m going to pick another one to work on this week. No hack pads or review requests yet.
  3. I plan to get the first issue fixed or at least reviewed and start working on the second. Also, I plan to setup a hack pad for notes.
  4. Django, and I’m having trouble figuring out how the code in review board is organized. I’m going to have to work though it this coming week.
  5. Will we be going back to the hotel each night during the code sprint? or sleeping there?
    How do tickets, tokens and the buses in toronto work?
    Can someone help me get an understanding of how the code is organized?
    How do we get review board running in our test environment to test if the bug has been fixed?
    Also any tips on traveling by plane?

Andrew Hong

  1. N/A
  2. Not much but I am figuring out my way around the code so I have found the class which may be causing bug: (Autocomplete is too agressive) https://code.google.com/p/reviewboard/issues/detail?id=3444
  3. Tackle #2 and trying to learn Django from various web sources at the same time. Also try to get more familiar with the code base.
  4. Django is, but I’m working on it 🙂
  5. Nope. Just an FYI, I will be logging attending the team meeting through my phone this Sunday, so if I’m slow to respond, you’ll know why.

Azad Salahli

  1. I’ve set up my dev environment by hand (without a VM).
  2. I’ve looked at EaseFix bugs, and is interested in issue #3438 (https://code.google.com/p/reviewboard/issues/detail?id=3438)
  3. I’m planning to get myself familiar with the code organization next week. In the meantime, I’ll be fixing #3438 trying to get it done until I arrive in Toronto.
  4. My poor understanding of the code structure prevents me from focusing on the bug itself. Somehow, I cannot reproduce the bug (I’m not getting any emails at all when a user registers) and I don’t know if I should configure something in order to get admin emails.
  5. See #4

Mark Loyzer

  1. I DO! have my development environment setup.  Thanks to Mike and Chris for helping me out.
  2. I initially started looking into issue 3438 (https://code.google.com/p/reviewboard/issues/detail?id=3438) but after a long time I just couldn’t reproduce the issue.

    @Azad: Mike suggested this to me: https://docs.djangoproject.com/en/dev/topics/email/#console-backend

    It supposedly redirects emails to the console/file (depending on which setting you choose). So, after not being able to reproduce issue 3438 I decided to delve into issue 3452 (https://code.google.com/p/reviewboard/issues/detail?id=3452) and now see that Ryan is working on it too…

  3. I’d like to get an Easy fix done.  I planned to get a [WIP] checked in but I didn’t know Ryan was also working on this bug too and he said he was going to submit a WIP.  As of right now I’m not sure which bug I will focus on because it doesn’t make sense for two people to fix the same bug.
  4. Lack of knowledge regarding Django and how the code is structured and functions within the application.
  5. How should we determine which work item we do?  Like I started working on issue 3452 but now I see that Ryan has started to as well.  Should we post a comment in the issue’s detail page to grab dibs?