Miscellaneous code cleanup: clean up old code in form_builder, adding important comments for the CheckboxField class; make the functional test diff checker for PDF files more flexible; allow tab chars to be mapped so they can be properly stored in the db.

lib/galaxy/util/__init__.py

@@ -122,7 +122,8 @@ mapped_chars = { '>' :'__gt__',
                  '}' :'__cc__',
                  '@' : '__at__',
                  '\n' : '__cn__',
-                 '\r' : '__cr__'
+                 '\r' : '__cr__',
+                 '\t' : '__tc__'
                  }
 
 def restore_text(text):

lib/galaxy/web/form_builder.py

@@ -88,7 +88,7 @@ class TextArea(BaseField):
         self.value = value or ""
     def get_html( self, prefix="" ):
         return '<textarea name="%s%s" rows="%d" cols="%d">%s</textarea>' \
-            % ( prefix, self.name, self.rows, self.cols, escape(str(self.value), quote=True) )
+            % ( prefix, self.name, self.rows, self.cols, escape( str( self.value ), quote=True ) )
     def set_size(self, rows, cols):
         self.rows = rows
         self.cols = cols
@@ -104,27 +104,31 @@ class CheckboxField(BaseField):
     """
     def __init__( self, name, checked=None ):
         self.name = name
-        self.checked = ( checked == True ) or ( type( checked ) == type( 'a' ) and ( checked.lower() in ( "yes", "true", "on" ) ) ) 
+        self.checked = ( checked == True ) or ( isinstance( checked, basestring ) and ( checked.lower() in ( "yes", "true", "on" ) ) )
     def get_html( self, prefix="" ):
         if self.checked:
             checked_text = "checked"
-        else: checked_text = ""
+        else:
+            checked_text = ""
+        # The hidden field is necessary because if the check box is not checked on the form, it will
+        # not be included in the request params.  The hidden field ensure that this will happen.  When
+        # parsing the request, the value 'true' in the hidden field actually means it is NOT checked.
+        # See the is_checked() method below.  The prefix is necessary in each case to ensure functional
+        # correctness when the param is inside a conditional.
         return '<input type="checkbox" name="%s%s" value="true" %s><input type="hidden" name="%s%s" value="true">' \
             % ( prefix, self.name, checked_text, prefix, self.name )
     @staticmethod
     def is_checked( value ):
-        if value == True: # wierd behaviour caused by following check for 2 valued list - wtf? ross august 22
-           return value
-        if type( value ) == list and len( value ) == 2:
-            return True
-        else:
-            return False
+        if value == True:
+            return value
+        # This may look strange upon initial inspection, but see the comments in the get_html() method
+        # above for clarification.  Basically, if value is not True, then it will always be a list with
+        # 2 input fields ( a checkbox and a hidden field ) if the checkbox is checked.  If it is not
+        # checked, then value will be only the hidden field.
+        return isinstance( value, list ) and len( value ) == 2
     def set_checked(self, value):
-        if type(value) == type('a'):
-            if value.lower() in [ "yes", "true", "on" ]:
-                self.checked = True
-            else:
-                self.checked = False
+        if isinstance( value, basestring ):
+            self.checked = value.lower() in [ "yes", "true", "on" ]
         else:
             self.checked = value
 
@@ -335,8 +339,8 @@ class DrillDownField( BaseField ):
         self.name = name
         self.multiple = multiple or False
         self.options = options
-        if value is not None:
-            if not isinstance( value, list ): value = [ value ]
+        if value and not isinstance( value, list ):
+            value = [ value ]
         else:
             value = []
         self.value = value

test/base/twilltestcase.py

@@ -53,21 +53,35 @@ class TwillTestCase( unittest.TestCase ):
                 files_differ = True
             if files_differ:
                 diff = difflib.unified_diff( local_file, history_data, "local_file", "history_data" )
-                diff_slice = list( islice( diff, 40 ) )
+                diff_slice = list( islice( diff, 40 ) )        
                 if file1.endswith( '.pdf' ) or file2.endswith( '.pdf' ):
-                    # PDF files contain both a creation and modification date, so we need to
-                    # handle these differences.  As long as the rest of the PDF file does not differ,
-                    # we're ok.
-                    if len( diff_slice ) == 13 and \
-                    diff_slice[6].startswith( '-/CreationDate' ) and diff_slice[7].startswith( '-/ModDate' ) \
-                    and diff_slice[8].startswith( '+/CreationDate' ) and diff_slice[9].startswith( '+/ModDate' ):
-                        return True
-                for line in diff_slice:
-                    for char in line:
-                        if ord( char ) > 128:
-                            raise AssertionError( "Binary data detected, not displaying diff" )
-                raise AssertionError( "".join( diff_slice ) )
-        return True
+                    # PDF files contain creation dates, modification dates, ids and descriptions that change with each
+                    # new file, so we need to handle these differences.  As long as the rest of the PDF file does
+                    # not differ we're ok.
+                    valid_diff_strs = [ 'description', 'createdate', 'creationdate', 'moddate', 'id' ]
+                    valid_diff = False
+                    for line in diff_slice:
+                        # Make sure to lower case strings before checking.
+                        line = line.lower()
+                        # Diff lines will always start with a + or - character, but handle special cases: '--- local_file \n', '+++ history_data \n'
+                        if ( line.startswith( '+' ) or line.startswith( '-' ) ) and line.find( 'local_file' ) < 0 and line.find( 'history_data' ) < 0:
+                            for vdf in valid_diff_strs:
+                                if line.find( vdf ) < 0:
+                                    valid_diff = False
+                                else:
+                                    valid_diff = True
+                                    # Stop checking as soon as we know we have a valid difference
+                                    break
+                            if not valid_diff:
+                                # Print out diff_slice so we can see what failed
+                                print "###### diff_slice ######"
+                                raise AssertionError( "".join( diff_slice ) )
+                                break
+                else:
+                    for line in diff_slice:
+                        for char in line:
+                            if ord( char ) > 128:
+                                raise AssertionError( "Binary data detected, not displaying diff" )
 
     def get_filename( self, filename ):
         full = os.path.join( self.file_dir, filename)