From 5e062b22b7cb67a1d5c4133a437bf5dc9ccf8c6d Mon Sep 17 00:00:00 2001 From: Carson Gee Date: Wed, 4 Dec 2013 14:36:41 -0500 Subject: [PATCH] sysadmin dashboard - Removal of popen, more streamlined git commands, and other optimizations --- .../management/commands/git_add_course.py | 67 ++++++++++--------- lms/djangoapps/dashboard/sysadmin.py | 42 +++++++----- lms/djangoapps/dashboard/sysadmin_urls.py | 1 + .../dashboard/tests/test_sysadmin.py | 6 +- 4 files changed, 66 insertions(+), 50 deletions(-) diff --git a/lms/djangoapps/dashboard/management/commands/git_add_course.py b/lms/djangoapps/dashboard/management/commands/git_add_course.py index ee19a6665fae..30cec2169043 100644 --- a/lms/djangoapps/dashboard/management/commands/git_add_course.py +++ b/lms/djangoapps/dashboard/management/commands/git_add_course.py @@ -6,6 +6,7 @@ import re import datetime import StringIO +import subprocess import logging from django.utils.translation import ugettext as _ @@ -70,12 +71,15 @@ def add_repo(repo, rdir_in): if os.path.exists(rdirp): log.info(_('directory already exists, doing a git pull instead ' 'of git clone')) - cmd = 'cd {0}/{1}; git pull'.format(GIT_REPO_DIR, rdir) + cmd = ['git', 'pull', ] + cwd = '{0}/{1}'.format(GIT_REPO_DIR, rdir) else: - cmd = 'cd {0}; git clone "{1}"'.format(GIT_REPO_DIR, repo) + cmd = ['git', 'clone', repo, ] + cwd = GIT_REPO_DIR log.debug(cmd) - ret_git = os.popen(cmd).read() + cwd = os.path.abspath(cwd) + ret_git = subprocess.check_output(cmd, cwd=cwd) log.debug(ret_git) if not os.path.exists('{0}/{1}'.format(GIT_REPO_DIR, rdir)): @@ -83,46 +87,43 @@ def add_repo(repo, rdir_in): return -1 # get commit id - commit_id = os.popen('cd {0}; git log -n 1 | head -1'.format( - rdirp)).read().strip().split(' ')[1] + cmd = ['git', 'log', '-1', '--format=%H', ] + commit_id = subprocess.check_output(cmd, cwd=rdirp) ret_git += _('\nCommit ID: {0}').format(commit_id) # get branch - branch = '' - for k in os.popen('cd {0}; git branch'.format(rdirp)).readlines(): - if k[0] == '*': - branch = k[2:].strip() - + cmd = ['git', 'rev-parse', '--abbrev-ref', 'HEAD', ] + branch = subprocess.check_output(cmd, cwd=rdirp) ret_git += ' \nBranch: {0}'.format(branch) # Get XML logging logger and capture debug to parse results output = StringIO.StringIO() - import_logger = logging.getLogger('xmodule.modulestore.xml_importer') - git_logger = logging.getLogger('git_add_script') - xml_logger = logging.getLogger('xmodule.modulestore.xml') - xml_seq_logger = logging.getLogger('xmodule.seq_module') - import_log_handler = logging.StreamHandler(output) import_log_handler.setLevel(logging.DEBUG) - for logger in [import_logger, git_logger, xml_logger, xml_seq_logger, ]: + logger_names = ['xmodule.modulestore.xml_importer', 'git_add_course', + 'xmodule.modulestore.xml', 'xmodule.seq_module', ] + loggers = [] + + for logger_name in logger_names: + logger = logging.getLogger(logger_name) logger.old_level = logger.level logger.setLevel(logging.DEBUG) logger.addHandler(import_log_handler) + loggers.append(logger) try: management.call_command('import', GIT_REPO_DIR, rdir, nostatic=not GIT_IMPORT_STATIC) - except CommandError, ex: - log.critical(_('Unable to run import command.')) - log.critical(_('Error was {0}').format(str(ex))) + except CommandError: + log.exception(_('Unable to run import command.')) return -1 ret_import = output.getvalue() # Remove handler hijacks - for logger in [import_logger, git_logger, xml_logger, xml_seq_logger, ]: + for logger in loggers: logger.setLevel(logger.old_level) logger.removeHandler(import_log_handler) @@ -144,14 +145,21 @@ def add_repo(repo, rdir_in): if os.path.exists(cdir) and not os.path.islink(cdir): log.debug(_(' -> exists, but is not symlink')) - log.debug(os.popen('ls -l {0}'.format(cdir)).read()) - log.debug(os.popen('rmdir {0}'.format(cdir)).read()) + log.debug(subprocess.check_output(['ls', 'l', ], + cwd=os.path.abspath(cdir))) + try: + os.rmdir(os.path.abspath(cdir)) + except OSError: + log.exception(_('Failed to remove course directory')) if not os.path.exists(cdir): - log.debug(_(' -> creating symlink')) - log.debug(os.popen('ln -s {0} {1}'.format(rdirp, - cdir)).read()) - log.debug(os.popen('ls -l {0}'.format(cdir)).read()) + log.debug(_(' -> creating symlink between {0} and {1}').format(rdirp, cdir)) + try: + os.symlink(os.path.abspath(rdirp), os.path.abspath(cdir)) + except OSError: + log.exception(_('Unable to create course symlink')) + log.debug(subprocess.check_output(['ls', '-l', ], + cwd=os.path.abspath(cdir))) # store import-command-run output in mongo mongouri = 'mongodb://{0}:{1}@{2}/{3}'.format( @@ -163,10 +171,9 @@ def add_repo(repo, rdir_in): mdb = mongoengine.connect(mongo_db['db'], host=mongouri) else: mdb = mongoengine.connect(mongo_db['db'], host=mongo_db['host']) - except mongoengine.connection.ConnectionError, ex: - log.critical(_('Unable to connect to mongodb to save log, please ' - 'check MONGODB_LOG settings')) - log.critical(_('Error was: {0}').format(str(ex))) + except mongoengine.connection.ConnectionError: + log.exception(_('Unable to connect to mongodb to save log, please ' + 'check MONGODB_LOG settings')) return -1 cil = CourseImportLog( course_id=course_id, diff --git a/lms/djangoapps/dashboard/sysadmin.py b/lms/djangoapps/dashboard/sysadmin.py index 6c99d367d558..77def064201e 100644 --- a/lms/djangoapps/dashboard/sysadmin.py +++ b/lms/djangoapps/dashboard/sysadmin.py @@ -12,7 +12,7 @@ from datetime import datetime from django.conf import settings -from django.contrib.auth.models import User, Group +from django.contrib.auth.models import User from django.utils.translation import ugettext as _ from student.models import CourseEnrollment, UserProfile, Registration from external_auth.models import ExternalAuthMap @@ -73,10 +73,9 @@ def dispatch(self, *args, **kwargs): def get_courses(self): """ Get an iterable list of courses regardless of module store type.""" - # Prefer mongo if using mixed or mongo store if self.is_using_mongo: courses = self.def_ms.get_courses() - courses = dict([c.id, c] for c in courses) # no course directory + courses = {c.id: c for c in courses} # no course directory else: courses = self.def_ms.courses.items() return courses @@ -377,27 +376,29 @@ def import_mongo_course(self, gitloc): # Grab logging output for debugging imports output = StringIO.StringIO() - - import_logger = logging.getLogger('xmodule.modulestore.xml_importer') - git_logger = logging.getLogger('dashboard.management.commands.git_add_course') - xml_logger = logging.getLogger('xmodule.modulestore.xml') - xml_seq_logger = logging.getLogger('xmodule.seq_module') - import_log_handler = logging.StreamHandler(output) import_log_handler.setLevel(logging.DEBUG) - for logger in [import_logger, git_logger, xml_logger, xml_seq_logger, ]: + logger_names = ['xmodule.modulestore.xml_importer', + 'dashboard.management.commands.git_add_course', + 'xmodule.modulestore.xml', 'xmodule.seq_module', ] + loggers = [] + + for logger_name in logger_names: + logger = logging.getLogger(logger_name) logger.old_level = logger.level logger.setLevel(logging.DEBUG) logger.addHandler(import_log_handler) + loggers.append(logger) git_add_course.add_repo(gitloc, None) ret = output.getvalue() # Remove handler hijacks - for logger in [import_logger, git_logger, xml_logger, xml_seq_logger, ]: + for logger in loggers: logger.setLevel(logger.old_level) logger.removeHandler(import_log_handler) + msg = u"

