[Jifty-commit] r3864 - in jifty/trunk: lib/Jifty

jifty-commit at lists.jifty.org jifty-commit at lists.jifty.org
Sun Aug 12 21:17:57 EDT 2007


Author: sterling
Date: Sun Aug 12 21:17:57 2007
New Revision: 3864

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

Log:
 r8583 at riddle:  andrew | 2007-08-12 20:17:16 -0500
 Improved code comments and minor perl tidy.


Modified: jifty/trunk/lib/Jifty/Action.pm
==============================================================================
--- jifty/trunk/lib/Jifty/Action.pm	(original)
+++ jifty/trunk/lib/Jifty/Action.pm	Sun Aug 12 21:17:57 2007
@@ -113,22 +113,33 @@
         current_user => undef,
         @_);
 
+    # Setup current user according to parameter or pickup the actual
     if ($args{'current_user'}) {
         $self->current_user($args{current_user});
     } else {
         $self->_get_current_user();
     }
 
-
+    # If given a moniker, validate/sanitize it
     if ( $args{'moniker'} ) {
+
+        # XXX Should this be pickier about sanitized monikers?
+
+        # Monkiers must not contain semi-colons
         if ( $args{'moniker'} =~ /[\;]/ ) {
+
+            # Replace the semis with underscores and warn
             $args{'moniker'} =~ s/[\;]/_/g;
             $self->log->warn(
                 "Moniker @{[$args{'moniker'}]} contains invalid characters. It should not contain any ';' characters. "
                     . "It has been autocorrected, but you should correct your code"
             );
         }
+
+        # Monikers must not start with a digit
         if ( $args{'moniker'} =~ /^\d/ ) {
+
+            # Stick "fixup-" to the front and warn
             $args{'moniker'} = "fixup-" . $args{'moniker'};
             $self->log->warn(
                 "Moniker @{[$args{'moniker'}]} contains invalid characters. It can not begin with a digit. "
@@ -138,28 +149,29 @@
         }
     }
 
-
+    # Setup the moniker and run order
     $self->moniker($args{'moniker'} || $self->_generate_moniker);
     $self->order($args{'order'});
 
+    # Fetch any arguments from a passed in request
     my $action_in_request = Jifty->web->request->action( $self->moniker );
-    # Fields explicitly passed to new_action take precedence over those passed
-    # from the request; we read from the request to implement "sticky fields".
     if ( $action_in_request and $action_in_request->arguments ) {
         $args{'request_arguments'} = $action_in_request->arguments;
     }
 
+    # Setup the argument values with the new_action arguments taking precedent
     $self->argument_values( { %{ $args{'request_arguments' } }, %{ $args{'arguments'} } } );
 
-    # Keep track of whether arguments came from the request, or were
-    # programmatically set elsewhere
+    # Track how an argument was set, again new_action args taking precedent
     $self->values_from_request({});
     $self->values_from_request->{$_} = 1 for keys %{ $args{'request_arguments' } };
     $self->values_from_request->{$_} = 0 for keys %{ $args{'arguments' } };
     
+    # Place this actions result in the response result if already processed
     $self->result(Jifty->web->response->result($self->moniker) || Jifty::Result->new);
     $self->result->action_class(ref($self));
 
+    # Remember stickiness
     $self->sticky_on_success($args{sticky_on_success});
     $self->sticky_on_failure($args{sticky_on_failure});
 
@@ -182,24 +194,46 @@
 sub _generate_moniker {
     my $self = shift;
 
+    # We use Digest::MD5 to generate the moniker
     use Digest::MD5 qw(md5_hex);
+
+    # Use information from the call stack as the data for the digest 
     my $frame = 1;
     my @stack = (ref($self) || $self);
     while (my ($pkg, $filename, $line) = caller($frame++)) {
         push @stack, $pkg, $filename, $line;
     }
 
-    # Increment the per-request moniker digest counter, for the case of looped action generation
+    # Generate the digest that forms the basis of the auto-moniker
     my $digest = md5_hex("@stack");
+
+    # Increment the per-request moniker digest counter, for the case of looped action generation
     # We should always have a stash. but if we don't, fake something up
     # (some hiveminder tests create actions outside of a Jifty::Web)
-    my $serial = Jifty->handler->stash ? ++(Jifty->handler->stash->{monikers}{$digest}) : rand();
+    # Multiple things happening here that need to be noted:
+    #  
+    #  1. We have a per-request moniker digest counter, which handles the
+    #     highly unlikely circumstance that the same digest were hit twice
+    #     within the same request.
+    #
+    #  2. We should always have a stash, but sometimes we don't. (Specifically,
+    #     some Hiveminder tests create actions outside of a Jifty::Web, which
+    #     don't.) In that case, add more random data at the end and cross our
+    #     fingers that we don't hit that one in a billion (or actually one in a
+    #     significantly larger than a billion odds here).
+
+    # Create a serial number that prevents collisions within a request
+    my $serial = Jifty->handler->stash 
+        ? ++(Jifty->handler->stash->{monikers}{$digest}) 
+        : rand();
+
+    # Build the actual moniker from digest + serial
     my $moniker = "auto-$digest-$serial";
     $self->log->debug("Generating moniker $moniker from stack for $self");
+
     return $moniker;
 }
 
-
 =head2 arguments
 
 B<Note>: this API is now deprecated in favour of the declarative syntax
@@ -254,6 +288,8 @@
 sub run {
     my $self = shift;
     $self->log->debug("Running action ".ref($self) . " " .$self->moniker);
+
+    # We've already had a validation failure. STOP!
     unless ($self->result->success) {
         $self->log->debug("Not taking action, as it doesn't validate");
 
@@ -270,16 +306,20 @@
 
         return;
     }
+
+    # Made it past validation, continue...
     $self->log->debug("Taking action ".ref($self) . " " .$self->moniker);
+
+    # Take the action (user-defined)
     my $ret = $self->take_action;
     $self->log->debug("Result: ".(defined $ret ? $ret : "(undef)"));
     
+    # Perform post action clean-up (user-defined)
     $self->cleanup;
 }
 
 =head2 validate
 
-
 Checks authorization with L</check_authorization>, calls C</setup>,
 canonicalizes and validates each argument that was submitted, but
 doesn't actually call L</take_action>.
@@ -307,7 +347,6 @@
 
 sub check_authorization { 1; }
 
-
 =head2 setup
 
 C<setup> is expected to return a true value, or
@@ -319,7 +358,6 @@
 
 sub setup { 1; }
 
-
 =head2 take_action
 
 Do whatever the action is supposed to do.  This and
@@ -334,7 +372,6 @@
 
 sub take_action { 1; }
 
-
 =head2 cleanup
 
 Perform any action-specific cleanup.  By default, does nothing.
@@ -360,14 +397,16 @@
     my $self = shift;
     my $arg = shift;
 
+    # Not only get it, but set it
     if(@_) {
         $self->values_from_request->{$arg} = 0;
         $self->argument_values->{$arg} = shift;
     }
+
+    # Get it
     return $self->argument_values->{$arg};
 }
 
