| === Ursinha-afk is now known as Ursinha | ||
| === Ursinha is now known as Ursinha-afk | ||
| gary_poster | bac benji gmb mumble/kanban now-ish | 13:30 |
|---|---|---|
| gmb | Right. | 13:30 |
| bac | ok | 13:30 |
| benji | I'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_poster | reviewers sometime request it. We do generate docs with it | 14:08 |
| gary_poster | sphinx and something else too I think | 14:08 |
| gary_poster | I would say "encouraged but not required" at least from my perspective | 14:09 |
| benji | for now I'll do it for APIs exported in a YUI namespace and leave the rest for later if we want | 14:12 |
| gary_poster | cool | 14:14 |
| bac | gmb: new rev pushed to bzr+ssh://bazaar.launchpad.net/~yellow/launchpad/accordionoverlay/ | 14:18 |
| gmb | bac, 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 |
| gmb | bac: Okay, I'll grab that shortly. | 14:19 |
| gary_poster | thank you bac | 14:19 |
| gary_poster | oh | 14:19 |
| gary_poster | I misread that | 14:19 |
| gary_poster | :-P | 14:19 |
| gary_poster | I'll review it gmb | 14:19 |
| benji | gmb: I can review it | 14:19 |
| benji | or gary | 14:19 |
| gary_poster | :P | 14:19 |
| gary_poster | benji wins :-) | 14:20 |
| benji | heh | 14:20 |
| gmb | :) | 14:20 |
| gmb | gary_poster: Thanks. | 14:20 |
| gary_poster | np. not sure where that left us, benji, so I claimed it to be clear | 14:21 |
| gary_poster | (if you really want it, please ask; I'm taking it to not block anyone else) | 14:21 |
| benji | have at it | 14:21 |
| gary_poster | cool | 14:21 |
| gary_poster | gmb, 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 |
| gmb | gary_poster: The latter. I'm writing a list and will file one or more bugs as necessary. | 14:27 |
| gary_poster | cool, thanks | 14:27 |
| gmb | gary_poster: It's a case of clicking the link and working out what wasn't supposed to happen, mostly :) | 14:27 |
| gary_poster | heh | 14:27 |
| gary_poster | the 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_poster | do as you will | 14:28 |
| * gmb looks | 14:32 | |
| gmb | gary_poster: I'll do the second Thanks. | 14:32 |
| gary_poster | cool | 14:33 |
| gary_poster | gmb, (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 |
| gmb | gary_poster: Agreed. I'll do that. | 14:42 |
| gary_poster | cool | 14:42 |
| gary_poster | gmb, 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_poster | Whatever 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 |
| gmb | gary_poster: Right, I see what you mean. I'll add it and comment it out. | 14:47 |
| gary_poster | cool | 14:47 |
| gary_poster | gmb, 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_poster | There'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_poster | All 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_poster | Anyway, approving. :-) | 14:58 |
| gmb | gary_poster: These are all things I aim to fix :) | 14:58 |
| gmb | But I'd rather land this and then start a fresh branch. | 14:58 |
| gary_poster | how will you fix that last one? | 14:58 |
| gary_poster | it seems inherent to the model we have | 14:58 |
| gmb | gary_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_poster | right, that's what I was thinking | 14:59 |
| gary_poster | ok | 14:59 |
| gary_poster | So, I'll copy over some edited version of all this and approve. Thank you | 14:59 |
| gmb | np. | 14:59 |
| gary_poster | approved, gmb | 15:05 |
| gmb | Thanks. | 15:06 |
| * gmb -> afk to run an errand; back in 30. | 15:10 | |
| bac | gmb, 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 | ||
| gmb | bac: I'm back. What oddities are you seeing? | 15:42 |
| bac | gmb: 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 function | 15:45 |
| gmb | bac: Er. Hmm. Can you push your changes to the accordionoverlay branch so I can pull them and take a look locally. | 15:46 |
| bac | gmb: yeah, will in a sec. thanks. | 15:46 |
| bac | gmb: done | 15:49 |
| gmb | bac: Ok, pulling now. | 15:49 |
| bac | gmb: i also have questions about the use of window.LP vs. LP. that can wait | 15:52 |
| gmb | Ok. | 15:52 |
| gmb | bac: Taking a look at the first problem; bear with me whilst I downshift my brain. | 15:52 |
| gmb | bac: So, if I'm reading this right, the current test run goes something like this: | 15:59 |
| gmb | Start test case 1 | 15:59 |
| gmb | Do stuff | 15:59 |
| gmb | Finish | 15:59 |
| gmb | Start test case 1 | 15:59 |
| gmb | s/1/2/, sorry | 15:59 |
| gmb | Call the patch method from test case 1 | 16:00 |
| gmb | do more stuff | 16:00 |
| gmb | Call the patch method from test case 2 | 16:00 |
| gmb | Finish. | 16:00 |
| gmb | bac: Right? | 16:00 |
| bac | yes | 16:00 |
| bac | and, if you duplicate the first test from TC 2, tests after the first one DTRT | 16:00 |
| * gmb prods a bit more | 16: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 |
| gmb | bac: So, I think your problem is that lp_client is a global variable in the structural-subscription.js. | 16:15 |
| gmb | bac: I've run up this patch: http://pastebin.ubuntu.com/577899/ | 16:15 |
| gmb | which 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 |
| gmb | bac: In fact, wait a moment, because I've broken things horribly there without meaning to. | 16:19 |
| gmb | bac: 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 |
| bac | gmb: ok. so it is a mistake to do the set up inside the DOM ready handler? | 16:43 |
| bac | it seems to not have any dependencies on the DOM | 16:43 |
| gmb | bac: 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 |
| bac | ok | 16:43 |
| gmb | Alternatively, the test s should only run after domready. | 16:44 |
| gmb | It was that unpleasant an idea, he ran away. | 16:44 |
| gary_poster | :-) | 16:47 |
| gary_poster | benji, 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_poster | team lead call I mean | 16:48 |
| benji | gary_poster: k | 16:48 |
| bac | gmb: sorry for disappearing...my box rebooted | 16:50 |
| gmb | np | 16:50 |
| gmb | Last think I did was agree with you and say: | 16:50 |
| gmb | "Alternatively, the test s should only run after domready" | 16:51 |
| bac | gmb: removing your changes and then moving the setup to outside the dom ready works | 16:53 |
| gmb | bac: Cool. Simpler is better :) | 16:53 |
| bac | so in yui unit tests, anything inside a domready handler just get executed as soon as possible? | 16:54 |
| bac | so to do that properly you do a dom ready wait inside the test? | 16:54 |
| gary_poster | benji, you getting on the call? | 17:00 |
| benji | gary_poster: yep | 17:01 |
| * benji lunchinates | 17:24 | |
| * gary_poster lunches | 17:45 | |
| gmb | bac: Sorry, didn't see your last couple of messages. | 17:53 |
| gmb | bac: I guess the right thing to do would be to have the Test.Runner.run() method called on domready and not before. | 17:54 |
| gmb | bac: Oh. | 17:54 |
| gmb | Which it is... | 17:54 |
| bac | er? | 17:54 |
| gmb | That's bizarre. | 17:54 |
| bac | tres bizarre | 17:55 |
| gmb | Hang on, let me try something... | 17:55 |
| gmb | bac: 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 |
| bac | weird | 17:59 |
| bac | well, for this case, rearranging the code is not a big deal | 17:59 |
| gmb | Right. | 18:01 |
| gmb | bac: 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 |
| gmb | I'll ping you if I find anything. | 18:02 |
| bac | ok | 18:02 |
| benji | gary_poster: what's the status of the new every-subscription-has-at-least-one-filter rule? | 19:44 |
| gary_poster | benji, several parts to it. | 19:45 |
| benji | ok, that would explain the behavior I'm seeing | 19:45 |
| gary_poster | 1) every newly created subscription has a filter | 19:45 |
| gary_poster | 2) staging follows the rule (or it did a short while ago) | 19:46 |
| gary_poster | 3) prod and qastaging do not | 19:46 |
| benji | cool | 19:46 |
| gary_poster | 4) I don't *think* sample data does | 19:46 |
| gary_poster | prod 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 sampledata | 19: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 |
| gary_poster | (done) | 19:47 |
| bac | gary_poster: ping | 22:00 |
| gary_poster | 'hey bac | 22:00 |
| bac | hi, just testing my connectivity. connection to canonical IRC and IMAP just disappeared and IRC has me marked as dead | 22:01 |
| bac | not sure what is going on | 22:01 |
| gary_poster | you're not dead yet | 22:01 |
| bac | but freenode appears to work. sorry to bother you | 22:01 |
| bac | :) | 22:01 |
| gary_poster | :-) | 22:01 |
| * bac loves some spamalot | 22:01 | |
| bac | gary_poster: are you connected to irc.canonical.com ok? i'm getting errors about it not providing an SSL cert | 22:04 |
| gary_poster | yes I am bac | 22:04 |
| gary_poster | I am not getting the errors | 22:04 |
| gary_poster | using linkinus | 22:05 |
| bac | ok. odd. i'll not worry about it until tomorrow | 22:05 |
| gary_poster | cool | 22:05 |
| bac | i'm using linkinus through a bip proxy | 22:05 |
| * gary_poster needs to look up bip proxy | 22:05 | |
| gary_poster | time to reboot! | 22:17 |
Generated by irclog2html.py 2.7 by Marius Gedminas - find it at mg.pov.lt!