[12:10] <henninge> adeuring: Hi! Are you ready to do a review? ;)
[12:10] <henninge> It will only be a minute.
[12:10] <henninge> ... until I have the mp ready
[12:11] <adeuring> henninge: hrmmm, if it's not too long. Actually, I'm again struggling with §$"&§&§/ private librarian files...
[12:12] <henninge> adeuring: It's actually a fairly mechanical change, so it should be easy.
[12:12] <adeuring> henninge: ok, where is the MP?
[12:12] <henninge> in a minute, still fighting lint ;)
[12:16] <henninge> adeuring: https://code.launchpad.net/~henninge/launchpad/db-devel-688479-kill-ut-in-tests-1/+merge/43342
[12:17]  * adeuring is looking
[12:34] <adeuring> henninge: overall, the changes look good. But the disabled tests conern me a bit: Couldn't you simply use the old factory method makeTranslationMessage() unitl the new method can be properly used in these tests?
[12:34] <adeuring> s/conern/concern/
[12:52] <henninge> adeuring: yes, I could. OTHOH that bug will be the next to fix after this bug and I want to remove the old factory method in the next branch for this bug.
[12:52] <henninge> adeuring: sorry for being afk, btw.
[12:54] <adeuring> henninge: well, you can fix the factory method first and then change the tests. I am perhaps a bit paranoid, but I remeber a somewhat weird bug in Zope 2 which looked like somebody made a short coffee break while working on a feature but never properly finished that feature aufter the coffee break ;)
[12:55] <henninge> ;-)
[12:55] <henninge> The factory method will not be fixed but removed.
[12:56] <adeuring> henninge: right, what I meant is: I think you should still use the old mehtod until the new one works properly for the currently disabled tests
[12:56] <henninge> adeuring: we keep running into these hen-egg problems with this branch. ;)
[12:57] <henninge> In this case it's not the factory that needs to be fixed but the method under test is still referring to the old model.
[12:58] <henninge> adeuring: but I can do that, if you insist ;-)
[12:58] <henninge> np
[12:58] <henninge> I'll have to keep the old factory around a little longer.
[12:58] <adeuring> henninge: I would feel defeinetly better if all tests stay enabled ;)
[12:59] <henninge> adeuring: It is important that you feel well!
[12:59] <henninge> ;-)
[12:59] <adeuring> ;)
[12:59] <adeuring> henninge: ok, so, r=me
[12:59] <henninge> adeuring: thanks
[14:12] <bac> hi adeuring.  busy today?
[14:13] <adeuring> bac: well, I must admit that I focused more on understanding the cause of a bad branch I committed yesterday...
[14:13] <bac> adeuring: a worthy goal
[14:17] <jcsackett> i've got one to throw on the queue, adeuring and bac: https://code.edge.launchpad.net/~jcsackett/launchpad/membership-status-matters-676494/+merge/43303
[14:17] <bac> jcsackett: i'll take it
[14:17] <jcsackett> bac: it's the stopgap for the TP stuff.
[14:17] <jcsackett> thanks, bac.
[14:43] <bac> jcsackett: your new test runs in 1.27 seconds!  dodged that bullet!
[14:44] <jcsackett> bac: whoo!
[14:44] <jcsackett> bac: it ran in 1.13 on my machine. you need something faster. :-)
[14:46] <bac> hmm
[14:46] <bac> my poor MBP is getting old
[14:46] <bac> jcsackett: one suggestion:  ACTIVE_STATES is now defined in two places.  how's about moving that into interfaces/teammembership and fixing the two places that use their own copy?
[14:47] <jcsackett> bac: good suggestion, i'll do that.
[14:47] <bac> thanks
[14:52] <bac> jcsackett: this branch has been referred to as a 'stopgap' but i think these changes needed to be made regardless of what happens in the cleanup TP method
[14:52] <bac> regardless, r=bac
[14:52] <jcsackett> bac: i think the long term plan is that the query this branch modifies gets replaces by a single way of cleaning up TP.
[14:52] <jcsackett> bac, thanks for the review. :-)
[17:48] <sinzui> bac: do you have time to review https://code.launchpad.net/~sinzui/launchpad/closed-teams-1/+merge/43375
[17:49] <bac> sinzui: of course i do.  i was about to grab some lunch though
[17:49] <sinzui> fab
[17:49] <adeuring> nice weekend everybody
[19:53] <bac> hi sinzui
[19:53] <bac> in your MP, for rule 1 you say
[19:53] <sinzui> hi bac
[19:53] <bac> Rule 1 means a closed team cannot become open if it is a member of
[19:53] <bac>          any team
[19:54] <sinzui> that is wrong
[19:54] <bac> ok
[19:54] <bac> it looks wrong
[19:54] <bac> any (closed) team?
[19:55] <sinzui> closed teams cannot become open if they are a member of another closed team
[19:55] <sinzui> I think I discovered that when writing the test...I should have updated the rule in my notes
[19:56] <bac> that makes more sense
[19:56] <bac> thanks
[19:59] <bac> sinzui: on TeamSubPolicyError were you forced to define 'doc' to get around your problem?
[19:59] <sinzui> yes
[19:59] <bac> do you know why it happens?  is that explained in 'Invalid'?
[19:59] <sinzui> The Invalid class has def: doc(): return self.__doc__
[20:00] <sinzui> That appears to be insane
[20:00] <bac> i'm not familar with the doc method.  what should it do?
[20:00] <sinzui> I understand that in the ideal that the cause of  type of error is obvious, but then why do they all permit use pass a message?
[20:02] <sinzui> bac I really cannot explain the issue well. I cannot think or a good reason why that method (rather than str()) is used to get the description of the issue in the Web UI
[20:02] <bac> sinzui: odd
[20:02] <bac> sinzui: i think this test is redundant and should just be 'else:'
[20:03] <bac> elif policy != TeamSubscriptionPolicy.OPEN:
[20:03] <bac> you just checked policy == OPEN
[20:03] <sinzui> I did see that zope wants unicode, translatable docstrings, so it does funky operations like class.__doc__ = _('my translatable error description')
[20:04] <sinzui> bac; I do not think that is right. The else clause covers safe transitions such as from moderated to restricted
[20:05] <bac> sinzui: at line 95 you test that the policy == OPEN.  so the next check that policy != OPEN must always be true
[20:07] <sinzui> I think I should add something to the elif.
[20:07] <bac> yeah, you need to check what you are coming from
[20:07] <bac> i think 108 should be team.subscriptionpolicy !=OPEN
[20:08] <sinzui> elif policy != TeamSubscriptionPolicy.OPEN and team.policy != TeamSubscriptionPolicy.OPEN
[20:08] <bac> you can eliminate the first part because it is already known to be true
[20:08] <sinzui> We only want to check the teams when we are certain that the team is transitioning from closed to open
[20:09] <bac> 108 should test that it is transitioning to closed from another closed
[20:10] <sinzui> okay I understand
[20:10]  * sinzui tries a rewrite
[20:11] <bac> no, i'm wrong.  108 should test going from OPEN to closed, so the test should be old == OPEN, since we know the new is one is not OPEN
[20:11] <bac> urgh.
[20:11] <bac> so i think you have a coding mistake
[20:12] <bac> why is there not a test case that is broken?
[20:12] <sinzui> elif team.policy != TeamSubscriptionPolicy.OPEN
[20:12] <bac> team.subscriptionpolicy
[20:15] <sinzui> +    elif team.subscriptionpolicy == TeamSubscriptionPolicy.OPEN:
[20:15] <sinzui> ^ that makes the tests happy
[20:19] <bac> sinzui: but the tests were already happy.  i think you may be missing a test case
[20:20] <sinzui> I do not think so. Do you want a test for moderated to restricted?
[20:20] <sinzui> the else that has a pass in it?
[20:21] <bac> i don't know
[20:21] <bac> i'm just bummed that you had a logic error but no test failure
[20:22] <bac> doesn't that indicate a lack of coverage?
[20:22] <sinzui> okay. I will add a test from moderated to restricted
[20:22] <sinzui> If I had pdb running in that test, I would have found that I never entered the pass block, I looked at the teams
[20:22] <bac> yeah, i think it'll be an uninteresting test now but would have shown a failure before.
[20:23] <sinzui> I want to avoid looking at teams
[20:23] <bac> you can add the test and then shelve your code fix and see if it fails
[20:23] <sinzui> it would not have failed, I was just making db checks that I intended to avoid
[20:25] <bac> yes you are right
[20:25] <bac> the error just caused you to do checks that are not needed in that case.  there is no way it could fail
[20:27] <bac> sinzui: r=bac with these changes.
[20:27] <sinzui> almost finished the test
[20:31] <sinzui> bac: http://pastebin.ubuntu.com/542016/
[20:31] <bac> sinzui: looks good