-
 =head2 has_argument ARGUMENT
 
 Returns true if the action has been provided with an value for the
@@ -383,7 +422,6 @@
     return exists $self->argument_values->{$arg};
 }
 
-
 =head2 form_field ARGUMENT
 
 Returns a L<Jifty::Web::Form::Field> object for this argument.  If
@@ -392,14 +430,15 @@
 
 =cut
 
-
 sub form_field {
     my $self = shift;
     my $arg_name = shift;
 
+    # Determine whether we want reads or write on this field
     my $mode = $self->arguments->{$arg_name}{'render_mode'};
     $mode = 'update' unless $mode && $mode eq 'read';
 
+    # Return the widget
     $self->_form_widget( argument => $arg_name,
                          render_mode => $mode,
                          @_);
@@ -418,6 +457,8 @@
 sub form_value {
     my $self = shift;
     my $arg_name = shift;
+
+    # Return the widget, but in read mode
     $self->_form_widget( argument => $arg_name,
                          render_mode => 'read',
                          @_);
@@ -431,17 +472,17 @@
                  render_mode => 'update',
                  @_);
 
+    # Setup the field name
     my $field = $args{'argument'};
-    
     my $arg_name = $field. '!!' .$args{'render_mode'};
 
+    # This particular field hasn't been added to the form yet
     if ( not exists $self->{_private_form_fields_hash}{$arg_name} ) {
-
         my $field_info = $self->arguments->{$field};
-
         my $sticky = 0;
+
         # Check stickiness iff the values came from the request
-        if(Jifty->web->response->result($self->moniker)) {
+        if (Jifty->web->response->result($self->moniker)) {
             $sticky = 1 if $self->sticky_on_failure and $self->result->failure;
             $sticky = 1 if $self->sticky_on_success and $self->result->success;
         }
@@ -449,7 +490,9 @@
         # $sticky can be overrided per-parameter
         $sticky = $field_info->{sticky} if defined $field_info->{sticky};
 
+        # It is in fact a form field for this action
         if ($field_info) {
+            
             # form_fields overrides stickiness of what the user last entered.
             my $default_value;
             $default_value = $field_info->{'default_value'}
@@ -457,6 +500,7 @@
             $default_value = $self->argument_value($field)
               if $self->has_argument($field) && !$self->values_from_request->{$field};
 
+            # Add the form field to the cache
             $self->{_private_form_fields_hash}{$arg_name}
                 = Jifty::Web::Form::Field->new(
                 %$field_info,
@@ -469,14 +513,22 @@
                 %args
                 );
 
-        }    # else $field remains undef
+        }    
+
+        # The field name is not known by this action
         else {
             Jifty->log->warn("$arg_name isn't a valid field for $self");
         }
-    } elsif ( $args{render_as} ) {
+    } 
+    
+    # It has been cached, but render_as is explicitly set
+    elsif ( $args{render_as} ) {
+
+        # Rebless the form control as something else
         bless $self->{_private_form_fields_hash}{$arg_name},
           "Jifty::Web::Form::Field::$args{render_as}";
     }
+
     return $self->{_private_form_fields_hash}{$arg_name};
 }
 
