| stub | Can I get a review for https://pastebin.canonical.com/25990/ ? It is a minor refactoring to a previously reviewed branch to get all the tests passing - the bulk is shuffling some useful code to a better location. | 02:22 |
|---|---|---|
| mwhudson | stub: doesn't look like the remove argument to kill_by_pidfile is ever used | 02:44 |
| stub | mwhudson: It isn't - just there to make it obvious that it gets removed by default and a bit of overengineering for future uses. | 02:45 |
| mwhudson | stub: also the docstring seems to lie about it | 02:46 |
| stub | I can remove it if you think that is better | 02:46 |
| mwhudson | stub: i think it is a bit better yes | 02:46 |
| stub | I supposed it is always success since we don't check if the -9 produced a zombie of some sort. | 02:47 |
| mwhudson | ok, maybe not a lie, but not super clear | 02:48 |
| stub | mwhudson: Remove the parameter entirely or just remove 'on success' from the docstring? | 02:48 |
| mwhudson | stub: remove the parameter entirely would get my vote | 02:48 |
| stub | def kill_by_pidfile(pidfile_path): | 02:49 |
| stub | """Kill a process identified by the pid stored in a file. | 02:49 |
| stub | The pid file is removed from disk. | 02:49 |
| stub | """ | 02:49 |
| mwhudson | stub: +1 | 02:49 |
| stub | (With a blank line between the two lines that the IRC client stripped | 02:49 |
| mwhudson | stub: otherwise, it looks fine | 02:50 |
| stub | Thanks :) | 02:50 |
| mwhudson | apart from the usual "anything that touches lib/canonical/testing/layers.py makes me want to cry a little" | 02:50 |
| === adiroiban changed the topic of #launchpad-reviews to: on call: - || reviewing: - || queue [adiroiban(bug-497438),adiroiban(bug-435165)] || This channel is logged: http://irclogs.ubuntu.com || https://code.edge.launchpad.net/launchpad/+activereviews | ||
| adiroiban | salgado: Hi! Do you have time to land this branch? https://code.edge.launchpad.net/~adiroiban/launchpad/bug-427319/+merge/15994 Thanks! | 10:46 |
| salgado | adiroiban, sure, I'll do it in a minute | 10:47 |
| al-maisan | hello allenap, I just replied to your review email from last Wednesday (sorry for the delay) -- could you please take a quick look before I land the branch? | 11:17 |
| allenap | al-maisan: Sure. | 11:17 |
| allenap | al-maisan: I have a team stand-up call now, but I'll look at it straight after that. | 11:18 |
| al-maisan | allenap: that's greatly appreciated :) | 11:18 |
| allenap | al-maisan: Is it very expensive to calculate changes_file_url? | 11:31 |
| al-maisan | allenap: it is somewhat expensive and it is calculated for each SourcePackagePublishingHistory on the LP API -- whether you're interested in it or not. | 11:32 |
| allenap | al-maisan: By making it a method, Launchpad must field an extra HTTP request for every SPPH. Maybe that's a bigger overhead? Or do you expect that it will be called rarely? | 11:33 |
| al-maisan | allenap: it would seem that it's not a datum of general interest .. there are some LP API apps that use it: e.g. code imports or scripts written by kees | 11:34 |
| al-maisan | but my feeling is that it's data that's not necessarily needed whenever we access a SPPH instance | 11:35 |
| al-maisan | .. and calculating the 'changes_file_url' property now leads the Soyuz OOPS top-10 by a big margin | 11:36 |
| allenap | al-maisan: Okay, sounds like you have a handle on it. It can be changed later if it turns out to be otherwise, so I'm not too worried. | 11:36 |
| allenap | al-maisan: Timeout or error? | 11:37 |
| al-maisan | allenap: time out | 11:37 |
| allenap | al-maisan: Wow. | 11:37 |
| al-maisan | yeah | 11:37 |
| === adiroiban changed the topic of #launchpad-reviews to: on call: - || reviewing: - || queue [adiroiban(bug-497438),adiroiban(bug-435165),adiroiban(bug-359180)] || This channel is logged: http://irclogs.ubuntu.com || https://code.edge.launchpad.net/launchpad/+activereviews | ||
| allenap | al-maisan: Okay, one other thing. The test_changesFileLFA method is misnamed; I think it should be test_getChangesFileLFA(). Everything else looks fine. | 11:38 |
| al-maisan | allenap: ah, good point. Will fix that. | 11:39 |
| al-maisan | allenap: thanks for taking another look at this :) | 11:39 |
| allenap | al-maisan: I'll add the conversation as a comment in the mp. | 11:39 |
| allenap | al-maisan: Welcome :) | 11:39 |
| al-maisan | allenap: great, thanks! | 11:39 |
| === wgrant changed the topic of #launchpad-reviews to: on call: - || reviewing: - || queue [adiroiban(bug-497438),adiroiban(bug-435165),adiroiban(bug-359180),wgrant] || This channel is logged: http://irclogs.ubuntu.com || https://code.edge.launchpad.net/launchpad/+activereviews | ||
| henninge | jtv: we don't hear your echo | 12:16 |
| henninge | adiroiban: Hi! | 12:40 |
| adiroiban | henninge: hi | 12:40 |
| henninge | adiroiban: it's time for Christmas goodies: I am finishing your review now! ;-) | 12:41 |
| henninge | adiroiban: This is bug 435165 | 12:42 |
| mup | Bug #435165: Make it easier to navigate to the full list of templates in source packages <trivial> <ui> <Launchpad Translations:In Progress by adiroiban> <https://launchpad.net/bugs/435165> | 12:42 |
| henninge | adiroiban: what does "request/URL/5" do? | 12:45 |
| adiroiban | get the URL up the the fifth segment | 12:46 |
| adiroiban | maybe I should replace it with an object URL formater | 12:47 |
| henninge | well, relying on the format of the url in this manner is liable to break. | 12:49 |
| henninge | so, yeah, using a formatter is the right thing but I have another suggestion. | 12:49 |
| adiroiban | al-maisan: Hi. Do you have time to land this branch https://code.edge.launchpad.net/~adiroiban/launchpad/bug-492375/+merge/16279 ? Thanks! | 12:54 |
| henninge | adiroiban: I am also not happy with using more_templates_by_source as a condition. We always test explicitely in Python code (i.e. more_templates_by_source != 0) and I think TAL should not be different. | 12:55 |
| henninge | adiroiban: my suggestion to circumvent this is to construct the whole fragment in the view code. | 12:57 |
| adiroiban | henninge: we also have <p tal:condition="context/relatives_by_source" id="potemplate-relatives"> | 12:57 |
| henninge | adiroiban: true, maybe that is not so bad... | 12:59 |
| henninge | adiroiban: Maybe it is just my test that I like to unclutter the tal an create fragments in the view, even call tal formatters from the view. | 12:59 |
| adiroiban | henninge: I can create the whole text in the view...as I only need to append "and" | 12:59 |
| al-maisan | adiroiban: can do. | 12:59 |
| henninge | adiroiban: as I said, that is what I would do but I acknowledge that it is not the only way tod do it. | 13:00 |
| henninge | I leave it up to you, important thing is to get rid of the request/URL/5 and use a fmt:url instead (or canonical_url in view code). | 13:01 |
| adiroiban | henninge: for the condition http://paste.ubuntu.com/344078/ | 13:02 |
| henninge | adiroiban: what I meant would make the TAL look like this http://paste.ubuntu.com/344080/ | 13:05 |
| adiroiban | henninge: for the fmt:url ... I am not sure how to create it. Is there a url formater for sourcepackage name? (I could not find it) Or I should implement it? | 13:06 |
| henninge | and more_templates_string simply returns "and <a href="...">17 more templates</a>" | 13:06 |
| adiroiban | henninge: got it. Thanks. I'll implement it that way. | 13:07 |
| henninge | adiroiban: or it returns an empty string, so no need for a condition in TAL. | 13:08 |
| adiroiban | henninge: yep | 13:08 |
| henninge | adiroiban: I think what you are looking for is canonical_url(related_template), right? | 13:10 |
| adiroiban | henninge: I don't know. Please allow me to take a look :) | 13:11 |
| henninge | adiroiban: btw, I just demoed that on launchpad.dev by hacking the database and simply moving some of the existing templates to evolution. | 13:12 |
| henninge | adiroiban: What do you want to link to? The logical place would be +templates, I guess. | 13:13 |
| adiroiban | henninge: ok. I think there is also a pagetest for that. | 13:13 |
| henninge | adiroiban: I know, I saw it! | 13:13 |
| adiroiban | henninge: https://translations.edge.launchpad.net/ubuntu/karmic/+source/kdelibs/+translations | 13:13 |
| henninge | but to try it out locally i your branch I need a product with more than 5 templtes. | 13:14 |
| henninge | adiroiban: doesn't work that way for a product it looks | 13:15 |
| === salgado is now known as salgado-afk | ||
| henninge | adiroiban: https://translations.edge.launchpad.net/wiserearth/trunk/+translations | 13:17 |
| henninge | danilos: do you know why +translations works differntly on products and packages? | 13:19 |
| adiroiban | henninge: rebuilding schema. I'm taking a look now | 13:19 |
| danilos | henninge, because we didn't have time to make sourcepackages use the same approach (i.e. group by languages, not by templates) | 13:19 |
| henninge | bad news | 13:20 |
| henninge | so we don't have a place to link to that lists are templates for a source package, once +translations for source packages works like products. | 13:20 |
| henninge | s/are/all/ | 13:21 |
| al-maisan | adiroiban: your branch (lp:~adiroiban/launchpad/bug-492375) is running in an ec2 instance now and will be submitted to pqm (in approx. 4 hours) if all goes well. | 13:21 |
| adiroiban | al-maisan: thanks! | 13:22 |
| al-maisan | you are welcome | 13:22 |
| adiroiban | henninge: I will look to see what can be done fro productseries | 13:25 |
| henninge | adiroiban: it looks like the best bet is to link to +templates for productseries and +trantlations for packages atm. | 13:26 |
| henninge | but this will need to be fixed in another branch. | 13:26 |
| danilos | henninge, adiroiban: we should use the same views/pages for +templates and +translations on all of productseries, sourcepackage, distroseries, project (with focused series being shown for each product) | 13:29 |
| danilos | henninge, adiroiban: do not do that right away though :) | 13:29 |
| henninge | what I meant. | 13:29 |
| henninge | ;) | 13:29 |
| adiroiban | henninge: what needs to be fixed in another branch? | 13:31 |
| adiroiban | danilos: ok. just give me the bug number | 13:31 |
| adiroiban | just kidding :) | 13:31 |
| adiroiban | but still, it would be nice to have a bug ;) | 13:31 |
| danilos | adiroiban, it's not a bug per se, and it'd involve cleaning up a few interfaces/implementations first :) | 13:32 |
| adiroiban | henninge: I don't know what you mean with canonical_url(related_template) . Since I'm in a potemplate I assume I should look the context for self.context.distroseries | 13:40 |
| henninge | adiroiban: yes, taht was wrong | 13:41 |
| henninge | adiroiban: It will be canonical_url(series)+"/+translations", I guess. | 13:41 |
| henninge | or canonical_url(context) ? | 13:41 |
| adiroiban | henninge: nope | 13:41 |
| adiroiban | context is the potemplate | 13:41 |
| adiroiban | context.distroseries is ubuntu/hoary | 13:42 |
| adiroiban | i need the sourcepackage name | 13:42 |
| adiroiban | maybe I need to create a url formater | 13:42 |
| adiroiban | as I could not find one | 13:42 |
| henninge | potemplate also has a sourcepackagename, doesn't it? | 13:42 |
| henninge | adiroiban: but don't do that for this branch. Just create the html in view. | 13:43 |
| henninge | 'and <a href="%s">%d more templates</a>' % (url, num_of_templates) | 13:44 |
| henninge | constructing the url string is the hard part. | 13:44 |
| henninge | adiroiban: you will have to check whether the template is from a product series or from a sourcepackage and construct the url accordingly. | 13:46 |
| adiroiban | henninge: yep. but there is no sourcepackagename url formater | 13:47 |
| henninge | adiroiban: I need to go now, I am sorry. | 13:47 |
| adiroiban | henninge: np. I should be able to handle it from now on | 13:47 |
| adiroiban | thanks | 13:47 |
| henninge | adiroiban: yes, you will need to create the url in your code. | 13:47 |
| henninge | good luck, I will look at it tomorrow. | 13:48 |
| === salgado-afk is now known as salgado | ||
| === abentley changed the topic of #launchpad-reviews to: on call: abentley || reviewing: - || queue [adiroiban(bug-497438),adiroiban(bug-435165),adiroiban(bug-359180),wgrant] || This channel is logged: http://irclogs.ubuntu.com || https://code.edge.launchpad.net/launchpad/+activereviews | ||
| === adiroiban changed the topic of #launchpad-reviews to: on call: abentley || reviewing: - || queue [adiroiban(bug-497438),adiroiban(bug-359180),wgrant] || This channel is logged: http://irclogs.ubuntu.com || https://code.edge.launchpad.net/launchpad/+activereviews | ||
| adiroiban | abentley: Hi. Do you have time to land this branch? https://code.edge.launchpad.net/~adiroiban/launchpad/bug-496352-series-status-refactor/+merge/16108 Thanks! | 14:35 |
| abentley | adiroiban: Has it passed a full test suite run? | 14:36 |
| adiroiban | abentley: I don't know. I have just started a new one now. | 14:38 |
| abentley | adiroiban: Okay, let me know when it passes, and I'll land it. | 14:39 |
| adiroiban | abentley: sorry for that. I've only tested with -m translations, and for my other branches the reviewers have started the full test. | 14:42 |
| adiroiban | abentley: do I need to merge with devel ? | 14:42 |
| abentley | adiroiban: It is a good idea to merge with devel, but not strictly required. | 14:43 |
| === salgado is now known as salgado-lunch | ||
| sinzui | danilos: bac: Can either of your review https://code.edge.launchpad.net/~sinzui/launchpad/webkit-css/+merge/16388 | 14:55 |
| bac | sinzui: i can. is abentley unavailable? | 14:56 |
| bac | or is his queue just too long? anyway, i'll grab it now. | 14:56 |
| sinzui | bac: I think you would appreciate a layout fix for safari more than most people. | 14:57 |
| bac | sinzui: i should point out i never *really* saw the issue last week. the problems i saw on staging were fixed when they did a 'make clean' | 14:57 |
| sinzui | bac: lpnet and edge are very broken in epiphany. The the sidebar is rarely on the side. Most of the time it is below the content | 15:00 |
| abentley | adiroiban: was there a pre-implementation discussion for your bug-497438 branch? | 15:03 |
| adiroiban | abentley: yes. | 15:05 |
| bac | sinzui: can you paste an URL that is currently broken on production in epiphany so i can see the problem? | 15:05 |
| sinzui | bac: | 15:07 |
| sinzui | https://edge.launchpad.net/launchpad-registry/+milestone/3.1.13 | 15:07 |
| sinzui | https://edge.launchpad.net/launchpad-registry | 15:07 |
| sinzui | https://edge.launchpad.net/~bac | 15:07 |
| bac | sinzui: ah, i see it if i have a super-wide browser | 15:07 |
| sinzui | bac: I have a narrow browser | 15:07 |
| abentley | adiroiban: It's really helpful to mention it. There's supposed to be a section in your cover letter between "Proposed fix" and "Implementation details" called "Pre-implementation notes". | 15:10 |
| abentley | adiroiban: https://dev.launchpad.net/QuickCoverLetterTemplate | 15:12 |
| adiroiban | abentley: sorry for not including that part. The preimplementation chat was with Danilo and the summary is part of Implementation details | 15:13 |
| abentley | adiroiban: In lib/canonical/launchpad/security.py, you have "Disribution" rather than "Distribution" twice in the docstrings. | 15:15 |
| === ursula_ is now known as Ursinha-lunch | ||
| adiroiban | abentley: thanks. fixed. | 15:17 |
| abentley | adiroiban: UTC always makes me think of Universal Coordinated Time. Is there another name you could use for utc_browser? | 15:19 |
| adiroiban | dtc ? | 15:19 |
| adiroiban | distribution translations coordinator | 15:20 |
| abentley | adiroiban: works for me. | 15:21 |
| adiroiban | abentley: ok. I'll rename it | 15:21 |
| abentley | adiroiban: Thanks. I think that will save confusion in the future. | 15:22 |
| === jelmer_ changed the topic of #launchpad-reviews to: on call: abentley || reviewing: - || queue [adiroiban(bug-497438),adiroiban(bug-359180),wgrant,jelmer] || This channel is logged: http://irclogs.ubuntu.com || https://code.edge.launchpad.net/launchpad/+activereviews | ||
| deryck | BjornT_, since you're living Windmill these days, could I get a Windmill fixer-upper review? :) | 15:26 |
| abentley | adiroiban: It seems like you create utg_member in order to specify their email and password, so that you can pass known values to setupBrowser. Is that correct? | 15:28 |
| adiroiban | abentley: yes | 15:28 |
| BjornT_ | deryck: maybe :) | 15:28 |
| deryck | BjornT_, heh. It's a small fix. Trying to make inline subscriber test less fragile. | 15:29 |
| abentley | adiroiban: I think it would be useful to provide an alternative to setupBrowser that accepted an arbitrary Person, but what you've done is also fine. | 15:30 |
| BjornT_ | deryck: sure | 15:30 |
| deryck | BjornT_, https://code.edge.launchpad.net/~deryck/launchpad/fix-subscriber-windmill-fragility-497112/+merge/16431 | 15:30 |
| deryck | BjornT_, thanks! | 15:30 |
| adiroiban | abentley: now that it is called DTC, it would have make some sense to pass the distribution as argument | 15:31 |
| BjornT_ | deryck: what does css_name look like? is it unique for each subscription? | 15:39 |
| deryck | BjornT_, it's the form "subscriber-XXX" where XXX is the db id for the subscribed user. | 15:41 |
| deryck | BjornT_, so yeah, should be unique. | 15:41 |
| BjornT_ | deryck: ok, approved | 15:41 |
| deryck | BjornT_, thanks! | 15:43 |
| abentley | adiroiban: (didn't notice I was disconnected) Why are you assigning to ubuntu.translationgroup rather than making utg_member a member of it? | 15:46 |
| adiroiban | abentley: now that it is called DTC, it would have make some sense to pass the distribution as argument | 15:48 |
| adiroiban | abentley: I don't know if ubuntu.translationgroup is already created | 15:49 |
| abentley | adiroiban: Sure, that would be fine. But this function has side-effects that aren't apparent from the name. Especially, calling it twice will make the first return value invalid. | 15:50 |
| adiroiban | abentley: true. I also tried joining utg_member to translationgroup | 15:51 |
| adiroiban | but I got an Attribute now allowed | 15:51 |
| adiroiban | or something like that | 15:52 |
| adiroiban | I also tried to login(ubuntu.translationsgroup.owner) | 15:52 |
| adiroiban | and then add the new member | 15:52 |
| adiroiban | but with no luck | 15:52 |
| abentley | adiroiban: Okay, I'm glad you explored other options. I think this is acceptable in this case. | 15:53 |
| adiroiban | abentley: I think I need to look for a implementation that will also work for calling that function twice | 15:57 |
| abentley | adiroiban: I think that would be an improvement, but I can't think of a use case for calling it twice. | 15:57 |
| abentley | adiroiban: If you get the "Attribute not allowed" message again, I can help you look into it. It's important to distinguish between "forbidden" (never allowed) and "unauthorized" (not allowed in this context). | 15:59 |
| === salgado-lunch is now known as salgado | ||
| adiroiban | abentley: OK. Thanks! | 16:00 |
| abentley | adiroiban: I'm going to summarize our discussion and change it from "needs review" to "work in progress" for now. | 16:01 |
| adiroiban | abentley: sure. | 16:03 |
| abentley | adiroiban: Since I assume you're already working on stuff, I'll skip your other review for now. | 16:11 |
| === abentley changed the topic of #launchpad-reviews to: on call: abentley || reviewing: wgrant || queue [adiroiban(bug-359180),jelmer] || This channel is logged: http://irclogs.ubuntu.com || https://code.edge.launchpad.net/launchpad/+activereviews | ||
| adiroiban | abentley: there is no hurry :) | 16:19 |
| === abentley changed the topic of #launchpad-reviews to: on call: abentley || reviewing: || queue [adiroiban(bug-359180),jelmer] || This channel is logged: http://irclogs.ubuntu.com || https://code.edge.launchpad.net/launchpad/+activereviews | ||
| adiroiban | abentley: can you please take a look at this implementation http://paste.ubuntu.com/344161/ ? Thanks! | 16:31 |
| abentley | adiroiban: Sorry, on phone. With you shortly. | 16:33 |
| abentley | adiroiban: What would raise NameAlreadyTaken? | 16:46 |
| adiroiban | abentley: calling the function twice | 16:47 |
| adiroiban | abentley: here is the code that also accepts a person http://paste.ubuntu.com/344170/ | 16:47 |
| adiroiban | another person | 16:47 |
| salgado | adiroiban, did you get a success/error message for that branch you asked me to land? | 16:48 |
| adiroiban | salgado: success :) | 16:49 |
| salgado | adiroiban, but it didn't land on devel, did it? | 16:49 |
| adiroiban | salgado: nope, as it was not a RC | 16:49 |
| abentley | adiroiban: calling which function twice? | 16:50 |
| salgado | adiroiban, but pqm is open | 16:50 |
| adiroiban | salgado: it was closed last week | 16:50 |
| adiroiban | abentley: makePerson | 16:50 |
| adiroiban | abentley: or setupDTCBrowser | 16:50 |
| salgado | adiroiban, oh, I meant to ask if you got a success email today. I've submitted your branch in the morning and it's not on ec2 anymore | 16:51 |
| adiroiban | salgado: ah... no email | 16:51 |
| abentley | adiroiban: AFACT, calling setupDTCBrowser will not raise NameAlreadyTaken, because you don't re-raise it in the except block. | 16:51 |
| adiroiban | abentley: yes. I wanted to say that makePerson from setupDTCBrowser | 16:52 |
| adiroiban | can raise that exception | 16:52 |
| al-maisan | adiroiban: your lp:~adiroiban/launchpad/bug-492375 branch fails a test, please see: http://pastebin.ubuntu.com/344177/ | 16:57 |
| abentley | adiroiban: Your code is still unsafe if you call it twice with an explicit Person, and if you call it with an explicit person, it doesn't use that person to set up the browser. | 17:01 |
| adiroiban | abentley: true | 17:02 |
| abentley | adiroiban: So either we need to make the handling of person more robust, or we should remove it. | 17:03 |
| === Ursinha-lunch is now known as Ursinha | ||
| abentley | adiroiban: I am happy either way. | 17:03 |
| adiroiban | the browser is not setup. But why it will fail when called twice? | 17:03 |
| abentley | adiroiban: If there is an explicit person, you will reassign ubuntu.translationgroup unconditionally. | 17:05 |
| adiroiban | abentley: yes. I'm creating a new translationgroup having that person as an owner | 17:05 |
| abentley | adiroiban: That will make the return value from the previous call unusable. | 17:06 |
| adiroiban | abentley: then I can so something like this http://paste.ubuntu.com/344183/ | 17:08 |
| abentley | adiroiban: That would solve the double-call issue. I would prefer a try/except/else, like this: http://paste.ubuntu.com/344186/ | 17:10 |
| abentley | adiroiban: This ensures that you won't catch NameAlreadyTaken from other methods and assume they came from makePerson. | 17:11 |
| adiroiban | abentley: yes. thanks | 17:11 |
| abentley | adiroiban: With that change, I'd be happy to approve the review. | 17:13 |
| abentley | salgado: Thanks! | 17:13 |
| abentley | adiroiban: Going offline for a sec. | 17:14 |
| adiroiban | al-maisan: do you know how I can call that failing test? | 17:15 |
| al-maisan | just a minute | 17:15 |
| adiroiban | al-maisan: I tried with bin/test -m translations and everhing was ok. | 17:15 |
| adiroiban | also bin/test -t rosetta-potemplates is not starting that test | 17:16 |
| adiroiban | same for bin/test -t -t "stories/translations/.*" | 17:16 |
| al-maisan | adiroiban: please try "bin/test -vv -t stories/translations" | 17:20 |
| adiroiban | al-maisan: that did the trick. Thanks! | 17:23 |
| al-maisan | adiroiban: you are welcome | 17:23 |
| adiroiban | abentley: I've just pushed the new code | 17:23 |
| === deryck is now known as deryck[lunch] | ||
| abentley | adiroiban: r=abentley | 17:37 |
| === abentley changed the topic of #launchpad-reviews to: on call: abentley || reviewing: adiroiban || queue [jelmer] || This channel is logged: http://irclogs.ubuntu.com || https://code.edge.launchpad.net/launchpad/+activereviews | ||
| EdwinGrubbs | beuno: can you review https://code.edge.launchpad.net/~edwin-grubbs/launchpad/bug-482176-add-team-member-ajax-part2/+merge/16325 | 18:07 |
| EdwinGrubbs | beuno: The last comment in the mp contains a link to screenshots. | 18:08 |
| beuno | EdwinGrubbs, I can | 18:12 |
| beuno | EdwinGrubbs, I need to grab lunch or I will die | 18:13 |
| beuno | is it ok to get back to you in ~1 hour? | 18:14 |
| beuno | also, is there a spinner and such? | 18:14 |
| EdwinGrubbs | beuno: yes, there is a spinner. I can probably put together a video if that would be helpful. | 18:14 |
| beuno | EdwinGrubbs, I trust you that it works as everything else | 18:15 |
| beuno | be back in an hour! | 18:15 |
| === beuno is now known as beuno-lunch | ||
| === deryck[lunch] is now known as deryck | ||
| === beuno-lunch is now known as beuno | ||
| beuno | EdwinGrubbs, hi | 19:20 |
| beuno | still around? | 19:20 |
| EdwinGrubbs | beuno: hello | 19:21 |
| beuno | EdwinGrubbs, I see you've introduced an informational message in the overlay | 19:21 |
| beuno | or is that when you try to add somebody who's already been added? | 19:21 |
| EdwinGrubbs | beuno: yes, salgado didn't like me using the error message overlay to let the user know that someone has already been added. | 19:22 |
| beuno | EdwinGrubbs, could we go a step further and not let you add people who are already part of the team? | 19:23 |
| beuno | have them grayed out on the results? | 19:23 |
| EdwinGrubbs | beuno: that is a much bigger project, since it will involve changing the way we extract data from the vocabulary as JSON, and we need to have a standard way of specifying what values we are filtering. | 19:24 |
| beuno | EdwinGrubbs, gotcha. Could we file a bug for that? I think that's the right way to go | 19:24 |
| EdwinGrubbs | beuno: yes, I'll file one | 19:27 |
| beuno | EdwinGrubbs, thanks. I have a few comments on the layout of the "dialog", I'll add them to the MP | 19:27 |
| beuno | that's all really, hope it's a quick change | 19:27 |
| EdwinGrubbs | beuno: thanks | 19:27 |
| beuno | EdwinGrubbs, we'll need to make foundations aware of what we want to change in the vocabularies, but I guess they'll learn about it when they triage the bug :) | 19:28 |
| beuno | EdwinGrubbs, done | 19:35 |
| abentley | adiroiban: If tabindex_chain don't affect tabbing, can you call it something else? | 19:53 |
| adiroiban | abentley: fields_chain? | 19:54 |
| abentley | adiroiban: Maybe something like navigation_order? | 19:56 |
| adiroiban | abentley: sure | 19:56 |
| adiroiban | or translations_order? | 19:57 |
| abentley | adiroiban: Sure. | 19:57 |
| abentley | adiroiban: On line 344-345 of the patch, shouldn't you be deleting a semi-colon, not adding a space? | 19:57 |
| abentley | in lib/lp/translations/templates/translations-macros.pt, why is the whole thing conditional on autofocus_html_id? | 19:59 |
| adiroiban | abentley: line 344 should be left untouched. the two semicolon are valid as the first one is excaping the javascript semicolon | 20:01 |
| abentley | adiroiban: Okay. | 20:01 |
| adiroiban | abentley: autofocus is returning the first field and if we dont't have any field there is no need for that code | 20:02 |
| abentley | adiroiban: It looks like Y.lp.pofile.initializeKeyBindings is being called unconditionally and is assuming tabindex is set. Am I reading it wrong? | 20:05 |
| adiroiban | abentley: nope. | 20:07 |
| abentley | So that would cause javascript errors if autofocus_html_id is unset? | 20:08 |
| adiroiban | abentley: yes | 20:09 |
| adiroiban | abentley: i will add empty variables | 20:10 |
| abentley | adiroiban: Sounds good. | 20:10 |
| EdwinGrubbs | beuno: the error message is currently shown only after someone clicks on one of the results, which hides the picker. Are you saying in the review that you want me to re-show the picker so that I can display the error message in that? | 20:20 |
| beuno | EdwinGrubbs, well, you are re-showing something, right? | 20:20 |
| beuno | we can do one or the other | 20:20 |
| beuno | either make it look more like a dialog | 20:20 |
| beuno | or do it within the picker itself | 20:20 |
| beuno | we should give a user a way out other than clicking on the small x :) | 20:21 |
| EdwinGrubbs | beuno: I'll just add the OK button to dismiss the current dialog. | 20:23 |
| beuno | EdwinGrubbs, but that doesn't let the user recover, no? | 20:23 |
| EdwinGrubbs | beuno: they would only want to recover if they are adding multiple people as members. If they try to add multiple users quickly, we might end up in the situation where they start typing in the new search text, and the error message shows up in the picker, even though the error is related to the last usage of it. | 20:25 |
| === salgado is now known as salgado-afk | ||
| beuno | EdwinGrubbs, good point | 20:26 |
| EdwinGrubbs | rockstar: ping | 20:41 |
| rockstar | EdwinGrubbs, I'm technically on holiday right now, but would be happy to oblige small requests. | 20:41 |
| rockstar | (I got Warcraft III working in Wine last night, so am currently doing nothing useful) | 20:41 |
| EdwinGrubbs | rockstar: that's awesome. I'll try to find someone else to review a windmill test first. | 20:43 |
| rockstar | EdwinGrubbs, okay. If you don't find anyone, I can take it. | 20:43 |
| EdwinGrubbs | deryck: ping | 20:44 |
| EdwinGrubbs | jtv, mars: ping | 20:46 |
| jtv | EdwinGrubbs: zzz | 20:47 |
| EdwinGrubbs | jtv: sorry, go back to sleep. It's all just a bad dream. | 20:47 |
| jtv | <snore> | 20:47 |
| wgrant | abentley: Can you please land that branch for me? | 20:53 |
| abentley | wgrant: Has it passed the whole test suite? | 20:53 |
| wgrant | abentley: Not for two and a half months, but it looks like it should still pass, and I thought you normally ec2-landed things. | 20:54 |
| abentley | wgrant: I don't ever ec2-land things. | 20:54 |
| deryck | EdwinGrubbs, hi. I really need to EOD for kids being home. Was on phone. Can we take it up tomorrow? | 20:54 |
| wgrant | abentley: Ah. I guess I'll ec2test it myself. | 20:54 |
| EdwinGrubbs | deryck: I'll find someone else to do a short review. | 20:55 |
| abentley | wgrant: When it passes, ping me and I'll be happy to land it. | 20:55 |
| deryck | EdwinGrubbs, ah, ok. Sorry I couldn't help. | 20:55 |
| EdwinGrubbs | salgado-afk, beuno: I have updated the mp. Can you re-review it? | 21:06 |
| EdwinGrubbs | sinzui: do you feel comfortable reviewing a windmill test, since salgado passed on that? | 21:07 |
| sinzui | yes | 21:07 |
| EdwinGrubbs | sinzui: here it is: https://code.edge.launchpad.net/~edwin-grubbs/launchpad/bug-482176-add-team-member-ajax-part2/+merge/16325 | 21:07 |
| beuno | EdwinGrubbs, I can. I'm bringing in your branch to play with it and fully sign off | 21:08 |
| === abentley changed the topic of #launchpad-reviews to: on call: - || reviewing: - || queue [jelmer] || This channel is logged: http://irclogs.ubuntu.com || https://code.edge.launchpad.net/launchpad/+activereviews | ||
| beuno | EdwinGrubbs, still around? | 22:28 |
| beuno | when trying to add someone to the spanish team | 22:28 |
| beuno | I get: | 22:28 |
| beuno | The following errors were encountered: | 22:28 |
| beuno | * Unexpected status: foo | 22:28 |
| beuno | (and it got added) | 22:28 |
| EdwinGrubbs | beuno: I left in some debugging code on accident. | 22:28 |
| EdwinGrubbs | beuno: it's a one line fix. I can push it if you want. | 22:29 |
| beuno | EdwinGrubbs, no, it's ok | 22:29 |
| beuno | EdwinGrubbs, in general, I think we show the persons' name on the list when saving | 22:30 |
| beuno | grayed out | 22:30 |
| beuno | and flash that green/red | 22:30 |
| beuno | no? | 22:30 |
| beuno | like subscribing to bugs | 22:31 |
| beuno | also, the alignemnt on the overlay is a bit wonky | 22:32 |
| * EdwinGrubbs is checking | 22:32 | |
| beuno | the text could ideally be top-aligned with the info icon | 22:32 |
| EdwinGrubbs | beuno: I had forgotton about the grayed out name and spinner next to the name. I can look into that. | 22:37 |
| beuno | EdwinGrubbs, thanks. Sorry I didn't catch that earlier, I should of branched from the start | 22:37 |
| beuno | EdwinGrubbs, commented exactly this on the MP | 22:45 |
| beuno | and now, I'm off to buy my brother a last minute bday gifty | 22:45 |
| beuno | *gift | 22:45 |
Generated by irclog2html.py 2.7 by Marius Gedminas - find it at mg.pov.lt!