Minor lint fixes, mostly to the controller layer.

anna-gavrilova · Jul 25, 2014 · 2d2ead3 · 2d2ead3
1 parent 300b075
commit 2d2ead3
Show file tree

Hide file tree

Showing 22 changed files with 149 additions and 121 deletions.
diff --git a/.pylintrc b/.pylintrc
@@ -14,7 +14,7 @@ function-rgx=^[_a-z][a-z0-9_]*$
 method-rgx=^([_a-z][a-z0-9_]*|__[a-z0-9]+__)$
 
 # Good variable names which should always be accepted, separated by a comma
-good-names=e,_,setUp,tearDown,longMessage
+good-names=e,_,f,fs,id,setUp,tearDown,longMessage
 
 [DESIGN]
 

diff --git a/core/controllers/admin.py b/core/controllers/admin.py
@@ -174,7 +174,7 @@ def post(self):
                 job_id = self.payload.get('job_id')
                 job_type = self.payload.get('job_type')
                 for klass in jobs_registry.JOB_MANAGER_CLASSES:
-                    if klass.__name__ == self.payload.get('job_type'):
+                    if klass.__name__ == job_type:
                         klass.cancel(job_id, self.user_id)
                         break
 

diff --git a/core/controllers/admin_test.py b/core/controllers/admin_test.py
@@ -12,13 +12,20 @@
 # See the License for the specific language governing permissions and
 # limitations under the License.
 
+"""Tests for the admin page."""
+
 __author__ = 'Sean Lip'
 
 from core.controllers import editor
 from core.controllers import pages
 from core.domain import config_domain
+from core.tests import test_utils
+
 
-import test_utils
+ADMIN_EMAIL = 'admin@example.com'
+BOTH_MODERATOR_AND_ADMIN_EMAIL = 'moderator.and.admin@example.com'
+MODERATOR_EMAIL = 'moderator@example.com'
+SITE_NAME = 'sitename.org'
 
 
 class AdminIntegrationTest(test_utils.GenericTestBase):
@@ -93,12 +100,10 @@ def test_change_configuration_property(self):
 
     def test_change_splash_page_config_property(self):
         """Test that the correct variables show up on the splash page."""
-        ACTUAL_SITE_NAME = 'oppia.org'
-
         # Navigate to the splash page. The site name is not set.
         response = self.testapp.get('/')
         self.assertIn('SITE_NAME', response.body)
-        self.assertNotIn(ACTUAL_SITE_NAME, response.body)
+        self.assertNotIn(SITE_NAME, response.body)
 
         # Log in as an admin and customize the site name.
         self.login('admin@example.com', is_super_admin=True)
@@ -108,7 +113,7 @@ def test_change_splash_page_config_property(self):
         self.post_json('/adminhandler', {
             'action': 'save_config_properties',
             'new_config_property_values': {
-                pages.SITE_NAME.name: ACTUAL_SITE_NAME
+                pages.SITE_NAME.name: SITE_NAME
             }
         }, csrf_token)
 
@@ -117,13 +122,10 @@ def test_change_splash_page_config_property(self):
         # Navigate to the splash page. The site name is set.
         response = self.testapp.get('/')
         self.assertNotIn('SITE_NAME', response.body)
-        self.assertIn(ACTUAL_SITE_NAME, response.body)
+        self.assertIn(SITE_NAME, response.body)
 
     def test_change_rights(self):
         """Test that the correct role indicators show up on app pages."""
-        MODERATOR_EMAIL = 'moderator@example.com'
-        ADMIN_EMAIL = 'admin@example.com'
-        BOTH_MODERATOR_AND_ADMIN_EMAIL = 'moderator.and.admin@example.com'
 
         # Navigate to any page. The role is not set.
         response = self.testapp.get('/')

diff --git a/core/controllers/base_test.py b/core/controllers/base_test.py
@@ -14,19 +14,21 @@
 # See the License for the specific language governing permissions and
 # limitations under the License.
 
+"""Tests for generic controller behavior."""
+
 __author__ = 'Sean Lip'
 
 import copy
 import feconf
 import re
 import types
 
