[06:17] Good morning! === adeuring changed the topic of #launchpad-reviews to: on call: adeuring || reviewing: - || queue: [] || This channel is logged: http://irclogs.ubuntu.com === henninge_ is now known as henninge [09:56] henninge: care to review? [09:56] https://code.edge.launchpad.net/~jtv/launchpad/bug-440445/+merge/12777 [10:03] jtv: so you are assuming that it will also need access to language and translator? [10:04] henninge: yes; it's looking at TranslationGroup to check for membership, and that definitely involves Translator. [10:04] jtv: valid assumption, I guess. [10:05] jtatum: r=me [10:05] Not sure about Language, but it's one of those things that are likely to be fetched by the ORM as a side product [10:05] henninge: no, the other person. :) Thanks. [10:05] jtv: r=me [10:05] :) [10:05] jtatum: whatever branch you want reviewed, I have *not* approved it! ;-) [10:05] jtatum: sorry to bother you ... [10:07] jtv: please remember to set the mp to "approved". For some reason I cannot do it. [10:07] henninge: sure, thanks [10:19] hello adeuring -- could you please have a look at a small/simple branch? [10:19] it is here: https://code.launchpad.net/~al-maisan/launchpad/cmsg-425800/+merge/12774 [10:19] al-maisan: I'm already doing this :) [10:19] adeuring: ah, wow! [10:20] adeuring: I am duly impressed :) [10:29] al-maisan: there are two new tests: test_requestMirror_doesnt_demote_branch() and test_requestMirror_can_promote_branch(). Perhaps I'm missing something obvoius there -- but isn't future_time in the latter test a timestanp in the past: future_time = datetime.now(pytz.UTC) - timedelta(days=1) ? [10:29] * al-maisan looks [10:30] adeuring: argghh [10:30] the diff generated for the MP is wrong [10:30] just a minute [10:31] adeuring: http://pastebin.com/m4c94727b [10:32] sorry about that [10:32] this is a *really* simple branch [10:32] bbiab [10:32] al-maisan: Ah, indeed quite much shorter ;) [10:32] al-maisan: but the other test is there nevertheless ;) [10:33] adeuring: aha [10:34] adeuring: I don't see any of that in the diff I provided you with [10:34] * al-maisan rubs his eyes [10:34] al-maisan: what I menat: I am simply curious about this test, even it is is not part of the patch. [10:35] adeuring: yep, 'future_time' would have to lie in the past .. provided the time on the test machine is set correctly :) [10:37] * adeuring is quite confused... future time in the past? [10:37] al-maisan: but let's put that aside. [10:38] al-maisan: al-maisan: just one question about the short diff: You removed the text "Did you mean to upload to a PPA?". Isn't this sometimes a useful hint for users? [10:38] adeuring: not really .. it would rather confuse people [10:38] al-maisan: OK, so r=me [10:39] adeuring: thanks [10:41] adeuring: Fancy a devscripts.ec2test branch to review? [10:41] https://code.edge.launchpad.net/~allenap/launchpad/ec2-test-race-bug-422433/+merge/12778 [10:41] allenap: sure [10:41] adeuring: Thanks :) [10:51] al-maisan: don't remove that error text [11:03] bigjools: alright; what do you suggest instead? [11:03] al-maisan: I don't have any context - what are you doing? [11:04] bigjools: https://bugs.edge.launchpad.net/soyuz/+bug/425800 [11:04] Bug #425800: confusing message if signer can upload packages from some package sets but not others