@@ -490,6 +542,8 @@
 sub hidden {
     my $self = shift;
     my ($arg, $value, @other) = @_;
+
+    # Return the control as a hidden widget
     $self->form_field( $arg, render_as => 'hidden', default_value => $value, @other);
 }
 
@@ -519,16 +573,16 @@
 
 sub register {
     my $self = shift;
+
+    # Add information about the action to the form
     Jifty->web->out( qq!<div class="hidden"><input type="hidden"! .
                        qq! name="@{[$self->register_name]}"! .
                        qq! id="@{[$self->register_name]}"! .
                        qq! value="@{[ref($self)]}"! .
                        qq! /></div>\n! );
 
-
-
+    # Add all the default values as hidden fields to the form
     my %args = %{$self->arguments};
-
     while ( my ( $name, $info ) = each %args ) {
         next unless $info->{'constructor'};
         Jifty::Web::Form::Field->new(
@@ -540,6 +594,7 @@
             render_as     => 'Hidden'
         )->render();
     }
+
     return '';
 }
 
@@ -553,6 +608,7 @@
 sub render_errors {
     my $self = shift;
     
+    # Render the span that contians errors
     if (defined $self->result->error) {
         # XXX TODO FIXME escape?
         Jifty->web->out( '<div class="form_errors">'
@@ -583,11 +639,17 @@
                  register  => 0,
                  @_);
 
+    # The user has asked to register the action while we're at it
     if ($args{register}) {
+
         # If they ask us to register the action, do so
         Jifty->web->form->register_action( $self );
         Jifty->web->form->print_action_registration($self->moniker);
-    } elsif ( not Jifty->web->form->printed_actions->{ $self->moniker } ) {
+    } 
+    
+    # Not registered yet, so we need to place registration in the button itself
+    elsif ( not Jifty->web->form->printed_actions->{ $self->moniker } ) {
+
         # Otherwise, if we're not registered yet, do it in the button
         my $arguments = $self->arguments;
         $args{parameters}{ $self->register_name } = ref $self;
@@ -595,9 +657,12 @@
             = $self->argument_value($_) || $arguments->{$_}->{'default_value'}
             for grep { $arguments->{$_}{constructor} } keys %{ $arguments };
     }
+
+    # Add whatever additional arguments they've requested to the button
     $args{parameters}{$self->form_field_name($_)} = $args{arguments}{$_}
       for keys %{$args{arguments}};
 
+    # Render us a button
     Jifty->web->link(%args);
 }
 
@@ -614,16 +679,18 @@
 sub return {
     my $self = shift;
     my %args = (@_);
+
+    # Fetch the current continuation or build a new one
     my $continuation = Jifty->web->request->continuation;
     if (not $continuation and $args{to}) {
         $continuation = Jifty::Continuation->new(request => Jifty::Request->new(path => $args{to}));
     }
     delete $args{to};
 
+    # Render a button that will call the continuation
     $self->button( call => $continuation, %args );
 }
 
-
 =head1 NAMING METHODS
 
 These methods return the names of HTML form elements related to this
@@ -641,7 +708,7 @@
     return 'J:A-' . (defined $self->order ? $self->order . "-" : "") .$self->moniker;
 }
 
