/srv/irclogs.ubuntu.com/2009/12/15/#launchpad-reviews.txt

=== _thumper_ is now known as thumper
=== thumper is now known as _thumper_
=== _thumper_ is now known as thumper
jmlhttps://code.edge.launchpad.net/~jml/launchpad/fix-release-hot-bugs-486437/+merge/16173 -- could someone please review this?02:44
thumperis there a test yet?02:48
jmlthumper, I don't recall anyone asking for one.02:50
jmlthumper, or making suggestions as to where said test should go.02:51
thumperwhere is it tested now?02:51
jmlthumper, there's a page test.02:53
jmlthumper, which still passes02:53
jmlthumper, since firefox has no fix released bugs in the sample data.02:54
thumperhe02:54
thumpernot much of a test then is it?02:54
thumperonly saying the things I've learnt to say from you :)02:54
jml*grumble grumble*02:56
thumperhttps://code.edge.launchpad.net/~thumper/launchpad/fix-review-comment-field-enablement/+merge/1617802:58
thumpernow that one is hard to test02:58
thumperin fact I have no idea even if it is possible with windmill02:58
jmlthumper, have you tried?02:59
thumpertried to test with windmill, or tried the fix?03:00
jmlthumper, the former.03:00
thumperjml: I've looked at the windmill api03:00
jmldoesn't it let you record tests?03:00
thumperand it isn't obvious how to tab to the next field03:00
thumperah yeah03:00
thumpernot tried to do that03:00
thumperany idea how?03:00
jmlthumper, nope.03:01
jmlthumper, happy to say I've avoided the whole mess :)03:01
jmlthumper, changing the status of a bug is bloody difficult.03:12
thumperjml: why?03:12
thumperjml: why not just factory.makeBug?03:12
jmlthumper, because nothing else in the test does that.03:13
thumperjml: they have to learn some time :)03:13
* thumper sniggers03:13
jmlthumper, well, this patch isn't the place to teach them.03:13
thumper:)03:14
=== wgrant_ is now known as wgrant
jmlthumper, test added. code fixed.03:34
thumperjml: status=any(*UNRESOLVED_BUGTASK_STATUSES)03:37
jmlthumper, yet?03:37
jmlyes, rather?03:37
thumperdoes: status=UNRESOLVED_BUGTASK_STATUSES work?03:37
jmlthumper, no.03:37
thumperwhy?03:37
jmlthumper, that was the first thing I tried.03:37
thumperany is a generator isn't it?03:37
jmlthumper, no.03:37
jmlthumper, ha. ha. ha.03:37
jmlthumper, it's a thing specific to Launchpad03:38
jmlthumper, from canonical.launchpad.searchbuilder03:38
thumperhmm...03:38
thumperdamn03:38
jmlthumper, which I bet you didn't know existed03:38
thumpernope03:38
jmlthumper, 'any' is a generator by default03:38
jmlthumper, me neither :)03:38
thumperany becomes a keyword soon isn't it?03:38
jmlthumper, builtin, I think.03:38
thumperew03:39
thumperr=me03:39
jmlthumper, I don't think searchbuilder is that bad (he says without looking at the code)03:39
jmlthumper, but to me, it makes more sense to re-use storm primitives03:39
jmlthumper, thanks.03:40
jmlI'll land that next Monday!03:40
thumperhmm...03:40
thumperdamn javascript garbage collector03:41
=== abentley1 is now known as abentley
mrevellAnyone care to review my branch? It's for an RC -- https://code.edge.launchpad.net/~matthew.revell/launchpad/whatsnew-3112/+merge/1614710:02
=== adiroiban changed the topic of #launchpad-reviews to: on call: - || reviewing: - || queue [adiroiban(bug-492375 pre-implementation),adiroiban(bug-121520)] || This channel is logged: http://irclogs.ubuntu.com || https://code.edge.launchpad.net/launchpad/+activereviews
intellectronicamrevell: approved10:10
mrevellah, thanks intellectronica10:10
intellectronicamrevell: now you need to get an rc, though10:10
mrevellintellectronica, yeah, I've got Danilo right where I want him.10:11
danilo_mrevell, aha10:11
mrevell:)10:12
intellectronicamrevell: you do? can you tap him on the shoulder on my behalf?10:12
mrevellintellectronica, heh10:12
danilo_intellectronica, I've seen yours as well, will be getting to it shortly10:12
intellectronicadanilo_: great, thanks10:12
danilo_intellectronica, ok, so the code looks ok, I've got only some RC-related questions10:19
intellectronicayes?10:19
danilo_intellectronica, so, how much does this affect our users? is the tag editing completely broken? (I remember you experiencing something along those lines yesterday, but sinzui had no problems editing them for other projects)10:20
danilo_intellectronica, and, if it affects only a subset of our users, what subset is it? is it a subset we can direct to edge? how/when do you plan to QA this?10:20
intellectronicadanilo_: tbh it doesn't affect users all that much. only a subset of users can edit official tags anyway, and those have a workaround if they need to edit urgently10:21
intellectronicadanilo_: but the fix is small and doesn'ty touch anything else, and oem services have asked that we fix it asap10:21
danilo_intellectronica, right, what is that workaround?10:21
danilo_intellectronica, that's the other thing: it doesn't touch anything else? should this have a windmill test?10:22
intellectronicadanilo_: disable javascript and reload the page10:22
danilo_(i.e. something else to be touched :)10:22
danilo_intellectronica, ok, that sounds nasty, I think it's worth of an RC, but I'd like to see test added as well10:22
intellectronicadanilo_: ok, let me add a windmill test for this10:23
danilo_intellectronica, thanks, if it's going to be a big involved test, I don't want to pressure you, but if it's decently simple, I'd very much like to have it... at least right after the rollout, it doesn't have to be included in this landing10:23
intellectronicadanilo_: i'll see if i can modify the existing test to just add a dot somewhere and see that it doesn'10:24
intellectronicatt fail10:24
danilo_intellectronica, sounds good, thanks10:24
danilo_allenap, hi, I am looking at https://code.edge.launchpad.net/~allenap/launchpad/bugtracker-snapshot-bug-447100/+merge/16130 — looks very reasonable but the question is why RC? who are the affected users and how many of them are there?10:41
danilo_allenap, i.e. can we only fix it on edge after the rollout, would that be good enough for users likely to use it? (we do have both edge and production APIs as well now)10:42
allenapdanilo_: It probably is enough to just have it on edge.10:51
allenapdanilo_: There are probably not many affected users.10:51
allenapdanilo_: And it's not a frequently used page. At least, I hope it isn't; it's configuration.10:52
danilo_allenap, right, so I guess we can say no to this RC unless you feel really strongly about it? I don't want you to feel bad about it, but the more RCs we have, the bigger chances of something breaking with this little QA time we've got10:53
allenapdanilo_: Fair, I'm happy with that. I'll mention it to deryck when he's online. I doubt he'll have a problem with that.10:53
danilo_allenap, ok, thanks10:54
danilo_allenap, btw, I've marked your MP as 'approved' but that's for regular landing, RC is disapprove :)11:00
allenapdanilo_: No worries, I got that... but I was tempted to land and use the plausible deniability and ask forgiveness defences ;)11:02
danilo_hehe :)11:02
=== gmb changed the topic of #launchpad-reviews to: n call: gmb || reviewing: - || queue [adiroiban(bug-492375 pre-implementation),adiroiban(bug-121520)] || RC candidates take priority || This channel is logged: http://irclogs.ubuntu.com || https://code.edge.launchpad.net/launchpad/+activereviews
=== gmb changed the topic of #launchpad-reviews to: on call: gmb || reviewing: - || queue [adiroiban(bug-492375 pre-implementation),adiroiban(bug-121520)] || RC candidates take priority || This channel is logged: http://irclogs.ubuntu.com || https://code.edge.launchpad.net/launchpad/+activereviews
gmb*sigh*11:02
allenapdanilo_: I can't land anything in PQM. Do you know if it's it in testfix mode? It doesn't look like it should be...11:09
danilo_allenap, are you landing to db-devel or devel? (devel is already closed)11:09
allenapdanilo_: Oh rats.11:10
allenapdanilo_: When did devel close? I've been trying since yesterday afternoon.11:10
danilo_allenap, it was closed together with PQM, I wasn't aware of the option that it could be kept open and that I should have asked spm to do it in such a way11:11
allenapdanilo_: Okay. Just to check, is it safe to just submit my based-on-devel branch to db-devel?11:12
danilo_allenap, should be if all the devel revisions you have are already in stable11:12
allenapdanilo_: Cool, thanks.11:13
danilo_allenap, otherwise, it's still safe if all the tests pass for you :)11:13
salgadoadiroiban, around?11:22
adiroibansalgado: hi. yes.11:22
StevenKdanilo_, bigjools: Is my netbook-cron branch still going to make it in?11:23
bigjoolsStevenK: yes11:23
bigjoolsit landed already11:23
StevenKbigjools: That's odd, I see "Approved revision: not available" on the merge page11:23
salgadoadiroiban, hi there.  do you have the test failures for that branch handy?  In the merge proposal you said which test failed, but I'd like to see what the failure is11:23
=== matsubara-afk is now known as matsubara
adiroibansalgado: yes. let me paste it. 11:24
bigjoolsStevenK: I can assure you it landed11:24
adiroibansalgado: http://paste.ubuntu.com/341811/11:25
StevenKbigjools: So it's the merge page that's not correct. I can deal with that.11:25
salgadoadiroiban, is that the only test that failed?11:25
bigjoolsStevenK: what URL are you looking at?11:25
adiroibansalgado: the view needs to be refresh, as in that test the translationgroup is initialized with None11:25
adiroibansalgado: yes11:25
StevenKbigjools: https://code.edge.launchpad.net/~stevenk/launchpad/netbook-cron/+merge/1611511:26
bigjoolsStevenK: I landed a copy of your branch, that's why it didn't set yours11:26
adiroibansalgado: now, that we save the translationgroup on view it's like we cache it11:26
salgadoadiroiban, I see11:26
adiroibansalgado: and if meanwhile the group is changed, the view does not know about it.11:27
adiroibanbut is hard to have that behaviour in a real system11:27
adiroibanand with a page reload you will have the problem solved11:27
adiroibani mean you will see the new value11:27
adiroibanI  tried to force a refresh in the test, 11:27
adiroibanbut I don't know what I did wrong, as it did not solve the problem11:28
salgadoadiroiban, agreed.  I think the best thing is to change the test as it makes no sense for the test to expect that behavior from the view -- specially since it's not documented11:29
adiroibanwith those changed, I ran a new full test for translations module, and it was ok11:29
adiroibansalgado: ok. I will look into that11:30
adiroibansince this is not RC , i think there is no hurry11:30
adiroibanor do you want to land it now?11:30
salgadonot really, and even if I wanted, danilo_ wouldn't11:31
adiroibanalso there is a follow up for that branch to also clean those tests11:31
danilo_salgado, that's right :)11:31
adiroiban:)11:31
adiroibansalgado: thanks for looking into this 11:31
danilo_adiroiban, salgado: though, what you are discussing sounds like cachedproperties in a test, though I am just guessing11:32
adiroibandanilo_: nope11:32
adiroibanis just a = b.c , but c is none11:32
adiroibanso then b.c = True11:33
adiroibana is not updated11:33
salgadoadiroiban, the first thing I'd do is check that self.dsl.distroseries.distribution.translationgroup is not None11:33
danilo_adiroiban, I am not sure I follow you :)11:33
adiroibansalgado: and if it none ? what shall i do?11:34
adiroibanof why that check?11:34
salgadoadiroiban, then we need to find out why it's not being updated when you assign the group to self.distroseries.distribution.translationgroup11:35
adiroibanbecause view.translationsgroup is None11:36
adiroibanso it is not linked with self.distroseries.distribution.translationgroup11:36
salgadoadiroiban, I meant that after your change to the test, which creates a new view instance in your test method11:37
adiroibansalgado: ah :)11:37
adiroibanyep11:37
adiroibanI will look at that as I still need to learn how all those classed are linked and how does their timeline look like11:38
salgadoadiroiban, cool. don't hesitate to ask if you have any questions. :)11:39
adiroibansalgado: sure. np.11:39
gmbsalgado, adiroiban: To be clear, which branch are you talking about here? Is it one of the ones currently in the review queue in the /topic?11:46
adiroibangmb: nope11:46
gmbadiroiban: Righto, I'll crack on with reviewing those then.11:47
adiroibangmb: thanks. There is no hurry for them11:48
adiroibanthey are low priority11:48
gmbadiroiban: Maybe not, but I'm on-call reviewer and I'm bored ;)11:48
intellectronicadanilo_: i've added a windmill test and my branch has been re-reviewed. care to take a look again?11:48
=== gmb changed the topic of #launchpad-reviews to: on call: gmb || reviewing: adiroiban || queue [adiroiban(bug-492375 pre-implementation)] || RC candidates take priority || This channel is logged: http://irclogs.ubuntu.com || https://code.edge.launchpad.net/launchpad/+activereviews
danilo_intellectronica, sure, thanks!11:49
adiroibangmb: :D11:50
gmbadiroiban: So, to create a merged person, you do the following:11:50
gmbperson_1 = factory.makePerson()11:50
gmbperson_2 = factory.makePerson()11:50
gmbperson_2.merged = person_111:51
gmbThat then means that person_2 has been merged into person_1.11:51
gmbadiroiban: This branch really needs a test before it lands.11:51
adiroibangmb: ok. np. I will do that11:52
gmbadiroiban: Thanks. I'll mark the review as Needs Fixing for now.11:52
adiroibansure11:52
=== gmb changed the topic of #launchpad-reviews to: on call: gmb || reviewing: - || queue [adiroiban(bug-492375 pre-implementation)] || RC candidates take priority || This channel is logged: http://irclogs.ubuntu.com || https://code.edge.launchpad.net/launchpad/+activereviews
=== gmb changed the topic of #launchpad-reviews to: on call: gmb || reviewing: lunch || queue [adiroiban(bug-492375 pre-implementation)] || RC candidates take priority || This channel is logged: http://irclogs.ubuntu.com || https://code.edge.launchpad.net/launchpad/+activereviews
=== mrevell is now known as mrevell-lunch
=== bac changed the topic of #launchpad-reviews to: on call: gmb,bac || reviewing: lunch || queue [adiroiban(bug-492375 pre-implementation)] || RC candidates take priority || This channel is logged: http://irclogs.ubuntu.com || https://code.edge.launchpad.net/launchpad/+activereviews
StevenKbigjools: Excuse my complete ignorance about PQM, but it's normal that I can't see the changes in lp:~launchpad-pqm/launchpad/devel13:08
beunoStevenK, you should be able to see anything13:09
beunowhat is it you're trying to see specifically?13:09
StevenKbeuno: The changes from my netbook-cron branch in what will be released, since I'm slightly paranoid13:09
beunoStevenK, it seems to have been landed in db-devel13:11
beunohttp://bazaar.launchpad.net/~launchpad-pqm/launchpad/db-devel/revision/879013:11
beunowasn't UNR renamed to UNE?13:12
StevenKbeuno: Yes, hence the netbook name. It follows precedent from server.13:13
beunoah, missed that13:13
StevenKbeuno: So in documentation, it's the Ubuntu Server Edition, but the meta package and so on is ubuntu-server. So I'm doing the same thing with netbook.13:15
beunogotcha13:16
StevenKbeuno: Will db-devel turn into devel? What's the process there?13:17
beunoStevenK, db-devel gets rolled out to production directly, not edge daily13:18
beunoso, IIRC, it will merge into devel when the roll-out happens13:18
beunoif you're worried if it will land, don't worry, if it's in db-devel, it will  :)13:19
StevenKWhen it's scheduled to land?13:19
beunoI think roll-out is this week?  tomorrow or Thu?13:20
beunodanilos should know13:20
=== mrevell-lunch is now known as mrevell
beunoor flacoste13:21
danilosStevenK, it's going to be on production LP servers on Wednesday 22:00 UTC13:23
StevenKdanilos: Okay, thanks.13:24
danilosStevenK, if you really want the nitty gritty details, db-devel turns into db-stable when all the tests pass, and then we roll out whatever is in db-stable by 12:00 UTC on Wednesday13:24
StevenKdanilos: Ah!13:25
danilosStevenK, your revision (8790) is already in db-stable (up to 8794), so you should be safe13:25
StevenKdanilos: Excellent, thanks!13:26
bigjoolsStevenK: I told you to trust me ;)13:37
StevenKbigjools: You did, yes :-)13:38
=== gmb changed the topic of #launchpad-reviews to: on call: gmb, bac || reviewing: -, - || queue [adiroiban(bug-492375 pre-implementation)] || RC candidates take priority || This channel is logged: http://irclogs.ubuntu.com || https://code.edge.launchpad.net/launchpad/+activereviews
=== gmb changed the topic of #launchpad-reviews to: on call: bac || reviewing: -, - || queue [adiroiban(bug-492375 pre-implementation)] || RC candidates take priority || This channel is logged: http://irclogs.ubuntu.com || https://code.edge.launchpad.net/launchpad/+activereviews
* gmb goes off-call to fix an RC bug.14:53
gmbderyck: lp:~gmb/launchpad/windmill-test14:54
deryckgmb, got it.  thanks.14:54
derycktaking a break from your usually cleverly named branches? :)14:55
gmbderyck: Only the dupefinder got Hammer Horror titles ;)14:58
gmbI'm saving the next batch for bug heat :)14:58
deryckheh14:58
gmbRight. Coffee, then hacking.14:58
leonardrsalgado, do you have time to review https://code.edge.launchpad.net/~leonardr/launchpad/anonymous-oauth/+merge/16199? it shouldn't take long15:15
salgadoleonardr, otp15:15
salgadoleonardr, do you mind if I do it after lunch?15:24
=== irc.freenode.net changed the topic of #launchpad-reviews to: on call: bac || reviewing: - || queue [adiroiban(bug-492375 pre-implementation)] || RC candidates take priority || This channel is logged: http://irclogs.ubuntu.com || https://code.edge.launchpad.net/launchpad/+activereviews
allenapHi bac, got time for a short+simple review? https://code.edge.launchpad.net/~allenap/launchpad/bugwatch-bugtasks-as-a-list/+merge/1620015:40
bacallenap: for you?  sure!15:42
bacallenap: is this an RC candidate/15:42
bac?15:42
allenapbac: Thanks :) And no, not RC yet, but I might ask.15:42
=== matsubara is now known as matsubara-lunch
allenapbac: There's probably a bit too much lint cleaning in there - sorry, I got a little enthusiastic - but it's still quite short and understandable.15:43
=== deryck is now known as deryck[lunch]
gmbbac: Can you review https://code.edge.launchpad.net/~gmb/launchpad/checkwatches-keyerror-oops-bug-496988/+merge/16204 for me (RC candidate)16:17
=== bac changed the topic of #launchpad-reviews to: on call: bac || reviewing: gmb || queue [adiroiban(bug-492375 pre-implementation)] || RC candidates take priority || This channel is logged: http://irclogs.ubuntu.com || https://code.edge.launchpad.net/launchpad/+activereviews
bacgmb: in your final test it is not clear that either of those bug watches cause a key error.  is it b/c there is a bug 1 but not bug 2?  i think the casual reader might need more info there.16:23
mupBug #1: Microsoft has a majority market share <iso-testing> <ubuntu> <Clubdistro:Invalid> <Computer Science Ubuntu:Invalid by compscibuntu-bugs> <EasyPeasy Overview:In Progress by ramvi> <Ichthux:Invalid by raphink> <JAK LINUX:Invalid> <OpenOffice:Invalid by lh-maviya> <Tabuntu:Invalid by tinarussell> <Tivion:Confirmed for shakaran> <Ubuntu:In Progress by sabdfl> <ubuntu-express (Ubuntu):Invalid by jahyire2006> <Ubuntu Jaunty:In Progress> <16:24
bacthanks mup.  helpful!16:24
gmbbac: Ah, right. I'll add a comment.16:26
bacgmb: thanks, r=bac16:26
gmbbac: Ta.16:26
jpdsDo non-RC MPs get reviews too at the moment?16:27
bacjpds: depending on reviewer availability.16:27
bacjpds: i can do a review now if the changes are short16:27
jpdsI have: https://code.edge.launchpad.net/~jpds/launchpad/fix_450262/+merge/1620316:27
=== bac changed the topic of #launchpad-reviews to: on call: bac || reviewing: jpds || queue [adiroiban(bug-492375 pre-implementation)] || RC candidates take priority || This channel is logged: http://irclogs.ubuntu.com || https://code.edge.launchpad.net/launchpad/+activereviews
bacjpds: that's pretty short.  i'll do it now.16:28
bacjpds: in addition you'll need to get a UI review.  it is always welcome to have screen shots for those added to the bug and/or referenced from the MP16:34
bacjpds: also there is no test for this new functionality.  you'll need to add one.  there are plenty of examples of test that check to ensure email is sent or not sent.16:36
jpdsbac: Something like http://people.canonical.com/~jpds/fix_450262_screen.png ?16:37
bacjpds: yep16:48
jpdsbac: OK, prefect, I'll look for existing test and will add a new one.16:52
=== bac changed the topic of #launchpad-reviews to: on call: - || reviewing: - || queue [adiroiban(bug-492375 pre-implementation)] || RC candidates take priority || This channel is logged: http://irclogs.ubuntu.com || https://code.edge.launchpad.net/launchpad/+activereviews
jpdsbac: Tests for fix_450262 added and pushed.17:31
bacthanks jpds.  i have to go out now but will look at it later17:31
jpdsNo problem.17:31
=== ursula is now known as Ursinha-lunch
leonardrsalgado, ping re my review?18:05
salgadoleonardr, did you get it?18:06
leonardrsalgado: i'm looking on the web18:06
leonardrchecking email now18:06
salgadoI sent it 10 minutes ago, but I see the mp hasn't been updated yet18:06
leonardrsalgado: i haven't gotten the email yet either18:11
salgadoAn error occurred while processing a mail you sent to Launchpad's email18:11
salgadointerface.18:11
* salgado resends18:11
leonardrsalgado, can you point me to example code so i can see how to check that consumer is stored in the database?18:19
salgadoleonardr, doc/oauth.txt18:20
leonardrok18:20
salgadoyou might need to login() in your test, though18:21
leonardrsalgado, while you're at it, do you want to look at my launchpadlib branch?18:27
leonardrmy response to your review shouldn't affect this code18:27
salgadoleonardr, sure18:27
leonardrok, writing it up18:28
leonardrsalgado: https://code.edge.launchpad.net/~leonardr/launchpadlib/anonymous-access/+merge/1621318:38
=== mwhudson_ is now known as mwhudson
=== salgado is now known as salgado-afk
=== matsubara is now known as matsubara-afk

Generated by irclog2html.py 2.7 by Marius Gedminas - find it at mg.pov.lt!