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

[Jim Brandt]

The branch, 4.4/support-cf-max-values has been created
        at  355fd15e12246b94558a80d5ac8e1626eaa04e82 (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 355fd15e12246b94558a80d5ac8e1626eaa04e82
Author: Jim Brandt <jbrandt at bestpractical.com>
Date:   Wed Sep 4 10:33:37 2019 -0400

    Delete previous CF values only for single value CFs
    
    Leave existing values for multiple value CFs. A section of
    code before creating the new value checks MaxValues and makes sure
    there is room for the new value, so there is no need to delete
    after adding.

diff --git a/lib/RT/Record.pm b/lib/RT/Record.pm
index 0cc188463..bc3cbc37b 100644
--- a/lib/RT/Record.pm
+++ b/lib/RT/Record.pm
@@ -2026,7 +2026,11 @@ sub _AddCustomFieldValue {
         $new_value->Load( $new_value_id );
 
         # now that adding the new value was successful, delete the old one
-        if ( $old_value ) {
+        # But only delete if we are a single value CF. If multiple values are allowed
+        # there is nothing to do since we took care of too many values with the MaxValues
+        # checking and clean-up above.
+
+        if ( $old_value && $cf->MaxValues == 1 ) {
             my ( $val, $msg ) = $old_value->Delete();
             return ( 0, $msg ) unless $val;
         }

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