#launchpad-reviews 2010-01-18
 * thumper needs a pretty trivial review: 
<thumper> https://code.edge.launchpad.net/~thumper/launchpad/fix-branch-active-reviews/+merge/17574 please
<stub> done
<mwhudson> ah, racing approvals
<thumper> ta
* henninge changed the topic of #launchpad-reviews to: on-call: henninge || reviewing: - || queue [] || This channel is logged: http://irclogs.ubuntu.com || https://code.edge.launchpad.net/launchpad/+activereviews
<henninge> Bring out you dead!
<henninge> s/you/your/
<adiroiban> henninge: Hi, can we take a look at bug 340662 ?
<mup> Bug #340662: "Change details" for POTemplate should allow maintainers some more freedom <qa-bad> <ui> <Launchpad Translations:Fix Committed by adiroiban> <https://launchpad.net/bugs/340662>
<adiroiban> and bug 507498
<mup> Bug #507498: AttributeError on potemplate page <oops> <Launchpad Translations:In Progress by henninge> <https://launchpad.net/bugs/507498>
<henninge> adiroiban: yes, I was looking at that atm.
<henninge> adiroiban: Hi!
<henninge> adiroiban: where is the new branch?
<adiroiban> it is attached to the bug report, lp:~adiroiban/launchpad/bug-340662-take-2
<henninge> oh
<henninge> :)
<adiroiban> there is no MP
<adiroiban> since I was waiting for some feedback
<adiroiban> for the last bug comment
<henninge> I see
<henninge> adiroiban: I am reading the original diff atm and will then look at the new branch.
<henninge> adiroiban: Code reviews are meant to check that the code adheres to coding standards and has good test coverage.
<henninge> adiroiban: For a discussion on how to fix the bug properly you need to have a pre-implementation call or chat with some-one who knows the subject.
<henninge> henninge: The reviewer should make sure this has happened.
<henninge> adiroiban: The changes in take-2 look good.
<adiroiban> henninge: :) . Then I can consider this a pre-implementation chat and create the  MP for this branch ?
<henninge> adiroiban: Well, we are already mid-implementation ... ;-)
<henninge> adiroiban: You will still have to update xx-potemplate-edit.txt.
<adiroiban> or post?
<adiroiban> post-implementation :)
<henninge> yes, probably ... ;)
<henninge> actually, mid-implmentation is not that uncommon because often you will start a branch by "trying out" what might help and then discuss your findings for a proper fix.
<henninge> adiroiban: also, I have not yet completely understood the need for EditPOTemplateSubset insecurity.py
<henninge> adiroiban: when is it used exactly?
<adiroiban> to set âis_currentâ for a template where is_current=false
<adiroiban> In this case IPoTemplsteSubset is used, instead of IPOTemplate
<henninge> adiroiban: where does that happen? on +templates?
<adiroiban> or this is what I understood from a previous chat with Danilo, regarding the same issue for Ubuntu translation coordinators
<adiroiban> +edit
<henninge> let me look at this in detail
<adiroiban> at the URL traversal
<danilos> adiroiban, oh, hi :) I hope you don't take the comments I made too harsh, it was mostly about the problem with the review, since review should ensure pre-imp chat has happened
<adiroiban> danilos: Hi! Don't worry, it was constructive feedback. Still, I've proposed some brainless code, so it is also my fault.
<adiroiban> henninge: at browser/potemplate.py
<adiroiban> POTemplateSubsetNavigation
<adiroiban> around line 715
<henninge> adiroiban: Oh, it's for the navigation
<henninge> Ahhhh, now I get it! ;-)
<adiroiban> there is that code that I don't fully understand why it is there
<adiroiban> and I was thinking maybe there is a way to use IPotemplate for both enabled and disabled templates
<henninge> adiroiban: well, the context for traversing to a template is the subset that this template is from.
<henninge> adiroiban: indicated by the "+pots" in the url
<henninge> adiroiban: Or are you saying "Why check permissions on the Subset here and not on the template itself?"
<adiroiban> henninge: yes
<henninge> check_permission('launchpad.Edit', potemplate)
<henninge> like that?
<henninge> adiroiban: Did you say that danilo had a reason for it and you didn't understand it?
<adiroiban> henninge: I was hoping to see the permission checking in browser/configure.zcml for +edit for IPOTemplate. I'm still reading about zope adapters
<adiroiban> henninge: well, Danilo told me that code is not really ok... but while there is no branch to come with a better implemention we will live with it
<danilos> adiroiban, the first question to ask is: what exactly was wrong with existing permissions? (some things are assumed for launchpad.Edit permission, so you don't have to write a specific implementation for everything)
<henninge> lol
<danilos> adiroiban, i.e. what didn't work before and what did change with the introduction of EditPOTemplateSubset permission?
<adiroiban> danilos: with the current permission, a person could not re enable the template after it was disabled
<danilos> adiroiban, i.e. who wasn't able to access the +edit form and now is, an who was and now isn't?
<adiroiban> +edit is not available if is_current=false
<danilos> adiroiban, right, so how is that implemented?
<adiroiban> Henning asked me the same question and I tried to answer him
<adiroiban> browser/potemplate.py, POTemplateSubsetNavigation, around line 715
<adiroiban> the âchainâ is broken in URL traversal for IPOTempalteSubset
<danilos> adiroiban, at least EditPOTemplateDetails doesn't have the limitation
<danilos> adiroiban, well, then that should be changed
<danilos> adiroiban, i.e. simply use what henninge suggested above (check for 'launchpad.Edit' on potemplate object instead of on context)
<danilos> adiroiban, (sorry for not re-reading the entire discussion)
<adiroiban> danilos: np.
<adiroiban> henninge, danilos: thanks for the explanation
<henninge> that would make the new permission EditPOTemplateSubset superflouus
<danilos> adiroiban, the reason traverse() in there is like that is so we'd give a 404 instead of 'permission denied' for "deleted" templates
<danilos> henninge, exactly
<henninge> and possibly AdminPOTemplateSubset, too? I cannot imagine it'd be used anywhere else.
<adiroiban> yep
<adiroiban> henninge: yes. I will also remove the AdminPOTemplateSubset
<danilos> adiroiban, henninge: however, this will break the privilege for UTCs, because now they won't be able to traverse to it anymore for Ubuntu templates
<danilos> adiroiban, henninge: so, EditPOTemplateDetails would have to be extended to check for UTCs like other new permissions do
<adiroiban> danilos: but UTCs should have launchpad.Edit for +edit on IPoTemplate
<danilos> adiroiban, they should, I am not sure they do (i.e. my reading of EditPOTemplateDetails suggests they don't; if it's ok, even better :)
<adiroiban> danilos: ok. so now, I will just remove EditPOTemplateSubset
<danilos> adiroiban, ah, right, it extends AdminPOTemplateDetails, so it does, it's all good
<adiroiban> and fix the other problem in another branch
<danilos> adiroiban, what other problem? "name" being exposed? fwiw, I consider that one more important, because it can lead to inconsistent data in the DB, and we can't guarantee that project maintainers understand things about Launchpad templates like UTCs or rosetta-admins do not to mess it up
<danilos> s/do not/to not/
<adiroiban> danilos: no. the other problem is removing AdminPOTemplateSubset and checking UTCs rights
<danilos> adiroiban, ah, right, good call to keep that a separate branch
<henninge> adiroiban: I think you can still remove AdminPOTemplateSubset.
<henninge> adiroiban: as danilo pointed out, the UTC rights are also checked in AdminPOTemplateDetails.
<henninge> ;-)
<danilos> henninge, I agree, but let's keep it a separate branch so we fix the most immediate problems first, and then worry about cleanup (or, if it's really that simple, let's just remove it, but we are close to the release week so I would like us to be extra careful with QA)
<henninge> right
<henninge> adiroiban: all questions solved? ;-)
<adiroiban> henninge: nope :D
<henninge> adiroiban: go ahead! :)
<adiroiban> I have some (general) questions regarding writing tests
<henninge> ok, good.
<adiroiban> so âstoriesâ are functional tests and we only write stories for the most common use cases
<henninge> yes, and they are page tests.
<adiroiban> then there are browser/tests and they are also functional or unit tests?
<adiroiban> why do we use âprivateâ methods in browser/tests ?
<henninge> because the tests are bad? ;)
<henninge> i.e. test and browser code should not import from model code at all - public or private code
<henninge> ideally.
<adiroiban> and there are also doc and tests
<henninge> adiroiban: sorry, missed a point earlier
<henninge> browser/tests should be view tests that perform a unit test on the view code.
<henninge> doc tests should be mainly to document model code, just like stories document browser code (i.e. the whole page9
<adiroiban> ok. stories are like âsmoke testingsâ
<adiroiban> and browser/tests are unit testing for the view
<henninge> right
<henninge> and tests are unit tests for the model code.
<adiroiban> henninge: thanks!
<adiroiban> henninge: can I add this info to https://dev.launchpad.net/Testing ?
<henninge> adiroiban: oh yeah, that page i awfully short...
<henninge> is
<henninge> adiroiban: just remember that you will find a lot of code that violates these conventions, espcially older code.
<adiroiban> henninge: I will keep that in mind :)
<adiroiban> so in LP parlance, âsmoke testsâ are functional tests ?
<adiroiban> or how do you call the type of tests from stories and doc ?
<henninge> adiroiban: well, I guess that is the most common error that we have in the tree.
<henninge> a lot of doc tests are used as functional tests when they really should be unit tests.
<henninge> I think.
<henninge> ;)
<stub> henninge: https://code.edge.launchpad.net/~stub/launchpad/replication/+merge/17454 is a possible cherry pick (I guess depending if the failure mode hits us again)
<stub> https://code.edge.launchpad.net/~stub/launchpad/bug-504291-disconnection-errors/+merge/17371 is a bug fix - I doubt it bites enough for CP but that is for QA to decide.
<jamalta> Hi, I'm getting ready to create a merge proposal for a branch, and I'm wondering where exactly I put the cover letter. Would that be the description of the merge proposal?
<adiroiban> jamalta: yes
<jamalta> adiroiban: thanks
<adiroiban> jamalta: just make sure you had a pre-implementation chat
<jamalta> adiroiban: i had discussed the bug with sinzui last week :)
<adiroiban> jamalta: great, than you should be ready for the MP :)
<jamalta> adiroiban: just finishing up the MP description, will be ready in a few minutes
<jamalta> ok i think i'm ready for a review, it's a trivial bug so it shouldn't take long
<jamalta> https://code.edge.launchpad.net/~jamalta/launchpad/changesfile-253525/+merge/17587
<adiroiban> jamalta: Henning will review it. To signal that you are waiting for an review, add your irc nick in to topic (ex queue [jamalta])
<jamalta> adiroiban: alright
* jamalta changed the topic of #launchpad-reviews to: on-call: henninge || reviewing: - || queue [] || This channel is logged: http://irclogs.ubuntu.com ||  https://code.edge.launchpad.net/launchpad/+activereviews
* jamalta changed the topic of #launchpad-reviews to: on-call: henninge || reviewing: - || queue [jamalta] || This channel is logged: http://irclogs.ubuntu.com ||https://code.edge.launchpad.net/launchpad/+activereviews
<jamalta> adiroiban: thanks :)
<henninge> jamalta: Hi! Looking at it now.
* henninge changed the topic of #launchpad-reviews to: on-call: henninge || reviewing: jamalta || queue [] || This channel is logged: http://irclogs.ubuntu.com ||https://code.edge.launchpad.net/launchpad/+activereviews
<jamalta> henninge: thanks
<henninge> jamalta: do you know the "extract_text" function?
<jamalta> henninge: no
<henninge> jamalta: Please have a look at lib/canonical/launchpad/doc/pagetest_helpers.txt to read about it.
<jamalta> henninge: alright
<henninge> jamalta: could you please apply that in xx-distroseries-sources.txt to get rid of the HTML code in the tests? Find all occurances in that file for bonus points. ;)
<jamalta> henninge: sounds good, i'll do that
<henninge> jamalta: cool, thank you. I see no other issues with the branch.
<jamalta> henninge: awesome
<jamalta> henninge: so just to make sure i'm doing this right, this is how it should look using extract_text, right? http://paste.ubuntu.com/358552/
<jamalta> the test seems to succeed that way, just wanted to be sure
<henninge> jamalta: exactly!
<henninge> jamalta: btw, looks like no bonus points to get in that file ... ;(
<jamalta> henninge: what do you mean?
<jamalta> henninge: also, i can't really change this to use extract_text http://paste.ubuntu.com/358554/
<jamalta> considering it is testing the tags themselve
<henninge> jamalta: Yes, I was just looking at that.
<henninge> jamalta: what you could do is use BeautifulSoup to extract the tag and its href attribute.
<henninge> jamalta: do you know about BeautifulSoup?
 * jamalta goes digging for BeautifulSoup docs
<jamalta> no
<henninge> http://www.crummy.com/software/BeautifulSoup/documentation.html
<henninge> jamalta: pagetest_helpers builds on top of BeautifulSoup, so you have all that available.
<jamalta> henninge: guess i should use BeautifulSoup for this test as well, http://paste.ubuntu.com/358558/
<jamalta> considering it isn't testing the content of publishing_history, but that a link exists with that content
<jamalta> and target
<henninge> jamalta: yes, that would be good, too.
<henninge> jamalta: two things to note:
<henninge> #1 Using bs and extract_text is better because using ... on HTML causes the whole of the  HTML to appear in the output if the test fails.
<henninge> that makes for very unreadable test failures.
<henninge> also, what if "publishing_history" will be changed to be an "ul" in the future? It does not really matter for the test, does it?
<jamalta> henninge: not at all, that makes complete sense
<henninge> #2 On the other had, doing too heavy BeautifulSouping can make you too dependent on the structure, too.
<henninge> s/had/hand/
<jamalta> the test should only check that the a tag is there, pointing at the right thing, with the right string. correct?
<jamalta> henninge: that makes sense
<henninge> yes, if you can pick out the tag, that's best.
<henninge> jamalta: it is ok to add id's or classes just to be able to pick something out for a test.
<henninge> only use id's when you are sure the element can not occur multiple times on the page.
<jamalta> henninge: alright, that can makes things easier
<henninge> jamalta: so there are bonus points in this file! ;-)
<henninge> big ones, too.
<jamalta> henninge: hm.. i'll see if i can find them all then :) heh
<henninge> jamalta: Hey, I think you identified the two cases with those <a>-tags.
<henninge> jamalta: there is an example of handling such a case in that file, btw. Line 252.
<jamalta> so browser.getLink is using BeautifulSoup then?
<jamalta> ok i think i'm getting the hang of this
<jamalta> the only bottleneck is how long it takes to run a test  >< Lol
<jamalta> that' sodd
<jamalta> i'm using browser.getLink().url
<jamalta> the first time it just printed the url
<jamalta> but on a different test, the url was inside single quotes
<henninge> jamalta: that should be because it is "print"ed in the one case.
<henninge> jamalta: using "print" to test a value is the preferred way, btw.
<jamalta> henninge: oh, right, ok
<jamalta> i'll fix that
<jamalta> should i change others that don't use print?
<jamalta> henninge: ok, i have resubmitted the proposal
<henninge> jamalta: cool, I will look at it in a minute
<jamalta> henninge: thanks, whenever you have time
<henninge> jamalta: I'll be leaving soon, so there is not much time .... ;-)
<jamalta> henninge: uh oh, heh
<jamalta> i guess not!
<henninge> still here
<jamalta> henninge: hmm?
<henninge> jamalta: you can just leave the state of the mp in "Needs review", btw.
<henninge> oh, it's a new one!
<jamalta> henninge: i clicked "resubmit proposal"
<jamalta> that is what i was instructed to do before
<henninge> jamalta: ah, I see.
<henninge> really? I have never used that ...
<henninge> np
<jamalta> does the MP update itself if i push to the branch?
<henninge> jamalta: yes
<jamalta> oh it does
<jamalta> i didn't know that :)
<jamalta> ok, next time i won't do that.. sorry
<jamalta> i can't really remember who told me to do it that way, it was quite a while ago
<henninge> jamalta: Cool, I forgot about getLink! BIG BONUS POINTS ;)
<jamalta> henninge: hehe, that's the best example i could find on how to do that
<jamalta> henninge: thanks
<henninge> jamalta: yes, that is the best way to do it!
<henninge> jamalta: so, the only thing is that I think the change in line 129 is too much.
<henninge> the text explains that the emailaddress is linkified to the user's LP home page and the test should verify that.
<jamalta> henninge: alright
<henninge> you could add a "..." to omit the classes, though, as they are not important for the test.
<jamalta> so i should be testing that against the html?
<henninge> jamalta: it is already drilled down to the <a>-tag, so that is fine.
<jamalta> i could change that to use getLink instead
<jamalta> that way i can do it the same way as the other tests
<henninge> oh, that is a good idea, too
<henninge> getLink on the email address
<jamalta> henninge: right
<henninge> splendid
<jamalta> making sure what i did works, and i'll push it up
<henninge> please do, no new mp needed
<jamalta> henninge: of course ;).. won't do that again
<henninge> jamalta: what we usually do is add a comment with an incremental diff.
<jamalta> henninge: ah.. makes sense
<jamalta> henninge: ok posted it
<henninge> jamalta: please also add a commit message and I'll land it for you.
<jamalta> henninge: hm. by incremental diff, do you mean a diff of the things changed for that update, or a special kind of diff?
<henninge> "Set commit message"
<henninge> Yes, just a diff of the last changes.
<jamalta> that would be for the whole branch, not just what i did, right?
<henninge> no just what you did
<jamalta> ok
* adiroiban changed the topic of #launchpad-reviews to: on-call: henninge || reviewing: jamalta || queue [adiroiban(bug-340662-take-2)] || This channel is logged: http://irclogs.ubuntu.com ||https://code.edge.launchpad.net/launchpad/+activereviews
<henninge> jamalta: ah yes, that's what I meant ;-)
<jamalta> henninge: done
<henninge> adiroiban: I am sorry, but I am done for the day ...
<adiroiban> henninge: don't worry
* henninge changed the topic of #launchpad-reviews to: on-call: - || reviewing: - || queue [adiroiban(bug-340662-take-2)] || This channel is logged: http://irclogs.ubuntu.com ||https://code.edge.launchpad.net/launchpad/+activereviews
<adiroiban> I just added the MP in the queue :)
<adiroiban> it's not the end of the world 
<henninge> adiroiban: ;-)
<henninge> adiroiban: are you around tomorrow? We need to talk about the UDW session!
<adiroiban> henninge: yep. I'm around
<adiroiban> I have uploaded the virtual machine
<adiroiban> and I was planning for tonigh to write a quick blog post 
<adiroiban> and asking for some feedback before the session
<henninge> cool, good idea
<adiroiban> like give a homework
<adiroiban> and discuss it durring the session
<henninge> adiroiban: ok, let's talk about that tomorrow
<adiroiban> but I will just gather my thoughts and tomorrow we shall see
<adiroiban> yep
<jamalta> henninge: thanks for everything :)
<henninge> a pleasure
<jamalta> Hi there, I wanted to see if I could take a stab at bug #498643
<mup> Bug #498643: 'destination ppa' dropdown should show the url too <trivial> <ui> <Soyuz:Triaged> <https://launchpad.net/bugs/498643>
<jamalta> So I just need someone to have a pre-implementation chat with :)
<deryck> intellectronica, https://code.edge.launchpad.net/~deryck/launchpad/no-lp-bugs-homepage-439245/+merge/17634
 * kfogel is away: afk for some hours; see you antipodal people later probably
