Merge pull request #2827 from mulby/gabe/report-downloads
Generalize grade report to allow display of any report
This commit is contained in:
@@ -29,7 +29,7 @@ Feature: LMS.Instructor Dash Data Download
|
||||
Scenario: Generate & download a grade report
|
||||
Given I am "<Role>" for a course
|
||||
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:
|
||||
| Role |
|
||||
| instructor |
|
||||
|
||||
@@ -62,14 +62,14 @@ length=0"""
|
||||
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
|
||||
# Need to reload the page to see the grades download table
|
||||
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
|
||||
expected_file_regexp = 'edx_999_Test_Course_grade_report_\d{4}-\d{2}-\d{2}-\d{4}\.csv'
|
||||
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."
|
||||
)
|
||||
|
||||
@@ -130,7 +130,7 @@ class TestInstructorAPIDenyLevels(ModuleStoreTestCase, LoginEnrollmentTestCase):
|
||||
('send_email', {'send_to': 'staff', 'subject': 'test', 'message': 'asdf'}),
|
||||
('list_instructor_tasks', {}),
|
||||
('list_background_email_tasks', {}),
|
||||
('list_grade_downloads', {}),
|
||||
('list_report_downloads', {}),
|
||||
('calculate_grades_csv', {}),
|
||||
]
|
||||
# Endpoints that only Instructors can access
|
||||
@@ -794,9 +794,9 @@ class TestInstructorAPILevelsDataDump(ModuleStoreTestCase, LoginEnrollmentTestCa
|
||||
self.assertTrue(body.startswith('"User ID","Anonymized user ID"\n"2","42"\n'))
|
||||
self.assertTrue(body.endswith('"7","42"\n'))
|
||||
|
||||
def test_list_grade_downloads(self):
|
||||
url = reverse('list_grade_downloads', kwargs={'course_id': self.course.id})
|
||||
with patch('instructor_task.models.LocalFSGradesStore.links_for') as mock_links_for:
|
||||
def test_list_report_downloads(self):
|
||||
url = reverse('list_report_downloads', kwargs={'course_id': self.course.id})
|
||||
with patch('instructor_task.models.LocalFSReportStore.links_for') as mock_links_for:
|
||||
mock_links_for.return_value = [
|
||||
('mock_file_name_1', 'https://1.mock.url'),
|
||||
('mock_file_name_2', 'https://2.mock.url'),
|
||||
|
||||
@@ -33,7 +33,7 @@ from student.models import unique_id_for_user
|
||||
import instructor_task.api
|
||||
from instructor_task.api_helper import AlreadyRunningError
|
||||
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
|
||||
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
|
||||
@@ -770,16 +770,16 @@ def list_instructor_tasks(request, course_id):
|
||||
@ensure_csrf_cookie
|
||||
@cache_control(no_cache=True, no_store=True, must_revalidate=True)
|
||||
@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.
|
||||
"""
|
||||
grades_store = GradesStore.from_config()
|
||||
report_store = ReportStore.from_config()
|
||||
|
||||
response_payload = {
|
||||
'downloads': [
|
||||
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)
|
||||
|
||||
@@ -47,8 +47,8 @@ urlpatterns = patterns('', # nopep8
|
||||
name='show_student_extensions'),
|
||||
|
||||
# Grade downloads...
|
||||
url(r'^list_grade_downloads$',
|
||||
'instructor.views.api.list_grade_downloads', name="list_grade_downloads"),
|
||||
url(r'^list_report_downloads$',
|
||||
'instructor.views.api.list_report_downloads', name="list_report_downloads"),
|
||||
url(r'calculate_grades_csv$',
|
||||
'instructor.views.api.calculate_grades_csv', name="calculate_grades_csv"),
|
||||
)
|
||||
|
||||
@@ -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_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_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}),
|
||||
}
|
||||
return section_data
|
||||
|
||||
@@ -189,29 +189,29 @@ class InstructorTask(models.Model):
|
||||
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
|
||||
download. Should probably refactor later to create a GradesFile object that
|
||||
Simple abstraction layer that can fetch and store CSV files for reports
|
||||
download. Should probably refactor later to create a ReportFile object that
|
||||
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.
|
||||
"""
|
||||
@classmethod
|
||||
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.
|
||||
"""
|
||||
storage_type = settings.GRADES_DOWNLOAD.get("STORAGE_TYPE")
|
||||
if storage_type.lower() == "s3":
|
||||
return S3GradesStore.from_config()
|
||||
return S3ReportStore.from_config()
|
||||
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::
|
||||
|
||||
`{bucket}/{root_path}/{sha1 hash of course_id}/filename`
|
||||
@@ -233,11 +233,11 @@ class S3GradesStore(GradesStore):
|
||||
@classmethod
|
||||
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::
|
||||
|
||||
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
|
||||
use a leading or trailing slash. e.g. "staging" or
|
||||
"staging/2013", not "/staging", or "/staging/"
|
||||
@@ -325,10 +325,10 @@ class S3GradesStore(GradesStore):
|
||||
)
|
||||
|
||||
|
||||
class LocalFSGradesStore(GradesStore):
|
||||
class LocalFSReportStore(ReportStore):
|
||||
"""
|
||||
LocalFS implementation of a GradesStore. This is meant for debugging
|
||||
purposes and is *absolutely not for production use*. Use S3GradesStore for
|
||||
LocalFS implementation of a ReportStore. This is meant for debugging
|
||||
purposes and is *absolutely not for production use*. Use S3ReportStore for
|
||||
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
|
||||
copy them and open them in a separate browser window, for security reasons.
|
||||
@@ -350,11 +350,11 @@ class LocalFSGradesStore(GradesStore):
|
||||
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
|
||||
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::
|
||||
|
||||
STORAGE_TYPE : "localfs"
|
||||
ROOT_PATH : /tmp/edx/grade-downloads/
|
||||
ROOT_PATH : /tmp/edx/report-downloads/
|
||||
"""
|
||||
return cls(settings.GRADES_DOWNLOAD['ROOT_PATH'])
|
||||
|
||||
@@ -389,7 +389,7 @@ class LocalFSGradesStore(GradesStore):
|
||||
def links_for(self, course_id):
|
||||
"""
|
||||
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
|
||||
open it in a new browser window. Again, this class is only meant for
|
||||
local development.
|
||||
|
||||
@@ -23,7 +23,7 @@ from courseware.grades import iterate_grades_for
|
||||
from courseware.models import StudentModule
|
||||
from courseware.model_data import FieldDataCache
|
||||
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
|
||||
|
||||
# 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
|
||||
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
|
||||
are enrolled, and store using a `GradesStore`. Once created, the files can
|
||||
be accessed by instantiating another `GradesStore` (via
|
||||
`GradesStore.from_config()`) and calling `link_for()` on it. Writes are
|
||||
are enrolled, and store using a `ReportStore`. Once created, the files can
|
||||
be accessed by instantiating another `ReportStore` (via
|
||||
`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
|
||||
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
|
||||
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,
|
||||
course_id_prefix = urllib.quote(course_id.replace("/", "_"))
|
||||
|
||||
# Perform the actual upload
|
||||
grades_store = GradesStore.from_config()
|
||||
grades_store.store_rows(
|
||||
report_store = ReportStore.from_config()
|
||||
report_store.store_rows(
|
||||
course_id,
|
||||
u"{}_grade_report_{}.csv".format(course_id_prefix, timestamp_str),
|
||||
rows
|
||||
@@ -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 len(err_rows) > 1:
|
||||
grades_store.store_rows(
|
||||
report_store.store_rows(
|
||||
course_id,
|
||||
u"{}_grade_report_{}_err.csv".format(course_id_prefix, timestamp_str),
|
||||
err_rows
|
||||
|
||||
@@ -31,7 +31,7 @@ class DataDownload
|
||||
@$grades_request_response = @$grades.find '.request-response'
|
||||
@$grades_request_response_error = @$grades.find '.request-response-error'
|
||||
|
||||
@grade_downloads = new GradeDownloads(@$section)
|
||||
@report_downloads = new ReportDownloads(@$section)
|
||||
@instructor_tasks = new (PendingInstructorTasks()) @$section
|
||||
@clear_display()
|
||||
|
||||
@@ -115,12 +115,12 @@ class DataDownload
|
||||
# Clear display of anything that was here before
|
||||
@clear_display()
|
||||
@instructor_tasks.task_poller.start()
|
||||
@grade_downloads.downloads_poller.start()
|
||||
@report_downloads.downloads_poller.start()
|
||||
|
||||
# handler for when the section is closed
|
||||
onExit: ->
|
||||
@instructor_tasks.task_poller.stop()
|
||||
@grade_downloads.downloads_poller.stop()
|
||||
@report_downloads.downloads_poller.stop()
|
||||
|
||||
clear_display: ->
|
||||
# Clear any generated tables, warning messages, etc.
|
||||
@@ -134,35 +134,31 @@ class DataDownload
|
||||
$(".msg-error").css({"display":"none"})
|
||||
|
||||
|
||||
class GradeDownloads
|
||||
### Grade Downloads -- links expire quickly, so we refresh every 5 mins ####
|
||||
class ReportDownloads
|
||||
### Report Downloads -- links expire quickly, so we refresh every 5 mins ####
|
||||
constructor: (@$section) ->
|
||||
|
||||
|
||||
@$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"
|
||||
@$report_downloads_table = @$section.find ".report-downloads-table"
|
||||
|
||||
POLL_INTERVAL = 1000 * 60 * 5 # 5 minutes in ms
|
||||
@downloads_poller = new window.InstructorDashboard.util.IntervalManager(
|
||||
POLL_INTERVAL, => @reload_grade_downloads()
|
||||
POLL_INTERVAL, => @reload_report_downloads()
|
||||
)
|
||||
|
||||
reload_grade_downloads: ->
|
||||
endpoint = @$grade_downloads_table.data 'endpoint'
|
||||
reload_report_downloads: ->
|
||||
endpoint = @$report_downloads_table.data 'endpoint'
|
||||
$.ajax
|
||||
dataType: 'json'
|
||||
url: endpoint
|
||||
success: (data) =>
|
||||
if data.downloads.length
|
||||
@create_grade_downloads_table data.downloads
|
||||
@create_report_downloads_table data.downloads
|
||||
else
|
||||
console.log "No grade CSVs ready for download"
|
||||
error: std_ajax_err => console.error "Error finding grade download CSVs"
|
||||
console.log "No reports ready for download"
|
||||
error: std_ajax_err => console.error "Error finding report downloads"
|
||||
|
||||
create_grade_downloads_table: (grade_downloads_data) ->
|
||||
@$grade_downloads_table.empty()
|
||||
create_report_downloads_table: (report_downloads_data) ->
|
||||
@$report_downloads_table.empty()
|
||||
|
||||
options =
|
||||
enableCellNavigation: true
|
||||
@@ -174,7 +170,7 @@ class GradeDownloads
|
||||
id: 'link'
|
||||
field: 'link'
|
||||
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
|
||||
minWidth: 150
|
||||
cssClass: "file-download-link"
|
||||
@@ -183,8 +179,8 @@ class GradeDownloads
|
||||
]
|
||||
|
||||
$table_placeholder = $ '<div/>', class: 'slickgrid'
|
||||
@$grade_downloads_table.append $table_placeholder
|
||||
grid = new Slick.Grid($table_placeholder, grade_downloads_data, columns, options)
|
||||
@$report_downloads_table.append $table_placeholder
|
||||
grid = new Slick.Grid($table_placeholder, report_downloads_data, columns, options)
|
||||
grid.autosizeColumns()
|
||||
|
||||
|
||||
|
||||
@@ -444,7 +444,7 @@ section.instructor-dashboard-content-2 {
|
||||
}
|
||||
|
||||
.grades-download-container {
|
||||
.grade-downloads-table {
|
||||
.report-downloads-table {
|
||||
.slickgrid {
|
||||
height: 300px;
|
||||
padding: 5px;
|
||||
|
||||
@@ -26,7 +26,7 @@
|
||||
%if settings.FEATURES.get('ENABLE_S3_GRADE_DOWNLOADS'):
|
||||
<div class="grades-download-container action-type-container">
|
||||
<hr>
|
||||
<h2> ${_("Grade Reports")}</h2>
|
||||
<h2> ${_("Reports")}</h2>
|
||||
|
||||
%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>
|
||||
@@ -43,11 +43,15 @@
|
||||
%endif
|
||||
|
||||
<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.
|
||||
<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>
|
||||
%endif
|
||||
|
||||
|
||||
Reference in New Issue
Block a user