launchpad-reviewers team mailing list archive

[Guruprasad <mp+415261@xxxxxxxxxxxxxxxxxx>]

Guruprasad has proposed merging ~lgp171188/launchpad:disallow-null-values-bug-lock-status into launchpad:master with ~lgp171188/launchpad:lock-status-not-nullable as a prerequisite.

Commit message:
Disallow None as a value for Bug.lock_status

Requested reviews:
  Launchpad code reviewers (launchpad-reviewers)

For more details, see:
https://code.launchpad.net/~lgp171188/launchpad/+git/launchpad/+merge/415261
-- 
Your team Launchpad code reviewers is requested to review the proposed merge of ~lgp171188/launchpad:disallow-null-values-bug-lock-status into launchpad:master.

diff --git a/database/schema/patch-2210-38-1.sql b/database/schema/patch-2210-38-1.sql
deleted file mode 100644
index 6519b25..0000000
--- a/database/schema/patch-2210-38-1.sql
+++ /dev/null
@@ -1,9 +0,0 @@
--- Copyright 2022 Canonical Ltd.  This software is licensed under the
--- GNU Affero General Public License version 3 (see the file LICENSE).
-
-SET client_min_messages=ERROR;
-
-ALTER TABLE Bug
-    ALTER COLUMN lock_status SET NOT NULL;
-
-INSERT INTO LaunchpadDatabaseRevision VALUES (2210, 38, 1);
diff --git a/lib/lp/bugs/model/bug.py b/lib/lp/bugs/model/bug.py
index 6cb9825..f663baa 100644
--- a/lib/lp/bugs/model/bug.py
+++ b/lib/lp/bugs/model/bug.py
@@ -408,23 +408,12 @@ class Bug(SQLBase, InformationTypeMixin):
     heat = IntCol(notNull=True, default=0)
     heat_last_updated = UtcDateTimeCol(default=None)
     latest_patch_uploaded = UtcDateTimeCol(default=None)
-    _lock_status = DBEnum(
+    lock_status = DBEnum(
         name='lock_status', enum=BugLockStatus,
-        allow_none=True, default=BugLockStatus.UNLOCKED)
+        allow_none=False, default=BugLockStatus.UNLOCKED)
     lock_reason = StringCol(notNull=False, default=None)
 
     @property
-    def lock_status(self):
-        return (
-            BugLockStatus.UNLOCKED if self._lock_status is None
-            else self._lock_status
-        )
-
-    @lock_status.setter
-    def lock_status(self, value):
-        self._lock_status = value
-
-    @property
     def linked_branches(self):
         return [link.branch for link in self.linked_bugbranches]
 
diff --git a/lib/lp/bugs/tests/test_bug.py b/lib/lp/bugs/tests/test_bug.py
index ae35bb0..358b338 100644
--- a/lib/lp/bugs/tests/test_bug.py
+++ b/lib/lp/bugs/tests/test_bug.py
@@ -7,7 +7,9 @@ from datetime import timedelta
 import json
 
 from lazr.lifecycle.snapshot import Snapshot
+from storm.exceptions import NoneError
 from testtools.matchers import MatchesStructure
+from testtools.testcase import ExpectedException
 from zope.component import getUtility
 from zope.interface import providedBy
 from zope.security.management import checkPermission
@@ -442,6 +444,12 @@ class TestBugLocking(TestCaseWithFactory):
         self.assertEqual(BugLockStatus.UNLOCKED, bug.lock_status)
         self.assertIsNone(bug.lock_reason)
 
+    def test_bug_lock_status_cannot_be_none(self):
+        bug = self.factory.makeBug(owner=self.person, target=self.target)
+        self.assertEqual(BugLockStatus.UNLOCKED, bug.lock_status)
+        with ExpectedException(NoneError):
+            removeSecurityProxy(bug).lock_status = None
+
     def test_bug_locking_when_bug_already_locked(self):
         bug = self.factory.makeBug(owner=self.person, target=self.target)
         with person_logged_in(self.target.owner):
