launchpad-reviewers team mailing list archive
-
launchpad-reviewers team
-
Mailing list archive
-
Message #04525
[Merge] lp:~wallyworld/launchpad/remove-answer-contacts-424156 into lp:launchpad
Ian Booth has proposed merging lp:~wallyworld/launchpad/remove-answer-contacts-424156 into lp:launchpad.
Requested reviews:
Launchpad code reviewers (launchpad-reviewers)
Related bugs:
Bug #424156 in Launchpad itself: "Allow users with authority to remove answer contacts"
https://bugs.launchpad.net/launchpad/+bug/424156
For more details, see:
https://code.launchpad.net/~wallyworld/launchpad/remove-answer-contacts-424156/+merge/70857
== Implementation ==
Provide a new daily garbo job to remove answer contacts which are deactivated (after 1 day) or suspended (after 7 days). The job is implemented by subclassing BulkPruner.
An update to db permissions is required - the garbo job needs delete privileges on the answercontact table.
Also did a few lint fixes.
== Tests ==
Add tests to test_garbo.py:
test_AnswerContactPruner_deactivated_accounts
test_AnswerContactPruner_suspended_accounts
test_AnswerContactPruner_doesnt_prune_recently_changed_accounts
== Lint ==
Checking for conflicts and issues in changed files.
Linting changed files:
database/schema/security.cfg
lib/canonical/database/constants.py
lib/lp/scripts/garbo.py
lib/lp/scripts/tests/test_garbo.py
--
https://code.launchpad.net/~wallyworld/launchpad/remove-answer-contacts-424156/+merge/70857
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~wallyworld/launchpad/remove-answer-contacts-424156 into lp:launchpad.
=== modified file 'database/schema/security.cfg'
--- database/schema/security.cfg 2011-08-08 12:38:50 +0000
+++ database/schema/security.cfg 2011-08-09 11:50:56 +0000
@@ -2147,6 +2147,7 @@
[garbo]
groups=script,read
+public.answercontact = SELECT, DELETE
public.branchjob = SELECT, DELETE
public.bug = SELECT, UPDATE
public.bugaffectsperson = SELECT
=== modified file 'lib/canonical/database/constants.py'
--- lib/canonical/database/constants.py 2009-08-03 09:39:14 +0000
+++ lib/canonical/database/constants.py 2011-08-09 11:50:56 +0000
@@ -17,3 +17,9 @@
THIRTY_DAYS_AGO = SQL(
"CURRENT_TIMESTAMP AT TIME ZONE 'UTC' - interval '30 days'")
+
+SEVEN_DAYS_AGO = SQL(
+ "CURRENT_TIMESTAMP AT TIME ZONE 'UTC' - interval '7 days'")
+
+ONE_DAY_AGO = SQL(
+ "CURRENT_TIMESTAMP AT TIME ZONE 'UTC' - interval '1 day'")
=== modified file 'lib/lp/scripts/garbo.py'
--- lib/lp/scripts/garbo.py 2011-08-03 12:42:24 +0000
+++ lib/lp/scripts/garbo.py 2011-08-09 11:50:56 +0000
@@ -49,6 +49,7 @@
from canonical.launchpad.database.librarian import TimeLimitedToken
from canonical.launchpad.database.oauth import OAuthNonce
from canonical.launchpad.database.openidconsumer import OpenIDConsumerNonce
+from canonical.launchpad.interfaces.account import AccountStatus
from canonical.launchpad.interfaces.emailaddress import EmailAddressStatus
from canonical.launchpad.interfaces.lpstorm import IMasterStore
from canonical.launchpad.utilities.looptuner import TunableLoop
@@ -57,6 +58,7 @@
MAIN_STORE,
MASTER_FLAVOR,
)
+from lp.answers.model.answercontact import AnswerContact
from lp.bugs.interfaces.bug import IBugSet
from lp.bugs.model.bug import Bug
from lp.bugs.model.bugattachment import BugAttachment
@@ -94,7 +96,7 @@
)
-ONE_DAY_IN_SECONDS = 24*60*60
+ONE_DAY_IN_SECONDS = 24 * 60 * 60
class BulkPruner(TunableLoop):
@@ -305,7 +307,7 @@
We remove all OpenIDConsumerNonce records older than 1 day.
"""
- maximum_chunk_size = 6*60*60 # 6 hours in seconds.
+ maximum_chunk_size = 6 * 60 * 60 # 6 hours in seconds.
def __init__(self, log, abort_time=None):
super(OpenIDConsumerNoncePruner, self).__init__(log, abort_time)
@@ -611,7 +613,7 @@
self.max_offset = self.store.execute(
"SELECT MAX(id) FROM UnlinkedPeople").get_one()[0]
if self.max_offset is None:
- self.max_offset = -1 # Trigger isDone() now.
+ self.max_offset = -1 # Trigger isDone() now.
self.log.debug("No Person records to remove.")
else:
self.log.info("%d Person records to remove." % self.max_offset)
@@ -677,6 +679,34 @@
"""
+class AnswerContactPruner(BulkPruner):
+ """Remove old answer contacts which are no longer required.
+
+ Remove a person as an answer contact if:
+ their account has been deactivated for more than one day, or
+ suspended for more than one week.
+ """
+ target_table_class = AnswerContact
+ ids_to_prune_query = """
+ SELECT DISTINCT AnswerContact.id
+ FROM AnswerContact, Person, Account
+ WHERE
+ AnswerContact.person = Person.id
+ AND Person.account = Account.id
+ AND (
+ (Account.date_status_set <
+ CURRENT_TIMESTAMP AT TIME ZONE 'UTC'
+ - CAST('1 day' AS interval)
+ AND Account.status = %s)
+ OR
+ (Account.date_status_set <
+ CURRENT_TIMESTAMP AT TIME ZONE 'UTC'
+ - CAST('7 days' AS interval)
+ AND Account.status = %s)
+ )
+ """ % (AccountStatus.DEACTIVATED.value, AccountStatus.SUSPENDED.value)
+
+
class BranchJobPruner(BulkPruner):
"""Prune `BranchJob`s that are in a final state and more than a month old.
@@ -700,8 +730,8 @@
Only needed until they are all set, after that triggers will maintain it.
"""
- # Test migration did 3M in 2 hours, so 5000 is ~ 10 seconds - and thats the
- # max we want to hold a DB lock open for.
+ # Test migration did 3M in 2 hours, so 5000 is ~ 10 seconds - and that's
+ # the max we want to hold a DB lock open for.
minimum_chunk_size = 1000
maximum_chunk_size = 5000
@@ -709,7 +739,7 @@
super(MirrorBugMessageOwner, self).__init__(log, abort_time)
self.store = IMasterStore(BugMessage)
self.isDone = IMasterStore(BugMessage).find(
- BugMessage, BugMessage.ownerID==None).is_empty
+ BugMessage, BugMessage.ownerID == None).is_empty
def __call__(self, chunk_size):
"""See `ITunableLoop`."""
@@ -811,7 +841,7 @@
class OldTimeLimitedTokenDeleter(TunableLoop):
"""Delete expired url access tokens from the session DB."""
- maximum_chunk_size = 24*60*60 # 24 hours in seconds.
+ maximum_chunk_size = 24 * 60 * 60 # 24 hours in seconds.
def __init__(self, log, abort_time=None):
super(OldTimeLimitedTokenDeleter, self).__init__(log, abort_time)
@@ -933,10 +963,10 @@
class BaseDatabaseGarbageCollector(LaunchpadCronScript):
"""Abstract base class to run a collection of TunableLoops."""
- script_name = None # Script name for locking and database user. Override.
- tunable_loops = None # Collection of TunableLoops. Override.
- continue_on_failure = False # If True, an exception in a tunable loop
- # does not cause the script to abort.
+ script_name = None # Script name for locking and database user. Override.
+ tunable_loops = None # Collection of TunableLoops. Override.
+ continue_on_failure = False # If True, an exception in a tunable loop
+ # does not cause the script to abort.
# Default run time of the script in seconds. Override.
default_abort_script_time = None
@@ -987,7 +1017,7 @@
for count in range(0, self.options.threads):
thread = threading.Thread(
target=self.run_tasks_in_thread,
- name='Worker-%d' % (count+1,),
+ name='Worker-%d' % (count + 1,),
args=(tunable_loops,))
thread.start()
threads.add(thread)
@@ -1021,7 +1051,7 @@
@property
def script_timeout(self):
- a_very_long_time = 31536000 # 1 year
+ a_very_long_time = 31536000 # 1 year
return self.options.abort_script or a_very_long_time
def get_loop_logger(self, loop_name):
@@ -1034,7 +1064,7 @@
loop_logger = logging.getLogger('garbo.' + loop_name)
for filter in loop_logger.filters:
if isinstance(filter, PrefixFilter):
- return loop_logger # Already have a PrefixFilter attached.
+ return loop_logger # Already have a PrefixFilter attached.
loop_logger.addFilter(PrefixFilter(loop_name))
return loop_logger
@@ -1106,7 +1136,7 @@
loop_logger.debug3(
"Unable to acquire lock %s. Running elsewhere?",
loop_lock_path)
- time.sleep(0.3) # Avoid spinning.
+ time.sleep(0.3) # Avoid spinning.
tunable_loops.append(tunable_loop_class)
# Otherwise, emit a warning and skip the task.
else:
@@ -1169,6 +1199,7 @@
class DailyDatabaseGarbageCollector(BaseDatabaseGarbageCollector):
script_name = 'garbo-daily'
tunable_loops = [
+ AnswerContactPruner,
BranchJobPruner,
BugNotificationPruner,
BugWatchActivityPruner,
=== modified file 'lib/lp/scripts/tests/test_garbo.py'
--- lib/lp/scripts/tests/test_garbo.py 2011-08-02 09:57:41 +0000
+++ lib/lp/scripts/tests/test_garbo.py 2011-08-09 11:50:56 +0000
@@ -33,6 +33,8 @@
from canonical.config import config
from canonical.database import sqlbase
from canonical.database.constants import (
+ ONE_DAY_AGO,
+ SEVEN_DAYS_AGO,
THIRTY_DAYS_AGO,
UTC_NOW,
)
@@ -43,6 +45,7 @@
OAuthNonce,
)
from canonical.launchpad.database.openidconsumer import OpenIDConsumerNonce
+from canonical.launchpad.interfaces.account import AccountStatus
from canonical.launchpad.interfaces.emailaddress import EmailAddressStatus
from canonical.launchpad.interfaces.lpstorm import IMasterStore
from canonical.launchpad.scripts.tests import run_script
@@ -57,6 +60,7 @@
LaunchpadZopelessLayer,
ZopelessDatabaseLayer,
)
+from lp.answers.model.answercontact import AnswerContact
from lp.bugs.model.bugmessage import BugMessage
from lp.bugs.model.bugnotification import (
BugNotification,
@@ -93,11 +97,12 @@
SessionData,
SessionPkgData,
)
+from lp.services.worlddata.interfaces.language import ILanguageSet
from lp.testing import (
+ person_logged_in,
TestCase,
TestCaseWithFactory,
)
-from lp.translations.model.potemplate import POTemplate
from lp.translations.model.potmsgset import POTMsgSet
from lp.translations.model.translationtemplateitem import (
TranslationTemplateItem,
@@ -392,10 +397,10 @@
def test_OAuthNoncePruner(self):
now = datetime.now(UTC)
timestamps = [
- now - timedelta(days=2), # Garbage
- now - timedelta(days=1) - timedelta(seconds=60), # Garbage
- now - timedelta(days=1) + timedelta(seconds=60), # Not garbage
- now, # Not garbage
+ now - timedelta(days=2), # Garbage
+ now - timedelta(days=1) - timedelta(seconds=60), # Garbage
+ now - timedelta(days=1) + timedelta(seconds=60), # Not garbage
+ now, # Not garbage
]
LaunchpadZopelessLayer.switchDbUser('testadmin')
store = IMasterStore(OAuthNonce)
@@ -406,14 +411,14 @@
for timestamp in timestamps:
store.add(OAuthNonce(
access_token=OAuthAccessToken.get(1),
- request_timestamp = timestamp,
- nonce = str(timestamp)))
+ request_timestamp=timestamp,
+ nonce=str(timestamp)))
transaction.commit()
# Make sure we have 4 nonces now.
self.failUnlessEqual(store.find(OAuthNonce).count(), 4)
- self.runHourly(maximum_chunk_size=60) # 1 minute maximum chunk size
+ self.runHourly(maximum_chunk_size=60) # 1 minute maximum chunk size
store = IMasterStore(OAuthNonce)
@@ -435,10 +440,10 @@
HOURS = 60 * 60
DAYS = 24 * HOURS
timestamps = [
- now - 2 * DAYS, # Garbage
- now - 1 * DAYS - 1 * MINUTES, # Garbage
- now - 1 * DAYS + 1 * MINUTES, # Not garbage
- now, # Not garbage
+ now - 2 * DAYS, # Garbage
+ now - 1 * DAYS - 1 * MINUTES, # Garbage
+ now - 1 * DAYS + 1 * MINUTES, # Not garbage
+ now, # Not garbage
]
LaunchpadZopelessLayer.switchDbUser('testadmin')
@@ -456,7 +461,7 @@
self.failUnlessEqual(store.find(OpenIDConsumerNonce).count(), 4)
# Run the garbage collector.
- self.runHourly(maximum_chunk_size=60) # 1 minute maximum chunks.
+ self.runHourly(maximum_chunk_size=60) # 1 minute maximum chunks.
store = IMasterStore(OpenIDConsumerNonce)
@@ -465,7 +470,8 @@
# And none of them are older than 1 day
earliest = store.find(Min(OpenIDConsumerNonce.timestamp)).one()
- self.failUnless(earliest >= now - 24*60*60, 'Still have old nonces')
+ self.failUnless(
+ earliest >= now - 24 * 60 * 60, 'Still have old nonces')
def test_CodeImportResultPruner(self):
now = datetime.now(UTC)
@@ -492,7 +498,7 @@
new_code_import_result(now - timedelta(days=60))
for i in range(results_to_keep_count - 1):
- new_code_import_result(now - timedelta(days=19+i))
+ new_code_import_result(now - timedelta(days=19 + i))
# Run the garbage collector
self.runDaily()
@@ -565,7 +571,7 @@
store.execute("""
INSERT INTO %s (server_url, handle, issued, lifetime)
VALUES (%s, %s, %d, %d)
- """ % (table_name, str(delta), str(delta), now-10, delta))
+ """ % (table_name, str(delta), str(delta), now - 10, delta))
transaction.commit()
# Ensure that we created at least one expirable row (using the
@@ -779,6 +785,60 @@
BugNotification.date_emailed < THIRTY_DAYS_AGO).count(),
0)
+ def _test_AnswerContactPruner(self, status, interval, expected_count=0):
+ # Garbo should remove answer contacts for accounts with given 'status'
+ # which was set more than 'interval' days ago.
+ LaunchpadZopelessLayer.switchDbUser('testadmin')
+ store = IMasterStore(AnswerContact)
+
+ person = self.factory.makePerson()
+ person.addLanguage(getUtility(ILanguageSet)['en'])
+ question = self.factory.makeQuestion()
+ with person_logged_in(question.owner):
+ question.target.addAnswerContact(person, person)
+ Store.of(question).flush()
+ self.assertEqual(
+ store.find(
+ AnswerContact,
+ AnswerContact.person == person.id).count(),
+ 1)
+
+ account = person.account
+ account.status = status
+ # We flush because a trigger sets the date_status_set and we need to
+ # modify it ourselves.
+ Store.of(account).flush()
+ if interval is not None:
+ account.date_status_set = interval
+
+ self.runDaily()
+
+ LaunchpadZopelessLayer.switchDbUser('testadmin')
+ self.assertEqual(
+ store.find(
+ AnswerContact,
+ AnswerContact.person == person.id).count(),
+ expected_count)
+
+ def test_AnswerContactPruner_deactivated_accounts(self):
+ # Answer contacts with an account deactivated at least one day ago
+ # should be pruned.
+ self._test_AnswerContactPruner(AccountStatus.DEACTIVATED, ONE_DAY_AGO)
+
+ def test_AnswerContactPruner_suspended_accounts(self):
+ # Answer contacts with an account suspended at least seven days ago
+ # should be pruned.
+ self._test_AnswerContactPruner(
+ AccountStatus.SUSPENDED, SEVEN_DAYS_AGO)
+
+ def test_AnswerContactPruner_doesnt_prune_recently_changed_accounts(self):
+ # Answer contacts which are suspended or deactivated inside the
+ # minimum time interval are not pruned.
+ self._test_AnswerContactPruner(
+ AccountStatus.DEACTIVATED, None, expected_count=1)
+ self._test_AnswerContactPruner(
+ AccountStatus.SUSPENDED, ONE_DAY_AGO, expected_count=1)
+
def test_BranchJobPruner(self):
# Garbo should remove jobs completed over 30 days ago.
LaunchpadZopelessLayer.switchDbUser('testadmin')
@@ -900,7 +960,7 @@
finally:
con.close()
store = IMasterStore(BugMessage)
- unmigrated = store.find(BugMessage, BugMessage.ownerID==None).count
+ unmigrated = store.find(BugMessage, BugMessage.ownerID == None).count
self.assertNotEqual(0, unmigrated())
self.runHourly()
self.assertEqual(0, unmigrated())
