| jpds | stub, jml: Thanks for the db review. | 08:28 |
|---|---|---|
| jpds | noodles775: Ping. | 10:09 |
| noodles775_ | Hi jpds | 10:10 |
| jpds | noodles775_: Hi there, when you have time, I think I need a UI review for https://code.edge.launchpad.net/~jpds/launchpad/fix_361650/+merge/16749 . | 10:10 |
| noodles775_ | jpds: great! I'll take a look in a bit. | 10:12 |
| jpds | Awesome, thanks. | 10:14 |
| === al-maisan changed the topic of #launchpad-reviews to: on call: - || reviewing: - || queue [jpds, al-maisan<http://tinyurl.com/yhj8h7l>] || This channel is logged: http://irclogs.ubuntu.com || https://code.edge.launchpad.net/launchpad/+activereviews | ||
| === intellectronica changed the topic of #launchpad-reviews to: on call: intellectronica || reviewing: - || queue [jpds, al-maisan<http://tinyurl.com/yhj8h7l>] || This channel is logged: http://irclogs.ubuntu.com || https://code.edge.launchpad.net/launchpad/+activereviews | ||
| === noodles775_ is now known as noodles775 | ||
| noodles775 | jpds: hi! Can you add some details about testing your MP? (ie. steps to demo the new functionality, who to log in as, a url to access etc.). | 11:25 |
| noodles775 | Just for next time perhaps... | 11:26 |
| jpds | noodles775: Just a sec. | 11:29 |
| noodles775 | jpds: i've already looked at the code, so np this time. It's just helpful to provide them in the MP. | 11:30 |
| jpds | noodles775: OK, I'll add the details next time. | 11:31 |
| === mrevell is now known as mrevell-lunch | ||
| noodles775 | jpds: what's the reasoning behind not doing this as an admin-only field displayed on the normal edit form? | 12:10 |
| === adiroiban changed the topic of #launchpad-reviews to: on call: intellectronica || reviewing: - || queue [jpds, al-maisan<http://tinyurl.com/yhj8h7l>, adiroiban(bug-340662)] || This channel is logged: http://irclogs.ubuntu.com || https://code.edge.launchpad.net/launchpad/+activereviews | ||
| intellectronica | adiroiban: what's the branch? have you already arranged to have someone review it, or would you like me to look at it now? | 12:12 |
| adiroiban | https://code.edge.launchpad.net/~adiroiban/launchpad/bug-340662/+merge/16629 | 12:13 |
| adiroiban | intellectronica: hm... I did not arranged any review. Just created a MP and add it to the topic, to let you know it needs review | 12:13 |
| adiroiban | this is how it went for my previous branches | 12:14 |
| adiroiban | were there any changes in the MP process? | 12:14 |
| intellectronica | adiroiban: cool, i'll review it | 12:14 |
| al-maisan | intellectronica: could you please also review the branch I queued (http://tinyurl.com/yhj8h7l) when you get to it? | 12:24 |
| intellectronica | al-maisan: sure, will look at it as soon as i finish adiroiban's branch | 12:25 |
| al-maisan | intellectronica: thanking you :) | 12:26 |
| jpds | noodles775: Do you mean the +review form? | 12:26 |
| noodles775 | jpds: no, I meant the +edit form (but i'm logged in as an admin, maybe that's not presented for mirror-admins...). Checking now. | 12:32 |
| jpds | noodles775: I wanted to make it a little obvious by having the separate button for it. | 12:33 |
| intellectronica | adiroiban: looks fine to me. i will run the tests and land on your behalf if it passes cleanly | 12:35 |
| adiroiban | intellectronica: thanks. Let me know if there are any errors. Before creating the MP I did a full test for translations module | 12:36 |
| jpds | noodles775: And mirror registrants shouldn't be able to see the checkbox themselves. | 12:37 |
| === intellectronica changed the topic of #launchpad-reviews to: on call: intellectronica || reviewing: - || queue [jpds, al-maisan<http://tinyurl.com/yhj8h7l>] || This channel is logged: http://irclogs.ubuntu.com || https://code.edge.launchpad.net/launchpad/+activereviews | ||
| intellectronica | al-maisan: i must break for lunch, but will review your branch as soon as i'm back | 13:01 |
| al-maisan | intellectronica: enjoy your lunch! | 13:01 |
| === salgado changed the topic of #launchpad-reviews to: on call: intellectronica || reviewing: - || queue [jpds, al-maisan<http://tinyurl.com/yhj8h7l>,salgado] || This channel is logged: http://irclogs.ubuntu.com || https://code.edge.launchpad.net/launchpad/+activereviews | ||
| === noodles775 changed the topic of #launchpad-reviews to: on call: intellectronica || reviewing: - || queue [jpds, al-maisan<http://tinyurl.com/yhj8h7l>,salgado, mthaddon] || This channel is logged: http://irclogs.ubuntu.com || https://code.edge.launchpad.net/launchpad/+activereviews | ||
| noodles775 | Hi intellectronica, can I add mthaddon's branch to the queue? https://code.edge.launchpad.net/~mthaddon/launchpad/buildjob-nagios-check-perms/+merge/16532 | 13:43 |
| === jtv1 is now known as jtv | ||
| intellectronica | noodles775: sure | 14:18 |
| === salgado_ is now known as salgado | ||
| intellectronica | al-maisan: can you please add a comment before the then-block on line 33 of the diff? just to distinguish it from the condition | 14:21 |
| al-maisan | intellectronica: sure, no problem. | 14:21 |
| intellectronica | al-maisan: cool. r=me, everything else looks flawless | 14:21 |
| al-maisan | intellectronica: thank you very much! | 14:22 |
| === intellectronica changed the topic of #launchpad-reviews to: on call: intellectronica || reviewing: - || queue [jpds,salgado, mthaddon] || This channel is logged: http://irclogs.ubuntu.com || https://code.edge.launchpad.net/launchpad/+activereviews | ||
| === abentley changed the topic of #launchpad-reviews to: on call: intellectronica, abentley || reviewing: -,- || queue [jpds,salgado, mthaddon] || This channel is logged: http://irclogs.ubuntu.com || https://code.edge.launchpad.net/launchpad/+activereviews | ||
| abentley | jpds: Does your change break any existing pagetests? | 14:38 |
| jpds | abentley: No. | 14:40 |
| jpds | abentley: They change the message with ... . | 14:41 |
| jpds | abentley: check* | 14:41 |
| abentley | jpds: In that case, what would you think about adding a pagetest that ensures the correct text and link is used? | 14:44 |
| abentley | jpds: I see that the bug is not triaged. Have you discussed your change with anyone on the bugs team? | 14:45 |
| jpds | abentley: No, I filed it during the holidays. | 14:46 |
| abentley | deryck: jpds has a branch fixing bug #499997, which is untriaged. Do you agree it should be fixed? | 14:47 |
| mup | Bug #499997: Duplicate warning box should link to duplicate bug report <Launchpad Bugs:In Progress by jpds> <https://launchpad.net/bugs/499997> | 14:47 |
| * deryck looks | 14:48 | |
| deryck | abentley, yup, a link would be nice. I'll mark it accordingly triaged. | 14:50 |
| abentley | deryck: thanks. | 14:50 |
| abentley | jpds: What would you think about adding a pagetest that ensures the correct text and link is used? | 14:52 |
| jpds | abentley: I'm looking for the test now to add that. | 14:53 |
| === salgado is now known as salgado-afk | ||
| === salgado-afk is now known as salgado-lunch | ||
| jpds | abentley: Pushed revision with link test. | 15:02 |
| abentley | jpds: Thanks! Looking... | 15:06 |
| noodles775 | sinzui: I've just added you as a ui reviewer for jpds' branch - I hope that's ok. | 15:08 |
| abentley | jpds: r=me. Has this branch passed the full test suite? | 15:08 |
| jpds | abentley: For bugs, yes. That took a while to run... | 15:08 |
| sinzui | noodles775: yes | 15:08 |
| abentley | jpds: When it has passed the full test suite, I can land it for you. We don't land anything that hasn't passed the full test suite. | 15:09 |
| === matsubara is now known as matsubara-lunch | ||
| === abentley changed the topic of #launchpad-reviews to: on call: intellectronica, abentley || reviewing: -,- || queue [salgado, mthaddon] || This channel is logged: http://irclogs.ubuntu.com || https://code.edge.launchpad.net/launchpad/+activereviews | ||
| abentley | salgado-lunch: r=me | 15:20 |
| === abentley changed the topic of #launchpad-reviews to: on call: intellectronica, abentley || reviewing: -, mthaddon || queue [] || This channel is logged: http://irclogs.ubuntu.com || https://code.edge.launchpad.net/launchpad/+activereviews | ||
| === intellectronica changed the topic of #launchpad-reviews to: on call: intellectronica, abentley || reviewing: salgado, mthaddon || queue [] || This channel is logged: http://irclogs.ubuntu.com || https://code.edge.launchpad.net/launchpad/+activereviews | ||
| abentley | intellectronica: I just reviewed that. | 15:21 |
| intellectronica | ah, cool | 15:22 |
| === intellectronica changed the topic of #launchpad-reviews to: on call: intellectronica, abentley || reviewing: -, mthaddon || queue [] || This channel is logged: http://irclogs.ubuntu.com || https://code.edge.launchpad.net/launchpad/+activereviews | ||
| === matsubara-lunch is now known as matsubara | ||
| === beuno is now known as beuno-lunch | ||
| === abentley changed the topic of #launchpad-reviews to: on call: intellectronica, abentley || reviewing: -, - || queue [] || This channel is logged: http://irclogs.ubuntu.com || https://code.edge.launchpad.net/launchpad/+activereviews | ||
| === salgado-lunch is now known as salgado | ||
| === abentley1 is now known as abentley | ||
| === abentley1 is now known as abentley | ||
| === abentley1 is now known as abentley | ||
| === abentley1 is now known as abentley | ||
| === abentley1 is now known as abentley | ||
| === abentley1 is now known as abentley | ||
| === beuno-lunch is now known as beuno | ||
| === kfogel is now known as kfogel-lunch | ||
| === mrevell-lunch is now known as mrevell | ||
| === intellectronica changed the topic of #launchpad-reviews to: on call: abentley || reviewing: - || queue [] || This channel is logged: http://irclogs.ubuntu.com || https://code.edge.launchpad.net/launchpad/+activereviews | ||
| === deryck is now known as deryck[lunch] | ||
| === gary_poster is now known as gary-afk | ||
| === deryck[lunch] is now known as deryck | ||
| EdwinGrubbs | abentley: can you review a branch for me? https://code.edge.launchpad.net/~edwin-grubbs/launchpad/bug-482176-add-team-member-ajax-part3/+merge/16804 | 19:43 |
| abentley | EdwinGrubbs: Sure, just a minute. | 19:43 |
| === abentley changed the topic of #launchpad-reviews to: on call: abentley || reviewing: - || queue [EdwinGrubbs] || This channel is logged: http://irclogs.ubuntu.com || https://code.edge.launchpad.net/launchpad/+activereviews | ||
| === abentley changed the topic of #launchpad-reviews to: on call: abentley || reviewing: EdwinGrubbs || queue [] || This channel is logged: http://irclogs.ubuntu.com || https://code.edge.launchpad.net/launchpad/+activereviews | ||
| abentley | EdwinGrubbs: What is the new behavior for addMember? | 19:47 |
| abentley | EdwinGrubbs: It is a bit strange to see the names of James Blackwell and Jeff Waugh. They are former Canonical people, so I'd expect to see them only in old tests. | 19:54 |
| abentley | What would you think about getting foo.bar by email rather than 'name16'? | 19:56 |
| EdwinGrubbs | abentley: I'm fine with switching it to get foobar by email rather than name16. | 19:56 |
| abentley | EdwinGrubbs: I think that would make it a bit easier to understand. | 19:56 |
| EdwinGrubbs | abentley: addMember() now takes into account whether the user is an admin of both teams. If they are, there is no reason to make them go through the invite/accept-invitation dance. Just add the member as active. | 19:57 |
| abentley | EdwinGrubbs: Cool. | 19:58 |
| abentley | EdwinGrubbs: The behaviour of inTeam is not what I would expect. | 19:59 |
| EdwinGrubbs | abentley: Well, I decided to use James Blackwell, etc, since I'm trying to avoid a part4 of this branch. | 19:59 |
| abentley | EdwinGrubbs: I would expect that (x.inTeam(y)) == (y in x.allmembers) | 20:01 |
| abentley | EdwinGrubbs: Sorry, that should be (y.inTeam(x)) == (y in x.allmembers) | 20:01 |
| EdwinGrubbs | abentley: can you tell me in which file you are looking at inTeam()? | 20:04 |
| abentley | EdwinGrubbs: I am looking at your diff for lib/lp/registry/doc/teammembership.txt | 20:05 |
| abentley | EdwinGrubbs: http://pastebin.ubuntu.com/351412/ | 20:06 |
| EdwinGrubbs | abentley: yes, that is just the bizarro behavior, where the owner of a team can leave the team but still be the owner. I don't know why we let that happen to begin with, but that is too big of an issue to tackle here. | 20:10 |
| EdwinGrubbs | abentley: inTeam() is used by the security layer, so that's why they don't have it match currently. | 20:11 |
| abentley | EdwinGrubbs: It's the behaviour of inTeam in that circumstance that's surprising, not so much the fact that the circumstance can arise. inTeam is used by our security stuff, too. We may have to reconsider this. | 20:13 |
| leonardr | salgado, i made a change to my anonymous-oauth branch back on december 16th. can you take a quick look at it and change your vote from 'needs fixing' so i can land it? | 20:13 |
| salgado | leonardr, will do. do you have the URL of the m-p? btw, did you add a comment to the m-p after you made the changes? I rely heavily on email to do reviews, so commenting on the m-p when there's changes made to the branch is a good idea | 20:18 |
| leonardr | salgado: https://code.edge.launchpad.net/~leonardr/launchpad/anonymous-oauth/+merge/16199 | 20:18 |
| leonardr | i didn't leave a comment. i probably got the branch cleared with gary since i remember him agreeing to land it for me, but since that was a long time ago i want to finish it properly | 20:18 |
| salgado | leonardr, do you have an incremental diff? I really don't want to go through the whole thing all over again | 20:20 |
| leonardr | salgado: check out revisions 10558 and 10559... | 20:20 |
| leonardr | hm, launchpad isn't cooperating | 20:21 |
| leonardr | salgado: here you go | 20:22 |
| leonardr | https://pastebin.canonical.com/26172/ | 20:22 |
| salgado | thanks leonardr | 20:23 |
| === matsubara is now known as matsubara-afk | ||
| salgado | leonardr, just updated the m-p; r=me | 20:29 |
| === salgado is now known as salgado-afk | ||
| abentley | EdwinGrubbs: Is the doc correct? It looks inverted: http://pastebin.ubuntu.com/351422/ | 20:40 |
| bac | hi abentley - can you review another? https://code.edge.launchpad.net/~bac/launchpad/bug-498179/+merge/16806 | 20:41 |
| EdwinGrubbs | abentley: The doc is incorrect. Thanks for catching that. | 20:41 |
| abentley | bac: I will try, but the one I'm currently reviewing is large. | 20:41 |
| === abentley changed the topic of #launchpad-reviews to: on call: abentley || reviewing: EdwinGrubbs || queue [bac] || This channel is logged: http://irclogs.ubuntu.com || https://code.edge.launchpad.net/launchpad/+activereviews | ||
| bac | abentley: ok. trying is good. | 20:42 |
| EdwinGrubbs | bac: I can review that one for you. | 20:45 |
| bac | EdwinGrubbs: hey, that's great | 20:45 |
| abentley | EdwinGrubbs: Wow, doctests sure punish us for adding useful return values! | 20:47 |
| === abentley changed the topic of #launchpad-reviews to: on call: abentley || reviewing: - || queue [] || This channel is logged: http://irclogs.ubuntu.com || https://code.edge.launchpad.net/launchpad/+activereviews | ||
| abentley | EdwinGrubbs: r=me | 20:51 |
| EdwinGrubbs | abentley: thanks alot | 20:51 |
| EdwinGrubbs | abentley: I started reviewing Brad's branch, in case you didn't see that message. | 20:51 |
| abentley | EdwinGrubbs: I did, thanks. | 20:52 |
| EdwinGrubbs | bac: What is the purpose of adding the pub_member to the pub_team? It appears to have no effect on the test. | 21:00 |
| bac | EdwinGrubbs: likely a mistake | 21:00 |
| bac | EdwinGrubbs: oh, i was going to show that even though pub_owner could see private parts the mere pub_member could not | 21:01 |
| bac | EdwinGrubbs: i think that's still a good thing to do, i just forgot to do it | 21:01 |
| bac | EdwinGrubbs: this is what i intended: http://pastebin.ubuntu.com/351429/ | 21:05 |
| EdwinGrubbs | that looks good | 21:06 |
| EdwinGrubbs | bac: I was thinking about the test to see if the user is in getDirectAdministrators(), and it seems like it would cause a problem for teams that are owned by another team. Therefore, you should probably use can_edit_team(invited_team, user) | 21:30 |
| bac | EdwinGrubbs: good idea, i think | 21:32 |
| EdwinGrubbs | bac: review sent | 21:37 |
| bac | EdwinGrubbs: thanks | 21:37 |
| bac | EdwinGrubbs: what do you think about this instead? http://pastebin.ubuntu.com/351446 it avoids some redundant checking for admins that would happen if i used can_edit_team | 21:41 |
| === abentley changed the topic of #launchpad-reviews to: on call: - || reviewing: - || queue [] || This channel is logged: http://irclogs.ubuntu.com || https://code.edge.launchpad.net/launchpad/+activereviews | ||
| === EdwinGrubbs is now known as Edwin-afk | ||
Generated by irclog2html.py 2.7 by Marius Gedminas - find it at mg.pov.lt!