-
+# prefixes a fieldname with a given prefix and follows it with the moniker
 sub _prefix_field {
     my $self = shift;
     my ($field_name, $prefix) = @_;
@@ -723,7 +790,6 @@
   return 'canonicalization_note-' . $self->form_field_name($field_name);
 }
 
-
 =head1 VALIDATION METHODS
 
 =head2 argument_names
@@ -733,7 +799,6 @@
 
 =cut
 
-
 sub argument_names {
     my $self      = shift;
     my %arguments = %{ $self->arguments };
@@ -746,7 +811,6 @@
     );
 }
 
-
 =head2 _canonicalize_arguments
 
 Canonicalizes each of the L<arguments|Jifty::Manual::Glossary/arguments> that
@@ -763,6 +827,7 @@
 sub _canonicalize_arguments {
     my $self   = shift;
 
+    # For each, canonicalize them all
     $self->_canonicalize_argument($_)
       for $self->argument_names;
 }
@@ -793,6 +858,7 @@
     my $self  = shift;
     my $field = shift;
 
+    # Setup some variables
     my $field_info = $self->arguments->{$field};
     my $value = $self->argument_value($field);
     my $default_method = 'canonicalize_' . $field;
@@ -800,21 +866,32 @@
     # XXX TODO: Do we really want to skip undef values?
     return unless defined $value;
 
+    # Do we have a valid canonicalizer for this field?
     if ( $field_info->{canonicalizer}
-        and defined &{ $field_info->{canonicalizer} } )
-    {
+        and defined &{ $field_info->{canonicalizer} } ) {
+        
+        # Run it, sucka
         $value = $field_info->{canonicalizer}->( $self, $value );
-    } elsif ( $self->can($default_method) ) {
+    } 
+    
+    # How about a method named canonicalize_$field?
+    elsif ( $self->can($default_method) ) {
+
+        # Run that, foo'
         $value = $self->$default_method( $value );
-    } elsif (   defined( $field_info->{render_as} )
+    } 
+    
+    # Or is it a date?
+    elsif (   defined( $field_info->{render_as} )
              && lc( $field_info->{render_as} ) eq 'date') {
+
+        # Use the default date canonicalizer, Mr. T!
         $value = $self->_canonicalize_date( $value );
     }
 
     $self->argument_value($field => $value);
 }
 
-
 =head2 _canonicalize_date DATE
 
 Parses and returns the date using L<Jifty::DateTime::new_from_string>.
