| === wedgwood is now known as wedgwood_away | ||
| cjwatson | wgrant: Well, I flipped the order round in the test as well to match; not sure I can really do better automatically since they're all just passed through as strings | 00:09 |
|---|---|---|
| wgrant | cjwatson: We don't have some kind of test for the output? | 00:10 |
| === sagaci is now known as jpickett | ||
| cjwatson | Let me see if I can spot why the tests weren't spotting the "base-files/admin" garbage | 00:13 |
| wgrant | Yeah | 00:14 |
| wgrant | I would have expected an integration test of some kind to catch that. | 00:14 |
| === Ursinha is now known as Ursinha-afk | ||
| === Ursinha-afk is now known as Ursinha | ||
| === Ursinha is now known as Ursinha-afk | ||
| === Ursinha-afk is now known as Ursinha | ||
| === Ursinha is now known as Ursinha-afk | ||
| cjwatson | wgrant: I've pushed an integration test now. Thanks for the suggestion. | 01:20 |
| === Ursinha-afk is now known as Ursinha | ||
| wgrant | cjwatson: That looks safer, thanks. | 01:47 |
| wgrant | r=me | 01:47 |
| cjwatson | Thanks. I'll re-QA in the morning. | 01:48 |
| cjwatson | (Also ugh test_ftparchive.py is horrible.) | 01:48 |
| wgrant | Yes, yes it is | 01:48 |
| wgrant | StevenK: There's still an awful lot of person queries on qas | 03:51 |
| StevenK | :-( | 03:52 |
| StevenK | Yes, I was trying to work it out last night, and didn't get very far | 03:52 |
| === Ursinha is now known as Ursinha-afk | ||
| StevenK | And I've been distracted by IBranchMergeProposal being obstreperous today. | 03:52 |
| === Ursinha-afk is now known as Ursinha | ||
| StevenK | self.assertEqual('flibble', bmp.superseded_by.description) | 04:19 |
| StevenK | AttributeError: 'thread._local' object has no attribute 'interaction' | 04:19 |
| StevenK | And I'm using with person_logged_in() :-( | 04:19 |
| wgrant | It's apparently not logged in | 04:19 |
| StevenK | Why would person_logged_in uh, not? | 04:20 |
| StevenK | Without asserting or something | 04:21 |
| wgrant | You're not doing anything viewish? | 04:23 |
| * StevenK tries to figure why superseded_by now tosses a KeyError in zope.interface. | 04:26 | |
| StevenK | Oh. Because I can't read. | 04:28 |
| StevenK | wgrant: So while I have tests running, whyfor does my preloading not love me on qas? :-( | 04:29 |
| StevenK | The preloading did love me locally | 04:31 |
| wgrant | StevenK: Have you examined the tracebacks? | 04:33 |
| wgrant | You may want to be in a padded room first | 04:33 |
| StevenK | I thought I did, have you generated an OOPS? | 04:36 |
| wgrant | https://oops.canonical.com/oops.py/?oopsid=OOPS-1c124234d939329af74ae127df8224c7 | 04:37 |
| StevenK | wgrant: I don't get it. It loops over view/subscriptions which has the preloading? | 05:04 |
| wgrant | StevenK: I'm tnot seeing that.. | 05:07 |
| StevenK | wgrant: lib/lp/code/templates/branch-portlet-subscribers-content.pt and lib/lp/code/browser/branchsubscription.py | 05:09 |
| wgrant | StevenK: How'd you get those from the OOPS? | 05:10 |
| StevenK | Guessing, since the TAL isn't exactly forthcoming | 05:11 |
| wgrant | Which query are you looking at? | 05:11 |
| StevenK | WHERE Person.id = 3807340 LIMIT 1 near the bottom | 05:12 |
| wgrant | I think you misread the traceback | 05:12 |
| StevenK | Oh? I can see TAL is implicated, and IBranch.hasSubscription | 05:15 |
| wgrant | Yes | 05:15 |
| wgrant | There's no lp.code.browser.branchsubscription in there | 05:16 |
| wgrant | Nor view/subscriptions | 05:16 |
| StevenK | Clearly it was, since the query count did drop; but I'm having trouble seeing which bit of TAL is causing the query | 05:16 |
| wgrant | File "/srv/qastaging.launchpad.net/qastaging/launchpad-rev-16512/lib/lp/services/webapp/menu.py", line 493, in enable_if_allowed | 05:17 |
| wgrant | link = func(self) | 05:17 |
| wgrant | File "/srv/qastaging.launchpad.net/qastaging/launchpad-rev-16512/lib/lp/code/browser/branch.py", line 333, in subscription | 05:17 |
| wgrant | if self.context.hasSubscription(self.user): | 05:17 |
| wgrant | File "/srv/qastaging.launchpad.net/qastaging/launchpad-rev-16512/lib/lp/code/browser/decorations.py", line 97, in hasSubscription | 05:17 |
| wgrant | if sub.person == user: | 05:17 |
| StevenK | Ah | 05:19 |
| StevenK | That's terrible | 05:22 |
| StevenK | wgrant: 4 files changed, 126 insertions(+), 159 deletions(-) | 05:24 |
| StevenK | That moves as much as I could into lp.View for BMP | 05:25 |
| wgrant | What's left? | 05:25 |
| StevenK | id, registrant, source_branchID, prerequisite_branchID, next_preview_diff_job, private, source_branch, target_branch, prerequisite_branch, votes, getUsersVoteReference | 05:28 |
| wgrant | next_preview_diff_job, votes, getUsersVoteReference sound very excessive | 05:29 |
| wgrant | Which failures did they cause? | 05:29 |
| StevenK | Let me move them back to View | 05:29 |
| wgrant | Also, registrant is less bad but I can't see why it's necessary | 05:29 |
| === almaisan-away is now known as al-maisan | ||
| StevenK | wgrant: http://pastebin.ubuntu.com/5569759/ | 06:19 |
| wgrant | StevenK: That's not a hugely useful traceback :) | 06:22 |
| wgrant | StevenK: I could tell that code was calling it | 06:22 |
| wgrant | The question is what code, and why? | 06:22 |
| wgrant | In the case where it fails. | 06:23 |
| StevenK | wgrant: http://pastebin.ubuntu.com/5569770/ | 06:24 |
| wgrant | StevenK: Look at that test | 06:27 |
| wgrant | Think about what it's trying to do :) | 06:27 |
| StevenK | wgrant: I'm concerned about viewing BranchMergeProposal:+index as anoymous | 06:28 |
| wgrant | StevenK: Why? | 06:28 |
| wgrant | StevenK: If I can't see the BMP, I can't see BMP:+index | 06:29 |
| wgrant | So it's not an issue that not being able to see the BMP prevents me from seeing something that BMP:+index needs to see | 06:29 |
| StevenK | Oh, hah | 06:29 |
| StevenK | It's a silly test | 06:29 |
| wgrant | Yes | 06:29 |
| wgrant | The test attempts to assert that a user who can't see the merge proposal can't see reviews requested from private teams | 06:30 |
| wgrant | So it's asserting a security hole | 06:30 |
| wgrant | It shouldn't be possible to tell whether a team is a reviewer for a proposal that you can't see. | 06:30 |
| StevenK | Indeed | 06:31 |
| === Ursinha is now known as Ursinha-afk | ||
| StevenK | wgrant: Right, that's all tests passing with next_preview_diff_job, votes and getUsersVoteReference back in View | 06:40 |
| wgrant | StevenK: Lovely. What was the issue with next_preview_diff_job? | 06:40 |
| StevenK | A doctest using it directly | 06:40 |
| wgrant | Ah | 06:40 |
| StevenK | So I sprayed it with rSP | 06:40 |
| wgrant | ez | 06:40 |
| StevenK | wgrant: I can move registrant, or I can push it up | 06:41 |
| wgrant | StevenK: I don't see why registrant would be required (it's not relevant for security), so please move it to launchpad.View and fix the minimal fallout | 06:42 |
| wgrant | In general, security adapters should be using rSP so basically nothing but id is public, but the pattern today seems to be to keep security-relevant bits public | 06:43 |
| StevenK | source_branch and friends seem to cause oddness if they're not public | 06:45 |
| wgrant | Sure, lots of things will want those | 06:47 |
| wgrant | They're not completely terrible to have exposed, and there's going to be lots of fallout => probably not worth changing | 06:47 |
| wgrant | Registrant isn't completely terrible to have exposed either, but there won't be much fallout => change | 06:48 |
| StevenK | Right, and if you can't see either branch, you can't see the MP anyway | 06:49 |
| wgrant | If you can get the BMP object somehow you might be able to | 06:50 |
| wgrant | That's why we're moving these to launchpad.View in the first place | 06:51 |
| StevenK | I was considering that, if you can get the BMP object somehow won't the branch security adapters come into play anyway? | 06:51 |
| wgrant | You won't be able to see details about the branches | 06:53 |
| wgrant | But that doesn't mean the attributes should be public | 06:53 |
| wgrant | It just means it's not worth the fallout to make the View afterwards | 06:53 |
| StevenK | Mmmm | 06:53 |
| StevenK | Haha | 07:25 |
| StevenK | Fallout is one test that wants with person_logged_in(mp.registrant): | 07:25 |
| wgrant | Handy | 07:25 |
| wgrant | StevenK: Looking | 07:43 |
| StevenK | I wasn't fussed, TBH | 07:44 |
| wgrant | StevenK: Why are generateIncrementalDiff/revision_end_date/getIncrementalDiffs still named explicitly and in Public? | 07:44 |
| StevenK | None of them exist in the interface | 07:45 |
| wgrant | StevenK: Still, no reason for them to be public? | 07:46 |
| wgrant | Also, that AnyPerson wants to be AnyAllowedPerson | 07:46 |
| wgrant | And you'll want to replace the existing duplicatastic AnyAllowedPerson adapters with a single generic one | 07:46 |
| wgrant | Where checkAuthenticated delegates to View, and checkUnauthenticated returns False | 07:47 |
| StevenK | I think you should put all this on the MP :-) | 07:47 |
| wgrant | Shh | 07:47 |
| StevenK | wgrant: I thought AnyPerson was okay because we wanted randoms to be able to comment on MPs? | 07:49 |
| wgrant | StevenK: Not randoms who can't see the MP in the first place | 07:49 |
| wgrant | AnyAllowedPerson is usually View+authenticated | 07:49 |
| StevenK | Ah | 07:49 |
| StevenK | Right | 07:49 |
| wgrant | Whereas AnyPerson is just authenticated | 07:49 |
| StevenK | So it used to be make sense in the public world, but does not in the post-public world | 07:49 |
| wgrant | Right | 07:49 |
| wgrant | AnyPerson doesn't usually make sense for objects that have privacy | 07:50 |
| wgrant | (but I'm pretty sure bugs still use it) | 07:50 |
| StevenK | wgrant: Haha, cheater. | 07:51 |
| wgrant | I try. | 07:52 |
| * StevenK goes to shoot things in the face in BioShock 2 | 07:53 | |
| StevenK | Oh, sorry. Set them on fire, and then shoot them in the face. | 07:53 |
| czajkowski | aloha | 09:00 |
| === al-maisan is now known as almaisan-away | ||
| === Ursinha-afk is now known as Ursinha | ||
| === almaisan-away is now known as al-maisan | ||
| === wedgwood_away is now known as wedgwood | ||
| === deryck is now known as deryck[lunch] | ||
| === bac_ is now known as bac | ||
| === deryck[lunch] is now known as deryck | ||
| === matsubara is now known as matsubara-afk | ||
| === al-maisan is now known as almaisan-away | ||
Generated by irclog2html.py 2.7 by Marius Gedminas - find it at mg.pov.lt!