#launchpad-reviews 2010-01-19
<mrevell> henninge, ping
<henninge> mrevell: Hi!
<mrevell> hey henninge, remember my translation help branch? I'm getting an odd error from ec2 test. Mind if I pastebin it to you?
<henninge> mrevell: go ahead!
<henninge> https://pastebin.canonical.com/26742/
<henninge> mrevell: the import violations are not your error.
<henninge> mrevell: there has been a thread about that on the mailing list.
<mrevell> henninge, Ah, I must have missed that. I'll go re-read
<henninge> mrevell: you can still fix it, though, if you like.
<henninge> ;-)
<mrevell> henninge, I'm assuming I just need to merge the latest devel.
<mrevell> Is that right?
<henninge> mrevell: I thought ec2 uses the latest devel for the test.
<henninge> mrevell: but that is a good check in any case.
<henninge> mrevell: you do that and I'll prepare a patch that you can add to your branch, if it does not help.
<mrevell> henninge, Thanks :)
<henninge> mrevell: looks like one is gone, the other was my doing....
<mrevell> henninge, When you say "gone", does that mean I don't need to worry about it?
<henninge> mrevell: yes, it means it is fixed in latest devel (from looking at the code).
<mrevell> cool
<henninge> mrevell: I'll have a fix for the other one once I ran a test on it.
<mrevell> Ah thanks
<mrevell> henninge, I'm just looking at the xx-translation-import-queue.txt failure
<henninge> mrevell: yes, what does it look like?
<henninge> mrevell: here is the patch for import violation
<henninge> mrevell: you can just forward me the FAILURE mail btw.
<henninge> http://paste.ubuntu.com/358977/
<mrevell> henninge, Unfortunately, I never get the failure mails. All I have is the output in my terminal. I can paste that to you.
<mrevell> thanks for the patch, looking now.
<henninge> mrevell: please paste
<mrevell> henninge, Actually, isn't that what I've already pasted to you? https://pastebin.canonical.com/26742/
<henninge> mrevell: well, the actual output of the failure itself would be great but I can generate that myself from your branch.
<henninge> if you don't have it
<mrevell> henninge, I'll run the test alone now
<henninge> good idea
<henninge> ;_)
<henninge> mrevell: bin/test -vvct xx-translation-import-queue
<mrevell> thanks henninge
<henninge> mrevell: -vv for verbosity, -c for colours, -t for specifying the test.
<mrevell> henninge, I'm getting a traceback from bin/test -- is this related to the import problems? https://pastebin.canonical.com/26747/
<henninge> mrevell: the output has lots of rubbish in it atm. dunno why
<henninge> that's probably why I never cared about the import violations, either ... ;-)
<mrevell> henninge, I'm confused. It looks to me as though the test is failing to run, rather than giving me any output I can reasonably use to understand why the test is failing
<henninge> mrevell: ;-)
<henninge> I'll try it out. What's your branch again?
<mrevell> henninge, https://code.edge.launchpad.net/~matthew.revell/launchpad/translations-help-10.1
<henninge> mrevell: ok, got it.
<henninge> ;-)
<henninge> mrevell: This is what I get http://paste.ubuntu.com/358985/
 * mrevell looks
<henninge> not to mention that "'Upload either a single file' in ff_owner_browser.contents" is a bad test.
<mrevell> I don't understand why the test won't run on my machine. It's complaining that there's "No module named testtools". Hmm. Thanks for the test output. I shall fix the test and then put it back through ec2
<stub> mrevell: Might be similar to the issue I had per mailing list - sounds familiar.
<mrevell> thanks stub, i'll take a look at your mail
* bac changed the topic of #launchpad-reviews to: on-call: bac || reviewing: - || queue [adiroiban(bug-340662-take-2)] || This channel is logged: http://irclogs.ubuntu.com ||https://code.edge.launchpad.net/launchpad/+activereviews
<bac> good morning
<bigjools> morning bac
<bigjools> got a branch for you
<bigjools> will send it in 30m when I've a) eaten b) written a cover letter :)
<bac> bigjools: ok
* jamalta changed the topic of #launchpad-reviews to: on-call: bac || reviewing: - || queue [adiroiban(bug-340662-take-2), jamalta] || This channel is logged: http://irclogs.ubuntu.com||https://code.edge.launchpad.net/launchpad/+activereviews
<bac> hi EdwinGrubbs
<EdwinGrubbs> bac: hi
<bac> jamalta: please ping the on-call reviewer when you have a branch.  you'll get noticed faster.
<jamalta> bac: Oh! Ok :)
<jamalta> bac: ping ;) ... just kidding
<jamalta> I actually have two things, of course the MP first
<jamalta> https://code.edge.launchpad.net/~jamalta/launchpad/destination-ppa-498643/+merge/17628
<bac> jamalta: ok
<jamalta> After that, I need a chat about a bug I started on.. I guess it's more of a late implementation talk
<jamalta> It's another very trivial issue, I just want to make sure you are okay with the way I'm going to make the change, but more about that later.
<bac> jamalta: sounds good
<jamalta> bac: thanks :)
<bac> beuno: can you have a look at the screenshots for https://code.edge.launchpad.net/~bac/launchpad/bug-499351-batching-dls/+merge/17492 ?
<beuno> bac, will look in a minute
* bac changed the topic of #launchpad-reviews to: on-call: bac || reviewing: adiroiban || queue [adiroiban(bug-340662-take-2), jamalta] || This channel is logged: http://irclogs.ubuntu.com||https://code.edge.launchpad.net/launchpad/+activereviews
* bac changed the topic of #launchpad-reviews to: on-call: bac || reviewing: adiroiban || queue [jamalta] || This channel is logged: http://irclogs.ubuntu.com||https://code.edge.launchpad.net/launchpad/+activereviews
<bac> hi adiroiban
<adiroiban> bac: hi
<bac> adiroiban: i just started your review.  running the test i notice there is an import violation.  will you fix it please by importing IProductSeries from lp.registry.interface.productseries
<adiroiban> ...looking into this...
<jamalta> bac: now that you mention it, the test on my branch has an import violation as well, although it's not related to anything i changed...
<adiroiban> how can I find that import violation?
<jamalta> i tried asking about it yesterday but no one was around
<bac> adiroiban: just run your test.  it'll show up at the end
<bac> adiroiban: it is not related to your work but this is a good time to clean it up.  we're striving to stomp out import violations.
<bac> jamalta: what is the import violation message?
 * jamalta goes run the test again