-import webapp2
-import webtest
-
 from core.controllers import base
+from core.tests import test_utils
 import main
-import test_utils
+
+import webapp2
+import webtest
 
 
 class BaseHandlerTest(test_utils.GenericTestBase):

diff --git a/core/controllers/cron.py b/core/controllers/cron.py
@@ -24,6 +24,6 @@ class StatisticsHandler(base.BaseHandler):
     def get(self):
         """Handles GET requests."""
         for klass in jobs_registry.JOB_MANAGER_CLASSES:
-             if klass.__name__ == 'StatisticsPageJobManager':
-                 klass.enqueue(klass.create_new())
-                 break
+            if klass.__name__ == 'StatisticsPageJobManager':
+                klass.enqueue(klass.create_new())
+                break
diff --git a/core/controllers/editor.py b/core/controllers/editor.py
@@ -45,10 +45,11 @@
 
 
 def get_value_generators_js():
+    """Return a string that concatenates the JS for all value generators."""
     all_value_generators = (
         value_generators_domain.Registry.get_all_generator_classes())
     value_generators_js = ''
-    for gid, generator_cls in all_value_generators.iteritems():
+    for _, generator_cls in all_value_generators.iteritems():
         value_generators_js += generator_cls.get_js_template()
     return value_generators_js
 
@@ -72,6 +73,7 @@ def get_value_generators_js():
 
 
 def _require_valid_version(version_from_payload, exploration_version):
+    """Check that the payload version matches the given exploration version."""
     if version_from_payload is None:
         raise base.BaseHandler.InvalidInputException(
             'Invalid POST request: a version must be specified.')

diff --git a/core/controllers/editor_test.py b/core/controllers/editor_test.py
@@ -12,6 +12,8 @@
 # See the License for the specific language governing permissions and
 # limitations under the License.
 
+"""Tests for the exploration editor page."""
+
 __author__ = 'Sean Lip'
 
 import os
@@ -23,10 +25,9 @@
 from core.domain import exp_services
 from core.domain import stats_domain
 from core.domain import stats_jobs
-from core.domain import stats_services
 from core.domain import rights_manager
+from core.tests import test_utils
 import feconf
-import test_utils
 
 
 class BaseEditorControllerTest(test_utils.GenericTestBase):
@@ -642,36 +643,33 @@ def test_user_banning(self):
 class ExplorationRightsIntegrationTest(BaseEditorControllerTest):
     """Test the handler for managing exploration editing rights."""
 
+    ADMIN_EMAIL = 'admin@example.com'
+    OWNER_EMAIL = 'owner@example.com'
+    COLLABORATOR_EMAIL = 'collaborator@example.com'
+    COLLABORATOR2_EMAIL = 'collaborator2@example.com'
+    VIEWER_EMAIL = 'viewer@example.com'
+    VIEWER2_EMAIL = 'viewer2@example.com'
+
     def test_exploration_rights_handler(self):
         """Test exploration rights handler."""
 
         # Create several users
-        self.admin_email = 'admin@example.com'
-        self.owner_email = 'owner@example.com'
-        self.collaborator_email = 'collaborator@example.com'
-        self.collaborator2_email = 'collaborator2@example.com'
-        self.viewer_email = 'viewer@example.com'
-        self.viewer2_email = 'viewer2@example.com'
+        self.register_editor(self.ADMIN_EMAIL, username='adm')
+        self.register_editor(self.OWNER_EMAIL, username='owner')
+        self.register_editor(self.COLLABORATOR_EMAIL, username='collab')
+        self.register_editor(self.VIEWER_EMAIL, username='viewer')
 
