launchpad-reviewers team mailing list archive

[Ian Booth <ian.booth@xxxxxxxxxxxxx>]

Ian Booth has proposed merging lp:~wallyworld/launchpad/branch-rename-message-1053182 into lp:launchpad.

Commit message:
Improve notification messages for retargetted junk branches.

Requested reviews:
  Launchpad code reviewers (launchpad-reviewers)
Related bugs:
  Bug #1053182 in Launchpad itself: "Extra message when changing branch owner"
  https://bugs.launchpad.net/launchpad/+bug/1053182

For more details, see:
https://code.launchpad.net/~wallyworld/launchpad/branch-rename-message-1053182/+merge/125445

== Implementation ==

If there is an existing junk branch which is assigned to a new owner, there were 2 notification messages displayed - one for the owner change and one for the new branch target. The target message is redundant. So the branch change submit handler has been improved accordingly.

As a driveby, the target change message for new junk branches displays the lp name as well as the display name.

== Tests ==

A new test is added with moves a junk branch to a new owner.

== Lint ==

Checking for conflicts and issues in changed files.

Linting changed files:
  lib/lp/code/browser/branch.py
  lib/lp/code/browser/tests/test_branch.py
-- 
https://code.launchpad.net/~wallyworld/launchpad/branch-rename-message-1053182/+merge/125445
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~wallyworld/launchpad/branch-rename-message-1053182 into lp:launchpad.

=== modified file 'lib/lp/code/browser/branch.py'
--- lib/lp/code/browser/branch.py	2012-09-19 04:37:16 +0000
+++ lib/lp/code/browser/branch.py	2012-09-20 09:52:24 +0000
@@ -845,11 +845,12 @@
                 information_type, self.user)
         if 'target' in data:
             target = data.pop('target')
+            existing_junk = self.context.target.name == '+junk'
+            same_junk_status = target == '+junk' and existing_junk
             if target == '+junk':
                 target = None
-            if (target is None and self.context.target is not None
-                or target is not None and self.context.target is None
-                or target != self.context.target):
+            if not same_junk_status or (
+                target is not None and target != self.context.target):
                 try:
                     self.context.setTarget(self.user, project=target)
                 except BranchTargetError, e:
@@ -863,8 +864,9 @@
                         % (target.displayname, target.name))
                 else:
                     self.request.response.addNotification(
-                        "This branch is now a personal branch for %s"
-                        % self.context.owner.displayname)
+                        "This branch is now a personal branch for %s (%s)"
+                        % (self.context.owner.displayname,
+                            self.context.owner.name))
         if 'reviewer' in data:
             reviewer = data.pop('reviewer')
             if reviewer != self.context.code_reviewer:

=== modified file 'lib/lp/code/browser/tests/test_branch.py'
--- lib/lp/code/browser/tests/test_branch.py	2012-09-18 00:45:58 +0000
+++ lib/lp/code/browser/tests/test_branch.py	2012-09-20 09:52:24 +0000
@@ -997,8 +997,28 @@
         }
         view = create_initialized_view(branch, name='+edit', form=form)
         self.assertEqual(person, branch.target.context)
-        self.assertEqual(
-            'This branch is now a personal branch for %s' % person.displayname,
+        self.assertEqual(1, len(view.request.response.notifications))
+        self.assertEqual(
+            'This branch is now a personal branch for %s (%s)'
+                % (person.displayname, person.name),
+            view.request.response.notifications[0].message)
+
+    def test_save_to_different_junk(self):
+        # The branch target widget can retarget to a junk branch.
+        person = self.factory.makePerson()
+        branch = self.factory.makePersonalBranch(owner=person)
+        new_owner = self.factory.makeTeam(name='newowner', members=[person])
+        login_person(person)
+        form = {
+            'field.target': 'personal',
+            'field.owner': 'newowner',
+            'field.actions.change': 'Change Branch',
+        }
+        view = create_initialized_view(branch, name='+edit', form=form)
+        self.assertEqual(new_owner, branch.target.context)
+        self.assertEqual(1, len(view.request.response.notifications))
+        self.assertEqual(
+            'The branch owner has been changed to Newowner (newowner)',
             view.request.response.notifications[0].message)
 
     def test_branch_target_widget_saves_product(self):