[00:00] flacoste: http://blog.mountaingoatsoftware.com/agile-succeeds-three-times-more-often-than-waterfall [00:03] wgrant: I should fix security.cfg in the code branch, or the db branch? [00:06] StevenK: You can do it in the DB branch, so might as well do it there. [00:06] In fact, you have to do it there. [00:06] Because of person foreign keys. [00:07] Hmm, so I have to drop the column from person too [00:07] That is going to be fun. [00:07] Hm? [00:07] Oh, it's a join, not a column [00:08] Yes. [00:08] StevenK: ping [00:08] rick_h: O hai [00:08] StevenK: hey, I started landing the test fixes [00:08] I saw. [00:08] I think. [00:08] StevenK: in my dev work I noticed that make jsbuild doesn't update the build dir files [00:09] So I saw combobuild [00:09] but that didn't update an existing file either [00:09] I kept having to make clean && make to get the build dir updated to work on a js file as I worked [00:09] which was a tad slow as you can imagine [00:09] I tried to see if the combobuild didn't work right because it wasn't listed in PHONY, but that wasn't it. [00:09] I didn't get to look at all of it before EOD [00:10] bin/combo-rootdir will exit with a message if build/js/lp exists [00:10] StevenK: right, it basically ran without cp'ing the files [00:10] StevenK: we need some way to work on a .js file and getting it rebuilt like make jsbuild [00:11] rick_h: Right, so now that icing depends on the YUI directory inside build/js, I can't just blow away the entire directory with impunity like I used to. [00:12] StevenK: right, I have on my todo to work on a script you can run using pyinotify to auto rebuild changed files [00:12] but have things keeping me from getting to it yet [00:12] plus, that's going to be a pain since we've got build exceptions :/ [00:13] rick_h: Right, so I've not come up with a good plan yet. [00:13] StevenK: ok, well that's part of it. Wanted to make sure I wasn't expecting something to work that wasn't [00:13] sounds like it's on the todo list then [00:13] which is ok, now that I know [00:14] I think I can delete everything but yui and then cope if yui is a symlink [00:14] yea, I was thinking of just making a temp make command that blew away /build/lp and rebuilt it [00:15] Meh, don't do that [00:15] Fix combo-rootdir to not be so naive :-) [00:15] it was a sledgehammer solution :) [00:15] rick_h: I'll look at fixing combo-rootdir today [00:15] yea, I'll look at it then. Thanks for filling me in [00:16] ok, well no big deal. No one's really using it yet [00:16] * StevenK is destroying code and tables [00:16] StevenK: so qas ok? Is it setup I can run combo loader on qas if I set the FF? [00:16] rick_h: You can't set the flag, and no, it's still waiting for that RT. [00:17] ok, cool [00:23] wgrant: I have some bad news w/respect heat [00:23] wgrant: we're missing some update cases. [00:24] wgrant: I suspect dupes, subscribers and affects-me-too are not updating the bug record heat [00:28] lifeless: Are too. [00:28] at least affects-me-too is. [00:28] Subscribers work too [00:28] And I can't test duping non-destructively. [00:29] wgrant: Easy review https://code.launchpad.net/~stevenk/launchpad/kill-entitlements/+register-merge [00:29] BAH [00:29] When did that stop working? [00:29] Oh, you're afflicted too? [00:29] wallyworld_: ^^ [00:29] We have a sane reproducer. [00:29] wgrant: https://code.launchpad.net/~stevenk/launchpad/kill-entitlements/+merge/92888 [00:30] wgrant: really? oh good. I was watching heat fail to change on prod when doin stuff the other day [00:30] wgrant: what's up? [00:30] wallyworld_: The MP bug [00:30] We have a reproducer who is not stupid :) [00:30] bug nr? [00:31] Bug #929422 [00:31] <_mup_> Bug #929422: Fails to refresh the URL when making a merge request < https://launchpad.net/bugs/929422 > [00:31] That's it [00:31] Apparently it replaces the page content inline? [00:31] That sounds extremely evil. [00:32] It should stop doing so. [00:33] if you can come up with a solution that can avoid replacing the page content, i'm all ears [00:33] wallyworld_: Redirect! [00:34] window.location = whatever [00:34] isn't that another server request though [00:34] It used to redirect and it was fine! [00:34] wallyworld_: Don't you have to make the request to get the content anyway? [00:34] It will just be a normal navigation request rather than an AJAX one. [00:35] hard to explain - an xhr request is made and it may error so we need to stay on the current page [00:35] Sure [00:35] so that if there is an error [00:35] if can be displayed [00:35] And when it succeeds it will give a 303, right? [00:35] atm it gives 200 [00:35] Or 201 or something like that. [00:35] wallyworld_: The URL can updated via JS [00:36] * wgrant reads the code. [00:36] right, so that's all i need to do [00:36] No [00:36] That's evil. [00:36] i just forgot to update the url via js [00:36] why? [00:36] We should avoid it if we can; it's not widely supported. [00:36] wgrant: Thank you for the +1 [00:36] is it part of the standards? [00:36] It's part of HTML5 [00:37] so only ie < 8 or something will have an issue? [00:41] wallyworld_: No, not even IE9 supports it. [00:41] sigh. i hate ie [00:41] It's a new API and what you're doing is reasonably evil. [00:41] fsvo [00:42] Replacing the page content entirely is unprecedentedly evil. [00:42] That is not arguable :) [00:42] i'll take your word for it i suppose [00:43] i mean, it's merely doing what the browser does when it gets stuff to render [00:43] document.write is evil :) [00:43] why? it's a public api, or? [00:44] There are lots of evil public APIs. [00:44] so the issue is that the lp form infrastructure returns the html resulting from doing a form submit, and the xhr call needs to do something with this data or else just throw it away and waste the rendering effort [00:44] I had assumed that you used an API call. [00:45] I didn't realise until a few minutes ago that you actually POSTed to +register-merge. [00:45] so, FWIW [00:45] I think avoiding roundtrips is good; but is this actually doing that ? [00:45] yes [00:46] in which case, the UI impact is a regression, but it is otherwise tolerable, at least in the absence of e.g. doing the form via *stache [00:47] i posted to +register-merge to reuse all the existing code and avoid writing a bunch of new stuff and keep easy backward compat with non js browsers [00:48] I heartily approve [00:48] I approve of eliminating roundtrips, but I disapprove of introducing yet another non-standard way of doing forms. [00:48] Particularly one that is fragile and uses widely discouraged APIs like document.write. [00:49] except i need to figure out a non evil way to handle the form submission via the xhr call without adding a rt and wasting rendering effort [00:49] maybe a server side redirect? [00:49] thats what LP normally does [00:49] Are you sure the xhr call isn't following it itself? [00:49] There's still likely to be a round-trip, but it's not too terrible for this sort of page. [00:49] it would cause the server side rendering to be duplicated [00:49] perhaps [00:49] The way we normally avoid duplication is by not POSTing to the view. [00:50] But using an API instead. [00:50] did you set out to eliminate roundtrips, or do something else? [00:50] That's how all the other AJAX forms work, TTBOMK. [00:51] lifeless: i set out to convert what was a bog standard html form into a page that allowed the server to check for issues and send back an error message for display without leaving the page [00:51] and still act like a form submission on success [00:51] and live within the lp form infrastructure [00:52] so, neither fish nor fowl :) [00:52] yeah, guess so [00:52] wanted to avoid too much new code / change etc [00:52] so, deconstructing this situation [00:52] perhaps should look at using an api [00:53] There is one? [00:53] the new thing here was doin it in the picker [00:53] the work was already 5 or 6 branches long [00:53] IBranch.createMergeProposal() [00:53] eg. AJAX milestone creation is the normal form mapped to the API [00:53] you could have, for instance, let the picker choose anyone, and show an error on submission [00:53] would not have been as nice [00:53] lifeless: the picker does a check, but the submission also needs to do a check [00:53] wallyworld_: I know [00:54] wallyworld_: the picker check is redundant [00:54] so i did both [00:54] lifeless: yes, but it gives a nicer user experience [00:54] indeed [00:54] which I applaud [00:54] catch the error early [00:54] in the workflow [00:56] so, i needed a way to hit submit button but not leave the page and refresh if there is an issue, hence i made the xhr call, but the lp form stuff gives back a 200 with the new html for the resulting page on success [00:56] hence my use of document.write to render - it's the same data over the wire after all [00:57] wallyworld_: I don't quite follow that last bit; why couldn't it just do what our other forms do - render with an error? [00:57] (I'm not sayin that is good or optimal; just trying to understand) [00:58] lifeless: sure. there is no single field in error - it's a confirmation popup [00:58] not an error [00:58] and the form error stuff is a page refresh [00:58] which i didn't want [00:58] the user sees the confirmation popup and hits yes and a normal submission then occures [00:59] or if no, the popup goes away and they keep editing the mp [00:59] I understand, but how is that different to a fresh page with a 'please click here to ok adding the user as a subscriber to ...' ? [00:59] yuk. [00:59] better , more web 2.0 experience [01:00] i didn't think doing a separate confirmation html page was a good thing to implement [01:02] so if no objections, let me see if a server side redirect works. might double render, not sure, but better than the current bug [01:05] StevenK: can you check with webdev toolkit and see if a 30x is returned from the appserver? [01:05] ... better? [01:05] I'll be putting up an MP shortly [01:05] wallyworld_: so, as a datapoint, *all* our forms today double the same form as the confirmation page for all data entry issues [01:05] wallyworld_: e.g. with red boxes around bad data etc [01:06] wallyworld_: fitting in with that may well have been much less effort, and certainly would have been more consistent [01:06] lifeless: but here, there is no bad data [01:06] there's no error [01:06] wallyworld_: model it as a default-hidden field and there is [01:07] that said, what we do today isn't all that nice [01:07] it is however js-free [01:07] i was trying to avoid propagating the non-nice stuff [01:08] and we are allowed to do js only stuff now, right? [01:08] that is still vague, sadly. [01:08] we need to pin down the exact non-js use cases. [01:08] there are, AFAIK, three: [01:08] well, too late, horse has well and truly bolted [01:08] - login [01:08] - apport bug filing [01:09] - google indexing (all stuff we want indexed must have a server side render) [01:09] but, TTBOMK we haven't got a solid ratification around that bein the complete set, yet. [01:09] lifeless: yes, it was extra work, but you need to do that to establish a new way forward to get away from the old, "bad", stuff, even if we decide we want to do it another way, we need to blaze a trail [01:10] see what works, what doesn't [01:10] sure [01:10] I haven't, I hope, been critical so far [01:10] oh, no not at all [01:10] just trying to ensure I have a clear understanding [01:10] i appreciate a robust discussion, especially since i broke stuff :-( [01:11] i have a thick skin :-) [01:11] i do like the new behaviour, so would like to make that work rather than reverting to a confirmation page [01:11] You have to, to be a Queenslander [01:11] :-P [01:11] Damn, beat me to it. [01:14] anyhow [01:14] so, here is my take on it [01:14] we probably need two forms of forms [01:14] the bug filing / login case [01:14] everything else [01:15] we -don't- need server side rendering of forms, google doesn't need to index forms [01:15] the current approach for populating and validating forms / form widgets / etc is API calls, not view calls. [01:16] I'd rather we push on that unless we think its flawed, than overload the behaviour of server side forms [01:16] which this seems to do to me [01:16] lifeless: validation is done via form submission [01:17] the lp form view converts the post params into objects etc and then calls validate [01:17] wallyworld_: why? I mean, I know that is what you did, but API calls that mutate/create do their own validation [01:18] i'm relying on the standard lp form submission code to get the form data and convert to model objects [01:18] using the schema [01:18] The API does that too. [01:19] yes. but i also then resuse the exisiting form validation callbacks [01:19] but using the standard submit [01:19] Aren't they on the API too? [01:19] s/but/by === nhandler_ is now known as nhandler [01:20] they are? how? one defines validate() on LaunchpadFormView [01:20] the parameters are typed [01:20] with schema validators attached to that (e.g. Choice()) [01:20] sure, i'm not talking about param conversion, but validation [01:20] That should all be in the model. [01:21] The view is just meant to present it as HTML. [01:21] the LaunchpadFormView subclass offers a validate() method [01:21] which people use [01:21] and then call field.setError [01:21] Sure, but that should only be for formatting model errors as HTML. [01:21] Since the model should be doing the validation, not the form. [01:21] s/form/view/ [01:22] should be but doesn't always [01:22] i'll have to recheck this particular case [01:22] to see what it does [01:22] wallyworld_: if the model does not do the validation, that is a bug [01:22] Exactly. [01:22] then we have lots of bugs i think [01:22] Because there is an API for this. [01:22] wallyworld_: We have had many OOPSes - criticals - due to exactly that. [01:23] this discussion has been good, I think I can suggest a path now [01:23] (the one above), will send to list [01:23] lifeless: wgrant: the RegisterMergeProposalView does do it's validation in the view - so bug :-( [01:25] there is no well defined pattern that is in common use across lp for this [01:25] well, I beg to differ [01:25] we have two patterns and a bunch of sometimes poorly migrated forms [01:26] the migration *should* have been to move all validation to API's [01:26] the code i look at often suggests otherwise :-) [01:26] so there is an expected layout [01:26] If there is an API, it *must* do its own validation. [01:26] agreed [01:27] If there is a view and an API, the view should catch appropriate errors from the model, rather than duplicating validation code [01:27] yep, but mostly don't [01:27] If there is a view and no API, its old unmigrated code. [01:27] wallyworld_: -> lots of bugs [01:27] yep [01:35] lifeless: would you consider it a bug if a factory method makeFooBar() caused a side effect of leaving behind an admin login such that a check_permission in a test erroneously passes? [01:35] I would. [01:35] I ran into a similar thing last week. [01:35] Except it was logging out. [01:35] * wallyworld_ stabs factory [01:36] I was tempted to wrap the factory in an interaction preserver. [01:36] Fix the factory to do with celebrity_logged_in(): [01:36] in this case, makeRegistryExpert() is evil [01:36] yes, will fix, just wanted to check [01:36] * wallyworld_ wonders how many tests will fail now [01:36] wallyworld_: You will probably get fallout [01:36] Ah, you guessed :-) [01:36] * wallyworld_ sighs [01:36] Welcome to Launchpad. [01:37] is it too late to run away? [01:38] StevenK: what's bad is that the method just above uses the correct pattern, so somehow that was not noticed [01:39] wallyworld_: yes; an undocumented sideeffect is almost certainly a bug [01:39] lifeless: Yes, it returns a 303. [01:40] StevenK: thanks [01:40] wallyworld_: ^ :) [01:40] wallyworld_: server side is doing the right thing [01:40] for the register mp form? [01:41] yes [01:41] POST => 303 ; GET => 200 [01:41] And then Firebug refreshes the Net panel, because it's evil. [01:42] ok, so if the xhr call gets back stuff with code=303 and content type = text/html and responseText = the page html, what do i do? [01:42] i could have sworn i got code = 200 [01:43] you do [01:43] the browser follows the 303 [01:43] ah right, i remember now [01:43] question your assumptions, always :) [01:43] yes, i did figure that out [01:43] wgrant: https://code.launchpad.net/~stevenk/launchpad/smarter-combo-rootdir/+merge/92897 [01:43] i had ust forgotten [01:43] but i now recall my dilema [01:44] the server side 303 is mapped to a 200 by the browser [01:44] and the repsonseText has the html to render [01:44] It looked like two seperate requests to me [01:44] not if i recall correctly [01:45] but i could be wrong [01:45] In Firebug there was a POST, which returned a 303 See Other, and then a GET which returned 200 OK [01:45] but that's all at the browser level [01:45] i don't think the xhr call sees that [01:46] the xhr call does a post and the browser does it's stuff and ends up giving a repsonse with code = 200 [01:46] StevenK: yes, thats right [01:47] so the xhr call is none the wiser that the redirect has occurred [01:48] so, even if i use an api, that's still an extra round trip [01:48] 1. the api call to do the submission, 2. the call to get the correct url on success [01:49] whereas now it's just the one form submission [01:49] wallyworld_: actually no, an API can be less roundtrips [01:49] it's only one now atm [01:49] wallyworld_: load the form template (moustache) on the initial page load; get the data back from the API (a single POST : reply) and render in client [01:50] wallyworld_: its 2 POST:303 + final url:200 [01:50] it will be a huge job to convert the mp page(s) to mustache [01:51] i was trying to keep it all simple [01:52] ah yes, it is 2. [01:52] i'll have a look after i get this current branch done [01:52] now that i have figured out how factory is screwing me over [01:53] sure, like I said though, we need a consistent story [01:54] what you're doing isn't consistent, nor is it any faster than the API can/should be in principle [modulo headdesk bugs, of course] [01:54] it might be more efficient for porting existing forms [01:54] but it is a much nicer user experience [01:54] if we stay consistent, we keep the crappy ui forever [01:54] no, you misunderstand me [01:54] we have other ajax forms with lovely UI [01:55] that don't do what you have done [01:55] of course we have to break consistency to migrate to something else [01:55] do they need to be backwards compatible though? [01:55] mine does [01:55] well, its not [01:55] (because it breaks non-js browsers) [01:55] no, it works [01:56] so you have all the overhead of server side widget stuff etc [01:56] for non js-browsers, it just submits as normal [01:56] you've got me tied up in knots [01:56] but you don't get the popup confirmation but you do get an informational [01:56] to be functionally equivalent you have to be able to ack the confirmation box [01:57] so you must have added a widget, which you said 'yuck' to before [01:57] here, it's not quite functionally equivalent [01:57] non js just get the informational message [01:57] after form submission [01:57] js get the popup [01:57] ok [02:04] wallyworld_: Doing it the old way (setting document.location) is no more roundtrips. [02:04] wallyworld_: You can tell XHR to not follow the redirect. [02:04] wallyworld_: So you'll get back a 303 with a Location [02:04] Then you set window.location = location [02:04] Done [02:04] ok, cool. didn't know that [02:05] so thats a decent answer here [02:05] Oh, blah, you can't actually block redirects yet. [02:05] So the cheapest may be to say if is_ajax then return a 201 instead. [02:05] It's still evil, but a far, far milder variety. [02:06] can't block redirects? not part of the standard yet? [02:06] Right, it may be in future. [02:07] i did look for how to do that but didn't see anything i could use [02:07] Right. So the view needs to return a 201 like the API, or you need to use the API. [02:08] BugSecrecyEditView already does something similar. [02:11] yes, i wrote the ajax bit [02:12] but it's used as a portlet inside a page [02:53] wgrant: Can haz review? [02:57] StevenK: Why don't we want to clobber yui(2)? [03:18] lifeless: For optimising counts, have we tried doing a "count = SELECT COUNT(*) FROM (real query LIMIT 10000); if count == 10000 then count = 'shitloads';" sort of thing? [03:19] wgrant: we haven't [03:19] wgrant: I can see a few ways that that won't help much [03:20] wgrant: but yeah, sure, that could be a good approach. [03:20] also grr @ bad triage from users that that pump and run [03:20] In some cases it won't help, sure. [03:20] But a lot of common queries just end up being an index walk. [03:25] wgrant: Because it isn't going to change. [03:25] StevenK: Not even with a YUI upgrade? [03:26] wgrant: YUI2 won't change, and we can't upgrade YUI past 3.3 currently [03:26] We can once we stop using yui-deps and related garbage that needs deleting. [03:26] Sure, we can't do it now, but when we want to... [03:27] wgrant: combo-rootdir will likely change again to no longer create a symlink for yui once the icing dependancy dies [03:27] So this isn't the final solution [03:28] And then it can stop being buildout generated. [03:40] wgrant: Ah, thanks. I wasn't sure that I'd convinced you. [03:42] wallyworld_: Sorry, missed the review email before. I've just replied. [03:42] StevenK: Indeed. [03:43] wgrant: np. i'm just about to put up the fix for the mp registration [03:43] Excellent, thanks. [03:43] YAY [03:43] http://www.smh.com.au/environment/weather/weather-settles-after-a-day-of-storms-hail---and-a-tornado-20120214-1t2su.html << Sydney weather is whacked [03:43] it's quite simple in the end [03:44] wallyworld_: The cheap fix I suggested should be about a 10 line diff + tests. [03:44] wgrant: sadly, other model code calls check_permission so i figured it was 'tolerated' [03:44] wallyworld_: There's only a couple of other places. [03:45] PersonRoles makes it pretty cheap to avoid now. [03:45] well, only for those simple checks [03:45] Sure. [03:45] the logic needs to be broken out and the security adaptor call that [03:45] In the complex cases (mostly bugs, branches) it's already delegated to the model. [03:46] And that is going to change anyway? [03:46] Hm? [03:47] We're going to change the complex case of bugs and branches for disclosure? [03:47] Yes, but the precise rules are irrelevant here. [03:48] It's the code structure around them that is under scrutiny. [03:56] Tomboy fails at crash survival :/ [03:57] It mustn't have flushed my notes for about 6 hours yesterday :/ [03:57] less crashing thanks [04:09] wgrant: I have a test here that is doing setupBrowser(auth='Basic mark@ex...:password') I thought you removed that terribleness? [04:10] loltpg [04:11] StevenK: Hm, I suspect that's no longer doing what it thought. [04:11] StevenK: Especially since the domain seems to be ellipsised. [04:11] All the passwords are now 'test' [04:11] Tests still use basic auth. [04:12] wgrant: I'm happy to fix it with a hammer (in a Jeremy Clarkson style) while I'm here. === nhandler_ is now known as nhandler [04:12]