[03:20] <StevenK> wgrant: Did you see OOPS-fe4a82b31c1ecb7cca1a1da04b3aaef1?
[03:22] <wgrant> Yay
[03:23] <StevenK> I have a branch that adds a test, but I can't my test to break in the same way.
[03:23] <wgrant> StevenK: Note that it's deleting a series that contains a milestone that is referenced by a work item
[03:24] <StevenK> Right, and the code to delete a series will delete all milestones first
[03:24] <wgrant> Yes
[03:28] <StevenK> My test is triggering: AssertionError: You cannot delete a milestone which has specifications targeted to it.
[03:28] <wgrant> StevenK: It needs to be a workitem, not a spec.
[03:28] <StevenK> Which requires a spec
[03:28] <wgrant> Sure
[03:28] <wgrant> The spec doesn't have to be targeted.
[03:28] <StevenK> IntegrityError: update or delete on table "milestone" violates foreign key constraint "specificationworkitem_milestone_fkey" on table "specificationworkitem"
[03:28] <StevenK> DETAIL:  Key (id)=(13) is still referenced from table "specificationworkitem".
[03:29] <StevenK> wgrant: So, the delete code does not look for specificationworkitem where deleted = True
[03:29] <wgrant> Ah
[03:48] <StevenK>         mapper = lambda row: row[0]
[03:48] <StevenK>         return DecoratedResultSet(ordered_results, mapper)
[03:49] <wgrant> itemgetter
[03:49] <StevenK> (ordered_results, itemgetter(0)) ?
[03:49] <wgrant> Yes
[04:38] <StevenK> wgrant: https://code.launchpad.net/~stevenk/launchpad/workitems-delete-series/+merge/156457
[04:41] <wgrant> StevenK: Does the delete method need an assertion for specificationworkitems as it has for specifications?
[04:41] <wgrant> Also, you shouldn't be tested that at the series level.
[04:41] <wgrant> testing
[04:48] <StevenK> wgrant: You can't remove workitems, they just set to deleted=True
[04:49] <StevenK> wgrant: I can shift the test to the milestone level if you wish, but I suspect the test will fail because of the specification.
[04:50] <StevenK> Deleting a milestone performs checks against it, but I suspect the same checks aren't done against the series' milestones when you remove a series
[04:53] <wgrant> StevenK: But the spec shouldn't be targeted, should it?
[04:53] <wgrant> And I know you can't remove them -- that's why the browser code unsets them
[04:56] <StevenK> wgrant: It didn't seem to touch workitems
[04:57] <StevenK> http://pastebin.ubuntu.com/5669457/ fails with MismatchError: None is not <Milestone at 0xf1377d0>
[04:59] <wgrant> StevenK: With what error?
[05:04] <StevenK>     self.assertIs(None, workitem.milestone)
[05:04] <StevenK> MismatchError: None is not <Milestone at 0xf1377d0>
[05:05] <wgrant> StevenK: But what was the form error?
[05:05] <wgrant> There must have been one, if it was not deleted.
[05:05] <wgrant> Or your form post was invalid.
[05:06] <StevenK> wgrant: The assert above that one is self.assertEqual([], view.errors)
[05:06] <wgrant> StevenK: Sure, so perhaps you didn't specify a valid form action
[05:11] <StevenK> wgrant: I doubt it, as lp.registry.browser._deleteMilestone() fires
[05:22] <wgrant> StevenK: Oh, I guess they're not actually proper errors, or there is a confirmation step
[05:22] <wgrant> Check the body that you get back.
[05:41] <StevenK> wgrant: From view.request.response?
[05:43] <wgrant> StevenK: view()
[05:45] <StevenK> wgrant: http://pastebin.ubuntu.com/5669521/
[05:45] <wgrant> Oh
[05:45] <wgrant> That wasn't using c_i_v?
[05:45] <StevenK> It is using c_i_v()
[05:46] <wgrant>     view = create_view(
[05:46] <wgrant>         context, name, form, layer, server_url, method, principal,
[05:46] <wgrant>         query_string, cookie, request, path_info, rootsite=rootsite,
[05:46] <wgrant>         current_request=current_request, **kwargs)
[05:46] <wgrant>     view.initialize()
[05:46] <wgrant>     return view
[05:46] <wgrant> c_i_v, as the name suggests, initializes the view
[05:46] <StevenK> view() == view.render(), no?
[05:46] <wgrant> Sort of
[05:47] <wgrant> render() isn't a thing
[05:47] <wgrant> I think Launchpad views use it internally
[05:47] <wgrant> But I'm not sure Zope has it as a thing at all
[05:48] <StevenK> But yes, I see that it calls self.initialize(), which is odd
[05:48] <StevenK> Anyway, do you believe me now? :-)
[05:49] <wgrant> StevenK: Did you perchance do view() outside the person_logged_in block?
[05:49] <wgrant> Perhaps the form does other things when the owner is not logged in
[05:50] <StevenK> wgrant: I do, yes
[05:51] <StevenK> wgrant: Same traceback if I move the print into the with block
[05:51] <wgrant> StevenK: Add some prints to initialize() or pdb it
[05:51] <wgrant> And see why c_i_v doesn't trigger the exception
[05:52] <wgrant> Or read initialize() and work out wtf is so special about it
[05:52]  * wgrant finds the relevant form
[05:53] <wgrant> It appears to not override it
[05:53] <wgrant> StevenK: What if you manually call view.render() rather than view()?
[05:53] <wgrant> That'll let us see the content of the first submission without causing the exception
[05:53] <StevenK> wgrant: That thing that isn't a thing?
[05:53] <StevenK> LocationError: 'link-display-name-id'
[05:54] <StevenK> Let me reflow so it doesn't have to traverse milestone
[05:54] <wgrant> StevenK: It's a thing for LaunchpadView derivatives, since LaunchpadView.__call__ calls render()
[05:54] <wgrant> But in general it's not
[05:56] <StevenK> wgrant: http://pastebin.ubuntu.com/5669535/
[05:57] <wgrant> Ah, of course
[05:57] <wgrant> StevenK: store.flush()
[05:58] <StevenK> wgrant: Right, the flush now shows the error
[05:59] <wgrant> Yeah
[05:59] <wgrant> Forms often leave things unflushed
[05:59] <wgrant> Often exposing bug #812176
[05:59] <_mup_> Bug #812176: Exceptions during commits are not handled by the publisher <critical-analysis> <oops> <regression> <webapp-infrastructure> <Launchpad itself:Triaged> < https://launchpad.net/bugs/812176 >
[06:00] <StevenK> wgrant: So, this still leave us with the original issue, that _deleteMilestone didn't do anything with workitems
[06:00] <StevenK> Which you objected to
[06:00] <wgrant> StevenK: I'm confused
[06:01] <wgrant> _deleteMilestone clearly needs to do for workitems what it does for specs.
[06:02] <wgrant> StevenK: It may also be wise to consider how milestone deletion handles spec privacy
[06:02] <wgrant> It correctly bypasses it for bugs
[06:02] <StevenK> Badly
[06:02] <StevenK> And yeah, I've noticed that
[06:03] <wgrant> I don't remember what it does for specs
[06:03] <wgrant> And the MP diff is in another window
[06:03] <StevenK>             user = getUtility(ILaunchBag).user
[06:03] <StevenK>             return list(target.getSpecifications(user))
[06:03] <wgrant> Ah
[06:03] <wgrant> So broken.
[06:03] <StevenK> Yeah
[06:03] <wgrant> And slow
[06:04] <StevenK> wgrant: Would you like another test for that case?
[06:04] <wgrant> StevenK: Better that than a 403
[06:04] <wgrant> Actually, it'd just plain crash, wouldn't it
[06:04] <wgrant> With a 502, probably
[06:04] <StevenK> Yes, it will OOPS
[06:12] <StevenK> wgrant: Hmmm, my test passes, I suspect because the owner of the product has access to the spec
[06:13] <wgrant> StevenK: Indeed, you'll probably need to revoke the APG or similar.
[06:14] <StevenK> wgrant: I do wonder if I can be evil and just .set(access_policy=None, access_grants=None)
[06:16] <wgrant> StevenK: That's evil and depends on the implementation of the underlying queries. I wouldn't.
[06:18]  * StevenK fixes the passing test to revoke the correct AP
[06:18] <StevenK> DETAIL:  Key (product, id)=(27, 13) is still referenced from table "specification".
[06:18] <StevenK> And that's an OOPS
[06:36] <StevenK> wgrant: http://pastebin.ubuntu.com/5669598/
[06:38] <wgrant> StevenK: I don't understand why _get_specifications was like that before
[06:38] <wgrant> _getSpecifications
[06:38] <StevenK> Because Milestone doesn't use IHasSpecifications, and only provided getSpecifications()
[06:38] <wgrant> Ah
[06:39] <wgrant> So, having private members in an interface seems like a seriously strange idea
[06:39] <wgrant> Does getSpecifications not provide a give-me-everything mode?
[06:39] <StevenK> Nope
[06:40] <StevenK> Neither does search_specifications(), actually
[06:41] <StevenK> -        assert self.getSpecifications(user).count() == 0, (
[06:41] <StevenK> +        assert self._all_specifications.count() == 0, (
[06:41] <StevenK> wgrant: ^ That's in IMilestone.destroySelf(), changed after that paste.
[06:46] <wgrant> StevenK: :)
[06:46] <wgrant> (pointless use of count, but it'll be an OOPS either way)
[06:46] <StevenK> Oh? It's a resultset
[06:47] <wgrant> .any()
[06:47] <wgrant> .count() is expensive
[06:48] <StevenK> wgrant: http://pastebin.ubuntu.com/5669619/
[06:49] <wgrant> StevenK: Is _all_specifications really on the other interface?
[06:50] <wgrant> Also, those assertions are inverted
[06:50] <wgrant> And _getSpecifications is pointless
[06:51] <StevenK> Oh, hah
[06:51] <StevenK>     def _all_specifications(self):
[06:51] <StevenK>         """See IHasSpecifications."""
[06:51] <StevenK>         user = getUtility(ILaunchBag).user
[06:51] <StevenK>         return self.specifications(user, filter=[SpecificationFilter.ALL])
[06:51] <wgrant> Security proxies will also probably ruin your day
[06:54] <StevenK> _getSpecifications is called by children
[06:55] <wgrant> Sure
[06:55] <wgrant> But look what it does.
[07:00] <StevenK> _getSpecifications() ripped out, and I think I've defeated the security proxy
[07:00] <StevenK> wgrant: http://pastebin.ubuntu.com/5669642/
[07:03] <wgrant> +        return list(self.context._all_specifications) + [
[07:03] <wgrant> +            list(milestone.getSpecifications(self.user))
[07:03] <wgrant> +                for milestone in self.milestones]
[07:03] <wgrant> Isn't that filtering by user?
[07:08] <StevenK> wgrant: Sure, since that's actually displayed on the page
[07:08] <wgrant> Oh, right
[07:09] <wgrant> StevenK: By .any() I actually mean .is_empty(), of course. And I'm still not happy with the _ method in configure.zcml.
[07:15] <StevenK> wgrant: http://pastebin.ubuntu.com/5669672/  What else can I do? Milestone hardcodes everything in the ZCML
[07:19] <wgrant> StevenK: Well, declaring a private attribute as accessible from the outside seems a tad iffy...
[07:31] <StevenK> wgrant: I wanted to avoid rSP
[07:33] <wgrant> StevenK: That might mean that it's not a private attribute.
[07:52] <StevenK> wgrant: http://pastebin.ubuntu.com/5669749/