[11:05] al-maisan: that error text is still correct, but only if the uploader has no packagesets [11:05] he can upload to [11:06] bigjools: that's not quite right. [11:06] this error message indicates that the uploader has no upload permissions whatsoever [11:06] al-maisan: yes, that's exactly what I mean [11:07] so the existing check for that error text needs to also check packagesets [11:07] however, he may end up in this situation if he has *some* package set based permissions but is lacking one [11:07] no, that is wrong [11:08] hmm .. I looked at the code .. the source package based permissions (including package sets) are checked before this error is raised [11:09] the error should be raised if the upload has no component-based *and* no packageset-based permissions [11:09] is that error (formulated that way) useful in first place? [11:10] yes - it distinguishes between NO permissions and SOME permissions but the wrong ones [11:10] aha [11:10] PPA packages fall foul of it a lot, hence the extra text [11:11] * al-maisan takes another look at verify_upload() [11:12] al-maisan: basically if the uploader is trying to upload to say, main, but only has universe permissions, this error is not used [11:12] the same needs to apply with packagesets [11:12] bigjools: I see what you mean. [11:12] great [11:50] allenap: r=me [11:50] adeuring: Thanks :) [11:51] allenap: what I forgot to say iin the review: thanks for splitting the diff into two parts! [11:52] adeuring: I'm glad it helped :) === mrevell is now known as mrevell-lunch === mrevell-lunch is now known as mrevell === matsubara-afk is now known as matsubara === matsubara_ is now known as matsubara [13:55] adeuring: I replied to your review of ec2-test-race-bug-422433 with an incremental diff. Could you sanity check it, then I'll send it all off to ec2? Thanks. === barry changed the topic of #launchpad-reviews to: on call: adeuring, barry || reviewing: -,- || queue: [] || This channel is logged: http://irclogs.ubuntu.com [14:02] allenap: looks good. Thanks for the change! [14:02] adeuring: Thanks. [14:19] barry, have you checked some bug pages also? The "Bug #440220 reported by Barry Warsaw 16 hours ago" is also using the registration slot iirc... [14:19] Bug #440220: registering slot interferes with watermark [14:19] which means that it will *always* wrap (unless you've got a 2m screen ;) ). [14:20] noodles775: right. let me paste a bug page s/s [14:23] noodles775: attached two more s/s. they do always wrap but it doesn't look too bad to me [14:26] barry, I think the thing I don't like is that it's wrapping when it doesn't need to. [14:27] Have you tried leaving it where it was after adding the no_wrap on the app-tabs? [14:28] noodles775: it will run into the tabs if the registering contents are long (long person name). maybe that's rare enough to not worry about it though? [14:30] barry, if you give the registration slot a large left-margin (maybe 2em) it shouldn't get too close? (and will wrap since the app tabs can't, iirc?) === Ursinha-afk is now known as Ursinha [14:30] noodles775: e.g. this does not look as good to me: http://launchpadlibrarian.net/32862698/ff-nowrap.png [14:31] noodles775: that's a possibility too. i kind of like that the registering information sits neatly above the portlet [14:33] barry, hmm.... to me it often (ie. with longer slots like on bugs pages) looks like it's been squashed into a space that's too small. [14:34] barry, I personally agree with rockstar's comment - if it was just below the app tabs, it wouldn't need to wrap at all, and would not interfere... but maybe there's a reason for not doing that? [14:34] noodles775: it is a pretty small space (21%). [14:34] noodles775: i definitely want to explore that, but iirc beuno really wants that registering slot on the right side [14:35] noodles775: so i kind of wanted to run that to ground to see if we could make it work [14:35] barry, it would be on the right side wouldn't it? I just mean a little bit down from where it is in the last link above. [14:36] noodles775: hmm... possibly so. give me a few minutes and i'll try that [14:36] k [14:36] btw, thanks! [14:37] np! [14:48] noodles775: take a look at the s/s in comments 13-16 [14:51] barry, what do you think? My first thought is that the only thing that looks ugly about it is the super long username? [14:53] noodles775: agreed. i just did that to see worst case. this works for me [14:55] barry, great. Thanks for fixing it! [14:55] noodles775: thanks! can i request you as one of the two ui reviewers for the branch? [14:55] barry, sure! [14:55] awesome [15:07] adeuring: ping [15:07] barry: yes? [15:07] adeuring: have time for a 39 line CSS review? [15:07] barry: sure [15:07] cool. sending mp now [15:20] barry: I have also an MP (a bit larger than yours, but just trivial refactoring): https://code.edge.launchpad.net/~adeuring/launchpad/hwdb-refactor-haldevice-3/+merge/12733 could you review it? [15:26] adeuring: sure! this feels so incestuous though :) [15:26] barry: ;) [15:33] barry: r=me === Ursinha is now known as Ursinha-lunch [16:06] adeuring: thanks! === barry changed the topic of #launchpad-reviews to: on call: adeuring, barry || reviewing: -,adeuring || queue: [] || This channel is logged: http://irclogs.ubuntu.com [16:11] adeuring: did you request me as a reviewer on your branch? [16:11] barry: no [16:11] adeuring: url? [16:11] barry: https://code.edge.launchpad.net/~adeuring/launchpad/hwdb-refactor-haldevice-3/+merge/12733 [16:22] hello adeuring, can you please have a look at the fix for bug #425800 (2nd try) ? [16:22] Bug #425800: confusing message if signer can upload packages from some package sets but not others

