=== abentley1 is now known as abentley === abentley1 is now known as abentley [09:25] * thumper needs a pretty trivial review: [09:25] https://code.edge.launchpad.net/~thumper/launchpad/fix-branch-active-reviews/+merge/17574 please [09:29] done [09:29] ah, racing approvals [09:37] ta === abentley1 is now known as abentley === henninge changed the topic of #launchpad-reviews to: on-call: henninge || reviewing: - || queue [] || This channel is logged: http://irclogs.ubuntu.com || https://code.edge.launchpad.net/launchpad/+activereviews [10:50] Bring out you dead! [10:50] s/you/your/ [10:53] henninge: Hi, can we take a look at bug 340662 ? [10:53] Bug #340662: "Change details" for POTemplate should allow maintainers some more freedom [10:54] and bug 507498 [10:54] Bug #507498: AttributeError on potemplate page [10:54] adiroiban: yes, I was looking at that atm. [10:54] adiroiban: Hi! [10:56] adiroiban: where is the new branch? [10:57] it is attached to the bug report, lp:~adiroiban/launchpad/bug-340662-take-2 [10:57] oh [10:57] :) [10:57] there is no MP [10:58] since I was waiting for some feedback [10:58] for the last bug comment [11:00] I see [11:00] adiroiban: I am reading the original diff atm and will then look at the new branch. [11:02] adiroiban: Code reviews are meant to check that the code adheres to coding standards and has good test coverage. [11:03] adiroiban: For a discussion on how to fix the bug properly you need to have a pre-implementation call or chat with some-one who knows the subject. [11:03] henninge: The reviewer should make sure this has happened. === matsubara_ is now known as matsubara [11:08] adiroiban: The changes in take-2 look good. [11:09] henninge: :) . Then I can consider this a pre-implementation chat and create the MP for this branch ? [11:09] adiroiban: Well, we are already mid-implementation ... ;-) [11:09] adiroiban: You will still have to update xx-potemplate-edit.txt. [11:09] or post? [11:09] post-implementation :) [11:10] yes, probably ... ;) [11:10] actually, mid-implmentation is not that uncommon because often you will start a branch by "trying out" what might help and then discuss your findings for a proper fix. [11:11] adiroiban: also, I have not yet completely understood the need for EditPOTemplateSubset insecurity.py [11:11] adiroiban: when is it used exactly? [11:12] to set „is_current” for a template where is_current=false [11:12] In this case IPoTemplsteSubset is used, instead of IPOTemplate [11:13] adiroiban: where does that happen? on +templates? [11:13] or this is what I understood from a previous chat with Danilo, regarding the same issue for Ubuntu translation coordinators [11:14] +edit [11:14] let me look at this in detail [11:14] at the URL traversal [11:14] adiroiban, oh, hi :) I hope you don't take the comments I made too harsh, it was mostly about the problem with the review, since review should ensure pre-imp chat has happened [11:17] danilos: Hi! Don't worry, it was constructive feedback. Still, I've proposed some brainless code, so it is also my fault. [11:17] henninge: at browser/potemplate.py [11:17] POTemplateSubsetNavigation [11:17] around line 715 [11:17] adiroiban: Oh, it's for the navigation [11:18] Ahhhh, now I get it! ;-) [11:18] there is that code that I don't fully understand why it is there [11:19] and I was thinking maybe there is a way to use IPotemplate for both enabled and disabled templates [11:20] adiroiban: well, the context for traversing to a template is the subset that this template is from. [11:21] adiroiban: indicated by the "+pots" in the url [11:22] adiroiban: Or are you saying "Why check permissions on the Subset here and not on the template itself?" [11:22] henninge: yes [11:22] check_permission('launchpad.Edit', potemplate) [11:22] like that? [11:23] adiroiban: Did you say that danilo had a reason for it and you didn't understand it? [11:23] henninge: I was hoping to see the permission checking in browser/configure.zcml for +edit for IPOTemplate. I'm still reading about zope adapters [11:24] henninge: well, Danilo told me that code is not really ok... but while there is no branch to come with a better implemention we will live with it [11:24] adiroiban, the first question to ask is: what exactly was wrong with existing permissions? (some things are assumed for launchpad.Edit permission, so you don't have to write a specific implementation for everything) [11:24] lol [11:25] adiroiban, i.e. what didn't work before and what did change with the introduction of EditPOTemplateSubset permission? [11:25] danilos: with the current permission, a person could not re enable the template after it was disabled [11:25] adiroiban, i.e. who wasn't able to access the +edit form and now is, an who was and now isn't? [11:26] +edit is not available if is_current=false [11:27] adiroiban, right, so how is that implemented? [11:27] Henning asked me the same question and I tried to answer him [11:27] browser/potemplate.py, POTemplateSubsetNavigation, around line 715 [11:28] the „chain” is broken in URL traversal for IPOTempalteSubset [11:29] adiroiban, at least EditPOTemplateDetails doesn't have the limitation [11:30] adiroiban, well, then that should be changed [11:30] adiroiban, i.e. simply use what henninge suggested above (check for 'launchpad.Edit' on potemplate object instead of on context) [11:31] adiroiban, (sorry for not re-reading the entire discussion) [11:31] danilos: np. [11:31] henninge, danilos: thanks for the explanation [11:32] that would make the new permission EditPOTemplateSubset superflouus [11:32] adiroiban, the reason traverse() in there is like that is so we'd give a 404 instead of 'permission denied' for "deleted" templates [11:32] henninge, exactly [11:32] and possibly AdminPOTemplateSubset, too? I cannot imagine it'd be used anywhere else. [11:32] yep [11:33] henninge: yes. I will also remove the AdminPOTemplateSubset [11:33] adiroiban, henninge: however, this will break the privilege for UTCs, because now they won't be able to traverse to it anymore for Ubuntu templates [11:34] adiroiban, henninge: so, EditPOTemplateDetails would have to be extended to check for UTCs like other new permissions do [11:34] danilos: but UTCs should have launchpad.Edit for +edit on IPoTemplate [11:34] adiroiban, they should, I am not sure they do (i.e. my reading of EditPOTemplateDetails suggests they don't; if it's ok, even better :) [11:34] danilos: ok. so now, I will just remove EditPOTemplateSubset [11:35] adiroiban, ah, right, it extends AdminPOTemplateDetails, so it does, it's all good [11:35] and fix the other problem in another branch [11:36] adiroiban, what other problem? "name" being exposed? fwiw, I consider that one more important, because it can lead to inconsistent data in the DB, and we can't guarantee that project maintainers understand things about Launchpad templates like UTCs or rosetta-admins do not to mess it up [11:36] s/do not/to not/ [11:36] danilos: no. the other problem is removing AdminPOTemplateSubset and checking UTCs rights [11:37] adiroiban, ah, right, good call to keep that a separate branch [11:37] adiroiban: I think you can still remove AdminPOTemplateSubset. [11:37] adiroiban: as danilo pointed out, the UTC rights are also checked in AdminPOTemplateDetails. [11:38] ;-) [11:38] henninge, I agree, but let's keep it a separate branch so we fix the most immediate problems first, and then worry about cleanup (or, if it's really that simple, let's just remove it, but we are close to the release week so I would like us to be extra careful with QA) [11:38] right [11:39] adiroiban: all questions solved? ;-) [11:39] henninge: nope :D [11:40] adiroiban: go ahead! :) [11:40] I have some (general) questions regarding writing tests [11:40] ok, good. [11:41] so „stories” are functional tests and we only write stories for the most common use cases [11:42] yes, and they are page tests. [11:43] then there are browser/tests and they are also functional or unit tests? [11:43] why do we use „private” methods in browser/tests ? [11:44] because the tests are bad? ;) [11:44] i.e. test and browser code should not import from model code at all - public or private code [11:44] ideally. [11:45] and there are also doc and tests [11:46] adiroiban: sorry, missed a point earlier [11:46] browser/tests should be view tests that perform a unit test on the view code. [11:47] doc tests should be mainly to document model code, just like stories document browser code (i.e. the whole page9 [11:47] ok. stories are like „smoke testings” [11:47] and browser/tests are unit testing for the view [11:47] right [11:47] and tests are unit tests for the model code. [11:48] henninge: thanks! [11:49] henninge: can I add this info to https://dev.launchpad.net/Testing ? [11:49] adiroiban: oh yeah, that page i awfully short... [11:49] is [11:53] adiroiban: just remember that you will find a lot of code that violates these conventions, espcially older code. [12:02] henninge: I will keep that in mind :) [12:02] so in LP parlance, „smoke tests” are functional tests ? [12:03] or how do you call the type of tests from stories and doc ? [12:07] adiroiban: well, I guess that is the most common error that we have in the tree. [12:08] a lot of doc tests are used as functional tests when they really should be unit tests. [12:08] I think. [12:08] ;) [12:57] henninge: https://code.edge.launchpad.net/~stub/launchpad/replication/+merge/17454 is a possible cherry pick (I guess depending if the failure mode hits us again) [12:58] https://code.edge.launchpad.net/~stub/launchpad/bug-504291-disconnection-errors/+merge/17371 is a bug fix - I doubt it bites enough for CP but that is for QA to decide. === mrevell is now known as mrevell-lunch === gary_poster_ is now known as gary_poster === gary_poster_ is now known as gary_poster === salgado is now known as salgado-lunch [14:49] Hi, I'm getting ready to create a merge proposal for a branch, and I'm wondering where exactly I put the cover letter. Would that be the description of the merge proposal? [14:51] jamalta: yes [14:52] adiroiban: thanks [14:52] jamalta: just make sure you had a pre-implementation chat [14:53] adiroiban: i had discussed the bug with sinzui last week :) [14:54] jamalta: great, than you should be ready for the MP :) [14:55] adiroiban: just finishing up the MP description, will be ready in a few minutes [14:59] ok i think i'm ready for a review, it's a trivial bug so it shouldn't take long [14:59] https://code.edge.launchpad.net/~jamalta/launchpad/changesfile-253525/+merge/17587 [15:02] jamalta: Henning will review it. To signal that you are waiting for an review, add your irc nick in to topic (ex queue [jamalta]) === matsubara is now known as matsubara-lunch [15:03] adiroiban: alright === jamalta changed the topic of #launchpad-reviews to: on-call: henninge || reviewing: - || queue [] || This channel is logged: http://irclogs.ubuntu.com || https://code.edge.launchpad.net/launchpad/+activereviews === jamalta changed the topic of #launchpad-reviews to: on-call: henninge || reviewing: - || queue [jamalta] || This channel is logged: http://irclogs.ubuntu.com ||https://code.edge.launchpad.net/launchpad/+activereviews [15:04] adiroiban: thanks :) [15:30] jamalta: Hi! Looking at it now. === henninge changed the topic of #launchpad-reviews to: on-call: henninge || reviewing: jamalta || queue [] || This channel is logged: http://irclogs.ubuntu.com ||https://code.edge.launchpad.net/launchpad/+activereviews [15:31] henninge: thanks [15:36] jamalta: do you know the "extract_text" function? [15:36] henninge: no [15:36] jamalta: Please have a look at lib/canonical/launchpad/doc/pagetest_helpers.txt to read about it. [15:37] henninge: alright [15:39] jamalta: could you please apply that in xx-distroseries-sources.txt to get rid of the HTML code in the tests? Find all occurances in that file for bonus points. ;) [15:40] henninge: sounds good, i'll do that [15:42] jamalta: cool, thank you. I see no other issues with the branch. [15:42] henninge: awesome [15:46] henninge: so just to make sure i'm doing this right, this is how it should look using extract_text, right? http://paste.ubuntu.com/358552/ [15:46] the test seems to succeed that way, just wanted to be sure [15:46] jamalta: exactly! [15:47] jamalta: btw, looks like no bonus points to get in that file ... ;( [15:48] henninge: what do you mean? [15:48] henninge: also, i can't really change this to use extract_text http://paste.ubuntu.com/358554/ [15:48] considering it is testing the tags themselve [15:48] jamalta: Yes, I was just looking at that. [15:49] jamalta: what you could do is use BeautifulSoup to extract the tag and its href attribute. [15:49] jamalta: do you know about BeautifulSoup? [15:50] * jamalta goes digging for BeautifulSoup docs [15:50] no [15:50] http://www.crummy.com/software/BeautifulSoup/documentation.html [15:50] jamalta: pagetest_helpers builds on top of BeautifulSoup, so you have all that available. [15:53] henninge: guess i should use BeautifulSoup for this test as well, http://paste.ubuntu.com/358558/ [15:53] considering it isn't testing the content of publishing_history, but that a link exists with that content [15:53] and target [15:54] jamalta: yes, that would be good, too. [15:54] jamalta: two things to note: [15:55] #1 Using bs and extract_text is better because using ... on HTML causes the whole of the HTML to appear in the output if the test fails. [15:55] that makes for very unreadable test failures. [15:56] also, what if "publishing_history" will be changed to be an "ul" in the future? It does not really matter for the test, does it? [15:56] henninge: not at all, that makes complete sense [15:57] #2 On the other had, doing too heavy BeautifulSouping can make you too dependent on the structure, too. [15:57] s/had/hand/ [15:57] the test should only check that the a tag is there, pointing at the right thing, with the right string. correct? [15:57] henninge: that makes sense [15:57] yes, if you can pick out the tag, that's best. [15:58] jamalta: it is ok to add id's or classes just to be able to pick something out for a test. [15:59] only use id's when you are sure the element can not occur multiple times on the page. [15:59] henninge: alright, that can makes things easier === salgado-lunch is now known as salgado [16:03] jamalta: so there are bonus points in this file! ;-) [16:03] big ones, too. [16:03] henninge: hm.. i'll see if i can find them all then :) heh [16:04] jamalta: Hey, I think you identified the two cases with those -tags. [16:07] jamalta: there is an example of handling such a case in that file, btw. Line 252. [16:09] so browser.getLink is using BeautifulSoup then? [16:12] ok i think i'm getting the hang of this [16:12] the only bottleneck is how long it takes to run a test >< Lol === matsubara-lunch is now known as matsubara [16:20] that' sodd [16:20] i'm using browser.getLink().url [16:20] the first time it just printed the url [16:20] but on a different test, the url was inside single quotes [16:24] jamalta: that should be because it is "print"ed in the one case. [16:24] jamalta: using "print" to test a value is the preferred way, btw. [16:27] henninge: oh, right, ok [16:27] i'll fix that [16:29] should i change others that don't use print? [16:47] henninge: ok, i have resubmitted the proposal === henninge_ is now known as henninge [16:50] jamalta: cool, I will look at it in a minute [16:50] henninge: thanks, whenever you have time [16:51] jamalta: I'll be leaving soon, so there is not much time .... ;-) [16:51] henninge: uh oh, heh [16:51] i guess not! [16:53] still here [16:53] henninge: hmm? [16:54] jamalta: you can just leave the state of the mp in "Needs review", btw. [16:54] oh, it's a new one! [16:54] henninge: i clicked "resubmit proposal" [16:54] that is what i was instructed to do before [16:54] jamalta: ah, I see. [16:54] really? I have never used that ... [16:55] np [16:55] does the MP update itself if i push to the branch? [16:55] jamalta: yes [16:55] oh it does [16:55] i didn't know that :) [16:55] ok, next time i won't do that.. sorry [16:56] i can't really remember who told me to do it that way, it was quite a while ago [16:57] jamalta: Cool, I forgot about getLink! BIG BONUS POINTS ;) [16:58] henninge: hehe, that's the best example i could find on how to do that [16:58] henninge: thanks [16:58] jamalta: yes, that is the best way to do it! [16:59] jamalta: so, the only thing is that I think the change in line 129 is too much. [16:59] the text explains that the emailaddress is linkified to the user's LP home page and the test should verify that. [17:00] henninge: alright [17:00] you could add a "..." to omit the classes, though, as they are not important for the test. [17:00] so i should be testing that against the html? [17:01] jamalta: it is already drilled down to the -tag, so that is fine. [17:01] i could change that to use getLink instead [17:01] that way i can do it the same way as the other tests [17:01] oh, that is a good idea, too [17:01] getLink on the email address [17:01] henninge: right [17:01] splendid [17:02] making sure what i did works, and i'll push it up [17:02] please do, no new mp needed [17:02] henninge: of course ;).. won't do that again [17:02] jamalta: what we usually do is add a comment with an incremental diff. [17:03] henninge: ah.. makes sense [17:04] henninge: ok posted it [17:05] jamalta: please also add a commit message and I'll land it for you. [17:05] henninge: hm. by incremental diff, do you mean a diff of the things changed for that update, or a special kind of diff? [17:05] "Set commit message" [17:05] Yes, just a diff of the last changes. [17:05] that would be for the whole branch, not just what i did, right? [17:05] no just what you did [17:05] ok === adiroiban changed the topic of #launchpad-reviews to: on-call: henninge || reviewing: jamalta || queue [adiroiban(bug-340662-take-2)] || This channel is logged: http://irclogs.ubuntu.com ||https://code.edge.launchpad.net/launchpad/+activereviews [17:06] jamalta: ah yes, that's what I meant ;-) [17:06] henninge: done [17:06] adiroiban: I am sorry, but I am done for the day ... [17:06] henninge: don't worry === henninge changed the topic of #launchpad-reviews to: on-call: - || reviewing: - || queue [adiroiban(bug-340662-take-2)] || This channel is logged: http://irclogs.ubuntu.com ||https://code.edge.launchpad.net/launchpad/+activereviews [17:07] I just added the MP in the queue :) [17:07] it's not the end of the world [17:08] adiroiban: ;-) [17:08] adiroiban: are you around tomorrow? We need to talk about the UDW session! [17:09] henninge: yep. I'm around [17:09] I have uploaded the virtual machine [17:09] and I was planning for tonigh to write a quick blog post [17:09] and asking for some feedback before the session [17:10] cool, good idea [17:10] like give a homework [17:10] and discuss it durring the session [17:10] adiroiban: ok, let's talk about that tomorrow [17:11] but I will just gather my thoughts and tomorrow we shall see [17:11] yep [17:13] henninge: thanks for everything :) [17:14] a pleasure === henninge is now known as henninge-afk [19:25] Hi there, I wanted to see if I could take a stab at bug #498643 [19:25] Bug #498643: 'destination ppa' dropdown should show the url too [19:25] So I just need someone to have a pre-implementation chat with :) === matsubara is now known as matsubara-afk === salgado is now known as salgado-afk === mup_ is now known as mup [22:54] intellectronica, https://code.edge.launchpad.net/~deryck/launchpad/no-lp-bugs-homepage-439245/+merge/17634 [23:58] * kfogel is away: afk for some hours; see you antipodal people later probably