[00:09] <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:10] <wgrant> cjwatson: We don't have some kind of test for the output?
[00:13] <cjwatson> Let me see if I can spot why the tests weren't spotting the "base-files/admin" garbage
[00:14] <wgrant> Yeah
[00:14] <wgrant> I would have expected an integration test of some kind to catch that.
[01:20] <cjwatson> wgrant: I've pushed an integration test now.  Thanks for the suggestion.
[01:47] <wgrant> cjwatson: That looks safer, thanks.
[01:47] <wgrant> r=me
[01:48] <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
[03:51] <wgrant> StevenK: There's still an awful lot of person queries on qas
[03:52] <StevenK> :-(
[03:52] <StevenK> Yes, I was trying to work it out last night, and didn't get very far
[03:52] <StevenK> And I've been distracted by IBranchMergeProposal being obstreperous today.
[04:19] <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:20] <StevenK> Why would person_logged_in uh, not?
[04:21] <StevenK> Without asserting or something
[04:23] <wgrant> You're not doing anything viewish?
[04:26]  * StevenK tries to figure why superseded_by now tosses a KeyError in zope.interface.
[04:28] <StevenK> Oh. Because I can't read.
[04:29] <StevenK> wgrant: So while I have tests running, whyfor does my preloading not love me on qas? :-(
[04:31] <StevenK> The preloading did love me locally
[04:33] <wgrant> StevenK: Have you examined the tracebacks?
[04:33] <wgrant> You may want to be in a padded room first
[04:36] <StevenK> I thought I did, have you generated an OOPS?
[04:37] <wgrant> https://oops.canonical.com/oops.py/?oopsid=OOPS-1c124234d939329af74ae127df8224c7
[05:04] <StevenK> wgrant: I don't get it. It loops over view/subscriptions which has the preloading?
[05:07] <wgrant> StevenK: I'm tnot seeing that..
[05:09] <StevenK> wgrant: lib/lp/code/templates/branch-portlet-subscribers-content.pt and lib/lp/code/browser/branchsubscription.py
[05:10] <wgrant> StevenK: How'd you get those from the OOPS?
[05:11] <StevenK> Guessing, since the TAL isn't exactly forthcoming
[05:11] <wgrant> Which query are you looking at?
[05:12] <StevenK> WHERE Person.id = 3807340 LIMIT 1 near the bottom
[05:12] <wgrant> I think you misread the traceback
[05:15] <StevenK> Oh? I can see TAL is implicated, and IBranch.hasSubscription
[05:15] <wgrant> Yes
[05:16] <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:17] <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:19] <StevenK> Ah
[05:22] <StevenK> That's terrible
[05:24] <StevenK> wgrant:  4 files changed, 126 insertions(+), 159 deletions(-)
[05:25] <StevenK> That moves as much as I could into lp.View for BMP
[05:25] <wgrant> What's left?
[05:28] <StevenK> id, registrant, source_branchID, prerequisite_branchID, next_preview_diff_job, private, source_branch, target_branch, prerequisite_branch, votes, getUsersVoteReference
[05:29] <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
[06:19] <StevenK> wgrant: http://pastebin.ubuntu.com/5569759/
[06:22] <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:23] <wgrant> In the case where it fails.
[06:24] <StevenK> wgrant: http://pastebin.ubuntu.com/5569770/
[06:27] <wgrant> StevenK: Look at that test
[06:27] <wgrant> Think about what it's trying to do :)
[06:28] <StevenK> wgrant: I'm concerned about viewing BranchMergeProposal:+index as anoymous
[06:28] <wgrant> StevenK: Why?
[06:29] <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:30] <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:31] <StevenK> Indeed
[06:40] <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:41] <StevenK> wgrant: I can move registrant, or I can push it up
[06:42] <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:43] <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:45] <StevenK> source_branch and friends seem to cause oddness if they're not public
[06:47] <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:48] <wgrant> Registrant isn't completely terrible to have exposed either, but there won't be much fallout => change
[06:49] <StevenK> Right, and if you can't see either branch, you can't see the MP anyway
[06:50] <wgrant> If you can get the BMP object somehow you might be able to
[06:51] <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:53] <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
[07:25] <StevenK> Haha
[07:25] <StevenK> Fallout is one test that wants with person_logged_in(mp.registrant):
[07:25] <wgrant> Handy
[07:43] <wgrant> StevenK: Looking
[07:44] <StevenK> I wasn't fussed, TBH
[07:44] <wgrant> StevenK: Why are generateIncrementalDiff/revision_end_date/getIncrementalDiffs still named explicitly and in Public?
[07:45] <StevenK> None of them exist in the interface
[07:46] <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:47] <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:49] <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:50] <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:51] <StevenK> wgrant: Haha, cheater.
[07:52] <wgrant> I try.
[07:53]  * 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.
[09:00] <czajkowski> aloha