<bac> adiroiban: the problem is simply that in lp.trans.browser.product the import from IProductSeries is from the wrong module
<bac> s/from/for
<adiroiban> potemplate-edit looks clean on my side http://paste.ubuntu.com/359058/
<adiroiban> running the other tests to see where is the problem
<bac> adiroiban: have you merged from trunk lately?
<bac> adiroiban: i just ran your demo.  logged in as test@canonical.com and tried changing the 'translation domain' and i get an unauthorized exception.
<bac> adiroiban: date_last_updated requires launchpad.TranslationsAdmin
<adiroiban> bac: hm. I will merge the code and look into the problem. we need more tests for this branch
<bac> adiroiban: agreed.  can you reproduce the failure i'm seeing?
<adiroiban> bac: waiting for LP to start on my computer
<adiroiban> bac: strange. the potemplate-edit is also testing an update using âtest@canonical.comâ
<adiroiban> yep. I got that error
<adiroiban> I will improve potemplate-edit pagetest
<bac> thanks adiroiban
<adiroiban> np
* bac changed the topic of #launchpad-reviews to: on-call: bac || reviewing: jamalta || queue [-] || This channel is logged: http://irclogs.ubuntu.com||https://code.edge.launchpad.net/launchpad/+activereviews
<adiroiban> bac: regarding the import violation. I shall see it after the merge?
<bac> adiroiban: i suspect so.  i got a fresh copy from devel and then merged in your branch.  so if you merge in devel we'd be at the same spot.
<adiroiban> ok
<jamalta> bac: http://paste.ubuntu.com/359060/ that's the violation i'm receiving
<bac> adiroiban: sorry you got bit by that import violation...
<jamalta> don't worry about it for now though, we can cover that when you're ready for me :)
<adiroiban> bac: np. I'll solve it.
<bac> jamalta: the second one in that pastes is the same as i'm discussing with adiroiban
<jamalta> bac: oh ok
<jamalta> bac: is it safe for me to ignore it, or is it something i need to fix?
<bac> jamalta: you can ignore it
<jamalta> bac: ok, thanks :)
<bac> jamalta: and the other goes away if you merge from devel as someone else has fixed it.
<jamalta> bac: ok, i'll merge from devel and retest
<bac> jamalta: on second thought you may want to fix the existing one since you may land before adiroiban does.
<bac> jamalta: it's  a simple one line change
<jamalta> bac: sure
<deryck> beuno, ping
<beuno> deryck, pong
<beuno> bac, almost looking
<jamalta> is there a specific rocketfuel script to merge from devel?
<jamalta> or is bzr merge lp:~launchpad-pqm/launchpad/devel sufficient
<jamalta> bac: which of the 2 import violations did you want me to fix?
<bac> jamalta: IProductSeries
<deryck> beuno, can you do a UI review for me?
<bac> jamalta: the merge you stated will work.  you should be able to just do 'bzr merge'
<jamalta> bac: oh even easier
<deryck> beuno, mrjazzcat and I did a branch at the sprint and intellectronica and I are unsure about one part of it.
<bac> jamalta: as devel should be the parent to your branch, if you used rocketfuel-branch to set it up
<jamalta> bac: i did, so it should work :) thanks
<jamalta> bac: also, IProductSeries is referenced once in lp.translations.browser.product
<bac> jamalta: right, the problem is it is imported from .product instead of .productseries -- that's all
<jamalta> bac: OH! Ok
<jamalta> bac: makes sense, lets see if that fixed the test
<beuno> deryck, sure
<beuno> URL?
<jamalta> bac: so the other violation i should ignore, correct?
<bac> jamalta: the other should not be present after you merge from devel
<jamalta> bac: oh ok, then i should no see any violations with this test
<jamalta> sweet!
<jamalta> uhm,.. this is interesting
<jamalta> i've never come across this
<jamalta> so i merged, and there's a ton of changes
<jamalta> should i just commit and include that i merged in the description?
<bac> jamalta: yep.  you're seeing everything that has changed in devel since you made your branch.
<deryck> beuno, https://code.edge.launchpad.net/~deryck/launchpad/no-lp-bugs-homepage-439245/+merge/17634
<deryck> beuno, screenshots attached.  The main question is should we remove the search form when there are no bugs.
<bac> jamalta: i have a bzr alias for it:  bzr alias rfs="commit -m "Merge from trunk""
<deryck> beuno, I'm inclined to say yes and did the work.  intellectronica thinks maybe this isn't right.
<jamalta> bac: ah cool
<beuno> deryck, it
<beuno> it's a nice improvment
<beuno> I only hve 2 comments
<deryck> beuno, thanks
<bac> jamalta: your MP is approved with one small formatting suggestion.  thanks for the branch.
<beuno> 1) project-not-using-lp-new-ui.png, I'd make it simpler, "Project doesn't use LP... (i) Enable bug tracking"
<bac> bigjools: is your branch coming up soon?
<beuno> 2) roject-using-lp-no-bugs-new-ui.png, "There are currently no bugs against $project. _Report a bug_"
<beuno> deryck, minus those 2, ui=,e
<beuno> me
<beuno> :)
<bigjools> bac: gah, forgot
<jamalta> bac: wait, i ahven't commited the violation fix!
<beuno> bac, no screenshots?  :)
<bac> jamalta: i've approved it with the understanding you'll make the requested fixes
<beuno> ah
<beuno> bug
<jamalta> bac: oh! ok, got it :)
<bac> beuno: 4 screenshots on the bug.
 * bac wishes you could add screenshots to the MP
<jamalta> bac: thanks for catching that.. i don't know how i ended up shifting that code right heh
<jamalta> i'll fix that as well :) thanks so much
<deryck> beuno, will do.  Thanks.
<sinzui> bac: beuno: I did a UI review of https://code.edge.launchpad.net/~bac/launchpad/bug-499351-batching-dls/+merge/17492 because I am familiar with this problems that the page faces. merge proposals does not allow me to have separate reviews for UI and code.
<beuno> sinzui, great, I'll just review your review then  :)
<bac> sinzui: thanks.
* bac changed the topic of #launchpad-reviews to: on-call: bac || reviewing: - || queue [-] || This channel is logged: http://irclogs.ubuntu.com||https://code.edge.launchpad.net/launchpad/+activereviews
<jamalta> bac: am i to bug you about pre-implementation talks?
<bac> jamalta: i can do a pre-imp if you'd like.  that's one of the things the OCR is here to do.
<jamalta> bac: awesome, do i need to add myself to the queue again?
<bac> jamalta: no
<jamalta> bac: ok cool, so i'm looking at bug #87058 and #106924
<mup> Bug #87058: Instruction  'Yes, define new tag' is misleading <bugtag> <Launchpad Bugs:Triaged by jamalta> <https://launchpad.net/bugs/87058>
<jamalta> they are both very closely related, and it was suggested that both be fixed together
<jamalta> i just wanted to know what the best language to use is
<bigjools> bac: just sent off MP email
<sinzui> EdwinGrubbs: I am not sure CHR should have admin on admin_merge_teams. They have access to delete a team (which is a very controlled merge)
<sinzui> EdwinGrubbs: I think you made this change to update the story, but the problem is in a different story: delete teams. Since the fix in the view code, I expect the test to be of the view; I do not think a story test is right.
<sinzui> EdwinGrubbs: We are testing the view's contract, operation, and the objects final state. So a unit test or doc test is more appropriate.
<EdwinGrubbs> sinzui: I'll look into that.
<sinzui> EdwinGrubbs: I am writing a fuller summary of the test
<bac> bigjools: it still hasn't arrived on https://code.edge.launchpad.net/launchpad/+activereviews
<sinzui> EdwinGrubbs: your security change allows any team owner/admin to take control of any other team!
<deryck> beuno, I requested you as UI reviewer on that MP you just reviewed.  Can you click approve for the sake of the MP?
<beuno> deryck, yes I can
<beuno> free karma
<deryck> beuno, thanks
<deryck> bac, I have a branch for review I would like to add to the queue.
<bac> deryck: ok
<deryck> bac, https://code.edge.launchpad.net/~deryck/launchpad/hot-bugs-list-color-icons-439128/+merge/17664
<deryck> bac, and thanks!
* deryck changed the topic of #launchpad-reviews to: on-call: bac || reviewing: - || queue [deryck] || This channel is logged: http://irclogs.ubuntu.com||https://code.edge.launchpad.net/launchpad/+activereviews
<bigjools> bac: I got an email back with an oops!
<beuno> bac, I think sinzui is right on the mark with the problem
<bac> beuno: ok
<beuno> bac, one way of fixing this could be
<beuno> to isntead of presenting it as a navigation
<beuno> have an "See older releases >" link
<sinzui> EdwinGrubbs: I replied to your MP with an outline of the test that reproduces what is happening. I included a real-world example that you can do on edge to get the oops
<EdwinGrubbs> sinzui: thanks
<bigjools> bac: I resent the MP request (was OTP)
<bigjools> bac: https://code.launchpad.net/~julian-edwards/launchpad/ppa-expire-sources/+merge/17669
<jamalta> bac: submitted an MP for the issue we discussed earlier.. whenever you have some time for me again 
* jamalta changed the topic of #launchpad-reviews to: on-call: bac || reviewing: - || queue [deryck, jamalta] || This channel is logged:http://irclogs.ubuntu.com||https://code.edge.launchpad.net/launchpad/+activereviews
<gary_poster> gmb, allenap` are either of you available to review https://code.edge.launchpad.net/~gary/launchpad/fix-db-devel-sourcecode/+merge/17674 quickly?  two line change to work around wadl problem for now
<allenap`> gary_poster: Sure, I'll do it.
<gary_poster> Tahnks allenap` 
<allenap`> gary_poster: r=me
<gary_poster> thanks allenap` 
<allenap`> gary_poster: Welcome :)
<gary_poster> :-)
* bac changed the topic of #launchpad-reviews to: on-call: bac || reviewing: deryck || queue [jamalta] || This channel is logged:http://irclogs.ubuntu.com||https://code.edge.launchpad.net/launchpad/+activereviews
<bac> hi deryck
<bac> sinzui, beuno: any idea why https://bugs.edge.launchpad.net/launchpad/+bugs doesn't show the bug sprite in webkit?
<sinzui> bac: yes I know everything
<bac> sinzui: cool.  can you tell deryck how to fix it in his branch?
<bac> or just tell me and i'll pass it along and look smart
<sinzui> <span class="sprite bug-high" /> is invalid markup
<bac> sinzui: ah, span can't self-close?
<bac> sinzui: \0/
<sinzui> firefox likes this this markup, but wbkit nos it is bad
<bac> sinzui: webkit is a tough cookie
<sinzui> bac: the bug (me cannot remember its number) is fixable in tales formatter doing <span>&nbsp;</span>
<bac> sinzui: the one in question is bug 439128, which doesn't use a formatter
<mup> Bug #439128: "Hot bugs" doesn't show coloured bug icons <story-bug-heat> <ui> <Launchpad Bugs:In Progress by deryck> <https://launchpad.net/bugs/439128>
<sinzui> bac: that is a duplicate.
<bac> sinzui: do you have the other bug number?
 * sinzui is still looking
