Commit d0b12ee3 by Gabe Mulley

Generalize grade report to allow display of any report

We would like to be able to generate arbitrary reports and expose them to the instructor by simply uploading them to S3.  The existing grade download infrastructure pretty much supported that already, however, all of the internal and external structures were referring to the reports as exclusively grading related.

Fixes: AN-590
parent a10d8d41
...@@ -29,7 +29,7 @@ Feature: LMS.Instructor Dash Data Download ...@@ -29,7 +29,7 @@ Feature: LMS.Instructor Dash Data Download
Scenario: Generate & download a grade report Scenario: Generate & download a grade report
Given I am "<Role>" for a course Given I am "<Role>" for a course
When I click "Generate Grade Report" When I click "Generate Grade Report"
Then I see a csv file in the grade reports table Then I see a grade report csv file in the reports table
Examples: Examples:
| Role | | Role |
| instructor | | instructor |
......
...@@ -62,14 +62,14 @@ length=0""" ...@@ -62,14 +62,14 @@ length=0"""
assert_in(expected_config, world.css_text('#data-grade-config-text')) assert_in(expected_config, world.css_text('#data-grade-config-text'))
@step(u"I see a csv file in the grade reports table") @step(u"I see a grade report csv file in the reports table")
def find_grade_report_csv_link(step): # pylint: disable=unused-argument def find_grade_report_csv_link(step): # pylint: disable=unused-argument
# Need to reload the page to see the grades download table # Need to reload the page to see the grades download table
reload_the_page(step) reload_the_page(step)
world.wait_for_visible('#grade-downloads-table') world.wait_for_visible('#report-downloads-table')
# Find table and assert a .csv file is present # Find table and assert a .csv file is present
expected_file_regexp = 'edx_999_Test_Course_grade_report_\d{4}-\d{2}-\d{2}-\d{4}\.csv' expected_file_regexp = 'edx_999_Test_Course_grade_report_\d{4}-\d{2}-\d{2}-\d{4}\.csv'
assert_regexp_matches( assert_regexp_matches(
world.css_html('#grade-downloads-table'), expected_file_regexp, world.css_html('#report-downloads-table'), expected_file_regexp,
msg="Expected grade report filename was not found." msg="Expected grade report filename was not found."
) )
...@@ -130,7 +130,7 @@ class TestInstructorAPIDenyLevels(ModuleStoreTestCase, LoginEnrollmentTestCase): ...@@ -130,7 +130,7 @@ class TestInstructorAPIDenyLevels(ModuleStoreTestCase, LoginEnrollmentTestCase):
('send_email', {'send_to': 'staff', 'subject': 'test', 'message': 'asdf'}), ('send_email', {'send_to': 'staff', 'subject': 'test', 'message': 'asdf'}),
('list_instructor_tasks', {}), ('list_instructor_tasks', {}),
('list_background_email_tasks', {}), ('list_background_email_tasks', {}),
('list_grade_downloads', {}), ('list_report_downloads', {}),
('calculate_grades_csv', {}), ('calculate_grades_csv', {}),
] ]
# Endpoints that only Instructors can access # Endpoints that only Instructors can access
...@@ -794,9 +794,9 @@ class TestInstructorAPILevelsDataDump(ModuleStoreTestCase, LoginEnrollmentTestCa ...@@ -794,9 +794,9 @@ class TestInstructorAPILevelsDataDump(ModuleStoreTestCase, LoginEnrollmentTestCa
self.assertTrue(body.startswith('"User ID","Anonymized user ID"\n"2","42"\n')) self.assertTrue(body.startswith('"User ID","Anonymized user ID"\n"2","42"\n'))
self.assertTrue(body.endswith('"7","42"\n')) self.assertTrue(body.endswith('"7","42"\n'))
def test_list_grade_downloads(self): def test_list_report_downloads(self):
url = reverse('list_grade_downloads', kwargs={'course_id': self.course.id}) url = reverse('list_report_downloads', kwargs={'course_id': self.course.id})
with patch('instructor_task.models.LocalFSGradesStore.links_for') as mock_links_for: with patch('instructor_task.models.LocalFSReportStore.links_for') as mock_links_for:
mock_links_for.return_value = [ mock_links_for.return_value = [
('mock_file_name_1', 'https://1.mock.url'), ('mock_file_name_1', 'https://1.mock.url'),
('mock_file_name_2', 'https://2.mock.url'), ('mock_file_name_2', 'https://2.mock.url'),
......
...@@ -33,7 +33,7 @@ from student.models import unique_id_for_user ...@@ -33,7 +33,7 @@ from student.models import unique_id_for_user
import instructor_task.api import instructor_task.api
from instructor_task.api_helper import AlreadyRunningError from instructor_task.api_helper import AlreadyRunningError
from instructor_task.views import get_task_completion_info from instructor_task.views import get_task_completion_info
from instructor_task.models import GradesStore from instructor_task.models import ReportStore
import instructor.enrollment as enrollment import instructor.enrollment as enrollment
from instructor.enrollment import enroll_email, unenroll_email, get_email_params from instructor.enrollment import enroll_email, unenroll_email, get_email_params
from instructor.access import list_with_level, allow_access, revoke_access, update_forum_role from instructor.access import list_with_level, allow_access, revoke_access, update_forum_role
...@@ -770,16 +770,16 @@ def list_instructor_tasks(request, course_id): ...@@ -770,16 +770,16 @@ def list_instructor_tasks(request, course_id):
@ensure_csrf_cookie @ensure_csrf_cookie
@cache_control(no_cache=True, no_store=True, must_revalidate=True) @cache_control(no_cache=True, no_store=True, must_revalidate=True)
@require_level('staff') @require_level('staff')
def list_grade_downloads(_request, course_id): def list_report_downloads(_request, course_id):
""" """
List grade CSV files that are available for download for this course. List grade CSV files that are available for download for this course.
""" """
grades_store = GradesStore.from_config() report_store = ReportStore.from_config()
response_payload = { response_payload = {
'downloads': [ 'downloads': [
dict(name=name, url=url, link='<a href="{}">{}</a>'.format(url, name)) dict(name=name, url=url, link='<a href="{}">{}</a>'.format(url, name))
for name, url in grades_store.links_for(course_id) for name, url in report_store.links_for(course_id)
] ]
} }
return JsonResponse(response_payload) return JsonResponse(response_payload)
......
...@@ -47,8 +47,8 @@ urlpatterns = patterns('', # nopep8 ...@@ -47,8 +47,8 @@ urlpatterns = patterns('', # nopep8
name='show_student_extensions'), name='show_student_extensions'),
# Grade downloads... # Grade downloads...
url(r'^list_grade_downloads$', url(r'^list_report_downloads$',
'instructor.views.api.list_grade_downloads', name="list_grade_downloads"), 'instructor.views.api.list_report_downloads', name="list_report_downloads"),
url(r'calculate_grades_csv$', url(r'calculate_grades_csv$',
'instructor.views.api.calculate_grades_csv', name="calculate_grades_csv"), 'instructor.views.api.calculate_grades_csv', name="calculate_grades_csv"),
) )
...@@ -194,7 +194,7 @@ def _section_data_download(course_id, access): ...@@ -194,7 +194,7 @@ def _section_data_download(course_id, access):
'get_students_features_url': reverse('get_students_features', kwargs={'course_id': course_id}), 'get_students_features_url': reverse('get_students_features', kwargs={'course_id': course_id}),
'get_anon_ids_url': reverse('get_anon_ids', kwargs={'course_id': course_id}), 'get_anon_ids_url': reverse('get_anon_ids', kwargs={'course_id': course_id}),
'list_instructor_tasks_url': reverse('list_instructor_tasks', kwargs={'course_id': course_id}), 'list_instructor_tasks_url': reverse('list_instructor_tasks', kwargs={'course_id': course_id}),
'list_grade_downloads_url': reverse('list_grade_downloads', kwargs={'course_id': course_id}), 'list_report_downloads_url': reverse('list_report_downloads', kwargs={'course_id': course_id}),
'calculate_grades_csv_url': reverse('calculate_grades_csv', kwargs={'course_id': course_id}), 'calculate_grades_csv_url': reverse('calculate_grades_csv', kwargs={'course_id': course_id}),
} }
return section_data return section_data
......
...@@ -189,29 +189,29 @@ class InstructorTask(models.Model): ...@@ -189,29 +189,29 @@ class InstructorTask(models.Model):
return json.dumps({'message': 'Task revoked before running'}) return json.dumps({'message': 'Task revoked before running'})
class GradesStore(object): class ReportStore(object):
""" """
Simple abstraction layer that can fetch and store CSV files for grades Simple abstraction layer that can fetch and store CSV files for reports
download. Should probably refactor later to create a GradesFile object that download. Should probably refactor later to create a ReportFile object that
can simply be appended to for the sake of memory efficiency, rather than can simply be appended to for the sake of memory efficiency, rather than
passing in the whole dataset. Doing that for now just because it's simpler. passing in the whole dataset. Doing that for now just because it's simpler.
""" """
@classmethod @classmethod
def from_config(cls): def from_config(cls):
""" """
Return one of the GradesStore subclasses depending on django Return one of the ReportStore subclasses depending on django
configuration. Look at subclasses for expected configuration. configuration. Look at subclasses for expected configuration.
""" """
storage_type = settings.GRADES_DOWNLOAD.get("STORAGE_TYPE") storage_type = settings.GRADES_DOWNLOAD.get("STORAGE_TYPE")
if storage_type.lower() == "s3": if storage_type.lower() == "s3":
return S3GradesStore.from_config() return S3ReportStore.from_config()
elif storage_type.lower() == "localfs": elif storage_type.lower() == "localfs":
return LocalFSGradesStore.from_config() return LocalFSReportStore.from_config()
class S3GradesStore(GradesStore): class S3ReportStore(ReportStore):
""" """
Grades store backed by S3. The directory structure we use to store things Reports store backed by S3. The directory structure we use to store things
is:: is::
`{bucket}/{root_path}/{sha1 hash of course_id}/filename` `{bucket}/{root_path}/{sha1 hash of course_id}/filename`
...@@ -233,11 +233,11 @@ class S3GradesStore(GradesStore): ...@@ -233,11 +233,11 @@ class S3GradesStore(GradesStore):
@classmethod @classmethod
def from_config(cls): def from_config(cls):
""" """
The expected configuration for an `S3GradesStore` is to have a The expected configuration for an `S3ReportStore` is to have a
`GRADES_DOWNLOAD` dict in settings with the following fields:: `GRADES_DOWNLOAD` dict in settings with the following fields::
STORAGE_TYPE : "s3" STORAGE_TYPE : "s3"
BUCKET : Your bucket name, e.g. "grades-bucket" BUCKET : Your bucket name, e.g. "reports-bucket"
ROOT_PATH : The path you want to store all course files under. Do not ROOT_PATH : The path you want to store all course files under. Do not
use a leading or trailing slash. e.g. "staging" or use a leading or trailing slash. e.g. "staging" or
"staging/2013", not "/staging", or "/staging/" "staging/2013", not "/staging", or "/staging/"
...@@ -325,10 +325,10 @@ class S3GradesStore(GradesStore): ...@@ -325,10 +325,10 @@ class S3GradesStore(GradesStore):
) )
class LocalFSGradesStore(GradesStore): class LocalFSReportStore(ReportStore):
""" """
LocalFS implementation of a GradesStore. This is meant for debugging LocalFS implementation of a ReportStore. This is meant for debugging
purposes and is *absolutely not for production use*. Use S3GradesStore for purposes and is *absolutely not for production use*. Use S3ReportStore for
that. We use this in tests and for local development. When it generates that. We use this in tests and for local development. When it generates
links, it will make file:/// style links. That means you actually have to links, it will make file:/// style links. That means you actually have to
copy them and open them in a separate browser window, for security reasons. copy them and open them in a separate browser window, for security reasons.
...@@ -350,11 +350,11 @@ class LocalFSGradesStore(GradesStore): ...@@ -350,11 +350,11 @@ class LocalFSGradesStore(GradesStore):
Generate an instance of this object from Django settings. It assumes Generate an instance of this object from Django settings. It assumes
that there is a dict in settings named GRADES_DOWNLOAD and that it has that there is a dict in settings named GRADES_DOWNLOAD and that it has
a ROOT_PATH that maps to an absolute file path that the web app has a ROOT_PATH that maps to an absolute file path that the web app has
write permissions to. `LocalFSGradesStore` will create any intermediate write permissions to. `LocalFSReportStore` will create any intermediate
directories as needed. Example:: directories as needed. Example::
STORAGE_TYPE : "localfs" STORAGE_TYPE : "localfs"
ROOT_PATH : /tmp/edx/grade-downloads/ ROOT_PATH : /tmp/edx/report-downloads/
""" """
return cls(settings.GRADES_DOWNLOAD['ROOT_PATH']) return cls(settings.GRADES_DOWNLOAD['ROOT_PATH'])
...@@ -389,7 +389,7 @@ class LocalFSGradesStore(GradesStore): ...@@ -389,7 +389,7 @@ class LocalFSGradesStore(GradesStore):
def links_for(self, course_id): def links_for(self, course_id):
""" """
For a given `course_id`, return a list of `(filename, url)` tuples. `url` For a given `course_id`, return a list of `(filename, url)` tuples. `url`
can be plugged straight into an href. Note that `LocalFSGradesStore` can be plugged straight into an href. Note that `LocalFSReportStore`
will generate `file://` type URLs, so you'll need to copy the URL and will generate `file://` type URLs, so you'll need to copy the URL and
open it in a new browser window. Again, this class is only meant for open it in a new browser window. Again, this class is only meant for
local development. local development.
......
...@@ -23,7 +23,7 @@ from courseware.grades import iterate_grades_for ...@@ -23,7 +23,7 @@ from courseware.grades import iterate_grades_for
from courseware.models import StudentModule from courseware.models import StudentModule
from courseware.model_data import FieldDataCache from courseware.model_data import FieldDataCache
from courseware.module_render import get_module_for_descriptor_internal from courseware.module_render import get_module_for_descriptor_internal
from instructor_task.models import GradesStore, InstructorTask, PROGRESS from instructor_task.models import ReportStore, InstructorTask, PROGRESS
from student.models import CourseEnrollment from student.models import CourseEnrollment
# define different loggers for use within tasks and on client side # define different loggers for use within tasks and on client side
...@@ -473,11 +473,11 @@ def delete_problem_module_state(xmodule_instance_args, _module_descriptor, stude ...@@ -473,11 +473,11 @@ def delete_problem_module_state(xmodule_instance_args, _module_descriptor, stude
def push_grades_to_s3(_xmodule_instance_args, _entry_id, course_id, _task_input, action_name): def push_grades_to_s3(_xmodule_instance_args, _entry_id, course_id, _task_input, action_name):
""" """
For a given `course_id`, generate a grades CSV file for all students that For a given `course_id`, generate a grades CSV file for all students that
are enrolled, and store using a `GradesStore`. Once created, the files can are enrolled, and store using a `ReportStore`. Once created, the files can
be accessed by instantiating another `GradesStore` (via be accessed by instantiating another `ReportStore` (via
`GradesStore.from_config()`) and calling `link_for()` on it. Writes are `ReportStore.from_config()`) and calling `link_for()` on it. Writes are
buffered, so we'll never write part of a CSV file to S3 -- i.e. any files buffered, so we'll never write part of a CSV file to S3 -- i.e. any files
that are visible in GradesStore will be complete ones. that are visible in ReportStore will be complete ones.
As we start to add more CSV downloads, it will probably be worthwhile to As we start to add more CSV downloads, it will probably be worthwhile to
make a more general CSVDoc class instead of building out the rows like we make a more general CSVDoc class instead of building out the rows like we
...@@ -555,8 +555,8 @@ def push_grades_to_s3(_xmodule_instance_args, _entry_id, course_id, _task_input, ...@@ -555,8 +555,8 @@ def push_grades_to_s3(_xmodule_instance_args, _entry_id, course_id, _task_input,
course_id_prefix = urllib.quote(course_id.replace("/", "_")) course_id_prefix = urllib.quote(course_id.replace("/", "_"))
# Perform the actual upload # Perform the actual upload
grades_store = GradesStore.from_config() report_store = ReportStore.from_config()
grades_store.store_rows( report_store.store_rows(
course_id, course_id,
u"{}_grade_report_{}.csv".format(course_id_prefix, timestamp_str), u"{}_grade_report_{}.csv".format(course_id_prefix, timestamp_str),
rows rows
...@@ -564,7 +564,7 @@ def push_grades_to_s3(_xmodule_instance_args, _entry_id, course_id, _task_input, ...@@ -564,7 +564,7 @@ def push_grades_to_s3(_xmodule_instance_args, _entry_id, course_id, _task_input,
# If there are any error rows (don't count the header), write them out as well # If there are any error rows (don't count the header), write them out as well
if len(err_rows) > 1: if len(err_rows) > 1:
grades_store.store_rows( report_store.store_rows(
course_id, course_id,
u"{}_grade_report_{}_err.csv".format(course_id_prefix, timestamp_str), u"{}_grade_report_{}_err.csv".format(course_id_prefix, timestamp_str),
err_rows err_rows
......
...@@ -31,7 +31,7 @@ class DataDownload ...@@ -31,7 +31,7 @@ class DataDownload
@$grades_request_response = @$grades.find '.request-response' @$grades_request_response = @$grades.find '.request-response'
@$grades_request_response_error = @$grades.find '.request-response-error' @$grades_request_response_error = @$grades.find '.request-response-error'
@grade_downloads = new GradeDownloads(@$section) @report_downloads = new ReportDownloads(@$section)
@instructor_tasks = new (PendingInstructorTasks()) @$section @instructor_tasks = new (PendingInstructorTasks()) @$section
@clear_display() @clear_display()
...@@ -115,12 +115,12 @@ class DataDownload ...@@ -115,12 +115,12 @@ class DataDownload
# Clear display of anything that was here before # Clear display of anything that was here before
@clear_display() @clear_display()
@instructor_tasks.task_poller.start() @instructor_tasks.task_poller.start()
@grade_downloads.downloads_poller.start() @report_downloads.downloads_poller.start()
# handler for when the section is closed # handler for when the section is closed
onExit: -> onExit: ->
@instructor_tasks.task_poller.stop() @instructor_tasks.task_poller.stop()
@grade_downloads.downloads_poller.stop() @report_downloads.downloads_poller.stop()
clear_display: -> clear_display: ->
# Clear any generated tables, warning messages, etc. # Clear any generated tables, warning messages, etc.
...@@ -134,35 +134,31 @@ class DataDownload ...@@ -134,35 +134,31 @@ class DataDownload
$(".msg-error").css({"display":"none"}) $(".msg-error").css({"display":"none"})
class GradeDownloads class ReportDownloads
### Grade Downloads -- links expire quickly, so we refresh every 5 mins #### ### Report Downloads -- links expire quickly, so we refresh every 5 mins ####
constructor: (@$section) -> constructor: (@$section) ->
@$report_downloads_table = @$section.find ".report-downloads-table"
@$grades = @$section.find '.grades-download-container'
@$grades_request_response = @$grades.find '.request-response'
@$grades_request_response_error = @$grades.find '.request-response-error'
@$grade_downloads_table = @$grades.find ".grade-downloads-table"
POLL_INTERVAL = 1000 * 60 * 5 # 5 minutes in ms POLL_INTERVAL = 1000 * 60 * 5 # 5 minutes in ms
@downloads_poller = new window.InstructorDashboard.util.IntervalManager( @downloads_poller = new window.InstructorDashboard.util.IntervalManager(
POLL_INTERVAL, => @reload_grade_downloads() POLL_INTERVAL, => @reload_report_downloads()
) )
reload_grade_downloads: -> reload_report_downloads: ->
endpoint = @$grade_downloads_table.data 'endpoint' endpoint = @$report_downloads_table.data 'endpoint'
$.ajax $.ajax
dataType: 'json' dataType: 'json'
url: endpoint url: endpoint
success: (data) => success: (data) =>
if data.downloads.length if data.downloads.length
@create_grade_downloads_table data.downloads @create_report_downloads_table data.downloads
else else
console.log "No grade CSVs ready for download" console.log "No reports ready for download"
error: std_ajax_err => console.error "Error finding grade download CSVs" error: std_ajax_err => console.error "Error finding report downloads"
create_grade_downloads_table: (grade_downloads_data) -> create_report_downloads_table: (report_downloads_data) ->
@$grade_downloads_table.empty() @$report_downloads_table.empty()
options = options =
enableCellNavigation: true enableCellNavigation: true
...@@ -174,7 +170,7 @@ class GradeDownloads ...@@ -174,7 +170,7 @@ class GradeDownloads
id: 'link' id: 'link'
field: 'link' field: 'link'
name: gettext('File Name (Newest First)') name: gettext('File Name (Newest First)')
toolTip: gettext("Links are generated on demand and expire within 5 minutes due to the sensitive nature of student grade information.") toolTip: gettext("Links are generated on demand and expire within 5 minutes due to the sensitive nature of student information.")
sortable: false sortable: false
minWidth: 150 minWidth: 150
cssClass: "file-download-link" cssClass: "file-download-link"
...@@ -183,8 +179,8 @@ class GradeDownloads ...@@ -183,8 +179,8 @@ class GradeDownloads
] ]
$table_placeholder = $ '<div/>', class: 'slickgrid' $table_placeholder = $ '<div/>', class: 'slickgrid'
@$grade_downloads_table.append $table_placeholder @$report_downloads_table.append $table_placeholder
grid = new Slick.Grid($table_placeholder, grade_downloads_data, columns, options) grid = new Slick.Grid($table_placeholder, report_downloads_data, columns, options)
grid.autosizeColumns() grid.autosizeColumns()
......
...@@ -444,7 +444,7 @@ section.instructor-dashboard-content-2 { ...@@ -444,7 +444,7 @@ section.instructor-dashboard-content-2 {
} }
.grades-download-container { .grades-download-container {
.grade-downloads-table { .report-downloads-table {
.slickgrid { .slickgrid {
height: 300px; height: 300px;
padding: 5px; padding: 5px;
......
...@@ -26,7 +26,7 @@ ...@@ -26,7 +26,7 @@
%if settings.FEATURES.get('ENABLE_S3_GRADE_DOWNLOADS'): %if settings.FEATURES.get('ENABLE_S3_GRADE_DOWNLOADS'):
<div class="grades-download-container action-type-container"> <div class="grades-download-container action-type-container">
<hr> <hr>
<h2> ${_("Grade Reports")}</h2> <h2> ${_("Reports")}</h2>
%if settings.FEATURES.get('ALLOW_COURSE_STAFF_GRADE_DOWNLOADS') or section_data['access']['admin']: %if settings.FEATURES.get('ALLOW_COURSE_STAFF_GRADE_DOWNLOADS') or section_data['access']['admin']:
<p>${_("The following button will generate a CSV grade report for all currently enrolled students. Generated reports appear in a table below and can be downloaded.")}</p> <p>${_("The following button will generate a CSV grade report for all currently enrolled students. Generated reports appear in a table below and can be downloaded.")}</p>
...@@ -43,11 +43,15 @@ ...@@ -43,11 +43,15 @@
%endif %endif
<p><b>${_("Reports Available for Download")}</b></p> <p><b>${_("Reports Available for Download")}</b></p>
<p>${_("A new CSV report is generated each time you click the <b>Generate Grade Report</b> button above. A link to each report remains available on this page, identified by the UTC date and time of generation. Reports are not deleted, so you will always be able to access previously generated reports from this page.")}</p> <p>
${_("Grade reports listed below are generated each time the <b>Generate Grade Report</b> button is clicked. A link to each grade report remains available on this page, identified by the UTC date and time of generation. Grade reports are not deleted, so you will always be able to access previously generated reports from this page.")}
## Translators: "these rules" in this sentence references a detailed description of the grading reports that will appear in a table that contains a mix of different types of reports. This sentence intends to indicate that the description of the grading report does not apply to the other reports that may appear in the table.
${_("Other reports may appear in the table for which these rules will not necessarily apply.")}
</p>
## Translators: a table of URL links to report files appears after this sentence. ## Translators: a table of URL links to report files appears after this sentence.
<p>${_("<b>Note</b>: To keep student data secure, you cannot save or email these links for direct access. Copies of links expire within 5 minutes.")}</p><br> <p>${_("<b>Note</b>: To keep student data secure, you cannot save or email these links for direct access. Copies of links expire within 5 minutes.")}</p><br>
<div class="grade-downloads-table" id="grade-downloads-table" data-endpoint="${ section_data['list_grade_downloads_url'] }" ></div> <div class="report-downloads-table" id="report-downloads-table" data-endpoint="${ section_data['list_report_downloads_url'] }" ></div>
</div> </div>
%endif %endif
......
Markdown is supported
0% or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment