From d395c4448d09e77446ac83533c6342b4e0b738fa Mon Sep 17 00:00:00 2001 From: Brian Wilson Date: Tue, 22 Jan 2013 17:58:40 -0500 Subject: [PATCH] add more pearson tests, and update commands in response --- .../management/commands/pearson_export_cdd.py | 9 +- .../management/commands/pearson_export_ead.py | 8 +- .../management/commands/pearson_transfer.py | 11 +- .../management/commands/tests/test_pearson.py | 192 ++++++++++++++---- 4 files changed, 176 insertions(+), 44 deletions(-) diff --git a/common/djangoapps/student/management/commands/pearson_export_cdd.py b/common/djangoapps/student/management/commands/pearson_export_cdd.py index ba43dd3bc0..463eec6b70 100644 --- a/common/djangoapps/student/management/commands/pearson_export_cdd.py +++ b/common/djangoapps/student/management/commands/pearson_export_cdd.py @@ -5,7 +5,7 @@ from datetime import datetime from optparse import make_option from django.conf import settings -from django.core.management.base import BaseCommand +from django.core.management.base import BaseCommand, CommandError from student.models import TestCenterUser @@ -39,6 +39,9 @@ class Command(BaseCommand): ("LastUpdate", "user_updated_at"), # in UTC, so same as what we store ]) + # define defaults, even thought 'store_true' shouldn't need them. + # (call_command will set None as default value for all options that don't have one, + # so one cannot rely on presence/absence of flags in that world.) option_list = BaseCommand.option_list + ( make_option('--dest-from-settings', action='store_true', @@ -63,13 +66,13 @@ class Command(BaseCommand): # Name will use timestamp -- this is UTC, so it will look funny, # but it should at least be consistent with the other timestamps # used in the system. - if 'dest-from-settings' in options: + if 'dest-from-settings' in options and options['dest-from-settings']: if 'LOCAL_EXPORT' in settings.PEARSON: dest = settings.PEARSON['LOCAL_EXPORT'] else: raise CommandError('--dest-from-settings was enabled but the' 'PEARSON[LOCAL_EXPORT] setting was not set.') - elif 'destination' in options: + elif 'destination' in options and options['destination']: dest = options['destination'] else: raise CommandError('--destination or --dest-from-settings must be used') diff --git a/common/djangoapps/student/management/commands/pearson_export_ead.py b/common/djangoapps/student/management/commands/pearson_export_ead.py index 492cba154b..49cdc9957a 100644 --- a/common/djangoapps/student/management/commands/pearson_export_ead.py +++ b/common/djangoapps/student/management/commands/pearson_export_ead.py @@ -5,7 +5,7 @@ from datetime import datetime from optparse import make_option from django.conf import settings -from django.core.management.base import BaseCommand +from django.core.management.base import BaseCommand, CommandError from student.models import TestCenterRegistration @@ -39,10 +39,12 @@ class Command(BaseCommand): make_option('--dump_all', action='store_true', dest='dump_all', + default=False, ), make_option('--force_add', action='store_true', dest='force_add', + default=False, ), ) @@ -57,13 +59,13 @@ class Command(BaseCommand): # Name will use timestamp -- this is UTC, so it will look funny, # but it should at least be consistent with the other timestamps # used in the system. - if 'dest-from-settings' in options: + if 'dest-from-settings' in options and options['dest-from-settings']: if 'LOCAL_EXPORT' in settings.PEARSON: dest = settings.PEARSON['LOCAL_EXPORT'] else: raise CommandError('--dest-from-settings was enabled but the' 'PEARSON[LOCAL_EXPORT] setting was not set.') - elif 'destinations' in options: + elif 'destination' in options and options['destination']: dest = options['destination'] else: raise CommandError('--destination or --dest-from-settings must be used') diff --git a/common/djangoapps/student/management/commands/pearson_transfer.py b/common/djangoapps/student/management/commands/pearson_transfer.py index 5f126a24f0..c216d2ceac 100644 --- a/common/djangoapps/student/management/commands/pearson_transfer.py +++ b/common/djangoapps/student/management/commands/pearson_transfer.py @@ -1,10 +1,12 @@ +import os from optparse import make_option +from django.conf import settings from django.core.management.base import BaseCommand, CommandError +from django.core.management import call_command from dogapi import dog_http_api, dog_stats_api import paramiko import boto -import os dog_http_api.api_key = settings.DATADOG_API @@ -13,7 +15,7 @@ class Command(BaseCommand): help = """ This command handles the importing and exporting of student records for Pearson. It uses some other Django commands to export and import the - files and then uploads over SFTP to pearson and stuffs the entry in an + files and then uploads over SFTP to Pearson and stuffs the entry in an S3 bucket for archive purposes. Usage: django-admin.py pearson-transfer --mode [import|export|both] @@ -29,11 +31,12 @@ class Command(BaseCommand): def handle(self, **options): + # TODO: this doesn't work. Need to check if it's a property. if not settings.PEARSON: raise CommandError('No PEARSON entries in auth/env.json.') - for value in ['LOCAL_IMPORT', 'SFTP_IMPORT', 'BUCKET', 'LOCAL_EXPORT', - 'SFTP_EXPORT']: + for value in ['LOCAL_IMPORT', 'SFTP_IMPORT', 'LOCAL_EXPORT', + 'SFTP_EXPORT', 'SFTP_HOSTNAME', 'SFTP_USERNAME', 'SFTP_PASSWORD']: if value not in settings.PEARSON: raise CommandError('No entry in the PEARSON settings' '(env/auth.json) for {0}'.format(value)) diff --git a/common/djangoapps/student/management/commands/tests/test_pearson.py b/common/djangoapps/student/management/commands/tests/test_pearson.py index 538cd2812a..d5594926d2 100644 --- a/common/djangoapps/student/management/commands/tests/test_pearson.py +++ b/common/djangoapps/student/management/commands/tests/test_pearson.py @@ -10,7 +10,7 @@ import cStringIO import sys from django.test import TestCase -from django.core import management +from django.core.management import call_command from student.models import User, TestCenterRegistration, TestCenterUser, get_testcenter_registration @@ -30,11 +30,11 @@ def create_tc_user(username): 'phone' : '252-1866', 'phone_country_code' : '1', } - management.call_command('pearson_make_tc_user', username, **options) + call_command('pearson_make_tc_user', username, **options) return TestCenterUser.objects.get(user=user) -def create_tc_registration(username, course_id, exam_code, accommodation_code): +def create_tc_registration(username, course_id = 'org1/course1/term1', exam_code = 'exam1', accommodation_code = None): options = { 'exam_series_code' : exam_code, 'eligibility_appointment_date_first' : '2013-01-01T00:00', @@ -42,11 +42,55 @@ def create_tc_registration(username, course_id, exam_code, accommodation_code): 'accommodation_code' : accommodation_code, } - management.call_command('pearson_make_tc_registration', username, course_id, **options) + call_command('pearson_make_tc_registration', username, course_id, **options) user = User.objects.get(username=username) registrations = get_testcenter_registration(user, course_id, exam_code) return registrations[0] + +def get_error_string_for_management_call(*args, **options): + stdout_string = None + old_stdout = sys.stdout + old_stderr = sys.stderr + sys.stdout = cStringIO.StringIO() + sys.stderr = cStringIO.StringIO() + try: + call_command(*args, **options) + except BaseException, why1: + # The goal here is to catch CommandError calls. + # But these are actually translated into nice messages, + # and sys.exit(1) is then called. For testing, we + # want to catch what sys.exit throws, and get the + # relevant text either from stdout or stderr. + # TODO: this should really check to see that we + # arrived here because of a sys.exit(1). Otherwise + # we should just raise the exception. + stdout_string = sys.stdout.getvalue() + stderr_string = sys.stderr.getvalue() + except Exception, why: + raise why + finally: + sys.stdout = old_stdout + sys.stderr = old_stderr + + if stdout_string is None: + raise Exception("Expected call to {} to fail, but it succeeded!".format(args[0])) + return stdout_string, stderr_string + + +def get_file_info(dirpath): + filelist = os.listdir(dirpath) + print 'Files found: {}'.format(filelist) + numfiles = len(filelist) + if numfiles == 1: + filepath = os.path.join(dirpath, filelist[0]) + with open(filepath, 'r') as cddfile: + filecontents = cddfile.readlines() + numlines = len(filecontents) + return filepath, numlines + else: + raise Exception("Expected to find a single file in {}, but found {}".format(dirpath,filelist)) + class PearsonTestCase(TestCase): ''' Base class for tests running Pearson-related commands @@ -54,7 +98,9 @@ class PearsonTestCase(TestCase): import_dir = mkdtemp(prefix="import") export_dir = mkdtemp(prefix="export") - + def assertErrorContains(self, error_message, expected): + self.assertTrue(error_message.find(expected) >= 0, 'error message "{}" did not contain "{}"'.format(error_message, expected)) + def tearDown(self): def delete_temp_dir(dirname): if os.path.exists(dirname): @@ -67,14 +113,13 @@ class PearsonTestCase(TestCase): delete_temp_dir(self.export_dir) def test_missing_demographic_fields(self): - old_stdout = sys.stdout - sys.stdout = cStringIO.StringIO() + # We won't bother to test all details of form validation here. + # It is enough to show that it works here, but deal with test cases for the form + # validation in the student tests, not these management tests. username = 'baduser' User.objects.create_user(username, '{}@edx.org'.format(username), 'fakepass') options = {} - - self.assertRaises(BaseException, management.call_command, 'pearson_make_tc_user', username, **options) - output_string = sys.stdout.getvalue() + output_string, _ = get_error_string_for_management_call('pearson_make_tc_user', username, **options) self.assertTrue(output_string.find('Field Form errors encountered:') >= 0) self.assertTrue(output_string.find('Field Form Error: city') >= 0) self.assertTrue(output_string.find('Field Form Error: first_name') >= 0) @@ -83,35 +128,114 @@ class PearsonTestCase(TestCase): self.assertTrue(output_string.find('Field Form Error: phone_country_code') >= 0) self.assertTrue(output_string.find('Field Form Error: phone') >= 0) self.assertTrue(output_string.find('Field Form Error: address_1') >= 0) - sys.stdout = old_stdout + self.assertErrorContains(output_string, 'Field Form Error: address_1') def test_create_good_testcenter_user(self): testcenter_user = create_tc_user("test1") + self.assertIsNotNone(testcenter_user) def test_create_good_testcenter_registration(self): username = 'test1' - course_id = 'org1/course1/term1' - exam_code = 'exam1' - accommodation_code = 'NONE' - testcenter_user = create_tc_user(username) - registration = create_tc_registration(username, course_id, exam_code, accommodation_code) - - def test_export(self): - username = 'test1' - course_id = 'org1/course1/term1' - exam_code = 'exam1' - accommodation_code = 'NONE' - testcenter_user = create_tc_user(username) - registration = create_tc_registration(username, course_id, exam_code, accommodation_code) - #options = { 'destination' : self.export_dir } - options = { '--dest-from-settings' : None } - with self.settings(PEARSON={ 'LOCAL_EXPORT' : self.export_dir }): - management.call_command('pearson_export_cdd', **options) - print 'Files found: {}'.format(os.listdir(self.export_dir)) - self.assertEquals(len(os.listdir(self.export_dir)), 1, "Expect cdd file to be created") - management.call_command('pearson_export_ead', **options) - print 'Files found: {}'.format(os.listdir(self.export_dir)) - self.assertEquals(len(os.listdir(self.export_dir)), 2, "Expect ead file to also be created") + create_tc_user(username) + registration = create_tc_registration(username) + self.assertIsNotNone(registration) - # TODO: check that files were output.... + def test_cdd_missing_option(self): + _, error_string = get_error_string_for_management_call('pearson_export_cdd', **{}) + self.assertErrorContains(error_string, 'Error: --destination or --dest-from-settings must be used') + + def test_ead_missing_option(self): + _, error_string = get_error_string_for_management_call('pearson_export_ead', **{}) + self.assertErrorContains(error_string, 'Error: --destination or --dest-from-settings must be used') + + def test_export_single_cdd(self): + # before we generate any tc_users, we expect there to be nothing to output: + options = { 'dest-from-settings' : True } + with self.settings(PEARSON={ 'LOCAL_EXPORT' : self.export_dir }): + call_command('pearson_export_cdd', **options) + (filepath, numlines) = get_file_info(self.export_dir) + self.assertEquals(numlines, 1, "Expect cdd file to have no non-header lines") + os.remove(filepath) + + # generating a tc_user should result in a line in the output + username = 'test_single_cdd' + create_tc_user(username) + call_command('pearson_export_cdd', **options) + (filepath, numlines) = get_file_info(self.export_dir) + self.assertEquals(numlines, 2, "Expect cdd file to have one non-header line") + os.remove(filepath) + + # output after registration should not have any entries again. + call_command('pearson_export_cdd', **options) + (filepath, numlines) = get_file_info(self.export_dir) + self.assertEquals(numlines, 1, "Expect cdd file to have no non-header lines") + os.remove(filepath) + + # if we modify the record, then it should be output again: + user_options = { 'first_name' : 'NewTestFirst', } + call_command('pearson_make_tc_user', username, **user_options) + call_command('pearson_export_cdd', **options) + (filepath, numlines) = get_file_info(self.export_dir) + self.assertEquals(numlines, 2, "Expect cdd file to have one non-header line") + os.remove(filepath) + + def test_export_single_ead(self): + # before we generate any registrations, we expect there to be nothing to output: + options = { 'dest-from-settings' : True } + with self.settings(PEARSON={ 'LOCAL_EXPORT' : self.export_dir }): + call_command('pearson_export_ead', **options) + (filepath, numlines) = get_file_info(self.export_dir) + self.assertEquals(numlines, 1, "Expect ead file to have no non-header lines") + os.remove(filepath) + + # generating a registration should result in a line in the output + username = 'test_single_ead' + create_tc_user(username) + create_tc_registration(username) + call_command('pearson_export_ead', **options) + (filepath, numlines) = get_file_info(self.export_dir) + self.assertEquals(numlines, 2, "Expect ead file to have one non-header line") + os.remove(filepath) + + # output after registration should not have any entries again. + call_command('pearson_export_ead', **options) + (filepath, numlines) = get_file_info(self.export_dir) + self.assertEquals(numlines, 1, "Expect ead file to have no non-header lines") + os.remove(filepath) + # if we modify the record, then it should be output again: + create_tc_registration(username, accommodation_code='EQPMNT') + call_command('pearson_export_ead', **options) + (filepath, numlines) = get_file_info(self.export_dir) + self.assertEquals(numlines, 2, "Expect ead file to have one non-header line") + os.remove(filepath) + + def test_export_multiple(self): + username1 = 'test_multiple1' + create_tc_user(username1) + create_tc_registration(username1) + create_tc_registration(username1, course_id = 'org1/course2/term1') + create_tc_registration(username1, exam_code = 'exam2') + username2 = 'test_multiple2' + create_tc_user(username2) + create_tc_registration(username2) + username3 = 'test_multiple3' + create_tc_user(username3) + create_tc_registration(username3, course_id = 'org1/course2/term1') + username4 = 'test_multiple4' + create_tc_user(username4) + create_tc_registration(username4, exam_code = 'exam2') + + with self.settings(PEARSON={ 'LOCAL_EXPORT' : self.export_dir }): + options = { 'dest-from-settings' : True } + call_command('pearson_export_cdd', **options) + (filepath, numlines) = get_file_info(self.export_dir) + self.assertEquals(numlines, 5, "Expect cdd file to have four non-header lines: total was {}".format(numlines)) + os.remove(filepath) + + call_command('pearson_export_ead', **options) + (filepath, numlines) = get_file_info(self.export_dir) + self.assertEquals(numlines, 7, "Expect ead file to have six non-header lines: total was {}".format(numlines)) + os.remove(filepath) + +