launchpad-reviewers team mailing list archive
-
launchpad-reviewers team
-
Mailing list archive
-
Message #04420
[Merge] lp:~lifeless/launchpad/bug-761874 into lp:launchpad
Robert Collins has proposed merging lp:~lifeless/launchpad/bug-761874 into lp:launchpad.
Requested reviews:
Launchpad code reviewers (launchpad-reviewers)
For more details, see:
https://code.launchpad.net/~lifeless/launchpad/bug-761874/+merge/69748
Mailing ~registry on team deletes is not the win. Lets stop doing that.
--
https://code.launchpad.net/~lifeless/launchpad/bug-761874/+merge/69748
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~lifeless/launchpad/bug-761874 into lp:launchpad.
=== modified file 'lib/lp/registry/browser/peoplemerge.py'
--- lib/lp/registry/browser/peoplemerge.py 2011-07-27 17:34:09 +0000
+++ lib/lp/registry/browser/peoplemerge.py 2011-07-29 06:34:27 +0000
@@ -96,6 +96,7 @@
dupe_person_emails = ()
dupe_person = None
target_person = None
+ delete = False
@property
def cancel_url(self):
@@ -120,7 +121,7 @@
"""
emailset = getUtility(IEmailAddressSet)
self.dupe_person = data['dupe_person']
- self.target_person = data['target_person']
+ self.target_person = data.get('target_person', None)
self.dupe_person_emails = emailset.getByPerson(self.dupe_person)
def doMerge(self, data):
@@ -140,7 +141,8 @@
naked_email.accountID = self.target_person.accountID
naked_email.status = EmailAddressStatus.NEW
getUtility(IPersonSet).mergeAsync(
- self.dupe_person, self.target_person, reviewer=self.user)
+ self.dupe_person, self.target_person, reviewer=self.user,
+ delete=self.delete)
self.request.response.addInfoNotification(self.merge_message)
self.next_url = self.success_url
@@ -250,7 +252,7 @@
"""A view that deletes a team by merging it with Registry experts."""
page_title = 'Delete'
- field_names = ['dupe_person', 'target_person']
+ field_names = ['dupe_person']
merge_message = _('The team is queued to be deleted.')
@property
@@ -259,8 +261,7 @@
def __init__(self, context, request):
super(DeleteTeamView, self).__init__(context, request)
- if ('field.dupe_person' in self.request.form
- or 'field.target_person' in self.request.form):
+ if ('field.dupe_person' in self.request.form):
# These fields have fixed values and are managed by this method.
# The user has crafted a request to gain ownership of the dupe
# team's assets.
@@ -280,8 +281,7 @@
def default_values(self):
return {
'field.dupe_person': self.context.name,
- 'field.target_person': getUtility(
- ILaunchpadCelebrities).registry_experts.name,
+ 'field.delete': True,
}
@property
@@ -302,6 +302,7 @@
@action('Delete', name='delete', condition=canDelete)
def merge_action(self, action, data):
base = super(DeleteTeamView, self)
+ self.delete = True
base.deactivate_members_and_merge_action.success(data)
=== modified file 'lib/lp/registry/browser/tests/peoplemerge-views.txt'
--- lib/lp/registry/browser/tests/peoplemerge-views.txt 2011-05-27 19:53:20 +0000
+++ lib/lp/registry/browser/tests/peoplemerge-views.txt 2011-07-29 06:34:27 +0000
@@ -11,8 +11,12 @@
>>> from lp.app.interfaces.launchpad import ILaunchpadCelebrities
>>> from lp.registry.interfaces.person import IPersonSet
+ >>> from lp.registry.interfaces.persontransferjob import (
+ ... IPersonMergeJobSource,
+ ... )
>>> registry_experts = getUtility(ILaunchpadCelebrities).registry_experts
>>> person_set = getUtility(IPersonSet)
+ >>> merge_job_source = getUtility(IPersonMergeJobSource)
>>> registry_expert= factory.makeRegistryExpert()
>>> login_person(registry_expert)
@@ -128,7 +132,7 @@
>>> print view.cancel_url
http://launchpad.dev/~deletable
-The view uses the same form fields as the team merge view, but it does not
+The view uses the similar form fields as the team merge view, but it does not
render them because their values are preset. Only the action is rendered,
and it is only rendered if canDelete() returns True.
@@ -137,16 +141,16 @@
>>> view = create_initialized_view(
... deletable_team, '+delete', principal=team_owner)
>>> view.field_names
- ['dupe_person', 'target_person']
+ ['dupe_person']
>>> view.default_values
- {'field.target_person': u'registry', 'field.dupe_person': u'deletable'}
+ {'field.delete': True, 'field.dupe_person': u'deletable'}
>>> content = find_tag_by_id(view.render(), 'maincontent')
>>> print find_tag_by_id(content, 'field.dupe_person')
None
- >>> print find_tag_by_id(content, 'field.target_person')
+ >>> print find_tag_by_id(content, 'field.delete')
None
>>> print find_tag_by_id(content, 'field.actions.delete')['value']
@@ -179,6 +183,14 @@
>>> print view.next_url
http://launchpad.dev/people
+And there is a person merge job setup to delete them.
+
+ >>> job = merge_job_source.find(deletable_team).any()
+ >>> job.metadata['delete']
+ True
+ >>> job.from_person.name
+ u'deletable'
+
The delete team view cannot be hacked to delete another team, or change
the team that the merge operation is performed with.
=== modified file 'lib/lp/registry/interfaces/person.py'
--- lib/lp/registry/interfaces/person.py 2011-07-22 17:30:41 +0000
+++ lib/lp/registry/interfaces/person.py 2011-07-29 06:34:27 +0000
@@ -2215,7 +2215,7 @@
def latest_teams(limit=5):
"""Return the latest teams registered, up to the limit specified."""
- def mergeAsync(from_person, to_person, reviewer=None):
+ def mergeAsync(from_person, to_person, reviewer=None, delete=False):
"""Merge a person/team into another asynchronously.
This schedules a call to `merge()` to happen outside of the current
@@ -2227,9 +2227,25 @@
:param from_person: An IPerson or ITeam that is a duplicate.
:param to_person: An IPerson or ITeam that is a master.
:param reviewer: An IPerson who approved the ITeam merger.
+ :param delete: The merge is really a deletion.
:return: A `PersonMergeJob` or None.
"""
+ def delete(from_person, reviewer):
+ """Delete a person/team.
+
+ The old team (from_person) will be left as an atavism.
+
+ Deleting a person is not supported at this time.
+
+ When deleting teams, from_person must have no IMailingLists
+ associated with and no active members. Any active team members will be
+ deactivated.
+
+ :param from_person: An IPerson or ITeam that is a duplicate.
+ :param reviewer: An IPerson who approved the ITeam merger.
+ """
+
def merge(from_person, to_person, reviewer=None):
"""Merge a person/team into another.
@@ -2239,10 +2255,8 @@
addresses associated with.
When merging teams, from_person must have no IMailingLists
- associated with and no active members. If it has active members,
- though, it's possible to have them deactivated before the merge by
- passing deactivate_members=True. In that case the user who's
- performing the merge must be provided as well.
+ associated with it. If it has active members they will be deactivated -
+ and reviewer must be supplied.
:param from_person: An IPerson or ITeam that is a duplicate.
:param to_person: An IPerson or ITeam that is a master.
=== modified file 'lib/lp/registry/interfaces/persontransferjob.py'
--- lib/lp/registry/interfaces/persontransferjob.py 2011-04-08 21:42:59 +0000
+++ lib/lp/registry/interfaces/persontransferjob.py 2011-07-29 06:34:27 +0000
@@ -102,7 +102,7 @@
class IPersonMergeJobSource(IJobSource):
"""An interface for acquiring IPersonMergeJobs."""
- def create(from_person, to_person, reviewer=None):
+ def create(from_person, to_person, reviewer=None, delete=False):
"""Create a new IPersonMergeJob.
None is returned if either the from_person or to_person are already
@@ -112,6 +112,7 @@
:param from_person: An IPerson or ITeam that is a duplicate.
:param to_person: An IPerson or ITeam that is a master.
:param reviewer: An IPerson who approved ITeam merger.
+ :param delete: The merge is really a deletion.
"""
def find(from_person=None, to_person=None, any_person=False):
=== modified file 'lib/lp/registry/model/person.py'
--- lib/lp/registry/model/person.py 2011-07-27 19:13:40 +0000
+++ lib/lp/registry/model/person.py 2011-07-29 06:34:27 +0000
@@ -3914,13 +3914,25 @@
naked_from_team.retractTeamMembership(team, reviewer)
IStore(from_team).flush()
- def mergeAsync(self, from_person, to_person, reviewer=None):
+ def mergeAsync(self, from_person, to_person, reviewer=None, delete=False):
"""See `IPersonSet`."""
return getUtility(IPersonMergeJobSource).create(
- from_person=from_person, to_person=to_person, reviewer=reviewer)
+ from_person=from_person, to_person=to_person, reviewer=reviewer,
+ delete=delete)
+
+ def delete(self, from_person, reviewer):
+ """See `IPersonSet`."""
+ # Deletes are implemented by merging into registry experts. Force
+ # the target to prevent any accidental misuse by calling code.
+ to_person = getUtility(ILaunchpadCelebrities).registry_experts
+ return self._merge(from_person, to_person, reviewer, True)
def merge(self, from_person, to_person, reviewer=None):
"""See `IPersonSet`."""
+ return self._merge(from_person, to_person, reviewer)
+
+ def _merge(self, from_person, to_person, reviewer, delete=False):
+ """Helper for merge and delete methods."""
# since we are doing direct SQL manipulation, make sure all
# changes have been flushed to the database
store = Store.of(from_person)
@@ -4154,6 +4166,10 @@
UPDATE OpenIdIdentifier SET account=%s WHERE account=%s
""" % sqlvalues(to_person.accountID, from_person.accountID))
+ if delete:
+ # We don't notify anyone about deletes.
+ return
+
# Inform the user of the merge changes.
if to_person.isTeam():
mail_text = get_email_template(
=== modified file 'lib/lp/registry/model/persontransferjob.py'
--- lib/lp/registry/model/persontransferjob.py 2011-07-25 02:35:24 +0000
+++ lib/lp/registry/model/persontransferjob.py 2011-07-29 06:34:27 +0000
@@ -45,6 +45,7 @@
)
from canonical.launchpad.mailnotification import MailWrapper
from canonical.launchpad.webapp import canonical_url
+from lp.app.interfaces.launchpad import ILaunchpadCelebrities
from lp.registry.enum import PersonTransferJobType
from lp.registry.interfaces.person import (
IPerson,
@@ -345,14 +346,20 @@
class_job_type = PersonTransferJobType.MERGE
@classmethod
- def create(cls, from_person, to_person, reviewer=None):
+ def create(cls, from_person, to_person, reviewer=None, delete=False):
"""See `IPersonMergeJobSource`."""
- if from_person.is_merge_pending or to_person.is_merge_pending:
+ if (from_person.is_merge_pending or
+ (not delete and to_person.is_merge_pending)):
return None
if from_person.is_team:
metadata = {'reviewer': reviewer.id}
else:
metadata = {}
+ metadata['delete'] = bool(delete)
+ if metadata['delete']:
+ # Ideally not needed, but the DB column is not-null at the moment
+ # and this minor bit of friction isn't worth changing that over.
+ to_person = getUtility(ILaunchpadCelebrities).registry_experts
return super(PersonMergeJob, cls).create(
minor_person=from_person, major_person=to_person,
metadata=metadata)
@@ -412,16 +419,27 @@
to_person_name = self.to_person.name
from canonical.launchpad.scripts import log
- log.debug(
- "%s is about to merge ~%s into ~%s", self.log_name,
- from_person_name, to_person_name)
- getUtility(IPersonSet).merge(
- from_person=self.from_person, to_person=self.to_person,
- reviewer=self.reviewer)
-
- log.debug(
- "%s has merged ~%s into ~%s", self.log_name,
- from_person_name, to_person_name)
+ personset = getUtility(IPersonSet)
+ if self.metadata['delete']:
+ log.debug(
+ "%s is about to delete ~%s", self.log_name,
+ from_person_name)
+ personset.delete(
+ from_person=self.from_person,
+ reviewer=self.reviewer)
+ log.debug(
+ "%s has deleted ~%s", self.log_name,
+ from_person_name)
+ else:
+ log.debug(
+ "%s is about to merge ~%s into ~%s", self.log_name,
+ from_person_name, to_person_name)
+ personset.merge(
+ from_person=self.from_person, to_person=self.to_person,
+ reviewer=self.reviewer)
+ log.debug(
+ "%s has merged ~%s into ~%s", self.log_name,
+ from_person_name, to_person_name)
def __repr__(self):
return (
=== modified file 'lib/lp/registry/tests/test_person.py'
--- lib/lp/registry/tests/test_person.py 2011-06-10 14:31:49 +0000
+++ lib/lp/registry/tests/test_person.py 2011-07-29 06:34:27 +0000
@@ -60,6 +60,7 @@
PersonCreationRationale,
PersonVisibility,
)
+from lp.registry.interfaces.personnotification import IPersonNotificationSet
from lp.registry.interfaces.product import IProductSet
from lp.registry.model.karma import (
KarmaCategory,
@@ -755,6 +756,17 @@
account.openid_identifier = openid_identifier
return account
+ def test_delete_no_notifications(self):
+ team = self.factory.makeTeam()
+ owner = team.teamowner
+ transaction.commit()
+ reconnect_stores('IPersonMergeJobSource')
+ team = reload_object(team)
+ owner = reload_object(owner)
+ self.person_set.delete(team, owner)
+ notifications = getUtility(IPersonNotificationSet).getNotificationsToSend()
+ self.assertEqual(0, notifications.count())
+
def test_openid_identifiers(self):
# Verify that OpenId Identifiers are merged.
duplicate = self.factory.makePerson()
