launchpad-reviewers team mailing list archive

[Celso Providelo <celso.providelo@xxxxxxxxxxxxx>]

Celso Providelo has proposed merging lp:~cprov/launchpad/ic-diffpruner into lp:launchpad.

Requested reviews:
  Launchpad code reviewers (launchpad-reviewers)

For more details, see:
https://code.launchpad.net/~cprov/launchpad/ic-diffpruner/+merge/210304

Prevent periodic-cleanup of PreviewDiffs with inline comments.
-- 
https://code.launchpad.net/~cprov/launchpad/ic-diffpruner/+merge/210304
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~cprov/launchpad/ic-diffpruner into lp:launchpad.

=== modified file 'lib/lp/scripts/garbo.py'
--- lib/lp/scripts/garbo.py	2013-11-18 05:05:25 +0000
+++ lib/lp/scripts/garbo.py	2014-03-11 00:26:23 +0000
@@ -389,6 +389,8 @@
                 ORDER BY PreviewDiff.date_created DESC) AS pos
             FROM previewdiff) AS ss
         WHERE pos > 1
+        EXCEPT SELECT previewdiff FROM CodeReviewInlineComment
+        EXCEPT SELECT previewdiff FROM CodeReviewInlineCommentDraft
         """
 
 

=== modified file 'lib/lp/scripts/tests/test_garbo.py'
--- lib/lp/scripts/tests/test_garbo.py	2013-11-15 06:16:32 +0000
+++ lib/lp/scripts/tests/test_garbo.py	2014-03-11 00:26:23 +0000
@@ -111,8 +111,10 @@
 from lp.soyuz.enums import PackagePublishingStatus
 from lp.soyuz.model.reporting import LatestPersonSourcePackageReleaseCache
 from lp.testing import (
+    feature_flags,
+    person_logged_in,
+    set_feature_flag,
     FakeAdapterMixin,
-    person_logged_in,
     TestCase,
     TestCaseWithFactory,
     )
@@ -660,6 +662,33 @@
         self.assertEqual([mp1_diff.id], mp1_diff_ids)
         self.assertEqual([mp2_diff.id], mp2_diff_ids)
 
+    def test_PreviewDiffPruner_with_inline_comments(self):
+        # Old PreviewDiffs with published or draft inline comments
+        # are not removed.
+        switch_dbuser('testadmin')
+        mp1 = self.factory.makeBranchMergeProposal()
+        now = datetime.now(UTC)
+        mp1_diff_comment =  self.factory.makePreviewDiff(
+            merge_proposal=mp1, date_created=now - timedelta(hours=2))
+        mp1_diff_draft =  self.factory.makePreviewDiff(
+            merge_proposal=mp1, date_created=now - timedelta(hours=1))
+        mp1_diff = self.factory.makePreviewDiff(merge_proposal=mp1)
+        # Enabled 'inline_diff_comments' feature flag and attach comments
+        # on the old diffs, so they are kept in DB.
+        self.useContext(feature_flags())
+        set_feature_flag(u'code.inline_diff_comments.enabled', u'enabled')
+        mp1.createComment(
+            owner=mp1.registrant, previewdiff_id=mp1_diff_comment.id,
+            subject='Hold this diff!', inline_comments={'1': '!!!'})
+        mp1.saveDraftInlineComment(
+            previewdiff_id=mp1_diff_draft.id,
+            person=mp1.registrant, comments={'1': '???'})
+        # Run the 'garbo' and verify the old diffs were not removed.
+        self.runDaily()
+        self.assertEqual(
+            [mp1_diff_comment.id, mp1_diff_draft.id, mp1_diff.id],
+            [p.id for p in mp1.preview_diffs])
+
     def test_DiffPruner(self):
         switch_dbuser('testadmin')
         diff_id = removeSecurityProxy(self.factory.makeDiff()).id