[Rt-commit] rt branch, 4.4/support-cf-max-values, created. rt-4.4.4-63-gf2a2479d1

? sunnavy sunnavy at bestpractical.com
Wed Sep 4 13:13:19 EDT 2019


The branch, 4.4/support-cf-max-values has been created
        at  f2a2479d1d287dd35313715ca0fb569d7566b1b0 (commit)

- Log -----------------------------------------------------------------
commit 3c3050702dd428c1f3bdf11f9a5e74b2f1513286
Author: Jim Brandt <jbrandt at bestpractical.com>
Date:   Wed Sep 4 10:14:14 2019 -0400

    Only remove extra CF values when they are actually extra
    
    Only remove extra CF values when $extra_values is positive.
    Previously, $extra_values would often be negative when the
    number of values is less than MaxValues, but the code to remove
    values would still run. It would exit the loop only on error
    when $extra_item was eventually undef.

diff --git a/lib/RT/CustomField.pm b/lib/RT/CustomField.pm
index c0dfd3d34..def292e9e 100644
--- a/lib/RT/CustomField.pm
+++ b/lib/RT/CustomField.pm
@@ -1759,24 +1759,29 @@ sub AddValueForObject {
 
     if ( $self->MaxValues ) {
         my $current_values = $self->ValuesForObject($obj);
-        my $extra_values = ( $current_values->Count + 1 ) - $self->MaxValues;
 
         # (The +1 is for the new value we're adding)
+        my $extra_values = ( $current_values->Count + 1 ) - $self->MaxValues;
+
 
-        # If we have a set of current values and we've gone over the maximum
-        # allowed number of values, we'll need to delete some to make room.
-        # which former values are blown away is not guaranteed
-
-        while ($extra_values) {
-            my $extra_item = $current_values->Next;
-            unless ( $extra_item->id ) {
-                $RT::Logger->crit( "We were just asked to delete "
-                    ."a custom field value that doesn't exist!" );
-                $RT::Handle->Rollback();
-                return (undef);
+        # Could have a negative value if MaxValues is greater than count
+        if ( $extra_values > 0 ) {
+
+            # If we have a set of current values and we've gone over the maximum
+            # allowed number of values, we'll need to delete some to make room.
+            # which former values are blown away is not guaranteed
+
+            while ($extra_values) {
+                my $extra_item = $current_values->Next;
+                unless ( $extra_item->id ) {
+                    $RT::Logger->crit( "We were just asked to delete "
+                        ."a custom field value that doesn't exist!" );
+                    $RT::Handle->Rollback();
+                    return (undef);
+                }
+                $extra_item->Delete;
+                $extra_values--;
             }
-            $extra_item->Delete;
-            $extra_values--;
         }
     }
 

commit 14300260e7abbf4bee49ef2adc17482cb5135881
Author: Jim Brandt <jbrandt at bestpractical.com>
Date:   Wed Sep 4 10:18:23 2019 -0400

    Confirm record is defined before calling id
    
    In cases where the $current_values iterator would run out of values
    and return undef, the id call on $extra_item would cause an
    "undefined value" exception. When called by a scrip, this causes
    the scrip to die. It also left a dangling transaction when the Rollback
    was not called, and this could cause the outer ticket transaction
    to not commit properly.

diff --git a/lib/RT/CustomField.pm b/lib/RT/CustomField.pm
index def292e9e..9c91e1cb9 100644
--- a/lib/RT/CustomField.pm
+++ b/lib/RT/CustomField.pm
@@ -1773,7 +1773,7 @@ sub AddValueForObject {
 
             while ($extra_values) {
                 my $extra_item = $current_values->Next;
-                unless ( $extra_item->id ) {
+                unless ( $extra_item && $extra_item->id ) {
                     $RT::Logger->crit( "We were just asked to delete "
                         ."a custom field value that doesn't exist!" );
                     $RT::Handle->Rollback();

commit 2534e227710b3b18278112a4f29ae68b01dc5dc2
Author: sunnavy <sunnavy at bestpractical.com>
Date:   Wed Sep 4 23:56:59 2019 +0800

    No need to delete old CF values here as AddValueForObject takes care of it

diff --git a/lib/RT/Record.pm b/lib/RT/Record.pm
index 0cc188463..3503f67f3 100644
--- a/lib/RT/Record.pm
+++ b/lib/RT/Record.pm
@@ -2025,12 +2025,6 @@ sub _AddCustomFieldValue {
         $new_value->{include_set_initial} = 1 if $args{'ForCreation'};
         $new_value->Load( $new_value_id );
 
-        # now that adding the new value was successful, delete the old one
-        if ( $old_value ) {
-            my ( $val, $msg ) = $old_value->Delete();
-            return ( 0, $msg ) unless $val;
-        }
-
         if ( $args{'RecordTransaction'} ) {
             my ( $TransactionId, $Msg, $TransactionObj ) =
               $self->_NewTransaction(

commit f2a2479d1d287dd35313715ca0fb569d7566b1b0
Author: sunnavy <sunnavy at bestpractical.com>
Date:   Thu Sep 5 01:11:21 2019 +0800

    Test adding custom field values
    
    Especially to test if old values are correctly removed when necessary.

diff --git a/t/api/customfield.t b/t/api/customfield.t
index dedeaa236..714eb23ab 100644
--- a/t/api/customfield.t
+++ b/t/api/customfield.t
@@ -382,5 +382,35 @@ $cf = RT::CustomField->new( RT->SystemUser );
 $cf->LoadByName(Name => 'TestingCF', Queue => 1, IncludeDisabled => 0 );
 ok( ! $cf->id, "Doesn't find it if IncludeDisabled => 0" );
 
+$cf->LoadByName( Name => 'TestingCF', Queue => 0, IncludeGlobal => 1 );
+is( $cf->MaxValues, 0, 'Max value is 0' );
+my $ticket = RT::Test->create_ticket( Queue => 1, Subject => 'test cf values' );
+ok( $ticket->AddCustomFieldValue( Field => $cf, Value => 'first value' ) );
+ok( $ticket->AddCustomFieldValue( Field => $cf, Value => 'second value' ) );
+
+my $cf_values = $cf->ValuesForObject($ticket);
+is( $cf_values->Count, 2, 'Found 2 values' );
+is( $ticket->CustomFieldValuesAsString( $cf, Separator => ', ' ), 'first value, second value', 'Current cf contents' );
+
+($ok, $msg) = $cf->SetMaxValues(1);
+is( $cf->MaxValues, 1, 'Max value is 1' );
+ok( $ticket->AddCustomFieldValue( Field => $cf, Value => 'third value' ) );
+
+$cf_values = $cf->ValuesForObject($ticket);
+is( $cf_values->Count, 1, 'Found 1 value' );
+is( $ticket->CustomFieldValuesAsString( $cf, Separator => ', ' ), 'third value', 'Current cf contents' );
+
+($ok, $msg) = $cf->SetMaxValues(2);
+is( $cf->MaxValues, 2, 'Max value is 2' );
+ok( $ticket->AddCustomFieldValue( Field => $cf, Value => 'forth value' ) );
+
+$cf_values = $cf->ValuesForObject($ticket);
+is( $cf_values->Count, 2, 'Found 2 values' );
+is( $ticket->CustomFieldValuesAsString( $cf, Separator => ', ' ), 'third value, forth value', 'Current cf contents' );
+
+ok( $ticket->AddCustomFieldValue( Field => $cf, Value => 'fifth value' ) );
+$cf_values = $cf->ValuesForObject($ticket);
+is( $cf_values->Count, 2, 'Found 2 values' );
+is( $ticket->CustomFieldValuesAsString( $cf, Separator => ', ' ), 'forth value, fifth value', 'Current cf contents' );
 
 done_testing;

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


More information about the rt-commit mailing list