<bac> great
<bac> sinzui: bug 144101 ?
<mup> Bug #144101: Bug icons are not correct colour on bugs.lp.net homepage <trivial> <ui> <Launchpad Bugs:In Progress by deryck> <https://launchpad.net/bugs/144101>
<sinzui> It is a foundations bug about khtml/webkit
<bac> ah
* bac changed the topic of #launchpad-reviews to: on-call: bac || reviewing: - || queue [jamalta] || This channel is logged:http://irclogs.ubuntu.com||https://code.edge.launchpad.net/launchpad/+activereviews
* bac changed the topic of #launchpad-reviews to: on-call: bac || reviewing: bigjools || queue [jamalta] || This channel is logged:http://irclogs.ubuntu.com||https://code.edge.launchpad.net/launchpad/+activereviews
<sinzui> bug 393825 and I think it has a dup: bug 452799
<mup> Bug #393825: Icons missing from bug listings in WebKit and KHTML <sprite> <ui> <Launchpad Bugs:Triaged> <https://launchpad.net/bugs/393825>
<mup> Bug #452799: no pencil next to title of bug with konqueror <Launchpad Bugs:Triaged> <https://launchpad.net/bugs/452799>
<sinzui> bac: I did not fix bug 393825 after I had identified the issue a few weeks ago because I expects the fix could break many tests. 
<mup> Bug #393825: Icons missing from bug listings in WebKit and KHTML <sprite> <ui> <Launchpad Bugs:Triaged> <https://launchpad.net/bugs/393825>
<bac> sinzui: ok.  i'll mention to deryck to see how brave he is
* bac changed the topic of #launchpad-reviews to: on-call: bac || reviewing: jamalta || queue [] || This channel is logged:http://irclogs.ubuntu.com||https://code.edge.launchpad.net/launchpad/+activereviews
 * deryck is back from lunch, but about to see do lightening talks
<deryck> bac, will look closely after talks.  thanks for the review and comments
<bac> np deryck.  hope you're having a good sprint
<bac> jamalta: thank you for the nice MP including demo URLs.  sure makes reviewing easier
<sinzui> bac: as for deryck's work not using the tales formatter, He is sort of.    
<sinzui>     <span tal:attributes="class context/default_bugtask/image:sprite_css" />
<sinzui> should be
<sinzui>     <span tal:replace="structure context/default_bugtask/image:icon" />
<sinzui> once the tales formatter is fixed
<jamalta> bac: np :) glad to know!
<bac> sinzui: ah, you're right.  deryck ^^
<sinzui> bac: deryck: by asking for the icon, we get free updates to the markup and css when they change. The extravagant use of applying css directly to elements has caused a lot of bugs...the markup and the css are not independant
<bac> jamalta: it looks like you missed some tests
<bac> jamalta: try:  bzr ls -VR --kind=file --null | xargs -0 grep -In define\ new\ tag
<bac> jamalta: also, you can run all of the tests related to bugs by doing:
<bac> % bin/test -vvm lp.bugs
<bac> it's pretty handy to make sure you found all of the places in a given app
<jamalta> bac: oh wow, cool
<jamalta> makes a lot of sense
<jamalta> i did miss some places, heh
<bac> jamalta: do you use emacs?  if so, jml has a nice bzr-grep tool
<jamalta> bac: no, vim
<jamalta> i saw some of the emacs tools available on the dev wiki
<jamalta> but i would have to learn to use emacs
<bac> jamalta: you'll also need to request a UI review from sinzui or beuno due to the wording change.
 * bac hides from that discussion
<jamalta> that doesn't sound good... lol
* bac changed the topic of #launchpad-reviews to: on-call: bac || reviewing: - || queue [] || This channel is logged:http://irclogs.ubuntu.com||https://code.edge.launchpad.net/launchpad/+activereviews
<jamalta> sinzui, beuno let me know when either of you have a chance :)
<sinzui> jamalta: ui:r=me
<jamalta> sinzui: need a link to the MP?
<sinzui> jamalta: I just approved the UI for the MP. you will get the email shortly
<jamalta> sinzui: oh awesome, thanks! :)
<beuno> jamalta, sure
<beuno> show me screenshots!
<jamalta> beuno: sinzui already approved it, but i can show you
 * jamalta sits patiently while make run does a ton of stuff
<bac> speaking of screenshots, is people.ubuntu.com down/deprecated?
<beuno> jamalta, if sinzui approveed it
<beuno> then it's all good
<jamalta> beuno: ah ok
<bac> beuno: FTR, you still have to mentor his UI review, right?
<leonardr> bac, can you take a look at https://code.edge.launchpad.net/~leonardr/lazr.restful/use-bleedthrough-for-methods/+merge/17683 ?
* bac changed the topic of #launchpad-reviews to: on-call: bac || reviewing: leonardr || queue [] || This channel is logged:http://irclogs.ubuntu.com||https://code.edge.launchpad.net/launchpad/+activereviews
<bac> leonardr: on it
<leonardr> not sure if i explained it well enough, but i'm here
<jamalta> beuno: http://picasaweb.google.com/lh/photo/yNYcG4o9IpJqET__8gP_cw?feat=directlink
<bac> leonardr: at line 37 i think you need a comment explaining why you're doing a push(None)
<deryck> sinzui, bac -- if I'm reading the scrollback right, I should use replace and structure to get the element right, and the icons will appear cross-browser correctly?
<leonardr> bac:
<leonardr>             # Because we don't know the name of the earliest version
<leonardr>             # published by the web service (we won't know this until
<leonardr>             # runtime), we'll use None as the name of the earliest
<leonardr>             # version.
<bac> leonardr: +1
<bac> deryck: yes, but i think the TALES formatter has to be fixed first
<deryck> bac, ah, ok.  a predicament :-)
<deryck> bac, and yes, I can fix the related bug you mentioned on the MP.
<deryck> bac, sinzui -- is there an ETA on fixing the TALES formatter?
<bac> deryck: the bug i mentioned in the MP (bug 393825) is the one that does fix the TALES formatter...
<mup> Bug #393825: Icons missing from bug listings in WebKit and KHTML <sprite> <ui> <Launchpad Bugs:Triaged> <https://launchpad.net/bugs/393825>
<deryck> ah, sorry, misunderstood.
<deryck> bac, let me look closer at it then.
<deryck> bac, ah, I see now.  Sure, we can fix the formatter and then use it correctly in my current branch.
<bac> deryck: great
<deryck> bac, and thanks for the great review catching all this.
<bac> deryck: i have a vested interest.  :)
<deryck> :-)
<sinzui> deryck: I know the tales formatter has to be fixed for the UI changes I plan for +packaging and +needs-packaging. But I wont be working on the UI until tomorrow at the earliest
<bac> leonardr: done
* bac changed the topic of #launchpad-reviews to: on-call: bac || reviewing: - || queue [] || This channel is logged:http://irclogs.ubuntu.com||https://code.edge.launchpad.net/launchpad/+activereviews
<jamalta> bac: running all tests in lp.bugs takes forever -.-
<jamalta> but, provided it finishes successfully, i should be ready :)
<bac> jamalta: indeed it does.  well about 40 minutes i'd guess
<deryck> bac, what's the bug number for the privacy icon in webkit browsers?
<deryck> if you know
<sinzui> I saw that a moment ago
<sinzui> it is different
<sinzui> ââderyck: bug 397457
<mup> Bug #397457: Bug privacy edit icon is not visible in WebKit browsers <bug-page> <Launchpad Bugs:Triaged> <https://launchpad.net/bugs/397457>
<deryck> thanks sinzui 
<sinzui> um, how did I do that
<sinzui> deryck: did my message appear right-to-left in your display?
<deryck> sinzui, no
<sinzui> okay, My display is on doap again
<intellectronica> bac: fancy reviewing a nice little branch from bdmurray and me?
<deryck> intellectronica, https://code.edge.launchpad.net/~deryck/launchpad/show-package-hot-bugs-455995/+merge/17675
<intellectronica> bac: nm, b.t.w, deryck already reviewed our branch
<EdwinGrubbs> even though they had it before.
<EdwinGrubbs> sinzui: I was able to recreate the error with delete instead of merge, so I have no problems adding correct tests for that.
<sinzui> EdwinGrubbs: okay. I am sorry that I did not report how I discovered the bug on edge in the first place
<EdwinGrubbs> sinzui: that wasn't the problem. I think I accidentally tried deleting a team that the no-priv user owned, so I didn't see the error.
<sinzui> Yes, it a hard bug to discover.
<jamalta> bac: awesome, everything works
<jamalta> let me push up the changed tests.. hold on
<deryck> bac, I've updated the MP for the branch you reviewed earlier, when time allows for you to re-consider it.
<EdwinGrubbs> sinzui: so, do you want me to change /people/+adminmergeteam to launchpad.Admin from launchpad.Moderate?
<sinzui> EdwinGrubbs: No +adminteammerge should remain unchanged
<EdwinGrubbs> sinzui: then, I think I should keep the change to the story test, since it is more likely to break for the registry experts than for the admins.
<sinzui> EdwinGrubbs: Yes, keep the test and the permission change on the link.
<bac> deryck, jamalta: will have a look in a bit
<bac> thanks for your quick changes
<jamalta> bac: thanks
<deryck> bac, thanks
<sinzui> intellectronica: Hurray you fixed hotness. Now my branch to use hotness will be broken in a few hours
<intellectronica> sinzui: what do you mean by 'fixed hotness'?
<sinzui> hotness => heat
<sinzui> IBug.heat
<intellectronica> sinzui: you can thank adeuring and kfogel for that
<sinzui> did the schema change?
<kfogel> sinzui: you're welcome.  I think it's still in PQM, but yes, there's a small schema change.
<sinzui> bugger, that kills the UI changes for edge this release.
<kfogel> sinzui: easy search-and-replace, I hope?
<kfogel> or is it an issue of the freeze coming up?
<sinzui> no, edge and staging conflict.
<kfogel> oh
 * kfogel carefully inserts his head into the sand
