From: Aran Deltac <bluefeet@cpan.org>
Date: Thu, 23 Mar 2006 17:21:32 +0000 (+0000)
Subject: Various fixes to Positioned.  Tests no longer test AdjacencyList.
X-Git-Tag: v0.07002~75^2~279
X-Git-Url: http://git.shadowcat.co.uk/gitweb/gitweb.cgi?p=dbsrgits%2FDBIx-Class.git;a=commitdiff_plain;h=133dd22a652e4745443b7c45f7d40af5877aaab5

Various fixes to Positioned.  Tests no longer test AdjacencyList.
---

diff --git a/TODO b/TODO
index d0726b3..3e58f61 100644
--- a/TODO
+++ b/TODO
@@ -17,3 +17,8 @@
 2006-03-18 by bluefeet
  - Support table locking.
 
+2006-03-21 by bluefeet
+ - When subclassing a dbic class make it so you don't have to do 
+   __PACKAGE__->table(__PACKAGE__->table()); for the result set to 
+   return the correct object type.
+
diff --git a/lib/DBIx/Class/Positioned.pm b/lib/DBIx/Class/Positioned.pm
index a8daf69..8fe2d36 100644
--- a/lib/DBIx/Class/Positioned.pm
+++ b/lib/DBIx/Class/Positioned.pm
@@ -56,6 +56,12 @@ Thats it, now you can change the position of your objects.
 This module provides a simple interface for modifying the position 
 of DBIx::Class objects.
 
+=head1 AUTO UPDATE
+
+All of the move_* methods automatically update the rows involved in 
+the query.  This is not configurable and is due to the fact that if you 
+move a record it always causes other records in the list to be updated.
+
 =head1 METHODS
 
 =head2 position_column
@@ -69,6 +75,18 @@ positional value of each record.  Default to "position".
 
 __PACKAGE__->mk_classdata( 'position_column' => 'position' );
 
+=head2 collection_column
+
+  __PACKAGE__->collection_column('thing_id');
+
+This method specified a column to limit all queries in 
+this module by.  This effectively allows you to have multiple 
+positioned lists within the same table.
+
+=cut
+
+__PACKAGE__->mk_classdata( 'collection_column' );
+
 =head2 siblings
 
   my $rs = $employee->siblings();
