[Bps-public-commit] RT-Extension-MergeUsers branch, master, updated. 0.08-4-gb13c434

[Alex Vandiver]

The branch, master has been updated
       via  b13c434086b68286bc2da0e3602fa302573951f0 (commit)
       via  9f9da780d1d0569d3b3af814e7b6806257ed99bc (commit)
       via  569bd681b43a8cc585fab4f1fe00d42e2281d642 (commit)
      from  1eccf75c00ff9e4b7b8a71edeb20b1730db8fe80 (commit)

Summary of changes:
 lib/RT/Extension/MergeUsers.pm | 29 +++++++----------------------
 xt/01merge_users.t             | 15 +++++++++++++++
 2 files changed, 22 insertions(+), 22 deletions(-)

- Log -----------------------------------------------------------------
commit 569bd681b43a8cc585fab4f1fe00d42e2281d642
Author: Alex Vandiver <alexmv at bestpractical.com>
Date:   Wed Feb 27 18:35:31 2013 -0500

    Explicitly canonicalizing both $merged and $canonical_self is not required
    
    As the path to get both user objects uses ->Load, which calls
    ->LoadByCols, which we have overridden, we already have the most
    canonical form.  Checking it for EffectiveId is unnecessary and
    wasteful.

diff --git a/lib/RT/Extension/MergeUsers.pm b/lib/RT/Extension/MergeUsers.pm
index 309129c..0919416 100644
--- a/lib/RT/Extension/MergeUsers.pm
+++ b/lib/RT/Extension/MergeUsers.pm
@@ -199,21 +199,13 @@ sub MergeInto {
         $merge->Load($user);
         return (0, "Could not load user '$user'") unless $merge->id;
     }
+    return (0, "Could not load user to be merged") unless $merge->id;
 
     # Get copies of the canonicalized users
     my $email;
-    if (defined $merge->Attributes->Named('EffectiveId')) {
-        $email = $merge->CanonicalizeEmailAddress($merge->EmailAddress);
-        $merge->LoadByEmail($email);
-    }
-    return (0, "Could not load user to be merged") unless $merge->id;
 
     my $canonical_self = RT::User->new($RT::SystemUser);
     $canonical_self->Load($self->id);
-    if (defined $canonical_self->Attributes->Named('EffectiveId')) {
-        $email = $canonical_self->CanonicalizeEmailAddress($canonical_self->EmailAddress);
-        $canonical_self->LoadByEmail($email);
-    }
     return (0, "Could not load user to merge into") unless $canonical_self->id;
 
     # No merging into yourself!

commit 9f9da780d1d0569d3b3af814e7b6806257ed99bc
Author: Alex Vandiver <alexmv at bestpractical.com>
Date:   Wed Feb 27 18:40:38 2013 -0500

    Explicitly checking EffectiveId is also unneeded in CanonicalizeEmailAddress
    
    As CanonicalizeEmailAddress uses ->LoadByCols to look up the email
    address, and we override LoadByCols to chase EffectiveId after a
    successful LoadByCols call, looking up EffectiveId again is unnecessary.
    
    The recursive call is kept, on the possibility that
    CanonicalizeEmailAddressMatch will match the _new_, canonicalized form.
    This preserves the previous behavior.

diff --git a/lib/RT/Extension/MergeUsers.pm b/lib/RT/Extension/MergeUsers.pm
index 0919416..494b812 100644
--- a/lib/RT/Extension/MergeUsers.pm
+++ b/lib/RT/Extension/MergeUsers.pm
@@ -122,15 +122,7 @@ sub CanonicalizeEmailAddress {
     my $canonical_user = RT::User->new($RT::SystemUser);
     $canonical_user->LoadByCols( EmailAddress => $address );
     return $address unless $canonical_user->id;
-
-    # if we got a user, check for a parent
-    my ($effective_id) = $canonical_user->Attributes->Named("EffectiveId");
-    return $address unless $effective_id && $effective_id->id;
-
-    $canonical_user->LoadByCols( id => $effective_id->Content );
-    return $address unless $canonical_user->id;
-
-    # is there another parent user above this one?
+    return $address unless $canonical_user->EmailAddress ne $address;
     return $canonical_user->CanonicalizeEmailAddress(
         $canonical_user->EmailAddress
     );

commit b13c434086b68286bc2da0e3602fa302573951f0
Author: Alex Vandiver <alexmv at bestpractical.com>
Date:   Wed Feb 27 18:43:56 2013 -0500

    Ensure that $canonical_self, when inspected, still has unmerged data
    
    $canonical_self->SetComments() calls a $self->Load( $self->id ) during
    the process of updating the Comments.  Because this now loads the data
    of the user we are merged into, the information recorded on $merged was
    always contentless -- namely, it always showed the the user had been
    merged into itself.
    
    Swap the order of the ->SetComments calls, to ensure that
    $canonical_self is still the user being merged in when it is inspected
    to record data on $merge.

diff --git a/lib/RT/Extension/MergeUsers.pm b/lib/RT/Extension/MergeUsers.pm
index 494b812..707ce09 100644
--- a/lib/RT/Extension/MergeUsers.pm
+++ b/lib/RT/Extension/MergeUsers.pm
@@ -223,14 +223,15 @@ sub MergeInto {
     my $merged_users = $merge->GetMergedUsers;
     $merged_users->SetContent( [$canonical_self->Id, @{$merged_users->Content}] );
 
-    $canonical_self->SetComments( join "\n", grep /\S/,
-        $canonical_self->Comments||'',
-        "Merged into ". ($merge->EmailAddress || $merge->Name)." (". $merge->id .")",
-    );
     $merge->SetComments(join "\n", grep /\S/,
         $merge->Comments||'',
         ($canonical_self->EmailAddress || $canonical_self->Name)." (".$canonical_self->id.") merged into this user",
     );
+
+    $canonical_self->SetComments( join "\n", grep /\S/,
+        $canonical_self->Comments||'',
+        "Merged into ". ($merge->EmailAddress || $merge->Name)." (". $merge->id .")",
+    );
     return (1, "Merged users successfuly");
 }
 
diff --git a/xt/01merge_users.t b/xt/01merge_users.t
index 36b1553..32314cd 100644
--- a/xt/01merge_users.t
+++ b/xt/01merge_users.t
@@ -40,6 +40,21 @@ ok(!$id, "Doesn't merge a user into itself? $message");
 ($id, $message) = $secondary_user->MergeInto($primary_user);
 ok($id, "Successfully merges users? $message");
 
+# Check that the comments are useful
+{
+    my $from = RT::User->new( $RT::SystemUser );
+    $from->LoadOriginal( id => $secondary_user->id );
+    is( $from->Comments, "Merged into primary-$$\@example.com (".$primary_user->id.")",
+        "Comments contain the right user that was merged into"
+    );
+
+    my $into = RT::User->new( $RT::SystemUser );
+    $into->LoadOriginal( id => $primary_user->id );
+    is( $into->Comments, "secondary-$$\@example.com (".$secondary_user->id.") merged into this user",
+        "Comments contain the right user that was merged in"
+    );
+}
+
 # recognizes already-merged users
 ($id, $message) = $secondary_user->MergeInto($primary_user);
 ok(!$id, "Recognizes secondary as child? $message");

-----------------------------------------------------------------------