=== Ursinha is now known as Ursinha-afk
[04:53] <wallyworld_> any on-call reviewers around?
[05:07] <mwhudson> wallyworld_: i only play one on tv
[05:07] <mwhudson> wallyworld_: link me up?
[05:07] <wallyworld_> :-) here it comes...
[05:07] <wallyworld_> https://code.edge.launchpad.net/~wallyworld/launchpad/recursive-branch-resolution-failure/+merge/36403
[05:07] <wallyworld_> thhanks in advance
[05:10] <mwhudson> wallyworld_: http://launchpadlibrarian.net/56281944/Screenshot-1.png is pretty funny :-)
[05:11] <wallyworld_> yes, except if it was your code that introduced the behaviour :-/
[05:39] <mwhudson> wallyworld_: do you think you can change the name of the 'url' variable in redirect_branch ?
[05:40] <mwhudson> wallyworld_: perhaps to referer or http_referer ?
[05:40] <wallyworld_> sure. how about "mary"?
[05:40] <wallyworld_> :-)
[05:40] <wallyworld_> ok.
[05:40] <wallyworld_> that's a better name for sure
[05:40] <mwhudson> well, meaningless *might* be better than confusing....
[05:40] <mwhudson> otherwise fine
[05:40]  * mwhudson goes to do the clicky clicky
[05:40] <wallyworld_> cool. thanks.
[05:41] <wallyworld_> mwhudson: btw, tim let the url variable name through the first time around :-)
[05:41] <mwhudson> hm, really redirectSubTree(None) should explode
[05:41] <wallyworld_> yeah, on firefox and chrome it does
[05:41] <mwhudson> i mean serverside
[05:42] <wallyworld_> not sure, haven't looked into that code path
[05:42] <mwhudson> it's clearly a programming mistake
[05:42] <mwhudson> wallyworld_: it seems like it redirects to +branch/None :)
[05:42] <wallyworld_> yes, it does, and then it comes back into the redirect method and so on
[05:43] <wallyworld_> chrome/firefox after a while complain hat the target url can never be reached
[05:43] <mwhudson> right
[05:43] <wallyworld_> not sure what i did but it took me lots of stuffing around to even be able to get the screenshot attached to the bug report to show up
[05:44] <wallyworld_> so i just looked at the code and guess the problem
[05:44] <mwhudson> what i am saying is that calling redirectSubtree(None) should raise, say, AssertionError
[05:44] <wallyworld_> ack.
[05:44] <mwhudson> which would result in an OOPS
[05:44] <wallyworld_> +1
[05:44] <mwhudson> i guess i can file that as a bug
[05:44] <wallyworld_> want me to do a drive by?
[05:44] <wallyworld_> on second thoughts, too much other stuff could potential be affected
[05:45] <mwhudson> wallyworld_: not really
[05:45] <wallyworld_> best to do a new bug
[05:45] <wallyworld_> mwhudson:  i'll fix the variable and land it. thanks for the review
[05:46] <mwhudson> wallyworld_: np
[05:48] <mwhudson> wallyworld_: i filed https://bugs.edge.launchpad.net/launchpad-foundations/+bug/645751 btw
[05:48] <_mup_> Bug #645751: Navigation.redirectSubTree(None) should OOPS <Launchpad Foundations:New> <https://launchpad.net/bugs/645751>
[05:49] <wallyworld_> mwhudson: thanks. i looked at the variable name and it should not be called http_referer. it is actually the target url that will be redirected to. how about then "target_url"
[05:50] <wallyworld_> it is just initially set to the refer
[05:50] <wallyworld_> i can add a comment in the code if you like as well
[05:50] <mwhudson> oh
[05:50] <wallyworld_> just to make it explicitly clear
[05:50] <mwhudson> maybe then a comment to say that the only way it can be None is if the resolution failed _and_ there was no referrer?
[05:50] <mwhudson> by the "is None" check
[05:51] <wallyworld_> yes, ok. i think the main thing missing was some code comments to explain things
[05:52] <wallyworld_> i mentioned stuff in the merge proposal but not the code
[06:12] <wallyworld_> mwhudson: ping?
[06:12] <mwhudson> wallyworld_: hi
[06:13] <wallyworld_> there was a code path that i missed out
[06:13] <wallyworld_> if the branch was private, it could still redirect to a None url
[06:13] <wallyworld_> i fixed the code and added a new unit test
[06:13] <wallyworld_> can you please take a quick look?
[06:14] <mwhudson> wallyworld_: ok
[06:14] <wallyworld_> thks
[06:15] <mwhudson> wallyworld_: have you pushed the branch?
[06:15] <wallyworld_> yep. lp may be having a little snooze
[06:15] <mwhudson> wallyworld_: i bet the fact that bazaar.launchpad.net:22 has fallen over is stopping you
[06:15] <wallyworld_> bugger
[06:16] <mwhudson> wallyworld_: it's back now
[06:16] <wallyworld_> ack
[06:20] <wallyworld_> mwhudson: pushed this time. i didn't notice the error last time.
[06:24] <mwhudson> wallyworld_: looks fine
[06:24] <wallyworld_> excellent. thanks!
[06:24]  * mwhudson goes for the bus
=== noodles775 changed the topic of #launchpad-reviews to: On call: noodles775 || Reviewing: sinzui || queue: []  || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews
[09:10] <jml> can I get a quick review for https://code.edge.launchpad.net/~jml/launchpad/better-slave-mock/+merge/36417
[09:11] <noodles775> Sure
=== noodles775 changed the topic of #launchpad-reviews to: On call: noodles775 || Reviewing: jml  || queue: []  || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews
[09:24] <jml> noodles775, thanks.
[10:07] <noodles775> jml: all good, r=me
[10:08] <jml> noodles775,
[10:08] <jml> noodles775, cheers.
[10:09] <lifeless> noodles775: hi
[10:09] <noodles775> Hi lifeless
[10:09] <lifeless> noodles775: could you please try reenabling that test.
[10:09] <lifeless> it was fixed, on devel, I'm positive.
[10:10] <noodles775> jml: did you get to check the rev of devel when your branch failed ec2?
[10:10] <jml> noodles775, no, I didn't.
[10:11] <jml> noodles775, no one gave the revno of the threatened fix, and things are pretty hectic around here
[10:11] <lifeless> there is a subtle problem in oops with getLastOopsReport; that test didn't need to use it, and my branch which is landed fixed that.
[10:11] <lifeless> I know it fixed it because it was landed *with* a branch suffering the problem.
[10:12] <noodles775> jml: if you can forward your ec2 failure email, I'll happily chase it up.
[10:13] <noodles775> lifeless: sure, I'm happy to re-enable it, I just want to check the revs. Which rev was your fix?
[10:13] <lifeless> dunno, I just know I got the 'its merged' mail.
[10:13] <jml> noodles775, http://paste.ubuntu.com/497557/ ; http://paste.ubuntu.com/497555/
[10:14] <lifeless> 11597
[10:14] <noodles775> Great, re-enabling now... thanks jml, lifeless
[13:15] <leonardr> noodles775, yesterday i got a ui review from edwin. i'm making his suggested changes, and then i'd like to put my branch in the queue for a ui/code review from you
[13:16] <noodles775> leonardr: sure - one of us will take a look (I think allenap will be around too).
[13:16] <leonardr> thanks
[13:19] <noodles775> leonardr: actually, if you mean the rename-grant-permissions branch, I think lifeless was keen to have a lot more discussion on the dev list about it? (See comments on the MP).
[13:20] <leonardr> indeed, i was looking right at it
[13:20] <noodles775> Great.
[13:21] <noodles775> leonardr: the need for discussion about those changes came up earlier today on irc (lp-dev) - you can find it in the irclogs (search for your mp url) in case it's useful.
=== mrevell is now known as mrevell-lunch
=== noodles775 changed the topic of #launchpad-reviews to: On call: - || Reviewing: -  || queue: []  || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews
=== Ursinha-afk is now known as Ursinha
=== mrevell-lunch is now known as mrevell
=== allenap changed the topic of #launchpad-reviews to: On call: allenap || Reviewing: -  || queue: []  || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews
[14:27] <bac> hi allenap, i have a branch that is super-sized with a diff of 1432 lines.  can you review it?
[14:27] <allenap> bac: Yeah, sure. I'm just having a look at jam's branch right now, deciding if I should take it on.
=== allenap changed the topic of #launchpad-reviews to: On call: allenap || Reviewing: jam || queue: [bac] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews
[14:28] <bac> allenap: ok, thanks.  https://code.edge.launchpad.net/~bac/launchpad/bug-643538-code/+merge/36377
=== allenap changed the topic of #launchpad-reviews to: On call: allenap || Reviewing: bac || queue: [] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews
[14:32] <allenap> bac: I get conflicts merging into devel.
[14:33] <bac> allenap: oh, sorry.  let me fix that
[14:33] <allenap> bac: It's fairly trivial I think.
[14:49] <allenap> bac: It merges cleanly into the very latest devel. Sorry, I last pulled it a few hours ago.
[14:49] <bac> allenap: really?  i got conflicts.
[14:50] <bac> i've merged and am pushing it up now
[14:50] <allenap> bac: Weird!
[14:51] <bac> allenap: just pushed r 11528
[14:57] <noodles775> salgado: Hi! I was going to ask henninge - but he's not available. Are you up for a pre-lim. UI review?
[14:57] <noodles775> https://code.edge.launchpad.net/~michael.nelson/launchpad/644328-blacklist-ui/+merge/36442
[15:02] <salgado> noodles775, sure!
[15:07] <noodles775> Thanks salgado
[15:12] <james_w> noodles775: very nice
[15:12] <noodles775> Thanks james_w :)
[15:12] <james_w> noodles775: that's all for real?
[15:14] <noodles775> james_w: yep - you can run it with the instructions on the MP. But there's still lots missing (like the actual generation of the diffs etc.).
[15:16] <salgado> noodles775, what does blacklisting do here?
[15:16] <james_w> noodles775: that's great
[15:16] <james_w> noodles775: thanks for your work on this
[15:17] <noodles775> james_w: np - I'm enjoying the UI work :)
[15:17] <noodles775> salgado: it allows you to hide differences that you don't care about in your derived distroseries.
[15:17]  * noodles775 checks to see if the LEP has a better description.
[15:20] <noodles775> salgado: see the section 'Marking a package as blacklisted/ignored in the thread at: https://lists.launchpad.net/launchpad-dev/msg04536.html
[15:21] <salgado> oh, that clarifies things.  I was going to ask what's the difference between blacklisted and ignored (which is what I see in one of the mockups)
[15:22] <salgado> noodles775, so, when you blacklist all versions of something, they show up at +missingpackages?
[15:24] <noodles775> salgado: no - missing packages is just for packages that are in the parent series, but not the derived. You will see the blacklisted items on this same page, but only when you select the checkbox in the filter options ('include blacklisted packages' on the mock, not yet implemented).
[15:24] <noodles775> bug 641290
[15:24] <_mup_> Bug #641290: Add search for DistroSeriesDifferences <derivation> <Soyuz:Triaged> <https://launchpad.net/bugs/641290>
[15:34] <salgado> noodles775, this may not be pertinent, but have you thought about what the "This difference has been blacklisted ..." text will look like for blacklisted packages when the search allows them to be shown?
[15:36] <noodles775> salgado: Yes, a little. Part of the reason for graying them out and using that text was that it would/should be consistent with results when searching. Can you see a situation or reason why it would need to be different?
[15:39] <salgado> noodles775, I like the graying out because it makes it easy to see everything that was changed at a glance and maybe undo, but the text (as well as the links) is a bit too verbose (IMHO) and don't allow switching between all different states, so I was thinking of using 3 radio buttons there.  "Blacklisted: (*) No,  ( ) This version,  ( ) All versions"
[15:40] <salgado> but that's more like a nice-to-have thing, not that important
[15:42] <noodles775> salgado: that actually sounds a bit like what I'd proposed on the mockup (a single 'Add to blacklist' that would then give you those options), but henninge convinced me on a previous MP that it was a bit too much... hangon, I'll find the discussion.
[15:43] <salgado> well, except that the radio buttons allow you to see all the possible states without having to click anything
[15:43] <salgado> just like the links do
[15:43] <salgado> but sure, maybe he'll be able to convince me as well. :)
[15:46] <noodles775> salgado: ok - if you search for blacklist at https://code.edge.launchpad.net/~michael.nelson/launchpad/distro-series-difference-browser2/+merge/34739 you'll see a bit of background.
[15:48] <noodles775> salgado: but I think your radio button option you mention could work too... Also, if you've got suggestions for making the text less verbose while still being easy to understand, let me know :)
[15:48] <noodles775> s/you/that you/
[15:49] <salgado> what I like about the radio button is that you see all the options without having to click anything and there's no repetition
[15:49] <salgado> the downside is that you need vertical real estate, but you seem to have plenty of that
[15:51] <salgado> as for the text of the link, the only thing I can think of is something like "Blacklist: _this version_ or _all versions_"
[15:51] <salgado> but I don't quite like that as it seems to suggest you need to blacklist something
=== benji changed the topic of #launchpad-reviews to: On call: allenap || Reviewing: bac || queue: [benji] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews
[15:55] <benji> my MP is at https://code.edge.launchpad.net/~benji/launchpad/wadl-refactoring/+merge/36452; should be a quick one
[15:56] <noodles775> salgado: Yeah. RE the radio buttons, I was going to say that it would require two clicks (ie select/submit), but it doesn't of course... we could do the api update when an option is selected quite easily. And that would mean no need for 'Undo'. I like it more and more :)
[15:58] <salgado> indeed, I hadn't thought about the undo
[16:04] <abentley> EdwinGrubbs, I had to tweak my branch slightly since you reviewed it.  Do you mind taking another look? https://code.edge.launchpad.net/~abentley/launchpad/fix-groupby/+merge/36360
=== deryck is now known as deryck[lunch]
[16:11] <salgado> noodles775, I think that's the only suggestion I have.  shall I paste our chat there for future reference?
[16:11] <noodles775> salgado: yeah, that'd be great, thanks.
[16:13] <salgado> noodles775, np.  glad you liked my suggestion. :)
[16:13] <noodles775> I do indeed!
[16:21] <salgado> noodles775, if I can make another suggestion (this one not UI related), you could do a 'make newsampledata' after running http://pastebin.ubuntu.com/494656/ and merge just those changes to make it easier to play with that page on lp.dev in the future. :)
[16:22] <noodles775> salgado: you'd prefer that? I thought we wanted to get rid of sample data (maintaining it etc.), and we can make it a one-liner to add the demo data?
[16:23] <salgado> noodles775, we want to get rid of the test sample data.  if you run that script and run 'make sampledata', you'll be modifying just the demo data. :)
[16:23] <salgado> they're separate
[16:24] <salgado> when you run 'make sampledata', the changes done to launchpad_dev end up in the demo data and the ones done to launchpad_ftest end up in the test sample data
[16:25] <noodles775> Yep - makes sense..., but currently that flag adds a feature flag too, do you think that's valid for the sample data? I'd think the dev server should have the same features as production by default?
[16:26] <noodles775> s/that flag/that script/
[16:27] <salgado> that's a good question. I'd be in favour of enabling everything in .dev, but I can see why that may not be desired
[16:44] <bac> allenap: have time for another, very straightforward one?  or are you sick of me?
[16:44] <bac> allenap: it is for bug 644550
[16:44] <_mup_> Bug #644550: Robots can index blueprints when it is not used <bridging-the-gap> <Launchpad Blueprints:In Progress by bac> <https://launchpad.net/bugs/644550>
=== bac changed the topic of #launchpad-reviews to: On call: allenap || Reviewing: bac || queue: [benji, bac] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews
[16:45] <allenap> bac: Yeah. I'm nearly finished on your first branch, so sure.
[16:45] <bac> allenap: thanks.
[16:45] <allenap> Unless benji's is *huge* :)
[16:45] <bac> allenap: but, alas, i must step out, so feel free to skip if you have questions.
[16:46] <benji> nah, my branch is small and in fact all the code has been through review once before
[16:46] <allenap> Cool.
[16:46] <allenap> bac: No worries.
=== allenap changed the topic of #launchpad-reviews to: On call: allenap || Reviewing: benji || queue: [bac] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews
=== deryck[lunch] is now known as deryck
=== benji is now known as benji-lunch
=== Ursinha is now known as Ursinha-lunch
=== matsubara is now known as matsubara-lunch
=== jcsackett changed the topic of #launchpad-reviews to: On call: allenap || Reviewing: benji || queue: [bac, jcsackett] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews
[17:16] <jcsackett> allenap: MP for code review is here: https://code.edge.launchpad.net/~jcsackett/launchpad/unknown-translations-service-643545-0/+merge/36464
[17:16] <allenap> jcsackett: I'm probably not going to get to it today, sorry :-/
[17:16] <jcsackett> allenap: that's fine. i'll find someone else then. just saw you in the on call section. :-)
[17:17]  * jcsackett has yet to figure out when people's days end.
[17:17] <allenap> jcsackett: Cool. I would do it, but I'm near my EOD.
[17:17] <jcsackett> i gathered. as i said, no worries, allenap. have a good evening. :-)
[17:18] <allenap> You too :)
=== salgado is now known as salgado-lunch
=== allenap changed the topic of #launchpad-reviews to: On call: allenap || Reviewing: bac || queue: [jcsackett] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews
[17:37] <allenap> bac: I'll be back online in ~3 hours. I can review your branch then if someone else hasn't gotten there first.
=== allenap changed the topic of #launchpad-reviews to: On call: - || Reviewing: - || queue: [bac, jcsackett] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews
=== gary_poster is now known as gary-lunch
[17:42] <jcsackett> anyone available for a code review?
[17:45] <jcsackett> nm. i'll ping again in a few.
=== jcsackett is now known as jcsackett|lunch
=== benji-lunch is now known as benji
[18:27] <bac> allenap: thanks for the review!
[18:29] <bac> hey rockstar, you reviewing today?
[18:29] <rockstar> FUUUUUUUUUUUUUU...
[18:29] <rockstar> bac, yes
[18:29]  * rockstar hangs head in shame
=== gary-lunch is now known as gary_poster
=== rockstar changed the topic of #launchpad-reviews to: On call: - || Reviewing: - || queue: [bac, jcsackett] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews
[18:29] <bac> :)
=== rockstar changed the topic of #launchpad-reviews to: On call: rockstar || Reviewing: jcsackett || queue: [] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews
[18:30] <rockstar> bac, your review got done?
[18:30] <bac> rockstar: one did, one didn't
[18:30] <bac> the one that needs doing is pretty trivial
[18:30] <rockstar> bac, okay.  Lemme do jcsackett's really quick, and then I'll do yours.
[18:31] <bac> no rush
=== bac changed the topic of #launchpad-reviews to: On call: rockstar || Reviewing: jcsackett || queue: [bac] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews
[18:35] <rockstar> jcsackett|lunch, thanks for PEP8ing tales.py
[18:44] <rockstar> jcsackett|lunch, so configure_translations and configure_blueprints head to the same URL?  That doesn't make much sense to me.
=== salgado-lunch is now known as salgado
[18:51] <EdwinGrubbs> abentley: I'll look at it now.
[18:54] <rockstar> jcsackett|lunch, why did you remove the Copyright header in lib/lp/translations/tests/test_hastranslationtemplates.py
[18:55] <EdwinGrubbs> abentley: that looks good
[18:55] <EdwinGrubbs> abentley: r=me still
[18:55] <abentley> EdwinGrubbs, thanks.
=== jcsackett|lunch is now known as jsackett
[19:01] <jsackett> rockstar: copyright notice was a goof, corrected.
[19:03] <jsackett> configure_blueprints and configure_translations are both links to edit, which is the place to set up both of those fields. configure_blueprints was added in the unknown-blueprints branch so that the template could be relatively consistent; the same intent and method was borrowed for this branch with configure_translations.
[19:03] <jsackett> (this is for the distribution, on product they point to different spots).
[19:03] <jsackett> rockstar ^
[19:03] <rockstar> jsackett, okay.
[19:03] <rockstar> jsackett, I think this looks good.  Thanks for the patch.
[19:04] <jsackett> rockstar: thanks for the review. :-)
=== matsubara-lunch is now known as matsubara
=== Ursinha-lunch is now known as Ursinha
[20:15] <leonardr> rockstar, trivial review coming up for you
[20:15] <rockstar> leonardr, ack
=== rockstar changed the topic of #launchpad-reviews to: On call: rockstar || Reviewing: leonardr || queue: [] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews
[20:27] <leonardr> rockstar: https://code.edge.launchpad.net/~leonardr/lazr.restful/remove-26ism/+merge/36493
[20:27] <rockstar> leonardr, great.  I'll take a look when I'm off the phone.
[20:34] <jsackett> salgado: ping
[20:34] <salgado> hi jsackett
[20:35] <jsackett> hi, salgado. sinzui has once again directed me to you for round one of ui review on an MP
[20:35] <jsackett> https://code.edge.launchpad.net/~jcsackett/launchpad/unknown-translations-service-643545-0/+merge/36464
[20:35] <jsackett> (this time there's sample data that shows the setup :-P)
[20:36] <jsackett> if you could take a look at it sometime in the near future, salgado, i would appreciate it. no super rush--i'm not looking to land it today.
[20:38] <bac> thanks rockstar
[20:39] <salgado> jsackett, oh, sample data is cool, thanks.  I may not get to it today, but in the worst case I'll review it tomorrow
[20:39] <jsackett> salgado: sounds good. thanks!
[20:39] <salgado> dev sample data is cool, I mean
[20:39] <jsackett> salgado: yes. the testing kind is somewhat irritating. :)
[20:41] <salgado> jsackett, you're being kind.  but I guess you put it that way because you haven't been here for too long so they haven't caused you that much pain. ;)
[20:41] <jsackett> heheh.
=== rockstar changed the topic of #launchpad-reviews to: On call: - || Reviewing: || queue: [] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews
=== salgado is now known as salgado-afk