[07:34] <jtv> bac, I take it you're off-duty now?
=== jtv changed the topic of #launchpad-reviews to: On call: jtv || reviewing: - || queue: [-] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.edge.launchpad.net/launchpad/+activereviews
[09:45] <adeuring> jtv: could you please review this mp: https://code.edge.launchpad.net/~adeuring/launchpad/bug-333521-allow-mail-to-question-subscribers/+merge/23833 ?
[09:48] <jtv> adeuring: coming up
[09:48] <adeuring> jtv: thanks!
=== jtv changed the topic of #launchpad-reviews to: On call: jtv || reviewing: adeuring || queue: [-] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.edge.launchpad.net/launchpad/+activereviews
[09:51] <jtv> adeuring: does the test need to do its own login?  Isn't it enough to pass the identity's email address to the TestCaseWithFactory constructor?
[09:52] <adeuring> jtv: ah, I did not know that
[09:52] <jtv> adeuring: better make sure that it's really the same, because you want this test to fail if the db permissions aren't enough.
[09:54] <jtv> Next, I don't think you really need to create product_owner explicitly.  In the interest of test brevity, why not have factory.makeProduct create an owner and use self.product.owner.email in the test comparison?
[09:54] <jtv> Oh, I see you're doing switchDbUser anyway
[09:55] <jtv> adeuring: Does this test need Librarian at all?  If not, ZopelessDatabaseLayer may be a better layer for your test.  Faster to set up
[09:55] <adeuring> right, we don't need the librarian.
[09:58] <jtv> adeuring: and a definite no-no: don't compare the exact list of notification recipients.  Use self.assertContentEqual to eliminate ordering issues.
[09:58] <adeuring> jtv: OK, makes sense...
[09:59] <jtv> adeuring: what is a "sub" in the context of this test?  Not a sandwich?
[10:00] <jtv> See the docstring.
[10:00] <adeuring> that's plain nonsense. I'll remove it.
[10:00] <adeuring> the title should be ""Ensure that question subscribers are notified about bug expiration"
[10:05] <adeuring> jtv: can you give me an example where the DB user is passed as a constructor parameter to TestCaseWithFactory?
[10:06] <adeuring> jtv: ah, now I see what you meant...
[10:10] <adeuring> jtv: I need LPZopelessLayer because of the method switchDBUser() That is not avaliable in ZopelessDatabaseLayer
[10:12] <adeuring> jtv: current diff: http://paste.ubuntu.com/419731/
[10:12] <jtv> adeuring: oic
=== allenap changed the topic of #launchpad-reviews to: On call: jtv, allenap || reviewing: adeuring, - || queue: [-] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.edge.launchpad.net/launchpad/+activereviews
[10:13] <allenap> Morning jtv :)
[10:13] <jtv> allenap: morning!
[10:17] <jtv> adeuring: one thing I don't like about the test is the somewhat arbitrary division between setup and test.  I think it'd help a lot of you could gut setUp, and put as much as possible in methods with meaningful names.
[10:19] <adeuring> jtv: so... something like a method "createBugProductQuestionAndLinkThem()"?
[10:20] <jtv> adeuring: that name reveals the problem—that it's doing too much.
[10:20] <adeuring> jtv code in the test method is needed there: make a snapshot of a bugtask, change the bugtask, call notify() for the change
[10:21] <jtv> adeuring: that sounds like a more useful breakdown.
[10:21] <adeuring> jtv: well... that's what setUp is for, IMHO: create the test data you need to run the tests ;)
[10:21] <jtv> adeuring: opinion is divided on that subject.  It's easy to create a situation where future generations keep adding tests and depending in different ways on a convoluted setup.
[10:22] <adeuring> jtv: well, ok... but this is a a unit test for very special situation. I was suprised that it occured so late...
[10:22] <jtv> adeuring: ideally, the test would read as a series of logical steps very similar to the ones you just named.
[10:23] <adeuring> so, what about a comment in setUp: "we need a product, a bug and a question linked to the bug in order to..."
[10:24] <adeuring> ...perhaps enriched by "we want explicit person names so that we have stable test data just in case that the factory methos cahnge in a funny way"
[10:26] <jtv> adeuring: I don't even see why you need explicit person names, frankly.
[10:26] <jtv> You need known names, and that's no problem.
[10:26] <adeuring> jtv: yes, that's cruft. I'll remove it
[10:31] <adeuring> jtv: new diff
[10:31] <adeuring> http://paste.ubuntu.com/419745/
[10:32] <jtv> adeuring: btw I do realize this is annoying...  imagine me having a Rambo Moment here as I am reminded of experiences with convoluted tests.  :)
[10:33] <jtv> adeuring: in the comment, I wouldn't say "don't clutter the test data with irrelevant notifications" (which would need a capital letter btw) but just something like "Flush pending notifications for question creation."
[10:33] <adeuring> jtv: sure
[10:34] <jtv> adeuring: also, I always forget... do you need to commit before switchDbUser?
[10:34] <jtv> I guess not, otherwise you wouldn't get the notification, right?
[10:34] <adeuring> jtv: right, otherwise the test wouldn't work...
[10:39] <jtv> adeuring: I still don't see why you want the fixed email addresses.  I think it's actually clearer in the test to say "[product.owner.preferredemail, subscriber.preferredemail]" than to repeat the email address string.
[10:40] <adeuring> jtv: ok, no problem...
[10:41] <jtv> adeuring: then I think I've made it hard enough for you.  :)
[10:42] <jtv> adeuring: approved.
[10:42] <adeuring> thanks
=== jtv changed the topic of #launchpad-reviews to: On call: jtv, allenap || reviewing: -, - || queue: [-] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.edge.launchpad.net/launchpad/+activereviews
=== allenap changed the topic of #launchpad-reviews to: On call: jtv, allenap || reviewing: -, jtv || queue: [-] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.edge.launchpad.net/launchpad/+activereviews
=== allenap changed the topic of #launchpad-reviews to: On call: jtv, allenap || reviewing: -, - || queue: [-] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.edge.launchpad.net/launchpad/+activereviews
[10:50] <jtv> allenap: was that a very fast review?
[10:51] <allenap> jtv: Yes, it was perfect :)
[10:52] <jtv> allenap: thanks!  (hope it wasn't perfect in the sense of "the perfect chance to dash someone's hopes before my first coffee" or "at last a review I can type out in two letters using my right hand only")
[10:55] <wgrant> jtv: What, 'ok'? :P
[10:55] <jtv> wgrant: ooh hadn't thought of _that_ one...
[11:37] <jtv> allenap: me be relocatin
[11:37] <jtv> so copping out for now, sorry to leave you with this madhouse.  :)
=== jtv changed the topic of #launchpad-reviews to: On call: allenap || reviewing: - || queue: [-] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.edge.launchpad.net/launchpad/+activereviews
[11:37] <allenap> jtv: Heh, no worries :)
[11:39] <gmb> allenap, Do you have time to review a 931-line branch :D?
[11:39] <gmb> allenap, It's the first step to refactoring
[11:39] <allenap> gmb: Sure :)
=== allenap changed the topic of #launchpad-reviews to: On call: allenap || reviewing: gmb || queue: [-] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.edge.launchpad.net/launchpad/+activereviews
[11:39] <gmb> Renaming the existing BugWatchUpdater and moving checkwatches.updater to checkwatches.core.
[11:39] <gmb> allenap, https://code.edge.launchpad.net/~gmb/launchpad/cw-refactor-create-rename-bwu-567793/+merge/23842
[11:40] <gmb> allenap, Note, I hate the new name for BugWatchUpdater; feel free to suggest a better one. I just picked it to stop myself bikeshedding
[11:40] <gmb> allenap, Your branch is a prerequisite; I don't know if the MP will DTRT now.
[11:41] <allenap> gmb: CheckwatchesTopBanana
[11:42] <gmb> allenap, How about CheckwatchesYesItWorksNowShutup
[11:43] <allenap> gmb: Yes, that'll do it.
=== henninge_ is now known as henninge
[11:43] <allenap> gmb: TBH, I think CheckwatchesMaster is okay.
[11:44] <gmb> allenap, Fair enough. We can always change it later.
=== StevenK changed the topic of #launchpad-reviews to: On call: allenap || reviewing: gmb || queue: [StevenK] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.edge.launchpad.net/launchpad/+activereviews
[11:48] <allenap> gmb: My grey matter is now grey blancmange, but r=me.
[11:48] <gmb> allenap, Cool. Your desserting is important to us.
[11:48] <gmb> Boom
[11:48] <gmb> Boom
[11:48] <allenap> gmb: Oh god.
[11:49] <gmb> My lame joke know no bounds.
=== allenap changed the topic of #launchpad-reviews to: On call: allenap || reviewing: - || queue: [StevenK] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.edge.launchpad.net/launchpad/+activereviews
=== allenap changed the topic of #launchpad-reviews to: On call: allenap || reviewing: Edwin || queue: [StevenK] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.edge.launchpad.net/launchpad/+activereviews
=== allenap changed the topic of #launchpad-reviews to: On call: allenap || reviewing: - || queue: [StevenK] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.edge.launchpad.net/launchpad/+activereviews
[12:38] <allenap> StevenK: Hi, I don't see a merge proposal for you in https://code.edge.launchpad.net/launchpad/+activereviews. Can you point me to it?
[12:39] <StevenK> allenap: Certainly https://code.edge.launchpad.net/~stevenk/launchpad/fixes-bug-451396/+merge/21706
[12:40] <allenap> StevenK: Okay, I think I'll ask adeuring to follow up on that.
=== allenap changed the topic of #launchpad-reviews to: On call: allenap || reviewing: - || queue: [StevenK] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.edge.launchpad.net/launchpad/+activereviews
[12:42] <adeuring> allenap, StevenK: I'll look
[12:48] <StevenK> adeuring: Thanks!
[12:55] <adiroiban> henninge: Hi, any news about the landing of this branch https://code.edge.launchpad.net/~adiroiban/launchpad/bug-201749/+merge/21250 ? :)
[12:56] <adiroiban> also is there anything I need to do to have this branch ready for code review, https://code.edge.launchpad.net/~adiroiban/launchpad/bug-525992/+merge/23285 ?
=== salgado-afk is now known as salgado
[13:06] <allenap> wgrant: Shall I land lp:~wgrant/launchpad/bug-565739-dont-retry-superseded-builds for you?
[13:06] <adeuring> StevenK: it seems thet problem still remains that the tests pass when the line "if self.isPPA():" is re-introduced in lib/lp/soyuz/model/queue.py .
[13:12] <StevenK> adeuring: Right, I'll revert the changes in queue.py and keep checking
[13:12] <adiroiban> danilos: hi, when you have some time can we have a preimplementation chat for bug 61081? This should be about deciding a range for POTemplate priority. https://code.edge.launchpad.net/~adiroiban/launchpad/bug-61081/+merge/23811
[13:12] <mup> Bug #61081: PO template +edit form needs better validation for priority field <oops> <Launchpad Translations:In Progress by adiroiban> <https://launchpad.net/bugs/61081>
=== mrevell is now known as mrevell-lunch
[13:14] <wgrant> allenap: Yes please.
[13:14] <danilos> adiroiban, lookin
[13:14] <danilos> looking even :)
[13:16] <danilos> adiroiban, fyi, on staging: http://paste.ubuntu.com/419817/
[13:19] <gmb> allenap, I just found another reason not to do relative .foo imports: it makes grep-and-sed fragile :)
[13:19] <adiroiban> danilos: shoud I use those limits? Maybe -100 ... 5000000 ?
[13:20] <danilos> adiroiban, there is only one potemplate (pyroom) that has this priority, all the others are lower than 10k
[13:20] <danilos> adiroiban, as for negative ones, that sounds wrong and we should probably just reset them
[13:21] <danilos> adiroiban, 6 templates have negative priorities
[13:23] <adiroiban> danilos: what values do you suggest for the priority range?
[13:23] <danilos> adiroiban, I guess 0-100000 (i.e. 10x10000 to leave some room there)
[13:24] <adiroiban> danilos: ok. we can reset the values... but I suppose that user will be warned when the want to update again the priority and will have to fix it
[13:26] <adiroiban> danilos: is there anything on this bug that is worth mentioning or it is ready to code or I should just change the range, write tests and create MP ?
[13:26] <danilos> adiroiban, yeah, we can notify them as well
[13:26] <danilos> adiroiban, no, it should be ready to code; I'll add a comment to the bug explaining my "findings"
[13:30] <bac> hi mrevell-lunch
[13:30] <adiroiban> danilos: thanks. do you also have time to give a quick feedback based on some screenshots ? https://code.edge.launchpad.net/~adiroiban/launchpad/bug-146178/+merge/23760 :)
[13:31] <bac> mrevell-lunch:  i agree with your email wrt line wrapping.  keep it as is.
[13:32] <bac> adiroiban:  thanks for your reply to my review yesterday.  everything you said makes perfect sense.  sorry for the silly suggestion about using a set.
[13:33] <danilos> adiroiban, added a comment to the MP: "Looking at the screenshots: any reason to still add 'Current' to the titles on distroseries:+translations page? And don't forget to decapitalize "Base" in "Current Base" on the +language-packs page (not to mention that it'd be nice to find better terminology for these, but I am letting you take care of that with the UI reviewer)"
[13:35] <danilos> adiroiban, and do NOT use tables there
[13:35] <danilos> :)
[13:35] <adiroiban> danilos: I will remove the "Current" and fix the capitalization and try to find better names
[13:36] <danilos> adiroiban, thanks
[13:36] <adiroiban> danilos: +language-packs has no table
[13:36] <danilos> adiroiban, you added one to distroseries-translations
[13:36] <adiroiban> but for the +translations page
[13:37] <danilos> adiroiban, right, so don't do that, it's not tabular data
[13:37] <adiroiban> I added one since this is what it was suggested durring an UI review for another bug
[13:37] <adiroiban> ok
[13:37] <danilos> adiroiban, hum, where?
[13:37] <bac> adiroiban:  if you have no further changes to your branch i'll be happy to land it now.
[13:37] <adiroiban> danilos: https://bugs.edge.launchpad.net/rosetta/+bug/367844 ... I added this none on the MP
[13:37] <mup> Bug #367844: Language pack page needs ui improvements. <ui> <Launchpad Translations:Triaged> <https://launchpad.net/bugs/367844>
[13:37] <adiroiban> note
[13:37] <danilos> adiroiban, (wondering what review was that so if needed I can complain and resolve it with the UI team :)
[13:38] <adiroiban> danilos: ah... my bad. looks like the bug is talking about a different data
[13:39] <adiroiban> .. or not ? not sure :)
[13:39] <danilos> adiroiban, that bug is obsolete, for the old version of the page :)
[13:39] <adiroiban> bac: the branch should be ready for testing
[13:39] <adiroiban> danilos: ok. I will revert the table
[13:40] <danilos> adiroiban, thanks
[13:41] <bac> adiroiban:  great.  by 'land' i meant 'ec2 land'...full test and then submit.
=== mrevell-lunch is now known as mrevell
[13:47] <mrevell> thanks bac
[14:01] <bac> adiroiban: could i get you to resolve the text conflict in your branch and repush?
[14:01] <adiroiban> bac: sure. I will do it now and add a commnent on the  MP when it is done
[14:02] <bac> adiroiban: please ping me here too
[14:09] <adiroiban> bac: done :)
[14:09] <bac> adiroiban: thank
[14:09] <bac> s
[14:24] <bac> adiroiban: submitted to ec2.  in a short 3.5 - 4 hours it'll be landed.  :)
[15:03] <jtv> allenap: want me to take that one off the queue?
[15:04] <allenap> jtv: Yes please :)
[15:04] <allenap> jtv: No, actually, adeuring took that one already.
=== allenap changed the topic of #launchpad-reviews to: On call: allenap || reviewing: - || queue: [] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.edge.launchpad.net/launchpad/+activereviews
[15:04] <jtv> ah
=== jelmer is now known as ctrlsoft
=== kfogel is now known as kfogelunch
=== EdwinGrubbs changed the topic of #launchpad-reviews to: On call: allenap, Edwin || reviewing: - || queue: [] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.edge.launchpad.net/launchpad/+activereviews
=== salgado is now known as salgado-lunch
=== allenap changed the topic of #launchpad-reviews to: On call: Edwin || reviewing: - || queue: [] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.edge.launchpad.net/launchpad/+activereviews
=== gary_poster is now known as gary-lunch
[17:47] <adiroiban> bac: hi. I received an email with the subject „Test Runner FAILED” but the traceback does not give any hints about what test went wrong
=== leonardr is now known as leonardr-lunch
[18:13] <bac> adiroiban: it was a failure in the ec2 script.
[18:14] <bac> adiroiban: the portion that sends email with the test results.  so, i'm afraid, that run was wasted
=== salgado-lunch is now known as salgado
=== kfogelunch is now known as kfogel
=== gary-lunch is now known as gary_poster
[18:56] <bac> jml: https://code.launchpad.net/~bac/launchpad/bug-567985/+merge/23868
[18:59] <jml> bac: reviewed.
=== leonardr-lunch is now known as leonardr
=== adiroiban changed the topic of #launchpad-reviews to: On call: Edwin || reviewing: - || queue: [adiroiban(bug-61081)] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.edge.launchpad.net/launchpad/+activereviews
[21:24] <bac> adiroiban: ec2 has been fixed and i'm resubmitting your branch
[21:25] <adiroiban> bac: :) thanks!
=== adiroiban changed the topic of #launchpad-reviews to: On call: Edwin || reviewing: - || queue: [adiroiban(UI bug-146178), adiroiban(bug-61081)] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.edge.launchpad.net/launchpad/+activereviews
=== salgado is now known as salgado-afk