/srv/irclogs.ubuntu.com/2010/01/18/#launchpad-reviews.txt

=== abentley1 is now known as abentley
=== abentley1 is now known as abentley
* thumper needs a pretty trivial review: 09:25
thumperhttps://code.edge.launchpad.net/~thumper/launchpad/fix-branch-active-reviews/+merge/17574 please09:25
stubdone09:29
mwhudsonah, racing approvals09:29
thumperta09:37
=== 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
henningeBring out you dead!10:50
henninges/you/your/10:50
adiroibanhenninge: Hi, can we take a look at bug 340662 ?10:53
mupBug #340662: "Change details" for POTemplate should allow maintainers some more freedom <qa-bad> <ui> <Launchpad Translations:Fix Committed by adiroiban> <https://launchpad.net/bugs/340662>10:53
adiroibanand bug 50749810:54
mupBug #507498: AttributeError on potemplate page <oops> <Launchpad Translations:In Progress by henninge> <https://launchpad.net/bugs/507498>10:54
henningeadiroiban: yes, I was looking at that atm.10:54
henningeadiroiban: Hi!10:54
henningeadiroiban: where is the new branch?10:56
adiroibanit is attached to the bug report, lp:~adiroiban/launchpad/bug-340662-take-210:57
henningeoh10:57
henninge:)10:57
adiroibanthere is no MP10:57
adiroibansince I was waiting for some feedback10:58
adiroibanfor the last bug comment10:58
henningeI see11:00
henningeadiroiban: I am reading the original diff atm and will then look at the new branch.11:00
henningeadiroiban: Code reviews are meant to check that the code adheres to coding standards and has good test coverage.11:02
henningeadiroiban: 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
henningehenninge: The reviewer should make sure this has happened.11:03
=== matsubara_ is now known as matsubara
henningeadiroiban: The changes in take-2 look good.11:08
adiroibanhenninge: :) . Then I can consider this a pre-implementation chat and create the  MP for this branch ?11:09
henningeadiroiban: Well, we are already mid-implementation ... ;-)11:09
henningeadiroiban: You will still have to update xx-potemplate-edit.txt.11:09
adiroibanor post?11:09
adiroibanpost-implementation :)11:09
henningeyes, probably ... ;)11:10
henningeactually, 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:10
henningeadiroiban: also, I have not yet completely understood the need for EditPOTemplateSubset insecurity.py11:11
henningeadiroiban: when is it used exactly?11:11
adiroibanto set „is_current” for a template where is_current=false11:12
adiroibanIn this case IPoTemplsteSubset is used, instead of IPOTemplate11:12
henningeadiroiban: where does that happen? on +templates?11:13
adiroibanor this is what I understood from a previous chat with Danilo, regarding the same issue for Ubuntu translation coordinators11:13
adiroiban+edit11:14
henningelet me look at this in detail11:14
adiroibanat the URL traversal11:14
danilosadiroiban, 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 happened11:14
adiroibandanilos: Hi! Don't worry, it was constructive feedback. Still, I've proposed some brainless code, so it is also my fault.11:17
adiroibanhenninge: at browser/potemplate.py11:17
adiroibanPOTemplateSubsetNavigation11:17
adiroibanaround line 71511:17
henningeadiroiban: Oh, it's for the navigation11:17
henningeAhhhh, now I get it! ;-)11:18
adiroibanthere is that code that I don't fully understand why it is there11:18
adiroibanand I was thinking maybe there is a way to use IPotemplate for both enabled and disabled templates11:19
henningeadiroiban: well, the context for traversing to a template is the subset that this template is from.11:20
henningeadiroiban: indicated by the "+pots" in the url11:21
henningeadiroiban: Or are you saying "Why check permissions on the Subset here and not on the template itself?"11:22
adiroibanhenninge: yes11:22
henningecheck_permission('launchpad.Edit', potemplate)11:22
henningelike that?11:22
henningeadiroiban: Did you say that danilo had a reason for it and you didn't understand it?11:23
adiroibanhenninge: I was hoping to see the permission checking in browser/configure.zcml for +edit for IPOTemplate. I'm still reading about zope adapters11:23
adiroibanhenninge: 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 it11:24
danilosadiroiban, 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
henningelol11:24
danilosadiroiban, i.e. what didn't work before and what did change with the introduction of EditPOTemplateSubset permission?11:25
adiroibandanilos: with the current permission, a person could not re enable the template after it was disabled11:25
danilosadiroiban, i.e. who wasn't able to access the +edit form and now is, an who was and now isn't?11:25
adiroiban+edit is not available if is_current=false11:26
danilosadiroiban, right, so how is that implemented?11:27
adiroibanHenning asked me the same question and I tried to answer him11:27
adiroibanbrowser/potemplate.py, POTemplateSubsetNavigation, around line 71511:27
adiroibanthe „chain” is broken in URL traversal for IPOTempalteSubset11:28
danilosadiroiban, at least EditPOTemplateDetails doesn't have the limitation11:29
danilosadiroiban, well, then that should be changed11:30
danilosadiroiban, i.e. simply use what henninge suggested above (check for 'launchpad.Edit' on potemplate object instead of on context)11:30
danilosadiroiban, (sorry for not re-reading the entire discussion)11:31
adiroibandanilos: np.11:31
adiroibanhenninge, danilos: thanks for the explanation11:31
henningethat would make the new permission EditPOTemplateSubset superflouus11:32
danilosadiroiban, the reason traverse() in there is like that is so we'd give a 404 instead of 'permission denied' for "deleted" templates11:32
daniloshenninge, exactly11:32
henningeand possibly AdminPOTemplateSubset, too? I cannot imagine it'd be used anywhere else.11:32
adiroibanyep11:32
adiroibanhenninge: yes. I will also remove the AdminPOTemplateSubset11:33
danilosadiroiban, henninge: however, this will break the privilege for UTCs, because now they won't be able to traverse to it anymore for Ubuntu templates11:33
danilosadiroiban, henninge: so, EditPOTemplateDetails would have to be extended to check for UTCs like other new permissions do11:34
adiroibandanilos: but UTCs should have launchpad.Edit for +edit on IPoTemplate11:34
danilosadiroiban, 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
adiroibandanilos: ok. so now, I will just remove EditPOTemplateSubset11:34
danilosadiroiban, ah, right, it extends AdminPOTemplateDetails, so it does, it's all good11:35
adiroibanand fix the other problem in another branch11:35
danilosadiroiban, 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 up11:36
daniloss/do not/to not/11:36
adiroibandanilos: no. the other problem is removing AdminPOTemplateSubset and checking UTCs rights11:36
danilosadiroiban, ah, right, good call to keep that a separate branch11:37
henningeadiroiban: I think you can still remove AdminPOTemplateSubset.11:37
henningeadiroiban: as danilo pointed out, the UTC rights are also checked in AdminPOTemplateDetails.11:37
henninge;-)11:38
daniloshenninge, 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
henningeright11:38
henningeadiroiban: all questions solved? ;-)11:39
adiroibanhenninge: nope :D11:39
henningeadiroiban: go ahead! :)11:40
adiroibanI have some (general) questions regarding writing tests11:40
henningeok, good.11:40
adiroibanso „stories” are functional tests and we only write stories for the most common use cases11:41
henningeyes, and they are page tests.11:42
adiroibanthen there are browser/tests and they are also functional or unit tests?11:43
adiroibanwhy do we use „private” methods in browser/tests ?11:43
henningebecause the tests are bad? ;)11:44
henningei.e. test and browser code should not import from model code at all - public or private code11:44
henningeideally.11:44
adiroibanand there are also doc and tests11:45
henningeadiroiban: sorry, missed a point earlier11:46
henningebrowser/tests should be view tests that perform a unit test on the view code.11:46
henningedoc tests should be mainly to document model code, just like stories document browser code (i.e. the whole page911:47
adiroibanok. stories are like „smoke testings”11:47
adiroibanand browser/tests are unit testing for the view11:47
henningeright11:47
henningeand tests are unit tests for the model code.11:47
adiroibanhenninge: thanks!11:48
adiroibanhenninge: can I add this info to https://dev.launchpad.net/Testing ?11:49
henningeadiroiban: oh yeah, that page i awfully short...11:49
henningeis11:49
henningeadiroiban: just remember that you will find a lot of code that violates these conventions, espcially older code.11:53
adiroibanhenninge: I will keep that in mind :)12:02
adiroibanso in LP parlance, „smoke tests” are functional tests ?12:02
adiroibanor how do you call the type of tests from stories and doc ?12:03
henningeadiroiban: well, I guess that is the most common error that we have in the tree.12:07
henningea lot of doc tests are used as functional tests when they really should be unit tests.12:08
henningeI think.12:08
henninge;)12:08
stubhenninge: 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:57
stubhttps://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.12:58
=== 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
jamaltaHi, 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:49
adiroibanjamalta: yes14:51
jamaltaadiroiban: thanks14:52
adiroibanjamalta: just make sure you had a pre-implementation chat14:52
jamaltaadiroiban: i had discussed the bug with sinzui last week :)14:53
adiroibanjamalta: great, than you should be ready for the MP :)14:54
jamaltaadiroiban: just finishing up the MP description, will be ready in a few minutes14:55
jamaltaok i think i'm ready for a review, it's a trivial bug so it shouldn't take long14:59
jamaltahttps://code.edge.launchpad.net/~jamalta/launchpad/changesfile-253525/+merge/1758714:59
adiroibanjamalta: Henning will review it. To signal that you are waiting for an review, add your irc nick in to topic (ex queue [jamalta])15:02
=== matsubara is now known as matsubara-lunch
jamaltaadiroiban: alright15:03
=== 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
jamaltaadiroiban: thanks :)15:04
henningejamalta: Hi! Looking at it now.15:30
=== 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
jamaltahenninge: thanks15:31
henningejamalta: do you know the "extract_text" function?15:36
jamaltahenninge: no15:36
henningejamalta: Please have a look at lib/canonical/launchpad/doc/pagetest_helpers.txt to read about it.15:36
jamaltahenninge: alright15:37
henningejamalta: 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:39
jamaltahenninge: sounds good, i'll do that15:40
henningejamalta: cool, thank you. I see no other issues with the branch.15:42
jamaltahenninge: awesome15:42
jamaltahenninge: 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
jamaltathe test seems to succeed that way, just wanted to be sure15:46
henningejamalta: exactly!15:46
henningejamalta: btw, looks like no bonus points to get in that file ... ;(15:47
jamaltahenninge: what do you mean?15:48
jamaltahenninge: also, i can't really change this to use extract_text http://paste.ubuntu.com/358554/15:48
jamaltaconsidering it is testing the tags themselve15:48
henningejamalta: Yes, I was just looking at that.15:48
henningejamalta: what you could do is use BeautifulSoup to extract the tag and its href attribute.15:49
henningejamalta: do you know about BeautifulSoup?15:49
* jamalta goes digging for BeautifulSoup docs15:50
jamaltano15:50
henningehttp://www.crummy.com/software/BeautifulSoup/documentation.html15:50
henningejamalta: pagetest_helpers builds on top of BeautifulSoup, so you have all that available.15:50
jamaltahenninge: guess i should use BeautifulSoup for this test as well, http://paste.ubuntu.com/358558/15:53
jamaltaconsidering it isn't testing the content of publishing_history, but that a link exists with that content15:53
jamaltaand target15:53
henningejamalta: yes, that would be good, too.15:54
henningejamalta: two things to note:15:54
henninge#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
henningethat makes for very unreadable test failures.15:55
henningealso, 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
jamaltahenninge: not at all, that makes complete sense15:56
henninge#2 On the other had, doing too heavy BeautifulSouping can make you too dependent on the structure, too.15:57
henninges/had/hand/15:57
jamaltathe test should only check that the a tag is there, pointing at the right thing, with the right string. correct?15:57
jamaltahenninge: that makes sense15:57
henningeyes, if you can pick out the tag, that's best.15:57
henningejamalta: it is ok to add id's or classes just to be able to pick something out for a test.15:58
henningeonly use id's when you are sure the element can not occur multiple times on the page.15:59
jamaltahenninge: alright, that can makes things easier15:59
=== salgado-lunch is now known as salgado
henningejamalta: so there are bonus points in this file! ;-)16:03
henningebig ones, too.16:03
jamaltahenninge: hm.. i'll see if i can find them all then :) heh16:03
henningejamalta: Hey, I think you identified the two cases with those <a>-tags.16:04
henningejamalta: there is an example of handling such a case in that file, btw. Line 252.16:07
jamaltaso browser.getLink is using BeautifulSoup then?16:09
jamaltaok i think i'm getting the hang of this16:12
jamaltathe only bottleneck is how long it takes to run a test  >< Lol16:12
=== matsubara-lunch is now known as matsubara
jamaltathat' sodd16:20
jamaltai'm using browser.getLink().url16:20
jamaltathe first time it just printed the url16:20
jamaltabut on a different test, the url was inside single quotes16:20
henningejamalta: that should be because it is "print"ed in the one case.16:24
henningejamalta: using "print" to test a value is the preferred way, btw.16:24
jamaltahenninge: oh, right, ok16:27
jamaltai'll fix that16:27
jamaltashould i change others that don't use print?16:29
jamaltahenninge: ok, i have resubmitted the proposal16:47
=== henninge_ is now known as henninge
henningejamalta: cool, I will look at it in a minute16:50
jamaltahenninge: thanks, whenever you have time16:50
henningejamalta: I'll be leaving soon, so there is not much time .... ;-)16:51
jamaltahenninge: uh oh, heh16:51
jamaltai guess not!16:51
henningestill here16:53
jamaltahenninge: hmm?16:53
henningejamalta: you can just leave the state of the mp in "Needs review", btw.16:54
henningeoh, it's a new one!16:54
jamaltahenninge: i clicked "resubmit proposal"16:54
jamaltathat is what i was instructed to do before16:54
henningejamalta: ah, I see.16:54
henningereally? I have never used that ...16:54
henningenp16:55
jamaltadoes the MP update itself if i push to the branch?16:55
henningejamalta: yes16:55
jamaltaoh it does16:55
jamaltai didn't know that :)16:55
jamaltaok, next time i won't do that.. sorry16:55
jamaltai can't really remember who told me to do it that way, it was quite a while ago16:56
henningejamalta: Cool, I forgot about getLink! BIG BONUS POINTS ;)16:57
jamaltahenninge: hehe, that's the best example i could find on how to do that16:58
jamaltahenninge: thanks16:58
henningejamalta: yes, that is the best way to do it!16:58
henningejamalta: so, the only thing is that I think the change in line 129 is too much.16:59
henningethe text explains that the emailaddress is linkified to the user's LP home page and the test should verify that.16:59
jamaltahenninge: alright17:00
henningeyou could add a "..." to omit the classes, though, as they are not important for the test.17:00
jamaltaso i should be testing that against the html?17:00
henningejamalta: it is already drilled down to the <a>-tag, so that is fine.17:01
jamaltai could change that to use getLink instead17:01
jamaltathat way i can do it the same way as the other tests17:01
henningeoh, that is a good idea, too17:01
henningegetLink on the email address17:01
jamaltahenninge: right17:01
henningesplendid17:01
jamaltamaking sure what i did works, and i'll push it up17:02
henningeplease do, no new mp needed17:02
jamaltahenninge: of course ;).. won't do that again17:02
henningejamalta: what we usually do is add a comment with an incremental diff.17:02
jamaltahenninge: ah.. makes sense17:03
jamaltahenninge: ok posted it17:04
henningejamalta: please also add a commit message and I'll land it for you.17:05
jamaltahenninge: hm. by incremental diff, do you mean a diff of the things changed for that update, or a special kind of diff?17:05
henninge"Set commit message"17:05
henningeYes, just a diff of the last changes.17:05
jamaltathat would be for the whole branch, not just what i did, right?17:05
henningeno just what you did17:05
jamaltaok17:05
=== 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
henningejamalta: ah yes, that's what I meant ;-)17:06
jamaltahenninge: done17:06
henningeadiroiban: I am sorry, but I am done for the day ...17:06
adiroibanhenninge: don't worry17:06
=== 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
adiroibanI just added the MP in the queue :)17:07
adiroibanit's not the end of the world 17:07
henningeadiroiban: ;-)17:08
henningeadiroiban: are you around tomorrow? We need to talk about the UDW session!17:08
adiroibanhenninge: yep. I'm around17:09
adiroibanI have uploaded the virtual machine17:09
adiroibanand I was planning for tonigh to write a quick blog post 17:09
adiroibanand asking for some feedback before the session17:09
henningecool, good idea17:10
adiroibanlike give a homework17:10
adiroibanand discuss it durring the session17:10
henningeadiroiban: ok, let's talk about that tomorrow17:10
adiroibanbut I will just gather my thoughts and tomorrow we shall see17:11
adiroibanyep17:11
jamaltahenninge: thanks for everything :)17:13
henningea pleasure17:14
=== henninge is now known as henninge-afk
jamaltaHi there, I wanted to see if I could take a stab at bug #49864319:25
mupBug #498643: 'destination ppa' dropdown should show the url too <trivial> <ui> <Soyuz:Triaged> <https://launchpad.net/bugs/498643>19:25
jamaltaSo I just need someone to have a pre-implementation chat with :)19:25
=== matsubara is now known as matsubara-afk
=== salgado is now known as salgado-afk
=== mup_ is now known as mup
deryckintellectronica, https://code.edge.launchpad.net/~deryck/launchpad/no-lp-bugs-homepage-439245/+merge/1763422:54
* kfogel is away: afk for some hours; see you antipodal people later probably23:58

Generated by irclog2html.py 2.7 by Marius Gedminas - find it at mg.pov.lt!