diff --git a/lib/lp/scripts/garbo.py b/lib/lp/scripts/garbo.py
index 6067080..f1c75ad 100644
--- a/lib/lp/scripts/garbo.py
+++ b/lib/lp/scripts/garbo.py
@@ -51,7 +51,6 @@ from zope.component import getUtility
 from zope.security.proxy import removeSecurityProxy
 
 from lp.answers.model.answercontact import AnswerContact
-from lp.bugs.enums import BugLockStatus
 from lp.bugs.interfaces.bug import IBugSet
 from lp.bugs.model.bug import Bug
 from lp.bugs.model.bugattachment import BugAttachment
@@ -1785,42 +1784,6 @@ class RevisionStatusReportPruner(BulkPruner):
         RevisionStatusArtifactType.BINARY.value)
 
 
-class PopulateBugLockStatusDefaultUnlocked(TunableLoop):
-    """
-    Populates Bug.lock_status to BugLockStatus.UNLOCKED if not set
-    """
-
-    maximum_chunk_size = 5000
-
-    def __init__(self, log, abort_time=None):
-        super().__init__(log, abort_time)
-        self.store = IMasterStore(Bug)
-        self.start_at = 1
-
-    def findBugsLockStatusNone(self):
-        return self.store.find(
-            Bug.id,
-            Bug.id >= self.start_at,
-            Bug._lock_status == None,
-        ).order_by(Bug.id)
-
-    def setBugLockStatusUnlocked(self, bug_ids):
-        self.store.find(Bug, Bug.id.is_in(bug_ids)).set(
-            _lock_status=BugLockStatus.UNLOCKED
-        )
-
-    def isDone(self):
-        return self.findBugsLockStatusNone().is_empty()
-
-    def __call__(self, chunk_size):
-        bug_ids = [
-            bug_id for bug_id in self.findBugsLockStatusNone()[:chunk_size]
-        ]
-        self.setBugLockStatusUnlocked(bug_ids)
-        self.start_at = bug_ids[-1] + 1
-        transaction.commit()
-
-
 class BaseDatabaseGarbageCollector(LaunchpadCronScript):
     """Abstract base class to run a collection of TunableLoops."""
     script_name = None  # Script name for locking and database user. Override.
@@ -2112,7 +2075,6 @@ class DailyDatabaseGarbageCollector(BaseDatabaseGarbageCollector):
         OCIFilePruner,
         ObsoleteBugAttachmentPruner,
         OldTimeLimitedTokenDeleter,
-        PopulateBugLockStatusDefaultUnlocked,
         PopulateSnapBuildStoreRevision,
         POTranslationPruner,
         PreviewDiffPruner,
diff --git a/lib/lp/scripts/tests/test_garbo.py b/lib/lp/scripts/tests/test_garbo.py
index 99f36cd..90c4bb3 100644
--- a/lib/lp/scripts/tests/test_garbo.py
+++ b/lib/lp/scripts/tests/test_garbo.py
@@ -101,7 +101,6 @@ from lp.scripts.garbo import (
     load_garbo_job_state,
     LoginTokenPruner,
     OpenIDConsumerAssociationPruner,
-    PopulateBugLockStatusDefaultUnlocked,
     PopulateSnapBuildStoreRevision,
     ProductVCSPopulator,
     save_garbo_job_state,
@@ -2110,21 +2109,6 @@ class TestGarbo(FakeAdapterMixin, TestCaseWithFactory):
             list(getUtility(
                 IRevisionStatusReportSet).findByRepository(repo2)))
 
-    def test_PopulateBugLockStatusDefaultUnlocked(self):
-        switch_dbuser('testadmin')
-        populator = PopulateBugLockStatusDefaultUnlocked(log=None)
-        for _ in range(5):
-            bug = self.factory.makeBug()
-            removeSecurityProxy(bug).lock_status = None
-
-        result_set = populator.findBugsLockStatusNone()
-        self.assertGreater(result_set.count(), 0)
-
-        self.runDaily()
-
-        result_set = populator.findBugsLockStatusNone()
-        self.assertTrue(result_set.is_empty())
-
 
 class TestGarboTasks(TestCaseWithFactory):
     layer = LaunchpadZopelessLayer