[16:22] https://code.launchpad.net/~al-maisan/launchpad/cmsg-2-425800/+merge/12791 [16:22] al-maisan: sure [16:22] adeuring: thanking you :) [16:26] adeuring: when you raise NotImplementedError in your properties, you don't need to instantiate them if they have no arguments. Python will DTRT for you here, and it's also somewhat more efficient [16:27] barry: Ah, thanks! didn't know that [16:27] (Though I hope the excpetion will never be raised ;) [16:28] adeuring: if the exception has arguments, instantiating it in place is the right thing to do though. [16:28] adeuring: right! :) [16:32] al-maisan: r=me [16:33] adeuring: thank you very much! [16:33] al-maisan: immer gerne :) [16:33] adeuring: :) [16:38] adeuring: with that fix, r=me [16:38] barry: thanks! === barry changed the topic of #launchpad-reviews to: on call: adeuring, barry || reviewing: -,- || queue: [] || This channel is logged: http://irclogs.ubuntu.com [16:40] rockstar: ping [16:40] barry, yo [16:41] rockstar: hiya! could you do a quick second ui review on https://code.edge.launchpad.net/~barry/launchpad/440220-slot/+merge/12786 [16:41] barry, sure. [16:41] rockstar: thanks! [16:41] barry: Can you take a look at small help branch? https://code.edge.launchpad.net/~matthew.revell/launchpad/bug-215798/+merge/12792 [16:42] mrevell: sure thing [16:42] thanks barry [16:43] mrevell: did you forget to bzr add register-branch.html perhaps? it's not in the diff [16:43] * mrevell hangs head in shame [16:44] barry: Yup. Sorry. Pushing new version now [16:44] barry: done [16:44] mrevell: can you add the file as a comment. i don't think the mp will update the diff [16:44] mrevell, I have bzr ci aliased to `bzr commit --strict` and I use that all the time. It makes sure you don't have any rogue files laying around. [16:44] rockstar: +1 [16:44] barry, it will update the diff. [16:44] barry: I thought we had moving diffs now [16:44] rockstar: thanks for the top! [16:45] barry, it updates the diff on push of the source branch now. [16:45] rockstar: \o/ [16:45] s/top/tip [16:45] barry, abentley is pretty amazazing. [16:45] rockstar: he's amazing, too! [16:46] :) [16:46] barry, I was using a word from a movie, so the apparent typo was actually purposeful. [16:46] rockstar: oh! what movie? [16:48] barry, it's from "The Benchwarmers" The dialogue goes something like "You guys think you're ath-a-letes now?" "I didn't know athletes had three syllables. That's amazazing!" [16:48] mrevell: r=me [16:48] thanks barry [16:48] rockstar: guess i have to put that on my netflix now :) [16:49] barry, it's appropriate for Max too. [16:52] rockstar: great review quotes: "filled with sprays of vomit, fountains of spit and enough hot body air to launch a flotilla of passenger balloons" and "a movie packed with so many idiot characters that rob schneider is cast as the cool guy -- and sort of pulls it off" [16:52] * barry clicks add to queue :) [16:52] barry, :) === Ursinha-lunch is now known as Ursinha === adeuring changed the topic of #launchpad-reviews to: on call: barry || reviewing: - || queue: [] || This channel is logged: http://irclogs.ubuntu.com [21:58] EdwinGrubbs, rockstar ping [21:58] barry: pong [21:59] hi EdwinGrubbs can you take a quick look at a testfix diff for me? [21:59] EdwinGrubbs: http://pastebin.ubuntu.com/284096/ [22:02] barry: is the facetmenu class have any stylesheets, or is it just a marker? [22:02] EdwinGrubbs: it has some css, but base-layout.txt doesn't test it. it is a horrible horrible test, so this just repairs it in the easiest possible way [22:05] barry: r=me [22:06] EdwinGrubbs: thanks! testfix here i come === rockstar` is now known as rockstar