[Jifty-commit] r4852 - in jifty/trunk: lib/Jifty/Action/Record

[jifty-commit at lists.jifty.org]

Author: trs
Date: Mon Jan 14 21:14:44 2008
New Revision: 4852

Modified:
   jifty/trunk/   (props changed)
   jifty/trunk/lib/Jifty/Action/Record/Update.pm

Log:
 r30929 at zot:  tom | 2008-01-14 21:13:37 -0500
 Compare old/new values for updating _before_ checking if we can update it (no point in saying permission denied for a column we don't want to actually change).  However, only do the comparison if we can read the field so that we don't get bogus undefs.  Everybody's happy now.
 
 Bonus: make references X by Y work when comparing old/new values


Modified: jifty/trunk/lib/Jifty/Action/Record/Update.pm
==============================================================================
--- jifty/trunk/lib/Jifty/Action/Record/Update.pm	(original)
+++ jifty/trunk/lib/Jifty/Action/Record/Update.pm	Mon Jan 14 21:14:44 2008
@@ -124,16 +124,33 @@
             $value = scalar <$value>;
         }
 
-        # Skip fields that have not changed
-        my $old = $self->record->$field;
-        # XXX TODO: This ignore "by" on columns
-        $old = $old->id if blessed($old) and $old->isa( 'Jifty::Record' );
-
-        # ID is sometimes passed in, we want to ignore it if it doesn't change
-        next if $field eq 'id'
-           and defined $old
-           and defined $value
-           and "$old" eq "$value";
+        # Skip fields that have not changed, but only if we can read the field.
+        # This prevents us from getting an $old value that is wrongly undef
+        # when really we are just denied read access.  At the same time, it means
+        # we can keep the change checks before checking if we can update.
+        
+        if ( $self->record->current_user_can('read', column => $field) ) {
+            my $old = $self->record->$field;
+
+            # Handle columns which reference other tables
+            my $col = $self->record->column( $field );
+            my $by  = defined $col->by ? $col->by : 'id';
+            $old = $old->$by if blessed($old) and $old->isa( 'Jifty::Record' );
+
+            # ID is sometimes passed in, we want to ignore it if it doesn't change
+            next if $field eq 'id'
+               and defined $old
+               and defined $value
+               and "$old" eq "$value";
+
+            # if both the new and old values are defined and equal, we don't want to change em
+            # XXX TODO "$old" is a cheap hack to scalarize datetime objects
+            next if ( defined $old and defined $value and "$old" eq "$value" );
+
+            # If _both_ the values are ''
+            next if (  (not defined $old or not length $old)
+                        and ( not defined $value or not length $value ));
+        }
 
         # Error on columns we can't update
         # <Sartak> ah ha. I think I know why passing due => undef reports
@@ -155,14 +172,6 @@
             next;
         }
 
-        # if both the new and old values are defined and equal, we don't want to change em
-        # XXX TODO "$old" is a cheap hack to scalarize datetime objects
-        next if ( defined $old and defined $value and "$old" eq "$value" );
-
-        # If _both_ the values are ''
-        next if (  (not defined $old or not length $old)
-                    and ( not defined $value or not length $value ));
-
         # Calculate the name of the setter and set; asplode on failure
         my $setter = "set_$field";
         my ( $val, $msg ) = $self->record->$setter( $value );