{0} {1}

".format( _('Added course from'), gitloc) msg += _("
{0}
").format(escape(ret)) @@ -412,10 +413,14 @@ def import_xml_course(self, gitloc, datatable): if os.path.exists(gdir): msg += _("The course {0} already exists in the data directory! " "(reloading anyway)").format(cdir) - cmd = 'cd {0}; git pull'.format(settings.DATA_DIR, gitloc) + cmd = ['git', 'pull', ] + cwd = gdir else: - cmd = 'cd {0}; git clone {1}'.format(settings.DATA_DIR, gitloc) - msg += u'
%s
' % escape(os.popen(cmd).read()) + cmd = ['git', 'clone', gitloc, ] + cwd = settings.DATA_DIR + cmd_output = escape(subprocess.check_output(cmd, cwd=os.path.abspath(cwd))) + msg += u'
{0}
'.format(cmd_output) + if not os.path.exists(gdir): msg += _('Failed to clone repository to {0}').format(gdir) return msg @@ -489,7 +494,7 @@ def post(self, request): page='courses_sysdashboard') courses = self.get_courses() - if action == _('add_course'): + if action == 'add_course': gitloc = request.POST.get('repo_location', '').strip().replace( ' ', '').replace(';', '') datatable = self.make_datatable() @@ -628,9 +633,14 @@ class GitLogs(TemplateView): template_name = 'sysadmin_dashboard_gitlogs.html' @method_decorator(login_required) - def get(self, request, course_id=None): + def get(self, request, *args, **kwargs): """Shows logs of imports that happened as a result of a git import""" + if 'course_id' in kwargs: + course_id = kwargs['course_id'] + else: + course_id = None + # Set mongodb defaults even if it isn't defined in settings mongo_db = { 'host': 'localhost', diff --git a/lms/djangoapps/dashboard/sysadmin_urls.py b/lms/djangoapps/dashboard/sysadmin_urls.py index 24fbe178ee50..f412676c39a9 100644 --- a/lms/djangoapps/dashboard/sysadmin_urls.py +++ b/lms/djangoapps/dashboard/sysadmin_urls.py @@ -1,6 +1,7 @@ """ Urls for sysadmin dashboard feature """ +# pylint: disable=E1120 from django.conf.urls import patterns, url from dashboard import sysadmin diff --git a/lms/djangoapps/dashboard/tests/test_sysadmin.py b/lms/djangoapps/dashboard/tests/test_sysadmin.py index 43631ab79cd9..c519427b1f79 100644 --- a/lms/djangoapps/dashboard/tests/test_sysadmin.py +++ b/lms/djangoapps/dashboard/tests/test_sysadmin.py @@ -15,12 +15,11 @@ from dashboard.sysadmin import Users from external_auth.models import ExternalAuthMap from django.contrib.auth.hashers import check_password -from django.contrib.auth.models import Group from xmodule.modulestore.django import modulestore from courseware.tests.tests import TEST_DATA_MONGO_MODULESTORE from xmodule.modulestore.tests.django_utils import ModuleStoreTestCase from django.utils.html import escape -from courseware.roles import CourseStaffRole +from courseware.roles import CourseStaffRole, GlobalStaff from dashboard.models import CourseImportLog from xmodule.modulestore.xml import XMLModuleStore @@ -52,8 +51,7 @@ def setUp(self): def _setstaff_login(self): """Makes the test user staff and logs them in""" - self.user.is_staff = True - self.user.save() + GlobalStaff().add_users(self.user) self.client.login(username=self.user.username, password='foo') def _add_edx4edx(self):