launchpad-reviewers team mailing list archive

[William Grant <me@xxxxxxxxxxxxxxxxxx>]

Review: Needs Fixing code



Inline comments:

> --- 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()
> +

They won't be exposed publicly, but these DB operations should be in bulk methods on CodeReviewInlineCommentSet that take a number of PreviewDiffs (or their IDs) and removes all CRICs or CRICDs for them. Let's keep all the model code in one place, so we don't end up with CRIC looking like the rest of Launchpad.

>      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."""
> 


-- 
https://code.launchpad.net/~cprov/launchpad/delete-branches-with-ics/+merge/220317
Your team Launchpad code reviewers is subscribed to branch lp:launchpad.