-        self.register_editor(self.admin_email, username='adm')
-        self.register_editor(self.owner_email, username='owner')
-        self.register_editor(self.collaborator_email, username='collab')
-        self.register_editor(self.viewer_email, username='viewer')
-
-        self.admin_id = self.get_user_id_from_email(self.admin_email)
-        self.owner_id = self.get_user_id_from_email(self.owner_email)
+        self.admin_id = self.get_user_id_from_email(self.ADMIN_EMAIL)
+        self.owner_id = self.get_user_id_from_email(self.OWNER_EMAIL)
         self.collaborator_id = self.get_user_id_from_email(
-            self.collaborator_email)
-        self.viewer_id = self.get_user_id_from_email(self.viewer_email)
+            self.COLLABORATOR_EMAIL)
+        self.viewer_id = self.get_user_id_from_email(self.VIEWER_EMAIL)
 
         config_services.set_property(
             feconf.ADMIN_COMMITTER_ID, 'admin_emails', ['admin@example.com'])
 
-        self.viewer_role = rights_manager.ROLE_VIEWER
-        self.collaborator_role = rights_manager.ROLE_EDITOR
-
         # Owner creates exploration
-        self.login(self.owner_email)
+        self.login(self.OWNER_EMAIL)
         EXP_ID = 'eid'
         exploration = exp_domain.Exploration.create_default_exploration(
             EXP_ID, 'Title for rights handler test!',
@@ -688,27 +686,27 @@ def test_exploration_rights_handler(self):
         response_dict = self.put_json(
             rights_url, {
                 'version': exploration.version,
-                'new_member_email': self.viewer_email,
-                'new_member_role': self.viewer_role
+                'new_member_email': self.VIEWER_EMAIL,
+                'new_member_role': rights_manager.ROLE_VIEWER
             }, csrf_token)
         response_dict = self.put_json(
             rights_url, {
                 'version': exploration.version,
-                'new_member_email': self.collaborator_email,
-                'new_member_role': self.collaborator_role
+                'new_member_email': self.COLLABORATOR_EMAIL,
+                'new_member_role': rights_manager.ROLE_EDITOR
             }, csrf_token)
 
         self.logout()
 
         # Check that viewer can access editor page but cannot edit.
-        self.login(self.viewer_email)
+        self.login(self.VIEWER_EMAIL)
         response = self.testapp.get('/create/%s' % EXP_ID, expect_errors=True)
         self.assertEqual(response.status_int, 200)
         self.assert_cannot_edit(response.body)
         self.logout()
 
         # Check that collaborator can access editor page and can edit.
-        self.login(self.collaborator_email)
+        self.login(self.COLLABORATOR_EMAIL)
         response = self.testapp.get('/create/%s' % EXP_ID)
         self.assertEqual(response.status_int, 200)
         self.assert_can_edit(response.body)
@@ -737,16 +735,16 @@ def test_exploration_rights_handler(self):
         response_dict = self.put_json(
             rights_url, {
                 'version': exploration.version,
-                'new_member_email': self.viewer2_email,
-                'new_member_role': self.viewer_role
+                'new_member_email': self.VIEWER2_EMAIL,
+                'new_member_role': rights_manager.ROLE_VIEWER
             }, csrf_token, expect_errors=True, expected_status_int=401)
         self.assertEqual(response_dict['code'], 401)
 
         response_dict = self.put_json(
             rights_url, {
                 'version': exploration.version,
-                'new_member_email': self.collaborator2_email,
-                'new_member_role': self.collaborator_role,
+                'new_member_email': self.COLLABORATOR2_EMAIL,
+                'new_member_role': rights_manager.ROLE_EDITOR,
                 }, csrf_token, expect_errors=True, expected_status_int=401)
         self.assertEqual(response_dict['code'], 401)
 

diff --git a/core/controllers/feedback.py b/core/controllers/feedback.py
@@ -21,6 +21,8 @@
 
 
 class ThreadListHandler(base.BaseHandler):
+    """Handles operations relating to feedback thread lists."""
+
     PAGE_NAME_FOR_CSRF = 'editor'
 
     def get(self, exploration_id):
@@ -50,6 +52,8 @@ def post(self, exploration_id):
 
 
 class ThreadHandler(base.BaseHandler):
+    """Handles operations relating to feedback threads."""
+
     PAGE_NAME_FOR_CSRF = 'editor'
 
     def get(self, exploration_id, thread_id):