=== wedgwoodz is now known as wedgwood_away
StevenKwgrant: Did you see OOPS-fe4a82b31c1ecb7cca1a1da04b3aaef1?03:20
StevenKI have a branch that adds a test, but I can't my test to break in the same way.03:23
wgrantStevenK: Note that it's deleting a series that contains a milestone that is referenced by a work item03:23
StevenKRight, and the code to delete a series will delete all milestones first03:24
StevenKMy test is triggering: AssertionError: You cannot delete a milestone which has specifications targeted to it.03:28
wgrantStevenK: It needs to be a workitem, not a spec.03:28
StevenKWhich requires a spec03:28
wgrantThe spec doesn't have to be targeted.03:28
StevenKIntegrityError: update or delete on table "milestone" violates foreign key constraint "specificationworkitem_milestone_fkey" on table "specificationworkitem"03:28
StevenKDETAIL:  Key (id)=(13) is still referenced from table "specificationworkitem".03:28
StevenKwgrant: So, the delete code does not look for specificationworkitem where deleted = True03:29
StevenK        mapper = lambda row: row[0]03:48
StevenK        return DecoratedResultSet(ordered_results, mapper)03:48
StevenK(ordered_results, itemgetter(0)) ?03:49
StevenKwgrant: https://code.launchpad.net/~stevenk/launchpad/workitems-delete-series/+merge/15645704:38
wgrantStevenK: Does the delete method need an assertion for specificationworkitems as it has for specifications?04:41
wgrantAlso, you shouldn't be tested that at the series level.04:41
StevenKwgrant: You can't remove workitems, they just set to deleted=True04:48
StevenKwgrant: 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
StevenKDeleting a milestone performs checks against it, but I suspect the same checks aren't done against the series' milestones when you remove a series04:50
wgrantStevenK: But the spec shouldn't be targeted, should it?04:53
wgrantAnd I know you can't remove them -- that's why the browser code unsets them04:53
StevenKwgrant: It didn't seem to touch workitems04:56
StevenKhttp://pastebin.ubuntu.com/5669457/ fails with MismatchError: None is not <Milestone at 0xf1377d0>04:57
wgrantStevenK: With what error?04:59
StevenK    self.assertIs(None, workitem.milestone)05:04
StevenKMismatchError: None is not <Milestone at 0xf1377d0>05:04
wgrantStevenK: But what was the form error?05:05
wgrantThere must have been one, if it was not deleted.05:05
wgrantOr your form post was invalid.05:05
StevenKwgrant: The assert above that one is self.assertEqual([], view.errors)05:06
wgrantStevenK: Sure, so perhaps you didn't specify a valid form action05:06
=== bigjools_ is now known as bigjools
StevenKwgrant: I doubt it, as lp.registry.browser._deleteMilestone() fires05:11
=== almaisan-away is now known as al-maisan
wgrantStevenK: Oh, I guess they're not actually proper errors, or there is a confirmation step05:22
wgrantCheck the body that you get back.05:22
StevenKwgrant: From view.request.response?05:41
wgrantStevenK: view()05:43
StevenKwgrant: http://pastebin.ubuntu.com/5669521/05:45
wgrantThat wasn't using c_i_v?05:45
StevenKIt 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 view05:46
wgrantc_i_v, as the name suggests, initializes the view05:46
StevenKview() == view.render(), no?05:46
wgrantSort of05:46
wgrantrender() isn't a thing05:47
wgrantI think Launchpad views use it internally05:47
wgrantBut I'm not sure Zope has it as a thing at all05:47
StevenKBut yes, I see that it calls self.initialize(), which is odd05:48
StevenKAnyway, do you believe me now? :-)05:48
wgrantStevenK: Did you perchance do view() outside the person_logged_in block?05:49
wgrantPerhaps the form does other things when the owner is not logged in05:49
StevenKwgrant: I do, yes05:50
StevenKwgrant: Same traceback if I move the print into the with block05:51
wgrantStevenK: Add some prints to initialize() or pdb it05:51
wgrantAnd see why c_i_v doesn't trigger the exception05:51
wgrantOr read initialize() and work out wtf is so special about it05:52
* wgrant finds the relevant form05:52
wgrantIt appears to not override it05:53
wgrantStevenK: What if you manually call view.render() rather than view()?05:53
wgrantThat'll let us see the content of the first submission without causing the exception05:53
StevenKwgrant: That thing that isn't a thing?05:53
StevenKLocationError: 'link-display-name-id'05:53
StevenKLet me reflow so it doesn't have to traverse milestone05:54
wgrantStevenK: It's a thing for LaunchpadView derivatives, since LaunchpadView.__call__ calls render()05:54
wgrantBut in general it's not05:54
StevenKwgrant: http://pastebin.ubuntu.com/5669535/05:56
wgrantAh, of course05:57
=== al-maisan is now known as almaisan-away
wgrantStevenK: store.flush()05:57
StevenKwgrant: Right, the flush now shows the error05:58
wgrantForms often leave things unflushed05:59
wgrantOften exposing bug #81217605: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
StevenKwgrant: So, this still leave us with the original issue, that _deleteMilestone didn't do anything with workitems06:00
StevenKWhich you objected to06:00
wgrantStevenK: I'm confused06:00
wgrant_deleteMilestone clearly needs to do for workitems what it does for specs.06:01
wgrantStevenK: It may also be wise to consider how milestone deletion handles spec privacy06:02
wgrantIt correctly bypasses it for bugs06:02
StevenKAnd yeah, I've noticed that06:02
wgrantI don't remember what it does for specs06:03
wgrantAnd the MP diff is in another window06:03
StevenK            user = getUtility(ILaunchBag).user06:03
StevenK            return list(target.getSpecifications(user))06:03
wgrantSo broken.06:03
wgrantAnd slow06:03
StevenKwgrant: Would you like another test for that case?06:04
wgrantStevenK: Better that than a 40306:04
wgrantActually, it'd just plain crash, wouldn't it06:04
wgrantWith a 502, probably06:04
StevenKYes, it will OOPS06:04
StevenKwgrant: Hmmm, my test passes, I suspect because the owner of the product has access to the spec06:12
wgrantStevenK: Indeed, you'll probably need to revoke the APG or similar.06:13
StevenKwgrant: I do wonder if I can be evil and just .set(access_policy=None, access_grants=None)06:14
wgrantStevenK: 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 AP06:18
StevenKDETAIL:  Key (product, id)=(27, 13) is still referenced from table "specification".06:18
StevenKAnd that's an OOPS06:18
StevenKwgrant: http://pastebin.ubuntu.com/5669598/06:36
wgrantStevenK: I don't understand why _get_specifications was like that before06:38
StevenKBecause Milestone doesn't use IHasSpecifications, and only provided getSpecifications()06:38
wgrantSo, having private members in an interface seems like a seriously strange idea06:39
wgrantDoes getSpecifications not provide a give-me-everything mode?06:39
StevenKNeither does search_specifications(), actually06:40
StevenK-        assert self.getSpecifications(user).count() == 0, (06:41
StevenK+        assert self._all_specifications.count() == 0, (06:41
StevenKwgrant: ^ That's in IMilestone.destroySelf(), changed after that paste.06:41
wgrantStevenK: :)06:46
wgrant(pointless use of count, but it'll be an OOPS either way)06:46
StevenKOh? It's a resultset06:46
wgrant.count() is expensive06:47
StevenKwgrant: http://pastebin.ubuntu.com/5669619/06:48
wgrantStevenK: Is _all_specifications really on the other interface?06:49
wgrantAlso, those assertions are inverted06:50
wgrantAnd _getSpecifications is pointless06:50
StevenKOh, hah06:51
StevenK    def _all_specifications(self):06:51
StevenK        """See IHasSpecifications."""06:51
StevenK        user = getUtility(ILaunchBag).user06:51
StevenK        return self.specifications(user, filter=[SpecificationFilter.ALL])06:51
wgrantSecurity proxies will also probably ruin your day06:51
StevenK_getSpecifications is called by children06:54
wgrantBut look what it does.06:55
StevenK_getSpecifications() ripped out, and I think I've defeated the security proxy07:00
StevenKwgrant: 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
wgrantIsn't that filtering by user?07:03
StevenKwgrant: Sure, since that's actually displayed on the page07:08
wgrantOh, right07:08
wgrantStevenK: By .any() I actually mean .is_empty(), of course. And I'm still not happy with the _ method in configure.zcml.07:09
StevenKwgrant: http://pastebin.ubuntu.com/5669672/  What else can I do? Milestone hardcodes everything in the ZCML07:15
wgrantStevenK: Well, declaring a private attribute as accessible from the outside seems a tad iffy...07:19
StevenKwgrant: I wanted to avoid rSP07:31
wgrantStevenK: That might mean that it's not a private attribute.07:33
StevenKwgrant: 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!