Sanity checking improvements. All IntegerNumberOption really have to

be positive integers or zero. Replace type with new
IntegerNumberGeqZeroOption class definition to enforce this range.

Use update_option to test now unused IntegerNumberOption and
never used FloatNumberOption.

Test save function, make sure it creates .bak file.

Test a few more configuration option classes.

Author: rouilj

roundup/configuration.py

@@ -722,14 +722,14 @@ def str2value(self, value):
             "stop-words (eg. A,AND,ARE,AS,AT,BE,BUT,BY, ...)"),
         (OctalNumberOption, "umask", "0o002",
             "Defines the file creation mode mask."),
-        (IntegerNumberOption, 'csv_field_size', '131072',
+        (IntegerNumberGeqZeroOption, 'csv_field_size', '131072',
             "Maximum size of a csv-field during import. Roundups export\n"
             "format is a csv (comma separated values) variant. The csv\n"
             "reader has a limit on the size of individual fields\n"
             "starting with python 2.5. Set this to a higher value if you\n"
             "get the error 'Error: field larger than field limit' during\n"
             "import."),
-        (IntegerNumberOption, 'password_pbkdf2_default_rounds', '10000',
+        (IntegerNumberGeqZeroOption, 'password_pbkdf2_default_rounds', '10000',
             "Sets the default number of rounds used when encoding passwords\n"
             "using the PBKDF2 scheme. Set this to a higher value on faster\n"
             "systems which want more security.\n"
@@ -819,7 +819,7 @@ def str2value(self, value):
 additional REST-API parameters after the roundup web url configured in
 the tracker section. If this variable is set to 'no', the rest path has
 no special meaning and will yield an error message."""),
-        (IntegerNumberOption, 'api_calls_per_interval', "0",
+        (IntegerNumberGeqZeroOption, 'api_calls_per_interval', "0",
          "Limit API calls per api_interval_in_sec seconds to\n"
          "this number.\n"         
          "Determines the burst rate and the rate that new api\n"
@@ -830,7 +830,7 @@ def str2value(self, value):
          "another api request. A value of 0 turns off rate\n"
          "limiting in the API. Tune this as needed. See rest\n"
          "documentation for more info.\n"),
-        (IntegerNumberOption, 'api_interval_in_sec', "3600",
+        (IntegerNumberGeqZeroOption, 'api_interval_in_sec', "3600",
          "Defines the interval in seconds over which an api client can\n"
          "make api_calls_per_interval api calls. Tune this as needed.\n"),
         (CsrfSettingOption, 'csrf_enforce_token', "yes",
@@ -845,7 +845,7 @@ def str2value(self, value):
     the post.
 Set this to 'no' to ignore the field and accept the post.
             """),
-        (IntegerNumberOption, 'csrf_token_lifetime', "20160",
+        (IntegerNumberGeqZeroOption, 'csrf_token_lifetime', "20160",
             """csrf_tokens have a limited lifetime. If they are not
 used they are purged from the database after this
 number of minutes. Default (20160) is 2 weeks."""),
@@ -920,7 +920,7 @@ def str2value(self, value):
     log if the header is invalid or missing, but accept
     the post.
 Set this to 'no' to ignore the header and accept the post."""),
-        (IntegerNumberOption, 'csrf_header_min_count', "1",
+        (IntegerNumberGeqZeroOption, 'csrf_header_min_count', "1",
             """Minimum number of header checks that must pass
 to accept the request. Set to 0 to accept post
 even if no header checks pass. Usually the Host header check
@@ -978,11 +978,11 @@ def str2value(self, value):
         (NullableOption, 'read_default_group', 'roundup',
             "Name of the group to use in the MySQL defaults file (.my.cnf).\n"
             "Only used in MySQL connections."),
-        (IntegerNumberOption, 'sqlite_timeout', '30',
+        (IntegerNumberGeqZeroOption, 'sqlite_timeout', '30',
             "Number of seconds to wait when the SQLite database is locked\n"
             "Default: use a 30 second timeout (extraordinarily generous)\n"
             "Only used in SQLite connections."),
-        (IntegerNumberOption, 'cache_size', '100',
+        (IntegerNumberGeqZeroOption, 'cache_size', '100',
             "Size of the node cache (in elements)"),
         (BooleanOption, "allow_create", "yes",
             "Setting this option to 'no' protects the database against table creations."),
@@ -1039,7 +1039,7 @@ def str2value(self, value):
             "If username is not empty, password (below) MUST be set!"),
         (Option, "password", NODEFAULT, "SMTP login password.\n"
             "Set this if your mail host requires authenticated access."),
-        (IntegerNumberOption, "port", smtplib.SMTP_PORT,
+        (IntegerNumberGeqZeroOption, "port", smtplib.SMTP_PORT,
             "Default port to send SMTP on.\n"
             "Set this if your mail server runs on a different port."),
         (NullableOption, "local_hostname", '',
@@ -1236,7 +1236,7 @@ def str2value(self, value):
             "\"multiple\" then a separate email is sent to each\n"
             "recipient. If \"single\" then a single email is sent with\n"
             "each recipient as a CC address."),
-        (IntegerNumberOption, "max_attachment_size", sys.maxsize,
+        (IntegerNumberGeqZeroOption, "max_attachment_size", sys.maxsize,
             "Attachments larger than the given number of bytes\n"
             "won't be attached to nosy mails. They will be replaced by\n"
             "a link to the tracker's download page for the file.")

test/test_config.py

@@ -18,6 +18,8 @@
 import unittest
 import logging
 
+import os, shutil, errno
+
 import pytest
 from roundup import configuration
 
@@ -68,7 +70,10 @@ def testTrackerWeb(self):
         self.assertRaises(configuration.OptionValueError,
              config._get_option('TRACKER_WEB').set, "htt://foo.example/bar")
 
-    def testLoginRateLimit(self):
+        self.assertRaises(configuration.OptionValueError,
+             config._get_option('TRACKER_WEB').set, "")
+
+    def testLoginAttemptsMin(self):
         config = configuration.CoreConfig()
 
         self.assertEqual(None,
@@ -82,3 +87,143 @@ def testLoginRateLimit(self):
         self.assertRaises(configuration.OptionValueError,
                    config._get_option('WEB_LOGIN_ATTEMPTS_MIN').set, "-1")
 
+        self.assertRaises(configuration.OptionValueError,
+                   config._get_option('WEB_LOGIN_ATTEMPTS_MIN').set, "")
+
+    def testTimeZone(self):
+        config = configuration.CoreConfig()
+
+        self.assertEqual(None,
+                config._get_option('TIMEZONE').set("0"))
+
+        # not a valid timezone
+        self.assertRaises(configuration.OptionValueError,
+                config._get_option('TIMEZONE').set, "Zot")
+
+        # 25 is not a valid UTC offset: -12 - +14 is range
+        # possibly +/- 1 for DST. But roundup.date doesn't
+        # constrain to this range.
+        #self.assertRaises(configuration.OptionValueError,
+        #        config._get_option('TIMEZONE').set, "25")
+
+        try:
+            import pytz
+            self.assertEqual(None,
+                    config._get_option('TIMEZONE').set("UTC"))
+            self.assertEqual(None,
+                    config._get_option('TIMEZONE').set("America/New_York"))
+
+        except ImportError:
+            self.assertRaises(configuration.OptionValueError,
+                    config._get_option('TIMEZONE').set, "UTC")
+            self.assertRaises(configuration.OptionValueError,
+                    config._get_option('TIMEZONE').set, "America/New_York")
+
+    def testWebSecretKey(self):
+        config = configuration.CoreConfig()
+
+        self.assertEqual(None,
+                config._get_option('WEB_SECRET_KEY').set("skskskd"))
+
+        self.assertRaises(configuration.OptionValueError,
+                config._get_option('WEB_SECRET_KEY').set, "")
+
+
+    def testStaticFiles(self):
+        config = configuration.CoreConfig()
+
+        self.assertEqual(None,
+                config._get_option('STATIC_FILES').set("foo /tmp/bar"))
+
+        self.assertEqual(config.STATIC_FILES,
+                         ["./foo", "/tmp/bar"])
+
+        self.assertEqual(config['STATIC_FILES'],
+                         ["./foo", "/tmp/bar"])
+
+    def testIsolationLevel(self):
+        config = configuration.CoreConfig()
+
+        self.assertEqual(None,
+            config._get_option('RDBMS_ISOLATION_LEVEL').set("read uncommitted"))
+        self.assertEqual(None,
+            config._get_option('RDBMS_ISOLATION_LEVEL').set("read committed"))
+        self.assertEqual(None,
+            config._get_option('RDBMS_ISOLATION_LEVEL').set("repeatable read"))
+
+
+        self.assertRaises(configuration.OptionValueError,
+            config._get_option('RDBMS_ISOLATION_LEVEL').set, "not a level")
+
+    def testConfigSave(self):
+
+        config = configuration.CoreConfig()
+        # make scratch directory to create files in
+
+        self.startdir = os.getcwd()
+
+        self.dirname = os.getcwd() + '_test_config'
+        os.mkdir(self.dirname)
+
+        try:
+            os.chdir(self.dirname)
+            self.assertFalse(os.access("config.ini", os.F_OK))
+            self.assertFalse(os.access("config.bak", os.F_OK))
+            config.save()
+            config.save() # creates .bak file
+            self.assertTrue(os.access("config.ini", os.F_OK))
+            self.assertTrue(os.access("config.bak", os.F_OK))
+
+            self.assertFalse(os.access("foo.bar", os.F_OK))
+            self.assertFalse(os.access("foo.bak", os.F_OK))
+            config.save("foo.bar")
+            config.save("foo.bar") # creates .bak file
+            self.assertTrue(os.access("foo.bar", os.F_OK))
+            self.assertTrue(os.access("foo.bak", os.F_OK))
+
+        finally:
+            # cleanup scratch directory and files
+            try:
+                os.chdir(self.startdir)
+                shutil.rmtree(self.dirname)
+            except OSError as error:
+                if error.errno not in (errno.ENOENT, errno.ESRCH): raise
+
+    def testFloatAndInt_with_update_option(self):
+
+       config = configuration.CoreConfig()
+
+       # Update existing IntegerNumberGeqZeroOption to IntegerNumberOption
+       config.update_option('WEB_LOGIN_ATTEMPTS_MIN',
+                            configuration.IntegerNumberOption,
+                            "0", description="new desc")
+
+       # -1 is allowed now that it is an int.
+       self.assertEqual(None,
+                    config._get_option('WEB_LOGIN_ATTEMPTS_MIN').set("-1"))
+
+       # but can't float this
+       self.assertRaises(configuration.OptionValueError,
+                    config._get_option('WEB_LOGIN_ATTEMPTS_MIN').set, "2.4")
+
+       # but fred is still an issue
+       self.assertRaises(configuration.OptionValueError,
+                    config._get_option('WEB_LOGIN_ATTEMPTS_MIN').set, "fred")
+       
+       # Update existing IntegerNumberOption to FloatNumberOption
+       config.update_option('WEB_LOGIN_ATTEMPTS_MIN',
+                            configuration.FloatNumberOption,
+                            "0.0")
+
+       self.assertEqual(config['WEB_LOGIN_ATTEMPTS_MIN'], -1)
+
+       # can float this
+       self.assertEqual(None,
+                config._get_option('WEB_LOGIN_ATTEMPTS_MIN').set("3.1415926"))
+
+       # but fred is still an issue
+       self.assertRaises(configuration.OptionValueError,
+                    config._get_option('WEB_LOGIN_ATTEMPTS_MIN').set, "fred")
+
+       self.assertAlmostEqual(config['WEB_LOGIN_ATTEMPTS_MIN'], 3.1415926,
+                              places=6)