=== Ursinha-afk is now known as Ursinha
=== Ursinha is now known as Ursinha-afk
gary_posterbac benji gmb mumble/kanban now-ish13:30
benjiI've not been doing the big function header comments with parameter names, types, descriptions, etc.; is that something we feel strongly about wanting?14:03
gary_posterreviewers sometime request it.  We do generate docs with it14:08
gary_postersphinx and something else too I think14:08
gary_posterI would say "encouraged but not required" at least from my perspective14:09
benjifor now I'll do it for APIs exported in a YUI namespace and leave the rest for later if we want14:12
bacgmb: new rev pushed to  bzr+ssh://bazaar.launchpad.net/~yellow/launchpad/accordionoverlay/14:18
gmbbac, benji, gary_poster: Could one of you scholarly gents review https://code.launchpad.net/~gmb/launchpad/add-mute-button-204980-2/+merge/52686 for me? There aren't any OCRs around yet.14:19
gmbbac: Okay, I'll grab that shortly.14:19
gary_posterthank you bac14:19
gary_posterI misread that14:19
gary_posterI'll review it gmb14:19
benjigmb: I can review it14:19
benjior gary14:19
gary_posterbenji wins :-)14:20
gmbgary_poster: Thanks.14:20
gary_posternp.  not sure where that left us, benji, so I claimed it to be clear14:21
gary_poster(if you really want it, please ask; I'm taking it to not block anyone else)14:21
benjihave at it14:21
gary_postergmb, is there a quick description of what will need to change to fix the broken behavior you describe in the MP ("if you're already subscribed it will actually unsubscribe you") or is a bunch of small niggly things that would be hard to summarize?14:26
gmbgary_poster: The latter. I'm writing a list and will file one or more bugs as necessary.14:27
gary_postercool, thanks14:27
gmbgary_poster: It's a case of clicking the link and working out what wasn't supposed to happen, mostly :)14:27
gary_posterthe smalest niggle ever, but I'd be tempted to indent line 228 of the diff given the change you made--that, or compress lines 227, 228, and 229 into one.14:28
gary_posterdo as you will14:28
* gmb looks14:32
gmbgary_poster: I'll do the second Thanks.14:32
gary_postergmb, (circa line 58 in diff) I think that if you added a clause to test_mute_subscription_link (or a separate test) that subscribed at METADATA or COMMENTS or LIFECYCLE and then rendered the link, it would still do the right thing.  Might as well add such a clause/test; if it passes, it should be super fast to do, and if it doesn't, won't that be good to know. :-)14:38
gmbgary_poster: Agreed. I'll do that.14:42
gary_postergmb, I won't ask for a change, but if you are planning on landing this as is I'd be inclined to have the stuff in the pastebin (http://pastebin.ubuntu.com/577852/) included, commented out or otherwise disabled, with a comment explaining that this is a work in progress.14:46
gary_posterWhatever the method (I can think of others), the overriding thought is that I'd like someone viewing your code in the tree without access to your MP to not have to wonder, "hm, should I delete this unused cruft?  I don't see any reason why not...."14:46
gmbgary_poster: Right, I see what you mean. I'll add it and comment it out.14:47
gary_postergmb, the fact that you are sometimes having to make a fake JS subscription object (it is fake when is_subscribed is True, and it only exists because of UI considerations, AIUI) reminds me of my general unease about combining the mute functionality with the direct subscriptions.14:57
gary_posterThere's nothing to be done about this now--I still think proceeeding with the existing plan is reasonable given everything else--but I had to let the thought out or let it build pressure inside. :-)14:57
gary_posterAll that said, it looks like one of the core pieces of broken logic that you described is in lines 108-114 of the diff (starting with "if (! is_subscribed) {" in setup_mute_link_handlers).  That's where I'd expect to see code handling the "I'm subscribed but I need to mute" situation.  An XXX is probably appropriate there, unless I'm reading it wrong.14:57
gary_poster(I also dislike the fact that you can have a direct subscription, click mute by accident, click unmute, and then have lost your direct subscription)14:58
gary_posterAnyway, approving. :-)14:58
gmbgary_poster: These are all things I aim to fix :)14:58
gmbBut I'd rather land this and then start a fresh branch.14:58
gary_posterhow will you fix that last one?14:58
gary_posterit seems inherent to the model we have14:58
gmbgary_poster: That one may be the straw that breaks the camel's back with regard to saying "You know what, we can't do this properly without a BugMute table."14:58
gmb(Or some other DB change).14:59
gary_posterright, that's what I was thinking14:59
gary_posterSo, I'll copy over some edited version of all this and approve.  Thank you14:59
gary_posterapproved, gmb15:05
* gmb -> afk to run an errand; back in 30.15:10
bacgmb, when you get back:  i'm seeing oddities with monkey patching named_post and patch.  ping me when you have a moment.15:15
=== Ursinha-afk is now known as Ursinha
gmbbac: I'm back. What oddities are you seeing?15:42
bacgmb: i have two test cases.  in both setUp routines i monkeypatch the .patch method.  but i see tests in the second test case calling the patched version from the first or saying .patch is not a function15:45
gmbbac: Er. Hmm. Can you push your changes to the accordionoverlay branch so I can pull them and take a look locally.15:46
bacgmb:  yeah, will in a sec.  thanks.15:46
bacgmb: done15:49
gmbbac: Ok, pulling now.15:49
bacgmb: i also have questions about the use of window.LP vs. LP.  that can wait15:52
gmbbac: Taking a look at the first problem; bear with me whilst I downshift my brain.15:52
gmbbac: So, if I'm reading this right, the current test run goes something like this:15:59
gmbStart test case 115:59
gmbDo stuff15:59
gmbStart test case 115:59
gmbs/1/2/, sorry15:59
gmbCall the patch method from test case 116:00
gmbdo more stuff16:00
gmbCall the patch method from test case 216:00
gmbbac: Right?16:00
bacand, if you duplicate the first test from TC 2, tests after the first one DTRT16:00
* gmb prods a bit more16:01
bac*and* if i remove the monkey patching from TC1 the first test from TC2 fails as the patch is not called.16:02
gmbbac: So, I think your problem is that lp_client is a global variable in the structural-subscription.js.16:15
gmbbac: I've run up this patch: http://pastebin.ubuntu.com/577899/16:15
gmbwhich makes it a namespace property instead.16:15
gmb(Of course, that then breaks tests which expect it to be undefined when in fact we've monkeypatched it already)16:16
gmb(And I'm not completely sure that I'm right)16:16
gmbbac: In fact, wait a moment, because I've broken things horribly there without meaning to.16:19
gmbbac: I've fixed the failures and pushed my changes. I think that my changes might actually not be necessary, though; I realised that we're doing the lp_client setup in a domready handler, but the testrunner doesn't wait for domready, so it gets out of sync. At least I think that might have been the core problem (but I had to do all these other changes to spot that).16:41
bacgmb: ok.  so it is a mistake to do the set up inside the DOM ready handler?16:43
bacit seems to not have any dependencies on the DOM16:43
gmbbac: I don't think it's necessarily a mistake, but it might be worth trying to do it outside and see what breaks.16:43
gmbAlternatively, the test s should only run after domready.16:44
gmbIt was that unpleasant an idea, he ran away.16:44
gary_posterbenji, I suspect you should be pinged for todays call.  If you are not in, say, 8 minutes, lemme know (or ask jml, whom I believe to be today's host)16:48
gary_posterteam lead call I mean16:48
benjigary_poster: k16:48
bacgmb: sorry for disappearing...my box rebooted16:50
gmbLast think I did was agree with you and say:16:50
gmb"Alternatively, the test s should only run after domready"16:51
bacgmb:  removing your changes and then moving the setup to outside the dom ready works16:53
gmbbac: Cool. Simpler is better :)16:53
bacso in yui unit tests, anything inside a domready handler just get executed as soon as possible?16:54
bacso to do that properly you do a dom ready wait inside the test?16:54
gary_posterbenji, you getting on the call?17:00
benjigary_poster: yep17:01
* benji lunchinates17:24
* gary_poster lunches17:45
gmbbac: Sorry, didn't see your last couple of messages.17:53
gmbbac: I guess the right thing to do would be to have the Test.Runner.run() method called on domready and not before.17:54
gmbbac: Oh.17:54
gmbWhich it is...17:54
gmbThat's bizarre.17:54
bactres bizarre17:55
gmbHang on, let me try something...17:55
gmbbac: Hmm. So it should all *just work*, even with the domready stuff in place. I'm not quite sure why it doesn't, to be honest.17:58
bacwell, for this case, rearranging the code is not a big deal17:59
gmbbac: I've got no further yet with the stuff-still-shows-up-when-it-oughtn't problem. I'm going to grab some food and then take another look before my brain loses context, otherwise I'll just have to start over tomorrow.18:02
gmbI'll ping you if I find anything.18:02
benjigary_poster: what's the status of the new every-subscription-has-at-least-one-filter rule?19:44
gary_posterbenji, several parts to it.19:45
benjiok, that would explain the behavior I'm seeing19:45
gary_poster1) every newly created subscription has a filter19:45
gary_poster2) staging follows the rule (or it did a short while ago)19:46
gary_poster3) prod and qastaging do not19:46
gary_poster4) I don't *think* sample data does19:46
gary_posterprod and qastaging need to have me verify that things are speedy enough with filters everywhere, which I should be able to do on staging.  Then we need to acton bug 728818 to run the SQL on prod and qastaging, and update sampledata19:47
_mup_Bug #728818: Production and qastaging should have a filter for every structural subscription <story-better-bug-notification> <Launchpad itself:Triaged by yellow> < https://launchpad.net/bugs/728818 >19:47
bacgary_poster: ping22:00
gary_poster'hey bac22:00
bachi, just testing my connectivity.  connection to canonical IRC and IMAP just disappeared and IRC has me marked as dead22:01
bacnot sure what is going on22:01
gary_posteryou're not dead yet22:01
bacbut freenode appears to work.  sorry to bother you22:01
* bac loves some spamalot22:01
bacgary_poster: are you connected to irc.canonical.com ok?  i'm getting errors about it not providing an SSL cert22:04
gary_posteryes I am bac22:04
gary_posterI am not getting the errors22:04
gary_posterusing linkinus22:05
bacok.  odd.  i'll not worry about it until tomorrow22:05
baci'm using linkinus through a bip proxy22:05
* gary_poster needs to look up bip proxy22:05
gary_postertime to reboot!22:17

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