@@ -85,7 +103,7 @@ sub siblings {
     my $rs = $self->search(
         {
             $position_column => { '!=' => $self->get_column($position_column) },
-            $self->_parent_clause(),
+            $self->_collection_clause(),
         },
         { order_by => $self->position_column },
     );
@@ -104,7 +122,7 @@ Returns the first sibling object.
 sub first_sibling {
     my( $self ) = @_;
     return ($self->search(
-        { $self->_parent_clause() },
+        { $self->_collection_clause() },
         { rows=>1, order_by => $self->position_column },
     )->all())[0];
 }
@@ -120,7 +138,7 @@ Return the last sibling.
 sub last_sibling {
     my( $self ) = @_;
     return ($self->search(
-        { $self->_parent_clause() },
+        { $self->_collection_clause() },
         { rows=>1, order_by => $self->position_column.' DESC' },
     )->all())[0];
 }
@@ -140,7 +158,7 @@ sub previous_sibling {
     return ($self->search(
         {
             $position_column => { '<' => $self->get_column($position_column) },
-            $self->_parent_clause(),
+            $self->_collection_clause(),
         },
         { rows=>1, order_by => $position_column.' DESC' },
     )->all())[0];
@@ -158,10 +176,10 @@ is returned if the current object is the last one.
 sub next_sibling {
     my( $self ) = @_;
     my $position_column = $self->position_column;
-    return ($self->search(
+    return ($self->result_source->resultset->search(
         {
             $position_column => { '>' => $self->get_column($position_column) },
-            $self->_parent_clause(),
+            $self->_collection_clause(),
         },
         { rows=>1, order_by => $position_column },
     )->all())[0];
@@ -179,15 +197,8 @@ the first one.
 
 sub move_previous {
     my( $self ) = @_;
-    my $previous = $self->previous_sibling();
-    return undef if (!$previous);
-    my $position_column = $self->position_column;
-    my $self_position = $self->get_column( $position_column );
-    $self->set_column( $position_column, $previous->get_column($position_column) );
-    $previous->set_column( $position_column, $self_position );
-    $self->update();
-    $previous->update();
-    return 1;
+    my $position = $self->get_column( $self->position_column() );
+    return $self->move_to( $position - 1 );
 }
 
 =head2 move_next
@@ -201,15 +212,10 @@ success, and 0 is returned if the object is already the last in the list.
 
 sub move_next {
     my( $self ) = @_;
-    my $next = $self->next_sibling();
-    return undef if (!$next);
-    my $position_column = $self->position_column;
-    my $self_position = $self->get_column( $position_column );
-    $self->set_column( $position_column, $next->get_column($position_column) );
-    $next->set_column( $position_column, $self_position );
-    $self->update();
-    $next->update();
-    return 1;
+    my $position = $self->get_column( $self->position_column() );
+    my $count = $self->result_source->resultset->search({$self->_collection_clause()})->count();
+    return 0 if ($position==$count);
+    return $self->move_to( $position + 1 );
 }
 
 =head2 move_first
@@ -237,7 +243,7 @@ success, and 0 is returned if the object is already the last one.
 
 sub move_last {
     my( $self ) = @_;
-    my $count = $self->search({$self->_parent_clause()})->count();
+    my $count = $self->result_source->resultset->search({$self->_collection_clause()})->count();
     return $self->move_to( $count );
 }
 
@@ -255,13 +261,14 @@ sub move_to {
     my( $self, $to_position ) = @_;
     my $position_column = $self->position_column;
     my $from_position = $self->get_column( $position_column );
-    return undef if ( $from_position==$to_position );
-    my $rs = $self->search({
+    return 0 if ( $to_position < 1 );
+    return 0 if ( $from_position==$to_position );
+    my $rs = $self->result_source->resultset->search({
         -and => [
             $position_column => { ($from_position>$to_position?'<':'>') => $from_position },
             $position_column => { ($from_position>$to_position?'>=':'<=') => $to_position },
         ],
-        $self->_parent_clause(),
+        $self->_collection_clause(),
     });
     my $op = ($from_position>$to_position) ? '+' : '-';
     $rs->update({
@@ -283,7 +290,7 @@ the table +1, thus positioning the new record at the last position.
 sub insert {
     my $self = shift;
     my $position_column = $self->position_column;
-    $self->set_column( $position_column => $self->search( {$self->_parent_clause()} )->count()+1 ) 
+    $self->set_column( $position_column => $self->result_source->resultset->search( {$self->_collection_clause()} )->count()+1 ) 
         if (!$self->get_column($position_column));
     $self->next::method( @_ );
 }
@@ -307,22 +314,19 @@ sub delete {
 These methods are used internally.  You should never have the 
 need to use them.
 
-=head2 _parent_clause
-
-  sub _parent_clause {
-    my( $self ) = @_;
-    return ( parent_id => $self->parent_id );
-  }
+=head2 _collection_clause
 
-This method is a placeholder for you, or another component, to 
-provide additional limits for all the various queries in this 
-module.  This allows for more than one positionable list within 
-the same table since any move_* method will adhere to the clause 
-that you specify.
+This method returns a name=>value pare for limiting a search 
+by the collection column.  If the collection column is not 
+defined then this will return an empty list.
 
 =cut
 
-sub _parent_clause {
+sub _collection_clause {
+    my $self = shift;
+    if ($self->collection_column()) {
+        return ( $self->collection_column() => $self->get_column($self->collection_column()) );
+    }
     return ();
 }
 
@@ -331,6 +335,8 @@ __END__
 
 =head1 BUGS
 
+=head2 Race Condition on Insert
+
 If a position is not specified for an insert than a position 
 will be chosen based on COUNT(*)+1.  But, it first selects the 
 count then inserts the record.  The space of time between select 
@@ -338,6 +344,20 @@ and insert introduces a race condition.  To fix this we need the
 ability to lock tables in DBIC.  I've added an entry in the TODO 
 about this.
 
+=head2 Multiple Moves
+
+Be careful when issueing move_* methods to multiple objects.  If 
+you've pre-loaded the objects then when you move one of the objects 
+the position of the other object will not reflect their new value 
+until you reload them from the database.
+
+The are times when you will want to move objects as groups, such 
+as changeing the parent of several objects at once - this directly 
+conflicts with this problem.  One solution is for us to write a 
+ResultSet class that supports a parent() method, for example.  Another 
+solution is to somehow automagically modify the objects that exist 
+in the current object's result set to have the new position value.
+
 =head1 AUTHOR
 
 Aran Deltac <bluefeet@cpan.org>
diff --git a/t/run/26positioned.tl b/t/run/26positioned.tl
index 31503b0..ad6a919 100644
--- a/t/run/26positioned.tl
+++ b/t/run/26positioned.tl
@@ -5,89 +5,59 @@ sub run_tests {
     plan tests => 96;
     my $schema = shift;
 
-    foreach my $class ( 'Positioned', 'PositionedAdjacencyList', 'AdjacencyList' ) {
+    my $employees = $schema->resultset('Employee::Positioned');
 
-        my $employees = $schema->resultset('Employee::'.$class);
+    if ($employees->result_class->can('position_column')) {
 
-        if ($employees->result_class->can('position_column')) {
+        $employees->delete();
+        foreach (1..5) {
+            $employees->create({ name=>'temp' });
+        }
 
-            $employees->delete();
-            foreach (1..5) {
-                $employees->create({ name=>'temp' });
-            }
+        $employees = $employees->search(undef,{order_by=>'position'});
+        ok( check_positions($employees), "$class: intial positions" );
 
-            $employees = $employees->search(undef,{order_by=>'position'});
-            ok( check_positions($employees), "$class: intial positions" );
+        my $employee;
 
-            my $employee;
+        foreach my $position (1..$employees->count()) {
 
-            foreach my $position (1..$employees->count()) {
+            $employee = $employees->find({ position=>$position });
+            $employee->move_previous();
+            ok( check_positions($employees), "$class: move_previous( $position )" );
 
-                $employee = $employees->find({ position=>$position });
-                $employee->move_previous();
-                ok( check_positions($employees), "$class: move_previous( $position )" );
+            $employee = $employees->find({ position=>$position });
+            $employee->move_next();
+            ok( check_positions($employees), "$class: move_next( $position )" );
 
-                $employee = $employees->find({ position=>$position });
-                $employee->move_next();
-                ok( check_positions($employees), "$class: move_next( $position )" );
+            $employee = $employees->find({ position=>$position });
+            $employee->move_first();
+            ok( check_positions($employees), "$class: move_first( $position )" );
 
-                $employee = $employees->find({ position=>$position });
-                $employee->move_first();
-                ok( check_positions($employees), "$class: move_first( $position )" );
+            $employee = $employees->find({ position=>$position });
+            $employee->move_last();
+            ok( check_positions($employees), "$class: move_last( $position )" );
 
+            foreach my $to_position (1..$employees->count()) {
                 $employee = $employees->find({ position=>$position });
-                $employee->move_last();
-                ok( check_positions($employees), "$class: move_last( $position )" );
-
-                foreach my $to_position (1..$employees->count()) {
-                    $employee = $employees->find({ position=>$position });
-                    $employee->move_to($to_position);
-                    ok( check_positions($employees), "$class: move_to( $position => $to_position )" );
-                }
-
-            }
-        }
-        if ($employees->result_class->can('parent_column')) {
-
-            $employees->delete();
-            my $mom = $employees->create({ name=>'temp' });
-            foreach (1..14) {
-                $employees->create({ name=>'temp', parent_id=>$mom->id() });
-            }
-
-            my $children = $mom->children();
-            ok( ($children->count()==14), 'correct number of children' );
-
-            my $grandma = $mom;
-            my @children = $children->all();
-            $mom = pop(@children);
-            foreach my $child (@children) {
-                $child->parent( $mom );
+                $employee->move_to($to_position);
+                ok( check_positions($employees), "$class: move_to( $position => $to_position )" );
             }
 
-            ok( ($mom->children->count() == 13), 'correct number of grandchildren' );
-
-            if ($employees->result_class->can('position_column')) {
-                # TODO: Test positioning within a tree.
-            }
         }
     }
-
 }
 
 sub check_positions {
     my( $employees ) = @_;
     $employees->reset();
     my $expected_position = 0;
-    my $is_ok = 1;
     while (my $employee = $employees->next()) {
         $expected_position ++;
         if ($employee->position()!=$expected_position) {
-            $is_ok = 0;
-            last;
+            return 0;
         }
     }
-    return $is_ok;
+    return 1;
 }
 
 1;