<sinzui> So the devel's changes will not play on db-devel.
<sinzui> I need to land a second branch after the failure in db-devel to make the query work with a different schema
<sinzui> kfogel: I may be able to do some trick where I check the env to determine the correct column name.
<kfogel> sinzui: I think the question I'm really asking (except that I haven't, until now) is:  Is there something unusual/preventable about this situation, or is this just what schema changes tend to do?
<kfogel> sinzui: wasn't the python name "Bug.heat" already, though?
<sinzui> kfogel: This is a rare collision where I am adding model code on level 1 where you are changing schema SQL on level 0.
<sinzui> kfogel: I do not think there is enough crack in the world to tempt me to check the model of bug.heat 20,000 times when sorting a list of every package in Ubuntu
<kfogel> sinzui: see https://code.edge.launchpad.net/~kfogel/launchpad/509194-506514-bug-heat-db-fixes/+merge/17625  -- the interesting part of the diff is at the end.
#launchpad-reviews 2010-01-20
<rockstar> anyone around for a few reviews?
<jamalta> thanks bac 
<bac> jamalta: np.  sorry i didn't get to it earlier.  (had to go to the movie!)
<jamalta> bac: heh no worries, i'm sure there is no rush with a trivial bug like that :)
* bac changed the topic of #launchpad-reviews to: on-call: - || reviewing: - || queue [] || This channel is logged:http://irclogs.ubuntu.com||https://code.edge.launchpad.net/launchpad/+activereviews
<jamalta> considering the majority of users won't even see that dialog ;)
<bac> nope.  good work still.
<jamalta> thanks
<mrevell> Anyone wanna take a look at a small tour text change branch? https://code.edge.launchpad.net/~matthew.revell/launchpad/tour-commercial-tweak-bug-393348/+merge/17666
<jamalta-afk> well, looks like some people didn't like my change :( (bug #509791)
<mup> Bug #509791: "sources.changes" is not a well-known term <Soyuz:Triaged by julian-edwards> <https://launchpad.net/bugs/509791>
<jamalta> i guess i'll just make the change to "changes file" ...
<leonardr> gary, want to start the day off with a quick review?
<leonardr> https://code.edge.launchpad.net/~leonardr/lazr.restful/multiversion-rename-params/+merge/17741
<gary_poster> leonardr sure
<jamalta> could someone approve the change to "changes file" instead of "sources.changes" from bug 509791 so i can push this branch really quick?
<mup> Bug #509791: "sources.changes" is not a well-known term <Soyuz:Triaged by julian-edwards> <https://launchpad.net/bugs/509791>
<jamalta> bigjools: https://code.edge.launchpad.net/~jamalta/launchpad/changesfile-509791
* jamalta changed the topic of #launchpad-reviews to: on-call: - || reviewing: - || queue [] || This channel is  logged:http://irclogs.ubuntu.com||https://code.edge.launchpad.net/launchpad/+activereviews
* jamalta changed the topic of #launchpad-reviews to: on-call: - || reviewing: - || queue [jamalta] || This channel islogged:http://irclogs.ubuntu.com||https://code.edge.launchpad.net/launchpad/+activereviews
* sinzui changed the topic of #launchpad-reviews to: on-call: - || reviewing: - || queue [jamalta, sinzui] || This channel islogged:http://irclogs.ubuntu.com||https://code.edge.launchpad.net/launchpad/+activereviews
<bigjools> jamalta: looking
<jamalta> bigjools: sorry about that, i tried to make sure i used a good term to replace changesfile with :(
<bigjools> jamalta: np, easily fixed!
<bigjools> approved, thank you, I'll land it shortly
<jamalta> bigjools: yeah, specially since i already had a diff with the changes to sources.changes
<jamalta> bigjools: thanks! glad to hear this is better :)
* EdwinGrubbs changed the topic of #launchpad-reviews to: on-call: Edwin || reviewing: - || queue [jamalta, sinzui] || This channel islogged:http://irclogs.ubuntu.com||https://code.edge.launchpad.net/launchpad/+activereviews
<gary_poster> leonardr: I'd like to see an example of the typical use of getting a given version of the declarations.  All I see in webservice-declarations is using pop, which doesn't seem likely to be the usual usage.  Is the expectation that you will say special_dict.stack[version_name][name that you want]?
<leonardr> gary: no, i'm actually going to be using pop
<leonardr> i'm going to create the most recent version, then the version before that, etc
<gary_poster> leonardr: oh, huh.  So this is really a very transient data structure
<gary_poster> ok
<gary_poster> leonardr: the only other comment I've scribbled into the text box so far is that, for the name, VersionedDict makes sense to me, particularly with new approach.  OTOH, BleedThroughDict has the vampire-ish connotations that are so in vogue.
* jamalta changed the topic of #launchpad-reviews to: on-call: Edwin || reviewing: - || queue [sinzui] || This channelislogged:http://irclogs.ubuntu.com||https://code.edge.launchpad.net/launchpad/+activereviews
<leonardr> VersionedDict is good
<gary_poster> leonardr: ok cool.  I'll approve, with that suggestion.
<rockstar> EdwinGrubbs, do you mind if I chuck some branches in for review (I have 4 of them).
<EdwinGrubbs> rockstar: go for it
<rockstar> EdwinGrubbs, https://code.edge.launchpad.net/~rockstar/launchpad/upgrade-job-2.0-changes/+merge/17701
<rockstar> https://code.edge.launchpad.net/~rockstar/launchpad/upgrade-ui/+merge/17704
<rockstar> https://code.edge.launchpad.net/~rockstar/launchpad/pedantry-round2/+merge/17705
<rockstar> https://code.edge.launchpad.net/~rockstar/launchpad/pedantry-round3/+merge/17706
<gary_poster> leonardr: approved, with some small comments
<leonardr> gary: i may get rid of the class in the future, but who knows--it might get more complicated before this is all over
<gary_poster> leonardr: ack, cool
* Ursinha changed the topic of #launchpad-reviews to: on-call: Edwin || reviewing: - || queue [sinzui, Ursinha] || This channel is logged: http://irclogs.ubuntu.com || https://code.edge.launchpad.net/launchpad/+activereviews
<henninge> mrevell: can you please revert lib/lp/translations/browser/product.py in your branch? Ursula is fixing that in hers.
<mrevell> henninge, My branch won't pass the test suite if I revert it, though, won't it?
<henninge> mrevell: no, they are just warning atm.
<mrevell> ah, okay, thanks henninge
<henninge> bac: I am not sure now, actually. As sinzui's branch has landed, are import violations still warnings?
<sinzui> henninge: There were some that were introduced after I landed my branch
<henninge> sinzui: the question is if the test suite will pass if the branch has import violations.
<sinzui> yes, that is how we got so many in the first place
<sinzui> henninge: there is one import violation in devel:
<sinzui> There were 1 imports of names not appearing in the __all__.
<sinzui> You should not import IProductSeries from lp.registry.interfaces.product:
<sinzui>     lp.translations.browser.product
<henninge> sinzui: that's the one we are talking about ... ;-)
<EdwinGrubbs> sinzui: do you still need someone to review your needs-linking-bug-507937 branch?
<Ursinha> sinzui: I just fixed that, as mrevell 
<Ursinha> :)
<sinzui> EdwinGrubbs: yes please
<henninge> mrevell: didn't I review this branch before?
<mrevell> henninge, no, this is a new branch
<henninge> mrevell: but it also renames a button from Continue to Close. That looks awfully familiar.
<henninge> Did you not land the first branch?
<mrevell> henninge, Ah, I see. I merged that first branch into this one as I've had trouble getting it through the test suite, but needed the changes there. I'll land that branch and then ask you to review this new one.
<henninge> mrevell: you can add that branch as a pre-requisite when creating the mp for the new one and it will create a correct diff.
<mrevell> Ah, just a moment then
* rockstar changed the topic of #launchpad-reviews to: on-call: Edwin || reviewing: - || queue [sinzui, Ursinha, rockstar, rockstar, rockstar, rockstar] || This channelislogged:http://irclogs.ubuntu.com||https://code.edge.launchpad.net/launchpad/+activereviews
<bac> henninge: yeah, they are still warnings
<bac> henninge: but that particular one will be fixed in many branches.
<henninge> bac: we weren't sure earlier but is that likely to cause merge conflicts
<henninge> áº
<henninge> ?
<henninge> ^
<bac> henninge: probably
<henninge> so that's why I asked Matt to back it out.
<henninge> bac: do you know of a branch that has the fix that is about to land?
<bac> henninge: i don't.  it's complicated to try to shepherd through.  perhaps it would be best just to do a simple rs branch to fix it
<henninge> bac, I guess that's true. rs=bac?
<bac> henninge: sure
<bac> it's a two line fix...
<henninge> I know ...
<henninge> mrevell: so you only show the popup on the dashboard? What about the +translate page?
<mrevell> henninge, I'm not sure there's a suitable place for it on the +translate page. What do you think?
<henninge> mrevell: have you seen the balloon?
<henninge> mrevell: I wonder if we could just put another (not dismissiable) one on top of that "Translating for the first time? Read the tips for new translators."
<henninge> for no-karma people.
<mrevell> henninge, that'd be ideal. I still don't see the balloon -- where is it on this page, for example? https://translations.edge.launchpad.net/ubuntu/karmic/+source/gfxboot-theme-ubuntu/+pots/bootloader/en_GB/+translate
<henninge> mrevell: the en_GB team doesn't have any instructions, either.
<henninge> mrevell: see the German version:
<henninge> https://translations.edge.launchpad.net/ubuntu/karmic/+source/gfxboot-theme-ubuntu/+pots/bootloader/de/+translate
<mrevell> heh, ok
 * mrevell looks
<mrevell> Ah, yes, that'd be perfect henninge
<henninge> mrevell: only I'd not make it dismissible because that requires Javascript.
<henninge> mrevell: It is dismissed when people start translating.
<mrevell> Sounds good
<henninge> cool
<henninge> mrevell: BUT
<henninge> we do need an UI review for this ...
<mrevell> henninge, right
<henninge> two stacked balloons, maybe martin does not like that.
<henninge> mrevell: I think both ui reviewers (I know of) are unavailable today.
<henninge> but there are a few ui* reviewers on the schedule.
<intellectronica> EdwinGrubbs: busy review shift, eh?
<EdwinGrubbs> intellectronica: it seems like it
<intellectronica> EdwinGrubbs: i wanted to ask for another review, but i'll try to see if someone else can do it
<intellectronica> deryck: https://code.edge.launchpad.net/~intellectronica/launchpad/patch-in-mailnotification/+merge/17761
<EdwinGrubbs> sinzui: is the hotness being updated by triggers yet?
<sinzui> I do not know. I do not think that is a issue in my branch
<bac> sinzui: i've been reviewing your comments on https://code.edge.launchpad.net/~bac/launchpad/bug-499351-batching-dls/+merge/17492 together with beuno's.  i think i need to have a call after lunch to figure it out.
* adiroiban changed the topic of #launchpad-reviews to: on-call: Edwin || reviewing: - || queue [sinzui, Ursinha, rockstar, rockstar, rockstar, rockstar,adiroiban(bug-359180-take-2)] || This channelislogged:http://irclogs.ubuntu.com||https://code.edge.launchpad.net/launchpad/+activereviews
* adiroiban changed the topic of #launchpad-reviews to: on-call: Edwin || reviewing: - || queue [sinzui, Ursinha, rockstar, rockstar, rockstar, rockstar,adiroiban(bug-359180-take-2)] || This channel is logged: http://irclogs.ubuntu.com || https://code.edge.launchpad.net/launchpad/+activereviews
<sinzui> bac: okay
<rockstar> EdwinGrubbs, does the topic reflect your review progress?
* EdwinGrubbs changed the topic of #launchpad-reviews to: on-call: Edwin || reviewing: sinzui || queue [Ursinha, rockstar, rockstar, rockstar, rockstar,adiroiban(bug-359180-take-2)] || This channel is logged: http://irclogs.ubuntu.com || https://code.edge.launchpad.net/launchpad/+activereviews
<EdwinGrubbs> rockstar: now it does
<rockstar> EdwinGrubbs, okay, thanks.
<EdwinGrubbs> Ursinha-nom: do you need someone to review lp:~ursinha/launchpad/bug422056-add-translation-focus?
<Ursinha-nom> EdwinGrubbs: yes, please
<EdwinGrubbs> rockstar: fyi, your upgrade-ui branch shows zero diff lines on the +activereviews page.
<rockstar> EdwinGrubbs, yes, this is why I put the real diff in the comment.
<rockstar> EdwinGrubbs, and if you didn't see the comment in the prereq branch, you've already reviewed all the code in the upgrade-ui branch.
<rockstar> EdwinGrubbs, it might be some artifact of the pipeline.  I'm not sure.
<rockstar> EdwinGrubbs, howgoesit?
<EdwinGrubbs> rockstar: I'm working on Ursinha's branch.
* EdwinGrubbs changed the topic of #launchpad-reviews to: on-call: Edwin || reviewing: Ursinha || queue [rockstar, rockstar, rockstar, rockstar,adiroiban(bug-359180-take-2)] || This channel is logged: http://irclogs.ubuntu.com || https://code.edge.launchpad.net/launchpad/+activereviews
<rockstar> EdwinGrubbs, ah, okay.
<rockstar> EdwinGrubbs, I'll probably be heading to eat something in 30 minutes, so you may want to do the pedantry branches first (they're small).
<EdwinGrubbs> rockstar: I'll be getting something to eat soon, too.
<rockstar> EdwinGrubbs, great, that works out well then.
<rockstar> I also have another branch that I'll probably need to cut in front of adiroiban with, if that's okay.
<adiroiban> no hurry from my side :)
<rockstar> adiroiban, great, because I kinda do have a hurry on my side.
<adiroiban> go, go, go
* rockstar changed the topic of #launchpad-reviews to: on-call: Edwin || reviewing: Ursinha || queue [rockstar, rockstar, rockstar, rockstar, rockstar, adiroiban(bug-359180-take-2)] || This channel is logged: http://irclogs.ubuntu.com || https://code.edge.launchpad.net/launchpad/+activereviews
<bac> jamalta: ping
<jamalta> bac: hey there
<jamalta> or rather
<jamalta> pong :)
<bac> hi jamalta have you arranged for anyone to land your changes from yesterday?
 * jamalta tries to remember which changes from yesterday bac is referring to
