launchpad-reviewers team mailing list archive

[Celso Providelo <celso.providelo@xxxxxxxxxxxxx>]

Celso Providelo has proposed merging lp:~cprov/launchpad/delete-branches-with-ics into lp:launchpad.

Commit message:
Allow the proper deletion of branches (and merge proposals) associated with inline comments.

Requested reviews:
  Launchpad code reviewers (launchpad-reviewers)
Related bugs:
  Bug #1321348 in Launchpad itself: "Unable to remove branches associated with inline comments"
  https://bugs.launchpad.net/launchpad/+bug/1321348

For more details, see:
https://code.launchpad.net/~cprov/launchpad/delete-branches-with-ics/+merge/220317

Allow the proper deletion of branches (and merge proposals) associated with inline comments.

The related inline comments are also removed.
-- 
https://code.launchpad.net/~cprov/launchpad/delete-branches-with-ics/+merge/220317
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~cprov/launchpad/delete-branches-with-ics into lp:launchpad.

=== modified file 'lib/lp/code/model/branchmergeproposal.py'
--- lib/lp/code/model/branchmergeproposal.py	2014-02-25 19:40:56 +0000
+++ lib/lp/code/model/branchmergeproposal.py	2014-05-20 18:43:28 +0000
@@ -711,6 +711,26 @@
             notify(ReviewerNominatedEvent(vote_reference))
         return vote_reference
 
+    def _deleteInlineComments(self):
+        """Delete the related published and draft inline comments."""
+        # XXX cprov 2014-05-20: implement the removal queries localy
+        # since there is no need to expose them publicly.
+
+        # Avoid circular imports and also
+        from lp.code.model.codereviewinlinecomment import (
+            CodeReviewInlineCommentDraft,
+            CodeReviewInlineComment,
+        )
+        diff_ids = [pd.id for pd in self._preview_diffs]
+        IStore(CodeReviewInlineComment).find(
+            CodeReviewInlineComment,
+            CodeReviewInlineComment.previewdiff_id.is_in(diff_ids)
+        ).remove()
+        IStore(CodeReviewInlineCommentDraft).find(
+            CodeReviewInlineCommentDraft,
+            CodeReviewInlineCommentDraft.previewdiff_id.is_in(diff_ids)
+        ).remove()
+
     def deleteProposal(self):
         """See `IBranchMergeProposal`."""
         # Delete this proposal, but keep the superseded chain linked.
@@ -719,6 +739,8 @@
         # Delete the related CodeReviewVoteReferences.
         for vote in self.votes:
             vote.destroySelf()
+        # Delete published and draft inline comments related to this MP.
+        self._deleteInlineComments()
         # Delete the related CodeReviewComments.
         for comment in self.all_comments:
             comment.destroySelf()

=== modified file 'lib/lp/code/model/tests/test_branch.py'
--- lib/lp/code/model/tests/test_branch.py	2014-01-15 01:25:27 +0000
+++ lib/lp/code/model/tests/test_branch.py	2014-05-20 18:43:28 +0000
@@ -1408,6 +1408,40 @@
             branches=[branch1, branch2])
         branch2.destroySelf(break_references=True)
 
+    def test_destroySelf_with_inline_comments_draft(self):
+        # Draft inline comments related to a deleted branch (source
+        # or target MP branch) also get removed.
+        merge_proposal = self.factory.makeBranchMergeProposal(
+            registrant=self.user, target_branch=self.branch)
+        preview_diff = self.factory.makePreviewDiff(
+            merge_proposal=merge_proposal)
+        transaction.commit()
+        self.useFixture(FeatureFixture({
+            'code.inline_diff_comments.enabled': 'enabled'}))
+        merge_proposal.saveDraftInlineComment(
+            previewdiff_id=preview_diff.id,
+            person=self.user,
+            comments={'1': 'Should vanish.'})
+        self.branch.destroySelf(break_references=True)
+
+    def test_destroySelf_with_inline_comments_published(self):
+        # Published inline comments related to a deleted branch (source
+        # or target MP branch) also get removed.
+        merge_proposal = self.factory.makeBranchMergeProposal(
+            registrant=self.user, target_branch=self.branch)
+        preview_diff = self.factory.makePreviewDiff(
+            merge_proposal=merge_proposal)
+        transaction.commit()
+        self.useFixture(FeatureFixture({
+            'code.inline_diff_comments.enabled': 'enabled'}))
+        merge_proposal.createComment(
+            owner=self.user,
+            subject='Delete me!',
+            previewdiff_id=preview_diff.id,
+            inline_comments={'1': 'Must disappear.'},
+        )
+        self.branch.destroySelf(break_references=True)
+
 
 class TestBranchDeletionConsequences(TestCase):
     """Test determination and application of branch deletion consequences."""