@@ -842,10 +919,10 @@
 sub _validate_arguments {
     my $self   = shift;
     
+    # Validate each argument
     $self->_validate_argument($_)
       for $self->argument_names;
 
-
     return $self->result->success;
 }
 
@@ -867,15 +944,19 @@
     my $self  = shift;
     my $field = shift;
 
+    # Do nothing if we don't have a field name
     return unless $field;
     
     $self->log->debug(" validating argument $field");
 
+    # Do nothing if we don't know what that field is
     my $field_info = $self->arguments->{$field};
     return unless $field_info;
 
+    # Grab the current value
     my $value = $self->argument_value($field);
     
+    # When it isn't even given, check if it's mandatory and whine about it
     if ( !defined $value || !length $value ) {
         if ( $field_info->{mandatory} ) {
             return $self->validation_error( $field => _("You need to fill in this field") );
@@ -886,9 +967,9 @@
     # XXX TODO this should be a validate_valid_values sub
     if ( $value && $field_info->{valid_values} ) {
 
+        # If you're not on the list, you can't come to the party
         unless ( grep $_->{'value'} eq $value,
-            @{ $self->valid_values($field) } )
-        {
+            @{ $self->valid_values($field) } ) {
 
             return $self->validation_error(
                 $field => _("That doesn't look like a correct value") );
@@ -897,6 +978,7 @@
    # ... but still check through a validator function even if it's in the list
     }
 
+    # the validator method name is validate_$field
     my $default_validator = 'validate_' . $field;
 
     # Finally, fall back to running a validator sub
@@ -906,6 +988,7 @@
         return $field_info->{validator}->( $self, $value );
     }
 
+    # Check to see if it's the validate_$field method instead and use that
     elsif ( $self->can($default_validator) ) {
         return $self->$default_validator( $value );
     }
@@ -944,17 +1027,22 @@
     my $field_info = $self->arguments->{$field};
     my $value = $self->argument_value($field);
 
+    # the method is autocomplete_$field
     my $default_autocomplete = 'autocomplete_' . $field;
 
+    # If it's defined on the field, use that autocompleter
     if ( $field_info->{autocompleter}  )
     {
         return $field_info->{autocompleter}->( $self, $value );
     }
 
+    # If it's a method on the class, use that autocompleter
     elsif ( $self->can($default_autocomplete) ) {
         return $self->$default_autocomplete( $value );
     }
 
+    # Otherwise, return zip-zero-notta
+    return();
 }
 
 =head2 valid_values ARGUMENT
@@ -1001,18 +1089,32 @@
     my $field = shift;
     my $type = shift;
 
+    # Check for $type_values (valid_values or available_values)
     my $vv_orig = $self->arguments->{$field}{$type .'_values'};
     local $@;
+
+    # Try making that into a list or just return it
     my @values = eval { @$vv_orig } or return $vv_orig;
 
+    # This is a final return list
     my $vv = [];
 
+    # For each value in the *_values list
     for my $v (@values) {
+
+        # If it's a hash, it may be a collection spec or a display/value
         if ( ref $v eq 'HASH' ) {
+
+            # Check for a collection spec
             if ( $v->{'collection'} ) {
+
+                # Load the display_from/value_from paramters
                 my $disp = $v->{'display_from'};
                 my $val  = $v->{'value_from'};
+
                 # XXX TODO: wrap this in an eval?
+
+                # Fetch all the record from the given collection and keep'em
                 push @$vv, map {
                     {
                         display => ( $_->$disp() || '' ),
@@ -1021,12 +1123,16 @@
                 } grep {$_->check_read_rights} @{ $v->{'collection'}->items_array_ref };
 
             }
+
+            # Otherwise, push on the display/value hash
             else {
 
                 # assume it's already display/value
                 push @$vv, $v;
             }
         }
+
+        # Otherwise, treat plain string both display and value
         else {
 
             # just a string


More information about the Jifty-commit mailing list