<jamalta> bigjools said he would land my changes from today
<bac> jamalta: the two merge proposals i reviewed for you
<jamalta> as for yesterday, let me have a look
<jamalta> bac: oh
<jamalta> no, i didn't know i was supposed to find someone to land them
<bac> jamalta: ah, ok.  yeah it doesn't happy automatically
<bac> jamalta: i'll be glad to do it.
<jamalta> bac: well i know that! what i meant is that usually whoever approved them took care of landing them
<bac> jamalta: it's best if you can remember to either ask the reviewer or someone else
<jamalta> bac: thanks! that would be great
<jamalta> bac: right, well next time i'll make sure to ask :) thanks
<bac> jamalta: i should've asked.
<bac> if we both try to remember perhaps one will
<jamalta> bac: haha, sounds like a deal
<jamalta> well thanks for pointing that out
<jamalta> i really appreciate it
<EdwinGrubbs> Ursinha: what information do I need to add to make a series translatable?
<Ursinha> EdwinGrubbs: you need to add a template so you have what to translate on it
<Ursinha> EdwinGrubbs: to be really honest I've tested that by adding it in the tests, with factories
<bac> jamalta: can you go to the MPs and set the commit message.  it is the text used when we send it to PQM
<bac> https://code.edge.launchpad.net/~jamalta/launchpad/newtag-statement-redundancy/+merge/17670
<jamalta> bac: sure thing
<jamalta> sorry, forgot to do it for those
<jamalta> bac: ok it's changed
<bac> thx
<Ursinha> danilos: are you there?
<Ursinha> or henninge :)
<Ursinha> henninge: which scripts need to be run for the imports to be approved in my local instance?
<henninge> Ursinha: AFAIK you need to make run_all
<henninge> Ursinha: and then
<henninge> Ursinha: uploads or bzr imports?
<Ursinha> henninge: uploads
<EdwinGrubbs> Ursinha: since this came up in the reviewer meeting, I'll ask, did you have a pre-impl call with someone on the translations team.
<Ursinha> EdwinGrubbs: yes
<henninge> Ursinha: you can approve manually from the queue, unless you wish to try out the gardener
<Ursinha> henninge: admin user isn't a rosetta admin?
<henninge> Ursinha: and then "cronscripts/rosetta-poimport.py"
<Ursinha> henninge: because the Approve option is grayed
<henninge> Ursinha: you need to set an import target first.
<henninge> Ursinha: edit the entry
<henninge> Ursinha: if it's a new template, make up a domain and template name
<Ursinha> henninge: now everything makes sense :)
<Ursinha> henninge: what must happen for the imports to leave the queue?
<Ursinha> henninge: I see they were imported here, and approved, and still on the list
<henninge> Ursinha: the gardener will clear them out after a few days.
<henninge> Ursinha: same for "Deleted" entries.
<Ursinha> henninge: I see
<henninge> Ursinha: you do know what the gardener is?
<Ursinha> henninge: I presume it's a script that cleans things
<henninge> Ursinha: it used to called the "auto approver", it approves and cleans, yes.
<henninge> That's why jtv renamed it.
<Ursinha> henninge: I see
<Ursinha> thanks for the explanation
<gary_poster> flacoste or gmb: review https://code.edge.launchpad.net/~gary/launchpad/bzr-builder-symlink/+merge/17774 for testfix?
<flacoste> gary_poster: r=me
<gary_poster> flacoste: thanks
<Ursinha> henninge: merging caused no conflicts :)
<henninge> Ursinha: See how smart Bazaar is? ;-)
<Ursinha> henninge: hehehe :)
<bac> jamalta: your branches have been sent off to ec2.  they will certainly fail when submitted to PQM due to buildbot being in testfix but i'll land them later.
<flacoste> bac: testfix mode should be off
<flacoste> i just reviewed gary's fix
<bac> flacoste: so it'll be a foot race
* EdwinGrubbs changed the topic of #launchpad-reviews to: on-call: Edwin || reviewing: rockstar || queue [rockstar, rockstar, rockstar, rockstar, adiroiban(bug-359180-take-2)] || This channel is logged: http://irclogs.ubuntu.com || https://code.edge.launchpad.net/launchpad/+activereviews
<bac> flacoste: and, sadly, i have a head start
<flacoste> bac: i think gary will submit his branch directly to PQM
<flacoste> so it should land before yours EC2 tests complete
<gary_poster> flacoste, bac, yes
<gary_poster> bac, it is in PQM now
<bac> flacoste: ah, right, so we go out of testfix as soon as buildbot starts working on it, not finishing?
<flacoste> bac: yes
<bac> excellent
<flacoste> bac: as soon as PQM sees a testfix revision, it considers that the tree is clean
<flacoste> optimist algorithm
<gary_poster> from flacoste, the optimist :-)
<jamalta> bac: ah ok
<jamalta> thanks for everything :)
<bac> jamalta: np.  you did all the hard work.
<jamalta> bac: well, i really appreciate the help and insight :)
<jamalta> it's really helping me learn launchpad
<bac> adiroiban: thanks for the fix.  i've approved the MP with a two-line change
<adiroiban> bac: ok. will you send the branch for testing ? or should I run the full tests?
<bac> adiroiban: i will land it for you
<bac> adiroiban: just let me know when it is finalized
<adiroiban> bac: finalized?
<bac> adiroiban: also, i note some of the changes you made introduced trailing whitespace.  if you can remove those it'd be great.
<bac> adiroiban: yes, i make one  final code change suggestion
<bac> adiroiban: sorry if i'm being unclear
<adiroiban> bac: ah. got the email :)
<bac> ok
<adiroiban> a bit of delay
<adiroiban> ok
<adiroiban> bac: done. I was thinking that âmake lintâ can catch such errors
<bac> adiroiban: so you have pushed your changes and want me to land it as is?
<adiroiban> bac: what are the outstanding problems?
<bac> adiroiban: none.  i just got your diff and it looks good
<bac> adiroiban: so i will land it as is.  thanks.
<adiroiban> thanks for your help and guidance! much appreciated
<rockstar> EdwinGrubbs, hi.  Do you have any questions so far?
<EdwinGrubbs> rockstar: not yet. sorry I've been slow starting on it.
<rockstar> EdwinGrubbs, that's okay, as long as you can get to the branch upgrade ones today.  The pedantry branches can probably get done tomorrow.
<EdwinGrubbs> rockstar: so, is BZR_BRANCH_8 the same as 2a?
<rockstar> EdwinGrubbs, no, BZR_BRANCH_8 is a branch format.  2a is a repository format.
<rockstar> EdwinGrubbs, in a follow up branch, I add BZR_BRANCH_7 to the non-upgradables as well, since BZR_BRANCH_8 is still considered a development format.
<rockstar> EdwinGrubbs, also, all these branches will probably land in one final branch together (hopefully today)
<EdwinGrubbs> rockstar: I see where BranchUpgradeJob.create() checks upgrade_pending, but I don't see where that attribute is set. Is that in another branch?
<rockstar> EdwinGrubbs, which branch are you looking at?
<EdwinGrubbs> rockstar: upgrade-job-2.0-changes
<rockstar> EdwinGrubbs, it's a property defined in the model code.
<EdwinGrubbs> rockstar: Shouldn't lp/code/configure.zcml specify that requestUpgrade() requires launchpad.Edit instead of launchpad.View?
<rockstar> EdwinGrubbs, yes, you are right.
<rockstar> EdwinGrubbs, also, I'd like to expose that through the API eventually, but exposing jobs in the API right now is a little awkward.
<leonardr> gary: given edwin's queue, would you like to review another of my branches?
<leonardr> do you have time today?
<gary_poster> leonardr: I'll give it a whirl, sure
<EdwinGrubbs> rockstar: review sent
<rockstar> EdwinGrubbs, cheers! One down, five to go.
<rockstar> EdwinGrubbs, I can take adiroiban's review if you'd like.
<EdwinGrubbs> rockstar: you make it sound like a drinking game.
<EdwinGrubbs> rockstar: I would appreciate it if you take his mp
<rockstar> EdwinGrubbs, only when it's Soyuz branches.
<rockstar> adiroiban, ping
<adiroiban> rockstar: hi
<rockstar> adiroiban, why the change to lib/lp/registry/browser/peoplemerge.py
<rockstar> Or rather, any of the people-merge stuff.
<adiroiban> I messed up something 
<rockstar> adiroiban, actually, most of this branch looks unrelated to the bug you reference.
<adiroiban> hm...somethings get messed durring the devel merge
<rockstar> adiroiban, you submitted the merge against db-devel
<adiroiban> ah...
<adiroiban> true
<adiroiban> can I change the MP?
<adiroiban> or should I create a new one for lp-devel?
<leonardr> gary: https://code.edge.launchpad.net/~leonardr/lazr.restful/operation-removed-in/+merge/17784
<gary_poster> leonardr: ack, looking
<adiroiban> rockstar: sorry, here is the MP https://code.edge.launchpad.net/~adiroiban/launchpad/bug-359180-take-2/+merge/17785
<rockstar> adiroiban, could you please delete the old one then?
<gary_poster> leonardr: typo versuibed
<adiroiban> done
<leonardr> gary: i fixed that but haven't pushed yet
<gary_poster> leonardr: cool
<adiroiban> the new diff looks ok
<rockstar> adiroiban, yes, this looks much better.
<rockstar> adiroiban, I'm assuming you talked to someone in translations about this?
<adiroiban> rockstar: yes. the big discussion is on the but comments
<adiroiban> and this is also the second part of the change
<rockstar> adiroiban, line 104 of the patch needs force returns in it.  It is too long.
<gary_poster> leonardr: in @call_with(fixed=...), I am not familiar with that kwarg feature.  That means that, when the webservice calls the method, it includes the value as a kwarg (and does not allow a webservice caller to override it)?
<adiroiban> rockstar: yes. I don't know why make lint did not catch that error. I'm fixing my MP script to catch such errors
<rockstar> adiroiban, it's a pagetest, and lint doesn't look at non-python files for lint.
<leonardr> gary: yes, that argument does not show up as part of the named operation's signature
<gary_poster> leonardr: got it, thanks
<gary_poster> leonardr: approved
<leonardr> gary, great
<jamalta> i have to admit, that's one rockstar queue
<rockstar> jamalta, it's the best kind.
<jamalta> rockstar: ;)
<EdwinGrubbs> rockstar: it took me a while to find the upgrade link, since I expected it in the action menu for the branch as opposed to thinking about it as editing an attribute. Can you create a screenshot of that and ask beuno about the placement?
<rockstar> EdwinGrubbs, well, this isn't the final UI.  I have one more branch that does all the polish, etc. but I need some graphics.
<rockstar> EdwinGrubbs, I think the link needs to be much more prominent in these cases, since we REALLY want people to upgrade.  It's possible, however, that we'll be backing out the UI for this before next week's rollout.
<EdwinGrubbs> rockstar: yeah, it also seems you would want to encourage users to press the upgrade button but also warn them about what they are about to do on the form.
<rockstar> EdwinGrubbs, basically, I want a big button, but big buttons are a no-no now, apparently.  I'm not sure how it's going to happen, but I'm going to leave that until we know the rest of the feature is okay.
<rockstar> The plan is to spend today and tomorrow QAing.  If the actual process is good, then the new UI will land as soon as PQM opens back up.
<rockstar> (merely because UI work is not release critical)
<bac> adiroiban: could you set the commit message on https://code.edge.launchpad.net/~adiroiban/launchpad/bug-340662-take-2/+merge/17598
<adiroiban> bac: done
<bac> adiroiban: thanks
<gary_poster> flacoste: are you available for a pretty quick review of the hack fix for bug 491705, https://code.edge.launchpad.net/~gary/launchpad/bug-491705-hack/+merge/17788 ?
<mup> Bug #491705: AttributeError: 'NoneType' object has no attribute 'utf_8_decode' <Launchpad Foundations:In Progress by gary> <https://launchpad.net/bugs/491705>
<flacoste> gary_poster: was actually looking at it right now :-)
<gary_poster> flacoste: :-) thanks
<adiroiban> bac: was the import warning solved on devel?
<adiroiban> for IProduct
<bac> yes
<adiroiban> ok
<adiroiban> there is one more thing regarding translations tests.
<adiroiban> it looks like they are launched twice
<adiroiban> Tests with failures: lib/lp/translations/browser/tests/pofile-views.txt lib/lp/translations/tests/../browser/tests/pofile-views.txt
<bac> adiroiban: odd, i haven't seen that before
<bac> adiroiban: are those tests failing now?
<EdwinGrubbs> rockstar: when registering a new branch, I only see Hosted, Mirrored, and Remote branch types. How does an Imported branch get added?
<rockstar> EdwinGrubbs, there's a different flow for import branches.  It's on a link that says "Import a branch"
<rockstar> EdwinGrubbs, gnome-terminal/imported is a sample import branch.
<EdwinGrubbs> rockstar: is there any reason that a branch such as ~vcs-imports/evolution/main, which has an Imported branch type, but hasn't been imported yet, should not show that link?
<rockstar> No, I don't think so.
<rockstar> EdwinGrubbs, unless you don't have permission to see the link.
<rockstar> EdwinGrubbs, your best bet is to log in as mark.
<adiroiban> rockstar: I've fixed all long lines from that file.
<rockstar> adiroiban, great, thanks.
<adiroiban> bac: ah.. the test are not failing. Is just they are run twice
<adiroiban> bac: for testing, I changed a test so that it will fail... and I can see the test is run twice
<bac> adiroiban: that's interesting but unrelated to your branch.  perhaps you could open a bug
<EdwinGrubbs> rockstar: it's apparently caused by the <tal:has-codeimport condition="branch/code_import"> conditional in branch-import-details.pt. Do you think it would be ok to move it outside that condition?
<rockstar> EdwinGrubbs, I don't think.  An import branch shouldn't ever exist without a code_import as well.
<rockstar> I just fixed a bug pertaining to that.
<rockstar> EdwinGrubbs, so in this case, the sample data sucks.
<EdwinGrubbs> rockstar: ok, sounds good. r=me
<rockstar> EdwinGrubbs, great, thanks.
<EdwinGrubbs> rockstar: do you want me to review pedantry3 or jam-suggested-changes next?
<rockstar> EdwinGrubbs, probably the one from jam next, please.
<adiroiban> bac: bug filled
<adiroiban> rockstar: do I need to set the commit message for this branch ?
<bac> adiroiban: great.  your branch is off at ec2 running.  i'll bounce you the email when it comes back.
<rockstar> adiroiban, do you need your that branch landed?
<adiroiban> rockstar: what do you mean? :)
<adiroiban> should I found someone else to land it?
<rockstar> adiroiban, my answer to your question is in direct relation to the answer to my question.  If you want it landed, then yes, please set the commit message.
<adiroiban> rockstar: :) done :)
<EdwinGrubbs> rockstar: I sent the review. I will be afk for a little bit, but I'll be able to look at your reply and the last branch today.
<EdwinGrubbs> rockstar: I'm back
<rockstar> EdwinGrubbs, looking at your comments now.
<EdwinGrubbs> rockstar: btw, I just sent the other review.
* EdwinGrubbs changed the topic of #launchpad-reviews to: on-call: - || reviewing: rockstar || queue [] || This channel is logged: http://irclogs.ubuntu.com || https://code.edge.launchpad.net/launchpad/+activereviews
#launchpad-reviews 2010-01-21
<rockstar> EdwinGrubbs, just fixed the lint and test issue, pushing now.
* rockstar changed the topic of #launchpad-reviews to: on-call: - || reviewing: - || queue [] || This channel is logged: http://irclogs.ubuntu.com || https://code.edge.launchpad.net/launchpad/+activereviews
<EdwinGrubbs> rockstar: I'm getting AttributeError: type object 'RepositoryFormat' has no attribute 'BZR_BZR_CHK_2A'
<rockstar> EdwinGrubbs, where are you getting that?
<EdwinGrubbs> rockstar: when running test_needsUpgrading_repository_format_upgrade_not_needed (lp.code.model.tests.test_branch.TestBranch)
<rockstar> EdwinGrubbs, huh.  That's odd.
<EdwinGrubbs> rockstar: here is the command I used: ./bin/test -vv -m lp.code.model.tests.test_branch -t test_needsUpgrading_repository_format_upgrade_not_needed
<rockstar> EdwinGrubbs, test fixed.  I have some other tests that need to be fixed as well.  Nothing major though.
<EdwinGrubbs> rockstar: did you get the error when you ran the test?
<rockstar> EdwinGrubbs, yeah, I must have had an editor hiccup.
<rockstar> EdwinGrubbs, test changes pushed.
<EdwinGrubbs> rockstar: sent you some more lint errors. r=me
<rockstar> EdwinGrubbs, cheers.
<mrevell> Hey, anyone care to review a small text change branch? https://code.edge.launchpad.net/~matthew.revell/launchpad/tour-commercial-tweak-bug-393348/+merge/17808
<henninge> sinzui: ping
<henninge> BjornT: are post-3-ui-cleanups (still) rs'able?
<bac> hi jamalta-afk
<mrevell> Hey, anyone care to review a small text change branch? https://code.edge.launchpad.net/~matthew.revell/launchpad/tour-commercial-tweak-bug-393348/+merge/17808
<bac> mrevell: sure
<mrevell> thanks bac!
<bac> mrevell: maybe strike 'new' at line 9
<mrevell> yeah, good idea bac
<bac> mrevell: other than that it's a-ok
<mrevell> bac, You happy if I use "beta" in place of "new" in line 9?
<bac> mrevell: 'beta' is causing confusion
<mrevell> Is it?
<mrevell> Okay, I'll just scrub new and leave it at that.
<bac> i get email from people saying they don't want to trust their code to a 'beta-level' code hosting service
<mrevell> oh really? Crumbs
<mrevell> there's another mention of "beta" in that page, so maybe I should scrub that too.
<bac> yeah, then i lamely explain the 'beta' refers to our business model not the application
<mrevell> Ah, I see.
<bac> did VAT go up or were we just wrong?
<bac> hi gary_poster, got a sec?
<gary_poster> hi bac, yes
<gary_poster> (though have call soon)
<mrevell> hey bac, could you please look at the newly updated diff, when you have a moment? I've removed the other mention of "beta" on that page. https://code.edge.launchpad.net/~matthew.revell/launchpad/tour-commercial-tweak-bug-393348/+merge/17808
<jamalta> bac: hey there
<jamalta> bac: saw your email, i'll fix the issue in a few
<bac> ok
* rockstar changed the topic of #launchpad-reviews to: on-call: rockstar open for bizniss || reviewing: - || queue [] || This channel is logged: http://irclogs.ubuntu.com || https://code.edge.launchpad.net/launchpad/+activereviews
<jamalta> bac: i pushed the fix to that tset
<jamalta> test*
<bac> jamalta: ok
<jtv> rockstar: I have to leave now, but if you feel like reviewing a buildfarm-related branch...
<rockstar> jtv, shoot it on over.
 * jtv aims
