| === wedgwoodz is now known as wedgwood_away | ||
| StevenK | wgrant: Did you see OOPS-fe4a82b31c1ecb7cca1a1da04b3aaef1? | 03:20 |
|---|---|---|
| wgrant | Yay | 03:22 |
| 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:23 |
| StevenK | Right, and the code to delete a series will delete all milestones first | 03:24 |
| wgrant | Yes | 03:24 |
| 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:28 |
| StevenK | wgrant: So, the delete code does not look for specificationworkitem where deleted = True | 03:29 |
| wgrant | Ah | 03:29 |
| StevenK | mapper = lambda row: row[0] | 03:48 |
| StevenK | return DecoratedResultSet(ordered_results, mapper) | 03:48 |
| wgrant | itemgetter | 03:49 |
| StevenK | (ordered_results, itemgetter(0)) ? | 03:49 |
| wgrant | Yes | 03:49 |
| StevenK | wgrant: https://code.launchpad.net/~stevenk/launchpad/workitems-delete-series/+merge/156457 | 04:38 |
| 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:41 |
| StevenK | wgrant: You can't remove workitems, they just set to deleted=True | 04:48 |
| 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:49 |
| 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:50 |
| 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:53 |
| StevenK | wgrant: It didn't seem to touch workitems | 04:56 |
| StevenK | http://pastebin.ubuntu.com/5669457/ fails with MismatchError: None is not <Milestone at 0xf1377d0> | 04:57 |
| wgrant | StevenK: With what error? | 04:59 |
| StevenK | self.assertIs(None, workitem.milestone) | 05:04 |
| StevenK | MismatchError: None is not <Milestone at 0xf1377d0> | 05:04 |
| 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:05 |
| 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:06 |
| === bigjools_ is now known as bigjools | ||
| StevenK | wgrant: I doubt it, as lp.registry.browser._deleteMilestone() fires | 05:11 |
| === almaisan-away is now known as al-maisan | ||
| 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:22 |
| StevenK | wgrant: From view.request.response? | 05:41 |
| wgrant | StevenK: view() | 05:43 |
| 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:45 |
| 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:46 |
| 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:47 |
| StevenK | But yes, I see that it calls self.initialize(), which is odd | 05:48 |
| StevenK | Anyway, do you believe me now? :-) | 05:48 |
| 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:49 |
| StevenK | wgrant: I do, yes | 05:50 |
| 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:51 |
| wgrant | Or read initialize() and work out wtf is so special about it | 05:52 |
| * wgrant finds the relevant form | 05:52 | |
| 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:53 |
| 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:54 |
| StevenK | wgrant: http://pastebin.ubuntu.com/5669535/ | 05:56 |
| wgrant | Ah, of course | 05:57 |
| === al-maisan is now known as almaisan-away | ||
| wgrant | StevenK: store.flush() | 05:57 |
| StevenK | wgrant: Right, the flush now shows the error | 05:58 |
| 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 > | 05:59 |
| 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:00 |
| wgrant | _deleteMilestone clearly needs to do for workitems what it does for specs. | 06:01 |
| 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:02 |
| 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:03 |
| 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:04 |
| StevenK | wgrant: Hmmm, my test passes, I suspect because the owner of the product has access to the spec | 06:12 |
| wgrant | StevenK: Indeed, you'll probably need to revoke the APG or similar. | 06:13 |
| StevenK | wgrant: I do wonder if I can be evil and just .set(access_policy=None, access_grants=None) | 06:14 |
| wgrant | StevenK: That's evil and depends on the implementation of the underlying queries. I wouldn't. | 06:16 |
| * 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:18 |
| StevenK | wgrant: http://pastebin.ubuntu.com/5669598/ | 06:36 |
| 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:38 |
| 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:39 |
| StevenK | Neither does search_specifications(), actually | 06:40 |
| 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:41 |
| 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:46 |
| wgrant | .any() | 06:47 |
| wgrant | .count() is expensive | 06:47 |
| StevenK | wgrant: http://pastebin.ubuntu.com/5669619/ | 06:48 |
| wgrant | StevenK: Is _all_specifications really on the other interface? | 06:49 |
| wgrant | Also, those assertions are inverted | 06:50 |
| wgrant | And _getSpecifications is pointless | 06:50 |
| 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:51 |
| StevenK | _getSpecifications is called by children | 06:54 |
| wgrant | Sure | 06:55 |
| wgrant | But look what it does. | 06:55 |
| StevenK | _getSpecifications() ripped out, and I think I've defeated the security proxy | 07:00 |
| StevenK | wgrant: http://pastebin.ubuntu.com/5669642/ | 07:00 |
| 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:03 |
| StevenK | wgrant: Sure, since that's actually displayed on the page | 07:08 |
| wgrant | Oh, right | 07:08 |
| wgrant | StevenK: By .any() I actually mean .is_empty(), of course. And I'm still not happy with the _ method in configure.zcml. | 07:09 |
| StevenK | wgrant: http://pastebin.ubuntu.com/5669672/ What else can I do? Milestone hardcodes everything in the ZCML | 07:15 |
| wgrant | StevenK: Well, declaring a private attribute as accessible from the outside seems a tad iffy... | 07:19 |
| StevenK | wgrant: I wanted to avoid rSP | 07:31 |
| wgrant | StevenK: That might mean that it's not a private attribute. | 07:33 |
| StevenK | wgrant: http://pastebin.ubuntu.com/5669749/ | 07:52 |
| === wedgwood_away is now known as wedgwood | ||
| === wedgwood is now known as Guest6347 | ||
| === Ursinha_ is now known as Ursinha | ||
| === marco is now known as Guest28583 | ||
| === Guest28583 is now known as marcoceppi | ||
| === matsubara is now known as matsubara-lunch | ||
| === Ursinha is now known as Ursinha-afk | ||
| === Ursinha-afk is now known as Ursinha | ||
| === matsubara-lunch is now known as matsubara | ||
| === deryck is now known as deryck[lunch] | ||
| === deryck[lunch] is now known as deryck | ||
| === Ursinha_ is now known as Ursinha | ||
| === Guest6347 is now known as wedgwood_away | ||
Generated by irclog2html.py 2.7 by Marius Gedminas - find it at mg.pov.lt!