[ENSCORESW-675]. Adding methods for working with AltAlleleGroup members and attribs.

We needed more methods to be able to handle alt allele group interactions. We also
needed to make sure a single gene could not be added more than once. Think we got
this one fixed

modules/Bio/EnsEMBL/AltAlleleGroup.pm

@@ -105,6 +105,7 @@ use warnings;
 
 use Bio::EnsEMBL::Utils::Argument qw(rearrange);
 use Bio::EnsEMBL::Utils::Exception qw(warning throw);
+use Bio::EnsEMBL::Utils::Scalar qw(check_ref assert_integer);
 
 use base qw/Bio::EnsEMBL::Storable/;
 
@@ -147,15 +148,23 @@ sub new {
 =cut
 
 sub add_member {
-    my $self = shift;
-    my ($gene_id,$type_hash) = @_;
-    
-    my $members = $self->{'MEMBERS'};
-    push @$members,[$gene_id,$type_hash];
-    $self->{'MEMBERS'} = $members;
-    return;
+  my ($self, $gene_id,$type_hash) = @_;
+  if(!$self->contains_member($gene_id)) {
+    push(@{$self->{MEMBERS}}, [$gene_id, {}]);
+    $self->set_attribs($gene_id, $type_hash);
+  }
+  return;
 }
 
+=head2 get_all_members_with_type
+
+  Arg [1]     : String The type to search members by
+  Description : Loops through the internal members array returning all
+                attributes of the same type as what has been specified
+  Example     : my $members = $aag->get_all_members_with_type('IS_VALID_ALTERNATE');
+
+=cut
+
 sub get_all_members_with_type {
     my $self = shift;
     my $type = shift;
@@ -170,33 +179,173 @@ sub get_all_members_with_type {
     return \@filtered_members;
 }
 
+=head2 attribs
+
+  Arg [1]     : Int gene id to record attributes against
+  Description : Returns all known attributes of the given gene id. Attributes
+                are returned as a HashRef but is a copy of the interally
+                held attribute list
+  Returntype  : HashRef copy of all the given id's attributes
+  Example     : $aag->attribs(10, 'IS_VALID_ALTERNATE');
+                $aag->attribs(10, [ 'IS_VALID_ALTERNATE' ]);
+                $aag->attribs(10, {IS_VALID_ALTERNATE => 1});
+
+=cut
+
 sub attribs {
-    my $self = shift;
-    my $member_id = shift;
-    
-    
+  my ($self, $gene_id) = @_;
+  assert_integer($gene_id, 'gene_id');
+  foreach my $member (@{$self->{MEMBERS}}) {
+    if($member->[0] == $gene_id) {
+      my $attribs = $member->[1];
+      return { %{$attribs} }; # make a copy. never leak
+    }
+  }
+  return {};
+}
+
+=head2 set_attribs
+
+  Arg [1]     : Int gene id to set attributes against
+  Arg [2]     : ArrayRef/HashRef/Scalar The attribute you wish to record
+  Description : Adds the given type to the specified gene id in this group. You
+                can specify the type using an ArrayRef, HashRef or a single scalar
+  Example     : $aag->attribs(10, 'IS_VALID_ALTERNATE');
+                $aag->attribs(10, [ 'IS_VALID_ALTERNATE' ]);
+                $aag->attribs(10, {IS_VALID_ALTERNATE => 1});
+
+=cut
+
+sub set_attribs {
+  my ($self, $gene_id, $attribs) = @_;
+  assert_integer($gene_id, 'gene_id');
+  my $current_attribs = $self->attribs($gene_id);
+  if(check_ref($attribs, 'ARRAY')) {
+    #Loop & add
+    $current_attribs->{uc($_)} = 1 for @{$attribs};
+  }
+  elsif(check_ref($attribs, 'HASH')) {
+    #loop through the keys adding them in
+    foreach my $key (keys %{$attribs}) {
+      $current_attribs->{uc($key)} = 1;
+    }
+  }
+  #Simple scalar value so just add it in
+  else {
+    $current_attribs->{uc($attribs)} = 1;
+  }
+  foreach my $member (@{$self->{MEMBERS}}) {
+    if($member->[0] == $gene_id) {
+      $member->[1] = $current_attribs;
+    }
+  }
+  return;
+}
+
+=head2 remove_attribs
+
+  Arg [1]     : Int gene id to retrieve attributes against
+  Arg [2]     : ArrayRef/HashRef/Scalar The attribute you wish to remove
+  Description : Removes the given type from this group against the specified
+                gene identifier
+  Example     : $aag->remove_attribs(10, 'IS_VALID_ALTERNATE');
+                $aag->remove_attribs(10, [ 'IS_VALID_ALTERNATE' ]);
+                $aag->remove_attribs(10, {IS_VALID_ALTERNATE => 1});
+
+=cut
+
+sub remove_attribs {
+  my ($self, $gene_id, $attribs) = @_;
+  assert_integer($gene_id, 'gene_id');
+  my @to_remove;
+  if(check_ref($attribs, 'ARRAY')) {
+    @to_remove = map { uc($_) } @{$attribs};
+  }
+  elsif(check_ref($attribs, 'HASH')) {
+    @to_remove = map { uc($_) } keys %{$attribs};
+  }
+  #Simple scalar value so just add it in
+  else {
+    @to_remove = uc($attribs);
+  }
+  foreach my $member (@{$self->{MEMBERS}}) {
+    if($member->[0] == $gene_id) {
+      my $current_attribs = $member->[1];
+      delete $current_attribs->{$_} for @to_remove;
+    }
+  }
+  return;
+}
+
+=head2 remove_member
+
+  Arg [1]     : Int gene id to retrieve attributes against
+  Arg [2]     : ArrayRef/HashRef/Scalar The attribute you wish to remove
+  Description : Removes the given member from this group. Any changes
+                must be persisted back to the database via update() or
+                store() methods in Bio::EnsEMBL::DBSQL::AltAlleleGroupAdaptor.
+  Example     : $aag->remove_member(10);
+
+=cut
+
+sub remove_member {
+  my ($self, $gene_id) = @_;
+  assert_integer($gene_id, 'gene_id');
+  my $members = $self->{MEMBERS};
+  my $size = scalar(@{$members});
+  for(my $i = 0; $i < $size; $i++) {
+    my $current_id = $members->[$i]->[0];
+    #If this was the ID then splice it out of the array and exit
+    if($current_id == $gene_id) {
+      splice(@{$members}, $i, 1);
+      last;
+    }
+  }
+  return;
+}
+
+=head2 contains_member
+
+  Arg [1]     : Int gene id to retrieve attributes against
+  Description : Searches through the members list looking for the
+                specified gene id. Returns true if it was found
+                or false if not.
+  Returntype  : Boolean indicating if the given gene id is held in this group
+  Example     : $aag->contains_member(10);
+
+=cut
+
+sub contains_member {
+  my ($self, $gene_id) = @_;
+  assert_integer($gene_id, 'gene_id');
+  foreach my $member (@{$self->{MEMBERS}}) {
+    if($member->[0] == $gene_id) {
+      return 1;
+    }
+  }
+  return 0;
 }
 
 =head2 remove_all_members
 
-  Description: Remove members from this object, but NOT the database. See
-               AltAlleleGroupAdaptor->remove()
-               Use in conjunction with add_member if members need to be altered
+  Description : Remove members from this object, but NOT the database. See
+                AltAlleleGroupAdaptor->remove() to remove the group from the
+                database
 =cut
 
 sub remove_all_members {
-    my $self = shift;
-    $self->{'MEMBERS'} = [];
-    return;
+  my $self = shift;
+  $self->{'MEMBERS'} = [];
+  return;
 }
 
 =head2 rep_Gene_id
 
-  Arg[1]     : Optional - set a new representative Gene id for the group
-  Description: Reports or sets the representative Gene for this AltAlleleGroup
-               If you wish to remove the representative status of all genes without
-               setting a new one, see unset_rep_Gene_id
-  Returntype : Integer or undef if none set
+  Arg[1]      : Optional - set a new representative Gene id for the group
+  Description : Reports or sets the representative Gene for this AltAlleleGroup
+                If you wish to remove the representative status of all genes without
+                setting a new one, see unset_rep_Gene_id
+  Returntype  : Integer or undef if none set
 =cut
 
 sub rep_Gene_id {
@@ -235,10 +384,9 @@ sub rep_Gene_id {
 
 =head2 unset_rep_Gene_id
 
-  Description: Removes the representative Gene flag from this AltAlleleGroup.
-               This action is not possible through rep_Gene_id due to
-               validation of inputs.
-  Returntype : 
+  Description : Removes the representative Gene flag from this AltAlleleGroup.
+                This action is not possible through rep_Gene_id due to
+                validation of inputs.
 
 =cut
 
@@ -275,9 +423,18 @@ sub get_all_Gene_ids {
         if ($all_but_rep && $type->{IS_REPRESENTATIVE}) {next;} 
         push @gene_ids,$gene_id;
     }
-    return \@gene_ids;
+    return [sort {$a <=> $b} @gene_ids];
 }
 
+=head2 get_representative_Gene
+
+  Description : Used to fetch a Gene object which has been marked as the
+                representative Gene for this alt allele group.
+  Returntype  : Bio::EnsEMBL::Gene object which is the representative gene
+
+=cut
+
+
 sub get_representative_Gene {
     my $self = shift;
     my $ga = $self->adaptor->db->get_GeneAdaptor;
@@ -285,34 +442,77 @@ sub get_representative_Gene {
     return $ga->fetch_by_dbID($self->rep_Gene_id);
 }
 
+=head2 get_all_Genes
+
+  Arg[1]      : Boolean - Do not include representative gene in list of ids.
+  Description : Fetches all the Gene objects within the allele group. It can also
+                be used to list those Genes that are not the representative Gene.
+  Returntype  : ArrayRef of Bio::EnsEMBL::Gene objects
+
+=cut
+
+
 sub get_all_Genes {
-    my $self = shift;
-    my $all_but_rep = shift; # falls through to get_all_Gene_ids
-    my $gene_ids = $self->get_all_Gene_ids($all_but_rep);
-    my $genes;
-    my $ga = $self->adaptor->db->get_GeneAdaptor;
-    $genes = $ga->fetch_all_by_dbID_list($gene_ids);
-    
-    return $genes;
+  my ($self, $all_but_rep) = @_;
+  my $gene_ids = $self->get_all_Gene_ids($all_but_rep);
+  return $self->adaptor()->db()->get_GeneAdaptor()->fetch_all_by_dbID_list($gene_ids);
 }
 
+
+=head2 get_all_Gene_ids
+
+  Arg[1]      : Boolean - Do not include representative gene in list of ids.
+  Description : Fetches all the Gene objects within the allele group and their
+                associcated attributes. It can also be used to list those 
+                Genes that are not the representative Gene.
+  Returntype  : ArrayRef. 2 dimensional holding [Bio::EnsEMBL::Gene, {attribute_hash}]
+
+=cut
+
+sub get_all_Genes_types {
+  my ($self, $all_but_rep) = @_;
+  my $gene_ids = $self->get_all_Gene_ids($all_but_rep);
+  my $ga = $self->adaptor()->db()->get_GeneAdaptor();
+  my @output;
+  my $members = $self->{MEMBERS};
+  foreach my $allele (@{$members}) {
+    my ($gene_id,$attribs) = @$allele;
+    if ($all_but_rep && $attribs->{IS_REPRESENTATIVE}) {next;} 
+    my $gene = $ga->fetch_by_dbID($gene_id);
+    my %attribs_copy = %{$attribs};
+    push(@output, [$gene, \%attribs_copy])
+  }
+  return \@output;
+}
+
+=head2 size
+
+  Description : Returns the current size of this group in members
+  Returntype  : Int the size of the current alt allele group
+
+=cut
+
 sub size {
-    my $self = shift;
-    my $list = $self->{'MEMBERS'};
-    return scalar(@$list);
+  my $self = shift;
+  my $list = $self->{'MEMBERS'};
+  return scalar(@$list);
 }
 
 
 =head2 get_all_members
-  Description: Retrieves all of the information about all members.
-  Returntype : Listref of id and type list: [gene_id,type]
-  Caller     : AltAlleleGroupAdaptor->store
+
+  Description : Retrieves all of the information about all members. Be aware
+                that this emits the interal data structure so direct modification
+                should be done with caution.
+  Returntype  : ArrayRef of id and type list: [gene_id,type]
+  Caller      : AltAlleleGroupAdaptor->store
+
 =cut
 
 sub get_all_members {
-    my $self = shift;
-    my $members = $self->{'MEMBERS'};
-    return $members;
+  my $self = shift;
+  my $members = $self->{'MEMBERS'};
+  return $members;
 }
 
 

modules/Bio/EnsEMBL/DBSQL/AltAlleleGroupAdaptor.pm

@@ -59,7 +59,8 @@ use warnings;
 use base qw/Bio::EnsEMBL::DBSQL::BaseAdaptor/;
 
 use Bio::EnsEMBL::AltAlleleGroup;
-use Bio::EnsEMBL::Utils::Exception;
+use Bio::EnsEMBL::Utils::Exception qw/throw/;
+use Bio::EnsEMBL::Utils::Scalar qw/assert_ref/;
 use DBI qw( :sql_types );
 
 =head2 fetch_all_Groups
@@ -259,69 +260,65 @@ sub fetch_Group_by_Gene_dbID {
 sub store {
     my $self = shift;
     my $allele_group = shift;
+
+    assert_ref($allele_group, 'Bio::EnsEMBL::AltAlleleGroup', 'allele_group');
+    if ($allele_group->size < 2) {
+        warning('At least 2 genes must be provided to construct alternative alleles. Ignoring.');
+        return;
+    }
     
-    if (ref($allele_group) ne "Bio::EnsEMBL::AltAlleleGroup") {
-        throw ("Can only store Bio::EnsEMBL::AltAlleleGroup objects.");   
-    } else {
-        if ($allele_group->size < 2) {
-            warning('At least 2 genes must be provided to construct alternative alleles. Ignoring.');
+    my $helper = $self->dbc()->sql_helper();
+    my $dbID = $allele_group->dbID;
+    
+    my $new_group_sql = 'INSERT INTO alt_allele_group (alt_allele_group_id) VALUES (?)';
+    my $existing_group_sql = 'SELECT count(*) FROM alt_allele_group WHERE alt_allele_group_id = ?';
+
+    my $already_exists = $helper->execute_single_result(-SQL => $existing_group_sql, -PARAMS => [[$dbID, SQL_INTEGER]]);
+    
+    # If the ID is not already there then we need to add one
+    if($already_exists == 0) {
+        $helper->execute_update(-SQL => $new_group_sql, -CALLBACK => sub {
+            my ($sth, $dbh, $rv) = @_;
+            if($rv) {
+                my $id = $dbh->last_insert_id(undef, undef, 'alt_allele_group', 'alt_allele_group_id');
+                $dbID = $id;
+            }
             return;
-        }
-        
-        my $dbID = $allele_group->dbID;
-        
-        my $new_group_sth = $self->prepare("INSERT INTO alt_allele_group (alt_allele_group_id) VALUES (?)");
-        my $group_sth = $self->prepare("SELECT alt_allele_group_id FROM alt_allele_group WHERE alt_allele_group_id = ?");
-        my $altered_rows;
+        });
+    }
+    
+    my $sth = $self->prepare("INSERT INTO alt_allele (alt_allele_id, alt_allele_group_id, gene_id) VALUES (?,?,?)");
+    my $attrib_sth = $self->prepare("INSERT INTO alt_allele_attrib (alt_allele_id,attrib) VALUES (?,?)");
+    
+    foreach my $allele (@{ $allele_group->get_all_members() }) {
+        my $gene_id = $allele->[0];
+        my %flags = %{$allele->[1]};
         
-        # Do not create a new group ID if one already exists, such as when updating a group.
-        my $existing_rows = $group_sth->execute($dbID);
-        if ($existing_rows == 0) {
-            $altered_rows = $new_group_sth->execute($dbID);
-            
-            if ($altered_rows > 0) {
-                $dbID = $self->last_insert_id(undef,undef,undef,'alt_allele_group');
-                $allele_group->dbID($dbID);
-            } 
+        $sth->bind_param(1, undef, SQL_INTEGER);
+        $sth->bind_param(2, $dbID, SQL_INTEGER);
+        $sth->bind_param(3, $gene_id, SQL_INTEGER);
+        my $altered_rows = $sth->execute();
+        my $allele_id;
+        if ($altered_rows > 0) {
+            $allele_id = $self->last_insert_id(); # all alleles get added to the same alt_allele_id group
+        } else {
+            throw("Creation of new alt_allele failed: $@");
         }
         
-        my $sth = $self->prepare("INSERT INTO alt_allele (alt_allele_id, alt_allele_group_id, gene_id) VALUES (?,?,?)");
-        my $attrib_sth = $self->prepare("INSERT INTO alt_allele_attrib (alt_allele_id,attrib) VALUES (?,?)");
-        
-        foreach my $allele (@{ $allele_group->get_all_members() }) {
-            my $gene_id = $allele->[0];
-            my %flags = %{$allele->[1]};
-            
-            $sth->bind_param(1, undef, SQL_INTEGER);
-            $sth->bind_param(2, $dbID, SQL_INTEGER);
-            $sth->bind_param(3, $gene_id, SQL_INTEGER);
-            my $altered_rows = $sth->execute();
-            my $allele_id;
-            if ($altered_rows > 0) {
-                $allele_id = $self->last_insert_id(); # all alleles get added to the same alt_allele_id group
-            } else {
-                throw("Creation of new alt_allele failed: $@");
-            }
-            
-                
-            foreach my $flag (keys %flags) {
-                $attrib_sth->bind_param(1, $allele_id);
-                $attrib_sth->bind_param(2, $flag);
-                $attrib_sth->execute();
-            }
             
-            if (! $dbID) {
-                $group_sth->bind_param(1, $dbID);
-                $group_sth->execute($allele_id);
-            }
+        foreach my $flag (keys %flags) {
+            $attrib_sth->bind_param(1, $allele_id);
+            $attrib_sth->bind_param(2, $flag);
+            $attrib_sth->execute();
         }
-        if ($@) {throw ("Problem inserting new AltAlleleGroup into database: $@");}
-        $sth->finish;
-        $attrib_sth->finish;
-        $group_sth->finish;
-        
-        return $dbID;
     }
+    if ($@) {throw ("Problem inserting new AltAlleleGroup into database: $@");}
+    $sth->finish;
+    $attrib_sth->finish;
+    
+    $allele_group->dbID($dbID);
+    
+    return $dbID;
 }
 
 =head2 update
@@ -336,19 +333,17 @@ sub store {
 sub update {
     my $self = shift;
     my $allele_group = shift;
-    
-    if (ref($allele_group) ne "Bio::EnsEMBL::AltAlleleGroup") {
-        throw ("Can only update Bio::EnsEMBL::AltAlleleGroup objects.");   
-    } elsif (!$allele_group->dbID) {
-        throw ("Cannot update AltAlleleGroup with missing dbID. AltAlleleGroups should be fetched from the DB prior to updating them");
-    }    
-    $self->remove($allele_group);
+    assert_ref($allele_group, 'Bio::EnsEMBL::AltAlleleGroup', 'allele_group');
+    throw "Cannot update an AltAlleleGroup without a dbID. AltAlleleGroups should be fetched from the DB prior to updating them" if ! $allele_group->dbID();
+    my $keep_group = 1;
+    $self->remove($allele_group, $keep_group);
     return $self->store($allele_group);
 }
 
 =head2 remove
 
   Arg [1]    : The AltAlleleGroup to remove.
+  Arg [2]    : Boolean indicates if the entry in alt_allele_group should be retained or remove. Defaults to removing the entry
   Example    : $aaga->remove($alt_allele_group);
   Description: This removes an AltAlleleGroup from all tables of the database. 
   Exceptions : None
@@ -356,38 +351,22 @@ sub update {
 =cut
 
 sub remove {
-    my $self = shift;
-    my $allele_group = shift;
-    
-    my $group_id;
-    if (ref($allele_group) eq "Bio::EnsEMBL::AltAlleleGroup") {
-        $group_id = $allele_group->dbID;
-    } else {
-        throw("Cannot remove a non-AltAlleleGroup.");
-    }
-    
-    my $allele_sth = $self->prepare("SELECT alt_allele_id FROM alt_allele WHERE alt_allele_group_id = ?");
-    my $attrib_sth = $self->prepare("DELETE FROM alt_allele_attrib WHERE alt_allele_id IN (". join(',', ('?')x$allele_group->size) .")" );
-
-    $allele_sth->execute($group_id);
-    
-    my $allele_id;
-    $allele_sth->bind_columns(\$allele_id);
-    my @ids;
-    while ($allele_sth->fetch) {
-        push @ids,$allele_id;
-        $attrib_sth->bind_param($#ids+1,$ids[$#ids],SQL_INTEGER);
+    my ($self, $allele_group, $keep_group) = @_;
+    assert_ref($allele_group, 'Bio::EnsEMBL::AltAlleleGroup', 'allele_group');
+
+    my $helper = $self->dbc()->sql_helper();
+    my $delete_attribs_sql = 'DELETE aaa FROM alt_allele_attrib aaa join alt_allele aa using (alt_allele_id) where alt_allele_group_id =?';
+    my $delete_alt_alleles_sql = 'DELETE FROM alt_allele where alt_allele_group_id =?';
+    my $delete_group_sql = 'DELETE from alt_allele_group where alt_allele_group_id =?';
+    my $params = [[$allele_group->dbID, SQL_INTEGER]];
+
+    $helper->execute_update(-SQL => $delete_attribs_sql, -PARAMS => $params);
+    $helper->execute_update(-SQL => $delete_alt_alleles_sql, -PARAMS => $params);
+    if(! $keep_group) {
+        $helper->execute_update(-SQL => $delete_group_sql, -PARAMS => $params);
     }
-    $attrib_sth->execute();
 
-    my $sth = $self->prepare("DELETE FROM alt_allele WHERE alt_allele_group_id = ?");
-    
-    $sth->bind_param(1,$group_id,SQL_INTEGER);
-    
-    $sth->execute;
-    $sth->finish;
-    $allele_sth->finish;
-    $attrib_sth->finish;
+    return;
 }
 
 sub _tables {

modules/t/altAlleleGroup.t

@@ -51,12 +51,37 @@ ok($group->rep_Gene_id(3) == 3,"Successful setting of reference gene");
 $other_group->remove_all_members;
 ok($other_group->size == 0, "Test remove_all_members");
 
+# Test the methods which attempt to modify an AltAlleleGroup object
+{
+    my $copy_group = bless({%{$group}}, ref($group));
+    ok($copy_group->contains_member(1), 'Group contains the member 1');
+    ok(!$copy_group->contains_member(4), 'Group does not contain the member 4');
+    is_deeply($copy_group->attribs(1), {}, 'Group member 1 attributes are empty');
+    $copy_group->set_attribs(1, ['IN_CORRECTED_ASSEMBLY']);
+    $copy_group->set_attribs(1, {'manually_assigned' => 1}); #lower case intentional
+    $copy_group->set_attribs(1, 'IS_VALID_ALTERNATE');
+    is_deeply($copy_group->attribs(1), { map { $_, 1} qw/IN_CORRECTED_ASSEMBLY IS_VALID_ALTERNATE MANUALLY_ASSIGNED/ }, 'Group member 1 attributes are empty');
+    $copy_group->add_member(1,{});
+    is_deeply($copy_group->attribs(1), { map { $_, 1} qw/IN_CORRECTED_ASSEMBLY IS_VALID_ALTERNATE MANUALLY_ASSIGNED/ }, 'Attributes were not replaced because the member was already in');
+    $copy_group->remove_attribs(1, 'IS_VALID_ALTERNATE');
+    $copy_group->remove_attribs(1, ['IN_CORRECTED_ASSEMBLY']);
+    $copy_group->remove_attribs(1, {MANUALLY_ASSIGNED => 1});
+    is_deeply($copy_group->attribs(1), {}, 'Group member 1 attributes are empty');
+
+    $copy_group->remove_member(4);
+    is($copy_group->size(), 3, 'Removing an non-existent ID does nothing');
+    $copy_group->remove_member(1);
+    is($copy_group->size(), 2, 'Removing ID 1 reduces our group size');
+    ok(!$copy_group->contains_member(1), 'Group does not contain the member 1 post removal');
+    $copy_group->add_member(1,{});
+    is_deeply($copy_group->attribs(1), {}, 'Group member 1 attributes are empty post removal and addition');
+}
+
 # Tests for methods applied to test db
 
 my $multi = Bio::EnsEMBL::Test::MultiTestDB->new();
-ok(1);
 my $db = $multi->get_DBAdaptor( 'core' );
-ok($db);
+ok($db, 'DB was retrieved');
 
 my $aaga = $db->get_adaptor('AltAlleleGroup');
 # Test data consists of a single group, of type AUTOMATIC, with a reference Allele and 3 others
@@ -87,11 +112,11 @@ $aag = $group_list->[0];
 my $group_id = $aag->dbID;
 my $new_aag = $aaga->fetch_Group_by_id($group_id);
 
-is_deeply($aag,$new_aag,"Compare previously fetched group with group found by using the dbID");
+is_deeply($aag, $new_aag, "Compare previously fetched group with group found by using the dbID");
 
 $aag = $aaga->fetch_Group_by_id(undef);
 
-ok(!defined($aag),"See what happens if no argument is given");
+ok(!defined($aag), "See what happens if no argument is given");
 
 # fetch_Group_by_Gene_dbID
 $aag = $aaga->fetch_all_Groups->[0];
@@ -100,20 +125,25 @@ is_deeply($new_aag,$aag,"Check single gene ID returns entire group correctly");
 
 # check store method
 my $dbID = $aaga->store($group);
-ok($dbID);
-
-my $aag2 = $aaga->fetch_Group_by_id($dbID);
-is_deeply($aag2->get_all_members,$group->get_all_members,"Compare stored with original");
-ok( $aaga->remove($aag2) );
-$group->dbID($dbID);
-$group->rep_Gene_id(1);
-note("dbID = ".$group->dbID());
-my $new_dbID = $aaga->update($group);
-note($new_dbID);
-$aag2 = $aaga->fetch_Group_by_id($dbID);
-
-is_deeply($aag2->get_all_Gene_ids,[1,2,3], "Update and re-retrieve the same AltAlleleGroup") or diag explain $aag2->get_all_Gene_ids;
-eq_or_diff($aag2->get_all_Gene_ids,[1,2,3], "Update and re-retrieve the same AltAlleleGroup");
+ok($dbID, 'A dbID was returned from the store method');
+
+{
+    my $aag2 = $aaga->fetch_Group_by_id($dbID);
+    is_deeply($aag2->get_all_members,$group->get_all_members,"Compare stored with original");
+    $group->add_member(4, {});
+    my $update_dbID = $aaga->update($group);
+    cmp_ok($update_dbID, '==', $dbID, 'We should have kept the db id of the group');
+}
+
+$group->remove_member(4);
+$aaga->remove($group);
+ok(! defined $aaga->fetch_Group_by_id($dbID), 'Using a deleted ID means no group returned');
+my $new_dbID = $aaga->store($group);
+
+cmp_ok($new_dbID, '!=', $dbID, 'Should have been assgined a new ID');
+my $aag2 = $aaga->fetch_Group_by_id($new_dbID);
+my $gene_ids = $aag2->get_all_Gene_ids();
+eq_or_diff($gene_ids,[1,2,3], "Update and re-retrieve the same AltAlleleGroup") or diag explain $gene_ids;
 
 # Vaguely verify the AltAlleleGroupAdaptor's fetch_all_Groups with a multispecies database
 # Proper test data is hard to fabricate and no samples exist.