<jtv> https://code.edge.launchpad.net/~jtv/launchpad/bug-499405-translationtemplates-buildmanager/+merge/17811
<jtv> rockstar: thanks
<deryck> intellectronica, https://code.edge.launchpad.net/~deryck/launchpad/hot-bugtasks-to-hot-bugs-442170/+merge/17826
<intellectronica> deryck: https://code.edge.launchpad.net/~intellectronica/launchpad/search-for-bugs-affecting-me/+merge/17827 (if you're still up for another review)
<deryck> intellectronica, sure
<intellectronica> deryck: http://pastebin.ubuntu.com/360156/
<gmb> rockstar: allenap and I have a branch for review but we're about to head to lunch. Do you want to take it now and do an offline review or would you rather wait until we return?
<rockstar> gmb, your choice.
<rockstar> gmb, if it's a baby punching patch, you may want to walk away now.
<gmb> rockstar: If you could start looking at it now that would be wonderful.
<gmb> rockstar: Not quite baby-punching. Jobs system.
<gmb> Using thereof.
<rockstar> gmb, cool, I know that.
<gmb> Rawk.
<rockstar> gmb, url?
<gmb> rockstar: https://code.edge.launchpad.net/~gmb/launchpad/jobbifiy-bug-heat-calculations-509193/+merge/17831
<gmb> rockstar: We'll catch up when we've nommed, then. thanks!
 * rockstar lunches
