launchpad-dev team mailing list archive

["Francis J. Lacoste" <francis.lacoste@xxxxxxxxxxxxx>]

On September 4, 2009, Celso Providelo wrote:
> On Wed, Aug 12, 2009 at 9:11 AM, Guilherme Salgado<salgado@xxxxxxxxxxxxx> 
wrote:
> > On Tue, 2009-08-11 at 17:01 -0400, Stuart Bishop wrote:
> >> > On Tue, Aug 11, 2009 at 12:57:28PM -0300, Guilherme Salgado wrote:
> >> >> Another (simpler) option is to abuse the fact that the Person and
> >> >> Account tables are not linked by foreign keys and create the Person
> >> >> entry when needed but not link it to the existing Account. That way,
> >> >> everything would just magically work as the Person would seem to have
> >> >> no account. The only thing we'd have to do is change the
> >> >> registration/login (maybe reset password too) process to link the
> >> >> Person to the Account when they are not linked. Of course, by doing
> >> >> that we would also be hiding any bugs that might cause associated
> >> >> Person/Account entries to not be linked when they should.
> >>
> >> Please don't abuse the data model :-P If you do that, the alerts that
> >> have been setup to detect this data corruption will go off. There
> >> should be foreign key constraints to ensure that all these links
> >> remain sane, and it is unfortunate that we cannot use them due to our
> >> replication requirements.
> >>
> >> The way we designed this situation to be handled is you create the
> >> person record and link it to the existing emailaddress and account if
> >> they exist.
> >>
> >> If we need to differentiate Person records created through these
> >> automatic approaches (imports from external bug trackers, pofile
> >> imports, branch imports, mailing list imports) from Person records
> >> being actively used, we should add flags. We should not extend the
> >> meaning of creation_rationale - new flags and fields is better. I
> >> certainly don't want to turn it into a bitmap field that makes it
> >> impossible to query sanely.
> >
> > Agreed.
> >
> >> I guess the main reason we want to differentiate users who have
> >> actively created a Launchpad Person from the import created Person is
> >> so we don't spam people who haven't signed up themselves (which would
> >> happen if someone subscribed them to a bug. I think an 'acivated' or
> >> 'deactivated' boolean would handle this - do we need something more
> >> complex than this?
> >
> > In fact, what we should aim for is to make sure is to have these Person
> > entries show up as placeholder profiles in the web UI and not include
> > them in the ValidPerson* vocabs, so that they don't get linked to
> > anything -- that's much better than trying to fix all places that may
> > send email to people to skip the ones which have uses_lp=False.
> >
> >> We should certainly create the Person records with a specific
> >> creation_rationale.
> >>
> >> We should certainly set the hide-emailaddress flag.
> >
> > Agreed on both.
> 
> Right, guys, I had a call with Salgado this morning and presented my
> changes to IPersonSet.ensurePerson() allowing it to create missing
> Person for existing Accounts.
> 
> http://pastebin.ubuntu.com/264993/
> 
> The only remaining problem is that the Person being created in this
> circumstance would be presented as an active user in LP, which is not
> the case for any callsite of ensurePerson.

Right, we discussed this on the team leads call and we were basically fine 
with this limitation at this point.

> 
> I have a feeling that we all agreed on adding a new flag to Person
> ('active' possibly) which would be set as False in line 25 of the diff
> above. At that point we can also set hide_emailaddress True.

We should probably create the person with hide_emailaddress now already.

But yes, we want to add an 'active' flag to Person.

> 
> Assuming that we do have an agreement about it, would it be possible
> to accelerate the process and add the new flag in the production DB
> ASAP so we can CP the ensurePerson changes at least in iron.c.c and
> unblock debian imports appropriately ?

> 
> Otherwise I will have to add a hack in gina to ignore imports in this
> condition for reestablishing the service until we find a better
> solution.
> 

Why can't we just cherry-pick this change on iron now? It should work even if 
we don't have the person flag now.

We can retrofit that flag based on the presence of karma action later. SET 
flag = FALSE WHERE creation_rational != CREATED_ON_LAUNCHAPD AND No_Karma.

You don't need to block on the DB changes.

-- 
Francis J. Lacoste
francis.lacoste@xxxxxxxxxxxxx

Attachment: signature.asc
Description: This is a digitally signed message part.