Code review comment for lp:~wallyworld/launchpad/additional-affiliation-types-798764

Hi Ian.

I think this branch is good to land. I have a few remarks you may want to consider.

=== modified file 'lib/lp/registry/model/pillaraffiliation.py'
--- lib/lp/registry/model/pillaraffiliation.py 2011-08-04 13:56:55 +0000
+++ lib/lp/registry/model/pillaraffiliation.py 2011-08-05 06:14:28 +0000
...

> +@adapter(IBugTask)
> +class BugTaskPillarAffiliation(PillarAffiliation):
> + """An affiliation adapter for bug tasks."""
> + def getPillar(self):
> + return self.context.pillar
> +
> + def _getAffiliationDetails(self, person, pillar):
> + """ A person is affiliated with a bugtask based on (in order):
> + - owner of bugtask pillar
> + - driver of bugtask pillar
> + - bug supervisor of bugtask pillar
> + - security contact of bugtask pillar
> + """
> + result = super(BugTaskPillarAffiliation, self)._getAffiliationDetails(
> + person, pillar)
> + if result is not None:
> + return result
> + if person.inTeam(pillar.bug_supervisor):
> + return pillar.displayname, 'bug supervisor'
> + if person.inTeam(pillar.security_contact):
> + return pillar.displayname, 'security contact'

I think this will also work for branches. Could we do this as well?

@adapter(IBranch)
class BranchPillarAffiliation(BugTaskPillarAffiliation):
    """An affiliation adapter for branches."""

^ An extra class would not be be necessary using ZCML to register the adapter,
but we ouls still have two separate ZCML entried for both IBugTask
and IBranch.

> +@adapter(ISpecification)
> > +class SpecificationPillarAffiliation(PillarAffiliation):
> > + """An affiliation adapter for blueprints."""
> > + def getPillar(self):
> > + return (self.context.target)

You suggested that this adapter could check if the user is assigned a roll
for the specification. I think that could misinform users. Anyone can register
a specification and assign users to rolls. They are not in anyway affilliated
with the project. Most of Launchpad's visible blueprints are bogus, and they
where created by people other than ~launchpad. I think this implementation
is good.

> +@adapter(IQuestion)
> +class QuestionPillarAffiliation(PillarAffiliation):
> + """An affiliation adapter for questions."""
> + def getPillar(self):
> + return self.context.product or self.context.distribution
> +
> + def _getAffiliationDetails(self, person, pillar):
> + """ A person is affiliated with a question based on (in order):
> + - answer contact for question target
> + - owner of question target
> + - driver of question target
> + """
> + target = self.context.target
> + answer_contacts = target.answer_contacts
> + for answer_contact in answer_contacts:
> + if person.inTeam(answer_contact):
> + return target.displayname, 'answer contact'
> + return super(QuestionPillarAffiliation, self)._getAffiliationDetails(
> + person, pillar)

This looks a lot like code I wrote in 2007. In 2010 I updated security.py
to use an alternate approach that was much faster (fewer sql queries). Most
answer contacts choose one or more packages instead of the distro because
the email is overwhelming. I think we do care about the DSP in this case.

        target = self.context.target
        if IDistributionSourcePackage.providedBy(target):
            question_targets = (target, target.distribution)
        else:
            question_targets = (target, )
        questions_person = IQuestionsPerson(person)
        for target in questions_person.getDirectAnswerQuestionTargets():
            if target in question_targets:
                return target.displayname, 'answer contact'
        for target in questions_person.getTeamAnswerQuestionTargets():
            if target in question_targets:
                return target.displayname, 'answer contact'
        return super(QuestionPillarAffiliation, self)._getAffiliationDetails(
            person, pillar)