<gmb> rockstar: Thanks for the review; response sent.
<rockstar> gmb, r=me
<gmb> rockstar: Awesome, thanks!
* rockstar changed the topic of #launchpad-reviews to: on-call: - || reviewing: - || queue [] || This channel is logged: http://irclogs.ubuntu.com || https://code.edge.launchpad.net/launchpad/+activereviews
#launchpad-reviews 2010-01-22
<sinzui> EdwinGrubbs: ping
<EdwinGrubbs> sinzui: pong
<sinzui> EdwinGrubbs: You have a small conflict in your branch. adiroiban changed DistroSeriesStatus to SeriesStatus yeterday
<sinzui> EdwinGrubbs: I have one question. What does a failure look like when we test this:
<sinzui>     +    >>> assert vocabulary_tokens == series_names
<sinzui> EdwinGrubbs: we have avoided asserts in doctests since we discovered a bad one in a test 2 years ago
<sinzui> I would prefer that you print the vocabulary_tokens. We do know which ones they should be
<sinzui> EdwinGrubbs: I approved your branch on condition you resolve the merge conflict and change the assert in the doctest
<EdwinGrubbs> sinzui: I'll look at that. I have to go though.
<sinzui> okay. land it when you are staisfied
<sinzui> jml: ping
<rockstar> stub, around for a review?
<stub> yo
<rockstar> stub, hooray.  I'll send it over.
<stub> ?
<rockstar> stub, I accidentally sent it to devel.  Resubmitted as https://code.edge.launchpad.net/~rockstar/launchpad/fix-scan-branches-script/+merge/17869
<stub> rockstar: r=stub
<rockstar> stub, cheers.
<stub> https://code.edge.launchpad.net/~stub/launchpad/pending-db-changes/+merge/17872 needs a review. Staging is busted until this lands.
* jtv changed the topic of #launchpad-reviews to: on-call: - || reviewing: - || queue [jtv: http://tinyurl.com/yl59z62] || This channel is logged: http://irclogs.ubuntu.com || https://code.edge.launchpad.net/launchpad/+activereviews
<jtv> Any reviewers here today?
* sinzui changed the topic of #launchpad-reviews to: on-call: - || reviewing: - || queue [jtv: http://tinyurl.com/yl59z62, sinzui, sinzui] || This channel is logged: http://irclogs.ubuntu.com || https://code.edge.launchpad.net/launchpad/+activereviews
* sinzui changed the topic of #launchpad-reviews to: on-call: - || reviewing: - || queue [sinzui, sinzui] || This channel is logged: http://irclogs.ubuntu.com || https://code.edge.launchpad.net/launchpad/+activereviews
<sinzui> jtv: I approved your branch
<sinzui> I am seeking one or two reviewers for two small branches.
<intellectronica> sinzui: i'll take one
<sinzui> intellectronica: thanks: You will have some insights into this one and you can challenge me if you think I am wrong: https://code.edge.launchpad.net/~sinzui/launchpad/release-love
<intellectronica> feel the lurve
<intellectronica> sinzui: so basically, being launchpad's most active triager, you're being a karma ho with this branch, eh? ;)
<sinzui> intellectronica: I am also the affective owner of many projects, and I do not get karma for that. I need to add registry karmic events (my pretext will be to award karma for providing information needs to link ubuntu to upstream)
<intellectronica> i think that makes sense
<intellectronica> and i really like the improvements to release mgmt
<intellectronica> sinzui: r=me
<sinzui> intellectronica: thanks.
<intellectronica> sinzui: i'm still waiting on some tests to finish, so i can review the other one too if you still need a review
<sinzui> intellectronica: this one adds the UI to a hidden page (I want edge feed back before letting users use it). https://code.edge.launchpad.net/~sinzui/launchpad/needs-packaging-bug-509848
<intellectronica> sinzui: unless it's necessary (because the extra whitespace creates formatting problems), i prefer closing html tags on a new line. can you change the anchor on line 19 of your diff?
<intellectronica> and in the template too, of course
<sinzui> oh, that is clumbs of me. I will fix both
<sinzui> clumbsy
<intellectronica> cool, thanks
<intellectronica> sinzui: 'packagings' reads quite funny to me. are you sure this is the best choice? i would have dropped the plural s (but i'm not a native speaker so be critical with my english advice)
<intellectronica> sinzui: everything else looks good to me, so r=me
<sinzui> thanks
<intellectronica> sinzui: note that i haven't done any ui reviewing at all. i'll leave that for beuno-lunch (or noodles)
<sinzui> intellectronica: understood
* sinzui changed the topic of #launchpad-reviews to: on-call: - || reviewing: - || queue [sinzui] || This channel is logged: http://irclogs.ubuntu.com || https://code.edge.launchpad.net/launchpad/+activereviews
<sinzui> beuno: can you take a look at https://code.edge.launchpad.net/~sinzui/launchpad/needs-packaging-bug-509848/+merge/17848. There is a comment and picture for you
<beuno> sinzui, I have it opened with "needs fixing" and 2 lines already written
<beuno> the heat is making me work slower, but I'm getting there!
<sinzui> beuno: This page will be hidden even if it lands because I am certain it will need refining from edge feedback
<beuno> sinzui, ok, I'll take that into account then
<sinzui> Hi beuno I like your suggestions
<beuno> hey sinzui 
<beuno> thanks
<sinzui> beuno: Upstream links is the current +packaging page that shows what is linked
<beuno> sorry it took me so long
<beuno> sinzui, so that will end up showing a gazillion packages, right?
<sinzui> right.
<sinzui> That report will change to indicate what is missing from the upstream project information
<sinzui> We might get better adoption from users if we had good names for these two pages
<bac> hey beuno -- care to gander at some screenshot?
<beuno> bac, sure, shoot
<beuno> sinzui, how about a drop down?
<bac> this adds the linking to an upstream in the source package portlet
<bac> http://people.canonical.com/~bac/linkup-multiple.png
<bac> http://people.canonical.com/~bac/linkup-one.png
<bac> http://people.canonical.com/~bac/linkup-none.png
<beuno> bac, I will peak in 10, after this call
<bac> beuno: excellent
<bac> i shall make some tea
<deryck> gmb, if you don't mind sir?  https://code.edge.launchpad.net/~deryck/launchpad/sort-by-heat-as-default-77701/+merge/17919
<deryck> beuno, ping.  how do you feel about sorting by bug heat by default?
<beuno> deryck, everywhere?
<beuno> bac, back
<deryck> beuno, yes, for bug listings.  and then menu for selecting order by now would read bug heat rather than importance.
<beuno> deryck, I feel uneasy about it
<beuno> maybe afer we've had it rolling for a while?
<deryck> beuno, sure.  The work is done, so you can even look at my branch and decide later.  after it's been live for a bit and tweaked some.
 * deryck feels a bit unsure about it too
<beuno> deryck, we need to think about listings from the user vs developer perspective
<beuno> and which listing is for whom
<deryck> beuno, right, very good point.  and I'll admit I didn't consider the difference there for this one question.
<beuno> bac, a few questions
<beuno> why isn't the project linked?
<beuno> (as in, why is the "view project" seperate?)
<beuno> because it's a radio button, etc?
<beuno> what's the difference between "link" and "set" upstream link?
<bac> beuno: ah, good question.  TBH, just going off the design sinzui did
<bac> beuno: no reason we can't make that change
<beuno> there's some padding issue with those radio buttons as well
<sinzui> Maybe we want to say "Set upstream project"
<EdwinGrubbs> sinzui: did you need me to do a review for you?
<sinzui> oh, know, beuno was kind enough to complete the review
* sinzui changed the topic of #launchpad-reviews to: on-call: - || reviewing: - || queue [] || This channel is logged: http://irclogs.ubuntu.com || https://code.edge.launchpad.net/launchpad/+activereviews
<sinzui> beuno: are you sill about to see the picture of my revision? https://code.edge.launchpad.net/~sinzui/launchpad/needs-packaging-bug-509848/+merge/17848
<deryck> kfogel, http://pastebin.ubuntu.com/360916/
#launchpad-reviews 2011-01-17
<leonardr> edwingrubbs, want to review https://code.launchpad.net/~leonardr/lazr.restful/fix-ws-accept-on-redirect/+merge/46552 ?
<EdwinGrubbs> leonardr: sure
<EdwinGrubbs> leonardr: r=me
#launchpad-reviews 2011-01-18
<jcsackett> anyone able to take a look at https://code.launchpad.net/~jcsackett/launchpad/archivemirror-timeout-618400
<jcsackett> er, https://code.launchpad.net/~jcsackett/launchpad/archivemirror-timeout-618400/+merge/46550
<thumper> https://code.launchpad.net/~thumper/launchpad/webservice-person-adapter/+merge/46658
<thumper> leonardr: want to review that branch?
<leonardr> thumper: sure. one thing comes to mind right now
<leonardr> i found all the other places we use a custom IFieldHTMLRenderer
<leonardr> there's one we can replace with the generic person one
<thumper> ok
<thumper> we should do that in this branch ?
<leonardr> maybe, or we could do another branch. the thing i want to bring up is it looks like there's one we can use for IText in general
<leonardr> it obfuscates email addresses and calls text_to_html()
<leonardr> it's used for bug description, mp commit message, and mp description
<thumper> ok...
<leonardr> even if we don't use it for itext in general, we should keep it in some generic place. right now there are two implementations. where should the single implementation go, and does that change our story for where we put the user formatter?
<leonardr> while you chew on that i will look at the code
<thumper> ok
<thumper> I'm also in the taleo training meeting
<leonardr> yeah
<leonardr> sourcepackagerecipe.py seems to just have lint
<thumper> leonardr: yes, there was some lint cleanup there
<leonardr> aargh, my apidoc branch is being silently dropped on the floor. that's why it didn't land
<leonardr> thumper: a comment before the assertion in test_person_renderer wold be nice, given that you start off with comments
<leonardr> "render a recipe owner as a link" -> "render a person as a link to the person"
<thumper> leonardr: ack
<leonardr> the big comment on line 121 should be an xxx, and we should know more about it
<leonardr> i'll try to find mars
<thumper> leonardr: ok
<leonardr> "a marker for people choices" -> "a marker for a choice among people"
<thumper> leonardr: jml also did a review and has made similar comments :)
<leonardr> everything else looks good
<leonardr> ok, very good
<leonardr> i cannot find mars, so i will try to figure out the <span> thing myself
<thumper> leonardr: ok
<thumper> leonardr: it is probably worthwhile doing a generic IText adapter
<thumper> leonardr: I say JFDI (and I will)
<leonardr> thumper: we have discovered a bug in the branch. the value sent into the renderer is not an object, it's a link to the object
<thumper> ah...
<thumper> what?
<thumper> oh
<thumper> the value bit
<leonardr> thumper: basically, we need that field.__name__ thing
<thumper> um...
<thumper> so why does it work?
<leonardr> it works in the unit test because we pass in the object
<thumper> it worked interactively... didn't it?
 * thumper checks
<leonardr> it worked interactively when you passed in the object
<leonardr> value is the result of unmarshallField, which turns an object into a link
<thumper> okay...
<thumper> leonardr: yep...
 * thumper fixes
<thumper> so... we'll need the bind then?
<thumper> so what about IText?
<thumper> hmm... perhaps consistency on the implementat bit
<thumper> no, bind not needed
<thumper> as we aren't getting the value from the field but the context
<wgrant> jcsackett: https://code.launchpad.net/~wgrant/launchpad/bug-702819-bad-uris/+merge/46683
* jtv changed the topic of #launchpad-reviews to: On call: - || reviewing:  || queue: [jtv] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews
<jtv> gmb: would you do me the honour of reviewing our bugzilla bugwatch branch?
<jtv> This one: https://code.launchpad.net/~jtv/launchpad/bug-34102/+merge/46685
<gmb> jtv: Sure. I'll take a look presently.
<jtv> Splendid.
<thumper> EdwinGrubbs: https://code.launchpad.net/~thumper/launchpad/remove-webservice-xhtml-span/+merge/46686
<gmb> jtv: r=me with some nitpicks.
<jtv> gmb: wow, that's fast.  Pick away.  :)
<leonardr> i'd like a quick review of https://code.launchpad.net/~leonardr/launchpadlib/death-to-edge/+merge/46695
<leonardr> gary, maybe you'd be interested
<leonardr> wgrant: take a look at the branch if you like, otherwise i'll land it in about 5 minutes
<wgrant> leonardr: I am not yet a reviewer :(
<leonardr> wgrant: you can still have an opinion. i'm going to self-review on lifeless's advice
<wgrant> leonardr: It looks fine.
<leonardr> ok
<leonardr> awesome. the script i use to upload the release tarball uses edge, and now it's failing
<lifeless> \o/
* danilos changed the topic of #launchpad-reviews to: On call: - || reviewing:  || queue: [jtv, danilo] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews
* jtv changed the topic of #launchpad-reviews to: On call: - || reviewing:  || queue: [danilo] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews
#launchpad-reviews 2011-01-19
* jtv changed the topic of #launchpad-reviews to: On call: jtv || reviewing:  || queue: [danilo] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews
<wgrant> jcsackett: https://code.launchpad.net/~wgrant/launchpad/bug-694001-apache-username-spaces/+merge/46720
<wgrant> Prepare to cry.
<wgrant> But lifeless convinced me.
<lifeless> wgrant: thanks
<jtv> gmb: don't forgetâhttps://code.launchpad.net/~jtv/launchpad/bug-228940/+merge/46725
* jtv changed the topic of #launchpad-reviews to: On call: jtv || reviewing:  || queue: [] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews
<gmb> jtv: Haven't, don't worry :)
<jtv> âº
<jtv> allenap, bigjools: https://translations.launchpad.net/ubuntu/lucid/+lang/ko/+index?batch=300
<gmb> jtv: Could you review https://code.launchpad.net/~gmb/launchpad/expose-level-bug-704685/+merge/46829 for me?
<gmb> jtv: unping; nm.
<EdwinGrubbs> sinzui: can you review https://code.edge.launchpad.net/~edwin-grubbs/launchpad/bug-669724-projectgroup-milestones-timeout/+merge/46837
 * sinzui looks
<sinzui> EdwinGrubbs, your branch is good to land
<EdwinGrubbs> sinzui: thanks
<gmb> danilos: https://code.launchpad.net/~gmb/launchpad/expose-level-bug-704685/+merge/46829
<danilos> gmb, https://code.launchpad.net/~danilo/launchpad/ss-storm-rewrite-methods/+merge/46843
#launchpad-reviews 2011-01-20
<lifeless> anyone around?
* jtv 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
<wgrant> sinzui: https://code.launchpad.net/~wgrant/launchpad/more-log-parser-fixes/+merge/46951
<jcsackett> sinzui: can you look at https://code.launchpad.net/~jcsackett/launchpad/archivemirror-timeout-618400-2/+merge/46554 when you get the chance?
<bac> deryck: have ten seconds? https://code.launchpad.net/~bac/launchpad/bug-705594/+merge/46970
<deryck> bac, sure.  looking now....
<deryck> bac, looks great.  If you wouldn't mind, it would be good to reformat those lines to our 78 chars rule, but I'm not picky on that.... given we have failed at that in js historically.
<bac> thanks
<EdwinGrubbs> sinzui: can you review: https://code.edge.launchpad.net/~edwin-grubbs/launchpad/bug-663857-product-packages-timeout-part2/+merge/46985
<EdwinGrubbs> StevenK: can you review ^^^
<StevenK> thumper: https://code.launchpad.net/~edwin-grubbs/launchpad/bug-663857-product-packages-timeout-part2/+merge/46985 for mentor +1
#launchpad-reviews 2011-01-21
<StevenK> leonardr: https://code.launchpad.net/~stevenk/launchpad/one-recipe-redirect/+merge/47005
<leonardr> steven: this looks good. i wonder why you do the redirect rather than changing the link
<StevenK> leonardr: There's an easier way? TBH, thumper wrote the code, I wrote the test.
<leonardr> StevenK: as i recall, the problem is that a person clicks their "my recipes" link and goes to a list of recipes
<leonardr> your branch makes the list of recipes go to their only recipe
<leonardr> it would be simpler to just change the link
<leonardr>  StevenK, thumper et al: https://code.launchpad.net/~leonardr/lazr.restful/web-link/+merge/46992
