From 893d4ade35f9ced13450ec21e775e937fadf3b66 Mon Sep 17 00:00:00 2001 From: Clarence Ho Date: Thu, 7 Dec 2017 17:24:16 -0800 Subject: [PATCH] Allow comparison of instructor answers - Add new column in db to mark if the answer is comparable - Allow students to compare instructor answers - Modify Participation Report for Research Teams to include instructors/TAs. Dispaly rank and score. Handle multiple answers from same user. - Hide user info of answers from Comparison API calls - Hide answer scores from Comparison API calls - Add new API endpoint /api/courses/:course_id/users/instructionals to retrieve instructors/TAs of the course --- ...llow_instructor_answers_to_be_included_.py | 53 ++++++++ compair/api/answer.py | 31 ++++- compair/api/assignment.py | 8 +- compair/api/classlist.py | 56 +++++++- compair/api/comparison.py | 4 +- compair/api/dataformat/__init__.py | 26 ++-- compair/api/report.py | 94 ++++++++++--- compair/models/answer.py | 1 + compair/models/assignment.py | 22 +++ compair/models/comparison.py | 19 ++- .../modules/answer/answer-form-partial.html | 15 +- .../static/modules/answer/answer-module.js | 26 +++- .../modules/assignment/assignment-module.js | 20 ++- .../assignment/assignment-module_spec.js | 8 ++ .../assignment/assignment-view-partial.html | 19 ++- .../modules/comparison/comparison-module.js | 11 ++ .../comparison/comparison-view-partial.html | 7 +- .../static/modules/course/course-module.js | 3 +- compair/tests/api/test_answers.py | 61 +++++++-- compair/tests/api/test_assignment.py | 56 +++++--- compair/tests/api/test_classlist.py | 71 ++++++++++ compair/tests/api/test_comparisons.py | 70 ++++++++-- compair/tests/api/test_report.py | 128 +++++++++++------- data/fixtures/test_data.py | 19 ++- 24 files changed, 668 insertions(+), 160 deletions(-) create mode 100644 alembic/versions/2561c39ac4d9_allow_instructor_answers_to_be_included_.py diff --git a/alembic/versions/2561c39ac4d9_allow_instructor_answers_to_be_included_.py b/alembic/versions/2561c39ac4d9_allow_instructor_answers_to_be_included_.py new file mode 100644 index 000000000..1536f834c --- /dev/null +++ b/alembic/versions/2561c39ac4d9_allow_instructor_answers_to_be_included_.py @@ -0,0 +1,53 @@ +"""Allow instructor answers to be included in comparisons + +Revision ID: 2561c39ac4d9 +Revises: f6145781f130 +Create Date: 2017-12-06 21:07:23.219244 + +""" + +# revision identifiers, used by Alembic. +revision = '2561c39ac4d9' +down_revision = 'f6145781f130' + +from alembic import op +import sqlalchemy as sa +from sqlalchemy.sql import text + +from compair.models import convention + +def upgrade(): + # add a new "comparable" column. default as true + with op.batch_alter_table('answer', naming_convention=convention) as batch_op: + batch_op.add_column(sa.Column('comparable', sa.Integer(), nullable=False, default='1', server_default='1')) + + # Patch existing answers from instructors and TAs as non-comparable. + # Note that existing answers from sys admin are considered comparable (i.e. no need to patch). + # sqlite doesn't support in-clause with multiple columns... + # update = text( + # "UPDATE answer SET comparable = 0 " + # "WHERE (assignment_id, user_id) IN ( " + # " SELECT a.id, uc.user_id " + # " FROM user_course uc " + # " JOIN assignment a " + # " ON a.course_id = uc.course_id " + # " WHERE uc.course_role IN ('Instructor', 'Teaching Assistant'))" + # ) + # ... use a potentially slower query + update = text( + "UPDATE answer SET comparable = 0 " + "WHERE EXISTS ( " + " SELECT 1 " + " FROM user_course " + " JOIN assignment " + " ON assignment.course_id = user_course.course_id " + " WHERE " + " assignment.id = answer.assignment_id " + " AND user_course.user_id = answer.user_id " + " AND user_course.course_role IN ('Instructor', 'Teaching Assistant'))" + ) + op.get_bind().execute(update) + +def downgrade(): + with op.batch_alter_table('answer', naming_convention=convention) as batch_op: + batch_op.drop_column('comparable') diff --git a/compair/api/answer.py b/compair/api/answer.py index a3f80bf75..4b11dd1b4 100644 --- a/compair/api/answer.py +++ b/compair/api/answer.py @@ -21,6 +21,7 @@ new_answer_parser = RequestParser() new_answer_parser.add_argument('user_id', default=None) +new_answer_parser.add_argument('comparable', type=bool, default=True) new_answer_parser.add_argument('content', default=None) new_answer_parser.add_argument('file_id', default=None) new_answer_parser.add_argument('draft', type=bool, default=False) @@ -28,6 +29,7 @@ existing_answer_parser = RequestParser() existing_answer_parser.add_argument('id', required=True, help="Answer id is required.") existing_answer_parser.add_argument('user_id', default=None) +existing_answer_parser.add_argument('comparable', type=bool, default=True) existing_answer_parser.add_argument('content', default=None) existing_answer_parser.add_argument('file_id', default=None) existing_answer_parser.add_argument('draft', type=bool, default=False) @@ -72,7 +74,7 @@ def get(self, course_uuid, assignment_uuid): """ Return a list of answers for a assignment based on search criteria. The list of the answers are paginated. If there is any answers from instructor - or TA, their answers will be on top of the list. + or TA, their answers will be on top of the list (unless they are comparable). :param course_uuid: course uuid :param assignment_uuid: assignment uuid @@ -103,8 +105,14 @@ def get(self, course_uuid, assignment_uuid): UserCourse.course_id == course.id )) \ .add_columns( - UserCourse.course_role.__eq__(CourseRole.instructor).label("instructor_role"), - UserCourse.course_role.__eq__(CourseRole.teaching_assistant).label("ta_role") + and_( + UserCourse.course_role.__eq__(CourseRole.instructor), + not Answer.comparable + ).label("instructor_role"), + and_( + UserCourse.course_role.__eq__(CourseRole.teaching_assistant), + not Answer.comparable + ).label("ta_role") ) \ .filter(and_( Answer.assignment_id == assignment.id, @@ -194,12 +202,20 @@ def post(self, course_uuid, assignment_uuid): if user_uuid and not allow(MANAGE, Answer(course_id=course.id)): abort(400, title="Answer Not Submitted", message="Only instructors and teaching assistants can submit an answer on behalf of another.") + explicitSubmitAsStudent = False if user_uuid: user = User.get_by_uuid_or_404(user_uuid) answer.user_id = user.id + explicitSubmitAsStudent = user.get_course_role(course.id) == CourseRole.student else: answer.user_id = current_user.id + # instructor / TA can mark the answer as non-comparable, unless the answer is for a student + if allow(MANAGE, Answer(course_id=course.id)) and not explicitSubmitAsStudent: + answer.comparable = params.get("comparable") + else: + answer.comparable = True + user_course = UserCourse.query \ .filter_by( course_id=course.id, @@ -302,6 +318,15 @@ def post(self, course_uuid, assignment_uuid, answer_uuid): answer.content = params.get("content") user_uuid = params.get("user_id") + explicitSubmitAsStudent = False + if user_uuid: + user = User.get_by_uuid_or_404(user_uuid) + explicitSubmitAsStudent = user.get_course_role(course.id) == CourseRole.student + + # instructor / TA can mark the answer as non-comparable if it is not a student answer + if allow(MANAGE, Answer(course_id=course.id)) and not explicitSubmitAsStudent: + answer.comparable = params.get("comparable") + # we allow instructor and TA to submit multiple answers for other users in the class if user_uuid and user_uuid != answer.user_uuid: if not allow(MANAGE, answer) or not answer.draft: diff --git a/compair/api/assignment.py b/compair/api/assignment.py index 95ef5d26b..e2db9fc89 100644 --- a/compair/api/assignment.py +++ b/compair/api/assignment.py @@ -484,8 +484,8 @@ def get(self, course_uuid, assignment_uuid): comparison_count = assignment.completed_comparison_count_for_user(current_user.id) comparison_draft_count = assignment.draft_comparison_count_for_user(current_user.id) - other_student_answers = assignment.student_answer_count - answer_count - comparison_available = comparison_count < other_student_answers * (other_student_answers - 1) / 2 + other_comparable_answers = assignment.comparable_answer_count - answer_count + comparison_available = comparison_count < other_comparable_answers * (other_comparable_answers - 1) / 2 status = { 'answers': { @@ -659,8 +659,8 @@ def get(self, course_uuid): assignment_drafts = [draft for draft in drafts if draft.assignment_id == assignment.id] comparison_count = assignment.completed_comparison_count_for_user(current_user.id) comparison_draft_count = assignment.draft_comparison_count_for_user(current_user.id) - other_student_answers = assignment.student_answer_count - answer_count - comparison_available = comparison_count < other_student_answers * (other_student_answers - 1) / 2 + other_comparable_answers = assignment.comparable_answer_count - answer_count + comparison_available = comparison_count < other_comparable_answers * (other_comparable_answers - 1) / 2 statuses[assignment.uuid] = { 'answers': { diff --git a/compair/api/classlist.py b/compair/api/classlist.py index 877812127..006e64c7e 100644 --- a/compair/api/classlist.py +++ b/compair/api/classlist.py @@ -9,7 +9,7 @@ from flask_login import login_required, current_user from flask_restful import Resource, marshal from six import BytesIO -from sqlalchemy import and_ +from sqlalchemy import and_, or_ from sqlalchemy.orm import joinedload from werkzeug.utils import secure_filename from flask_restful.reqparse import RequestParser @@ -67,6 +67,7 @@ on_classlist_instructor_label = event.signal('CLASSLIST_INSTRUCTOR_LABEL_GET') on_classlist_instructor = event.signal('CLASSLIST_INSTRUCTOR_GET') on_classlist_student = event.signal('CLASSLIST_STUDENT_GET') +on_classlist_instructional = event.signal('CLASSLIST_INSTRUCTIONAL_GET') on_classlist_update_users_course_roles = event.signal('CLASSLIST_UPDATE_USERS_COURSE_ROLES') @@ -619,6 +620,59 @@ def get(self, course_uuid): api.add_resource(StudentsAPI, '/students') +# /instructionals - return list of Instructors and TAs of the course +class InstructionalsAPI(Resource): + @login_required + def get(self, course_uuid): + course = Course.get_active_by_uuid_or_404(course_uuid) + require(READ, course, + title="Instructional users Unavailable", + message="Instructional users can only be seen here by those enrolled in the course. Please double-check your enrollment in this course.") + restrict_user = not allow(MANAGE, course) + + instructionals = User.query \ + .with_entities(User, UserCourse.group_name, UserCourse.course_role) \ + .join(UserCourse, UserCourse.user_id == User.id) \ + .filter( + UserCourse.course_id == course.id, + or_( + UserCourse.course_role == CourseRole.instructor, + UserCourse.course_role == CourseRole.teaching_assistant + ) + ) \ + .order_by(User.lastname, User.firstname) \ + .all() + + users = [] + user_course = UserCourse(course_id=course.id) + for u in instructionals: + if allow(READ, user_course): + users.append({ + 'id': u.User.uuid, + 'name': u.User.fullname_sortable, + 'group_name': u.group_name, + 'role': u.course_role.value + }) + else: + name = u.User.displayname + users.append({ + 'id': u.User.uuid, + 'name': name, + 'group_name': u.group_name, + 'role': u.course_role.value + }) + + on_classlist_instructional.send( + self, + event_name=on_classlist_instructional.name, + user=current_user, + course_id=course.id + ) + + return { 'objects': users } + + +api.add_resource(InstructionalsAPI, '/instructionals') # /roles - set course role for multi users at once class UserCourseRoleAPI(Resource): diff --git a/compair/api/comparison.py b/compair/api/comparison.py index 3d6640981..6008f6bc8 100644 --- a/compair/api/comparison.py +++ b/compair/api/comparison.py @@ -104,7 +104,7 @@ def get(self, course_uuid, assignment_uuid): abort(500, title="Comparisons Unavailable", message="Generating scored pairs failed, this really shouldn't happen.") return { - 'comparison': marshal(comparison, dataformat.get_comparison(restrict_user)), + 'comparison': marshal(comparison, dataformat.get_comparison(restrict_user, include_answer_user=False, include_score=False)), 'new_pair': new_pair, 'current': comparison_count+1 } @@ -248,6 +248,6 @@ def post(self, course_uuid, assignment_uuid): is_comparison_example=is_comparison_example, data=marshal(comparison, dataformat.get_comparison(restrict_user))) - return {'comparison': marshal(comparison, dataformat.get_comparison(restrict_user))} + return {'comparison': marshal(comparison, dataformat.get_comparison(restrict_user, include_answer_user=False, include_score=False))} api.add_resource(CompareRootAPI, '') diff --git a/compair/api/dataformat/__init__.py b/compair/api/dataformat/__init__.py index b9f0a0621..ca3eb8d67 100644 --- a/compair/api/dataformat/__init__.py +++ b/compair/api/dataformat/__init__.py @@ -178,12 +178,11 @@ def get_assignment(restrict_user=True): 'created': fields.DateTime(dt_format='iso8601', attribute=lambda x: replace_tzinfo(x.created)) } -def get_answer(restrict_user=True): - return { +def get_answer(restrict_user=True, include_answer_user=True, include_score=True): + ret = { 'id': fields.String(attribute="uuid"), 'course_id': fields.String(attribute="course_uuid"), 'assignment_id': fields.String(attribute="assignment_uuid"), - 'user_id': fields.String(attribute="user_uuid"), 'content': fields.String, 'file': fields.Nested(get_file(), allow_null=True), @@ -191,16 +190,23 @@ def get_answer(restrict_user=True): 'draft': fields.Boolean, 'top_answer': fields.Boolean, - 'score': fields.Nested(get_score(restrict_user), allow_null=True), - 'comment_count': fields.Integer, 'private_comment_count': fields.Integer, 'public_comment_count': fields.Integer, - 'user': get_partial_user(restrict_user), - 'created': fields.DateTime(dt_format='iso8601', attribute=lambda x: replace_tzinfo(x.created)) + 'created': fields.DateTime(dt_format='iso8601', attribute=lambda x: replace_tzinfo(x.created)), + 'comparable': fields.Boolean } + if include_score: + ret['score'] = fields.Nested(get_score(restrict_user), allow_null=True) + + if include_answer_user: + ret['user_id'] = fields.String(attribute="user_uuid") + ret['user'] = get_partial_user(restrict_user) + + return ret + def get_assignment_comment(restrict_user=True): return { @@ -253,7 +259,7 @@ def get_kaltura_media(): } -def get_comparison(restrict_user=True, with_answers=True, with_feedback=False): +def get_comparison(restrict_user=True, with_answers=True, with_feedback=False, include_answer_user=True, include_score=True): ret = { 'id': fields.String(attribute="uuid"), 'course_id': fields.String(attribute="course_uuid"), @@ -269,8 +275,8 @@ def get_comparison(restrict_user=True, with_answers=True, with_feedback=False): } if with_answers: - ret['answer1'] = fields.Nested(get_answer(restrict_user)) - ret['answer2'] = fields.Nested(get_answer(restrict_user)) + ret['answer1'] = fields.Nested(get_answer(restrict_user, include_answer_user, include_score)) + ret['answer2'] = fields.Nested(get_answer(restrict_user, include_answer_user, include_score)) if with_feedback: ret['answer1_feedback'] = fields.List(fields.Nested(get_answer_comment(restrict_user))) diff --git a/compair/api/report.py b/compair/api/report.py index 51fda5164..4b14180dc 100644 --- a/compair/api/report.py +++ b/compair/api/report.py @@ -89,6 +89,7 @@ def post(self, course_uuid): title = [ 'Assignment', 'User UUID', 'Last Name', 'First Name', 'Answer Submitted', 'Answer ID', + 'Answer', 'Students Ranked', 'Overall Score', 'Evaluations Submitted', 'Evaluations Required', 'Evaluation Requirements Met', 'Replies Submitted'] titles = [title] @@ -134,6 +135,7 @@ def post(self, course_uuid): ] data = peer_feedback_report(course, assignments, group_name) titles = [titles1, titles2] + else: abort(400, title="Report Not Run", message="Please try again with a report type from the list of report types provided.") @@ -163,29 +165,45 @@ def post(self, course_uuid): def participation_stat_report(course, assignments, group_name, overall): report = [] - user_course_students = UserCourse.query \ - .filter_by( - course_id=course.id, - course_role=CourseRole.student + classlist = UserCourse.query \ + .join(User, User.id == UserCourse.user_id) \ + .filter( + and_( + UserCourse.course_id == course.id, + or_( + UserCourse.course_role == CourseRole.teaching_assistant, + UserCourse.course_role == CourseRole.instructor, + UserCourse.course_role == CourseRole.student + ) + ) ) if group_name: - user_course_students = user_course_students.filter_by(group_name=group_name) - user_course_students = user_course_students.all() + classlist = classlist.filter(group_name == UserCourse.group_name) + classlist = classlist.order_by(User.lastname, User.firstname, User.id).all() + + class_ids = [u.user.id for u in classlist] total_req = 0 total = {} for assignment in assignments: - # ANSWERS: assume max one answer per user + # ANSWERS: instructors / TAs could submit multiple answers. normally 1 answer per student answers = Answer.query \ - .filter_by( - active=True, - assignment_id=assignment.id, - draft=False, - practice=False - ) \ + .options(joinedload('score')) \ + .filter(and_( + Answer.active == True, + Answer.assignment_id == assignment.id, + Answer.comparable == True, + Answer.draft == False, + Answer.practice == False, + Answer.user_id.in_(class_ids) + )) \ + .order_by(Answer.created) \ .all() - answers = {a.user_id: a.uuid for a in answers} + user_answers = {} # structure - user_id/[answer list] + for answer in answers: + user_answers_list = user_answers.setdefault(answer.user_id, []) + user_answers_list.append(answer) # EVALUATIONS evaluations = Comparison.query \ @@ -208,8 +226,8 @@ def participation_stat_report(course, assignments, group_name, overall): total_req += assignment.total_comparisons_required # for overall required - for user_course_student in user_course_students: - user = user_course_student.user + for user_course in classlist: + user = user_course.user temp = [assignment.name, user.uuid, user.lastname, user.firstname] # OVERALL @@ -219,10 +237,15 @@ def participation_stat_report(course, assignments, group_name, overall): 'total_comments': 0 }) - submitted = 1 if user.id in answers else 0 - answer_uuid = answers[user.id] if submitted else 'N/A' + # each user has at least 1 line per assignment, regardless whether there is an answer + submitted = len(user_answers.get(user.id, [])) + the_answer = user_answers[user.id][0] if submitted else None + answer_uuid = the_answer.uuid if submitted else 'N/A' + answer_text = snippet(the_answer.content) if submitted else 'N/A' + answer_rank = the_answer.score.rank if submitted and the_answer.score else 'Not Evaluated' + answer_score = the_answer.score.normalized_score if submitted and the_answer.score else 'Not Evaluated' total[user.id]['total_answers'] += submitted - temp.extend([submitted, answer_uuid]) + temp.extend([submitted, answer_uuid, answer_text, answer_rank, answer_score]) evaluations = evaluation_submitted.get(user.id, 0) evaluation_req_met = 'Yes' if evaluations >= assignment.total_comparisons_required else 'No' @@ -235,8 +258,23 @@ def participation_stat_report(course, assignments, group_name, overall): report.append(temp) + # handle multiple answers from the user (normally only apply for instructors / TAs) + if submitted > 1: + for answer in user_answers[user.id][1:]: + answer_uuid = answer.uuid + answer_text = snippet(answer.content) + answer_rank = answer.score.rank if submitted and answer.score else 'Not Evaluated' + answer_score = answer.score.normalized_score if submitted and answer.score else 'Not Evaluated' + temp = [assignment.name, user.uuid, user.lastname, + user.firstname, submitted, answer_uuid, answer_text, + answer_rank, answer_score, + evaluations, assignment.total_comparisons_required, + evaluation_req_met, comment_count] + + report.append(temp) + if overall: - for user_course_student in user_course_students: + for user_course_student in classlist: user = user_course_student.user sum_submission = total.setdefault(user.id, { 'total_answers': 0, @@ -247,7 +285,8 @@ def participation_stat_report(course, assignments, group_name, overall): req_met = 'Yes' if sum_submission['total_evaluations'] >= total_req else 'No' temp = [ '(Overall in Course)', user.uuid, user.lastname, user.firstname, - sum_submission['total_answers'], '(Overall in Course)', + sum_submission['total_answers'], '(Overall in Course)', '(Overall in Course)', + '', '', sum_submission['total_evaluations'], total_req, req_met, sum_submission['total_comments']] report.append(temp) @@ -447,6 +486,7 @@ def peer_feedback_report(course, assignments, group_name): return report + def strip_html(text): text = re.sub('<[^>]+>', '', text) text = text.replace(' ', ' ') @@ -455,4 +495,14 @@ def strip_html(text): text = text.replace('>', '>') text = text.replace('"', '"') text = text.replace(''', '\'') - return text \ No newline at end of file + return text + +def snippet(content, length=100, suffix='...'): + if content == None: + return "" + content = strip_html(content) + content = content.replace('\n', ' ').replace('\r', '').strip() + if len(content) <= length: + return content + else: + return ' '.join(content[:length+1].split(' ')[:-1]) + suffix \ No newline at end of file diff --git a/compair/models/answer.py b/compair/models/answer.py index 5e844ffc5..4ed04ba69 100644 --- a/compair/models/answer.py +++ b/compair/models/answer.py @@ -27,6 +27,7 @@ class Answer(DefaultTableMixin, UUIDMixin, ActiveMixin, WriteTrackingMixin): nullable=True) draft = db.Column(db.Boolean(name='draft'), default=False, nullable=False, index=True) top_answer = db.Column(db.Boolean(name='top_answer'), default=False, nullable=False, index=True) + comparable = db.Column(db.Boolean(name='comparable'), default=True, nullable=False) # relationships # assignment via Assignment Model diff --git a/compair/models/assignment.py b/compair/models/assignment.py index b2bf16929..4858fe172 100644 --- a/compair/models/assignment.py +++ b/compair/models/assignment.py @@ -278,6 +278,28 @@ def __declare_last__(cls): group="counts" ) + # Comparable answer count + # To be consistent with student_answer_count, we are not counting + # answers from sys admin here + cls.comparable_answer_count = column_property( + select([func.count(Answer.id)]). + select_from(join(Answer, UserCourse, UserCourse.user_id == Answer.user_id)). + where(and_( + Answer.assignment_id == cls.id, + Answer.active == True, + Answer.draft == False, + Answer.practice == False, + UserCourse.course_id == cls.course_id, + UserCourse.course_role.in_( + [CourseRole.student, CourseRole.instructor, \ + CourseRole.teaching_assistant] + ), + Answer.comparable == True + )), + deferred=True, + group="counts" + ) + cls.top_answer_count = column_property( select([func.count(Answer.id)]). select_from(join(Answer, UserCourse, UserCourse.user_id == Answer.user_id)). diff --git a/compair/models/comparison.py b/compair/models/comparison.py index 0e26f8108..4d1336b82 100644 --- a/compair/models/comparison.py +++ b/compair/models/comparison.py @@ -109,14 +109,20 @@ def _get_new_comparison_pair(cls, course_id, assignment_id, user_id, pairing_alg from . import Assignment, UserCourse, CourseRole, Answer, AnswerScore, \ PairingAlgorithm, AnswerCriterionScore, AssignmentCriterion - # ineligible authors - eg. instructors, TAs, dropped student, current user - non_students = UserCourse.query \ + # exclude current user and those not with proper role. + # note that sys admin (not enrolled in the course and thus no course role) can create answers. + # they are considered eligible + ineligibles = UserCourse.query \ + .with_entities(UserCourse.user_id) \ .filter(and_( UserCourse.course_id == course_id, - UserCourse.course_role != CourseRole.student + UserCourse.course_role.notin_( + [ CourseRole.student, CourseRole.instructor, \ + CourseRole.teaching_assistant ] + ) )) - ineligible_user_ids = [non_student.user_id \ - for non_student in non_students] + ineligible_user_ids = [ineligible.user_id \ + for ineligible in ineligibles] ineligible_user_ids.append(user_id) answers_with_score = Answer.query \ @@ -127,7 +133,8 @@ def _get_new_comparison_pair(cls, course_id, assignment_id, user_id, pairing_alg Answer.assignment_id == assignment_id, Answer.active == True, Answer.practice == False, - Answer.draft == False + Answer.draft == False, + Answer.comparable == True )) \ .all() diff --git a/compair/static/modules/answer/answer-form-partial.html b/compair/static/modules/answer/answer-form-partial.html index 07fac7bb9..47ece49f7 100644 --- a/compair/static/modules/answer/answer-form-partial.html +++ b/compair/static/modules/answer/answer-form-partial.html @@ -37,7 +37,7 @@

{{assignment.name}}

-

Answers submitted as an instructor or TA will not be included in comparisons and appear to students only after they answer/compare.

+

Answers submitted as an instructor or TA will be included in comparisons unless this option is unchecked below. All answers will appear to students when the assignment completes.

Allowed file types for attachments include {{ UploadValidator.getAttachmentExtensionsForDisplay() }}, with a maximum file size of {{UploadValidator.getAttachmentUploadLimit() | filesize }}. Any files you upload will be attached when you save or submit this answer but not before.

@@ -45,11 +45,22 @@

{{assignment.name}}

+
+ +
+
diff --git a/compair/static/modules/answer/answer-module.js b/compair/static/modules/answer/answer-module.js index 8d007158a..623f2c9f9 100644 --- a/compair/static/modules/answer/answer-module.js +++ b/compair/static/modules/answer/answer-module.js @@ -102,10 +102,10 @@ module.factory("AnswerResource", ['$resource', '$cacheFactory', function ($resou module.controller( "AnswerWriteController", ["$scope", "$location", "$routeParams", "AnswerResource", "ClassListResource", "$route", - "AssignmentResource", "Toaster", "$timeout", "UploadValidator", + "AssignmentResource", "Toaster", "$timeout", "UploadValidator", "CourseRole", "answerAttachService", "EditorOptions", "xAPI", "xAPIStatementHelper", "resolvedData", function ($scope, $location, $routeParams, AnswerResource, ClassListResource, $route, - AssignmentResource, Toaster, $timeout, UploadValidator, + AssignmentResource, Toaster, $timeout, UploadValidator, CourseRole, answerAttachService, EditorOptions, xAPI, xAPIStatementHelper, resolvedData) { $scope.courseId = $routeParams.courseId; @@ -131,12 +131,16 @@ module.controller( ); }); $scope.showAssignment = true; + // since the "submit as" dropdown (if enabled) is default to current user (or empty if sys admin), + // the default value of submitAsInstructorOrTA is based on canManageAssignment + $scope.submitAsInstructorOrTA = resolvedData.canManageAssignment; if ($scope.method == "create") { $scope.answer = { draft: true, course_id: $scope.courseId, - assignment_id: $scope.assignmentId + assignment_id: $scope.assignmentId, + comparable: true }; if (!resolvedData.answerUnsaved.objects.length) { // if no answers found, create a new draft answer @@ -213,6 +217,20 @@ module.controller( ); }; + $scope.onSubmitAsChanged = function(selectedUserId) { + var instructorOrTA = selectedUserId != null && + $scope.classlist.filter(function (el) { + return el.id == selectedUserId && + (el.course_role == CourseRole.instructor || + el.course_role == CourseRole.teaching_assistant) + }).length > 0; + + $scope.submitAsInstructorOrTA = selectedUserId == null || instructorOrTA; + if (!instructorOrTA) { + $scope.answer.comparable = true; + } + } + $scope.answerSubmit = function () { $scope.submitted = true; var wasDraft = $scope.answer.draft; @@ -303,6 +321,8 @@ module.controller( ); }); + $scope.submitAsInstructorOrTA = $scope.canManageAssignment; + $scope.deleteFile = function(file) { $scope.answer.file = null; $scope.answer.uploadedFile = false; diff --git a/compair/static/modules/assignment/assignment-module.js b/compair/static/modules/assignment/assignment-module.js index 8b35cf52a..ab839c91e 100644 --- a/compair/static/modules/assignment/assignment-module.js +++ b/compair/static/modules/assignment/assignment-module.js @@ -700,8 +700,8 @@ module.filter("excludeInstr", function() { return function(items, instructors) { var filtered = []; angular.forEach(items, function(item) { - // if user id is NOT in the instructors array, keep it - if (!instructors[item.user_id]) { + // exclude instructor answer unless it is comparable + if (!instructors[item.user_id] || item.comparable) { filtered.push(item); } }); @@ -831,8 +831,8 @@ module.controller("AssignmentViewController", $scope.adminFilter = function() { return function (answer) { - // assume if any filter is applied - instructor/TAs answer will not meet requirement - return !$scope.answerFilters.author && !$scope.answerFilters.group + // true for non-comparable instructor/TA answer + return $scope.instructors[answer.user_id] && !answer.comparable; } }; @@ -1170,6 +1170,18 @@ module.controller("AssignmentViewController", $scope.updateAnswerList(); $scope.resetStudents($scope.allStudents); + CourseResource.getInstructionals({'id': $scope.courseId}).$promise.then( + function(ret) { + $scope.allInstructionals = ret.objects; + // Order by role, then name + // Underscore sorting is stable, so first sort by name, then role + $scope.allInstructionals = _($scope.allInstructionals).chain() + .sortBy(function(o) { return o.name; }) + .sortBy(function(o) { return o.role; }) + .value(); + } + ); + var filterWatcher = function(newValue, oldValue) { if (angular.equals(newValue, oldValue)) return; if (oldValue.group != newValue.group) { diff --git a/compair/static/modules/assignment/assignment-module_spec.js b/compair/static/modules/assignment/assignment-module_spec.js index 601a761d5..13747d77e 100644 --- a/compair/static/modules/assignment/assignment-module_spec.js +++ b/compair/static/modules/assignment/assignment-module_spec.js @@ -793,6 +793,14 @@ describe('assignment-module', function () { }); $httpBackend.expectGET('/api/courses/1abcABC123-abcABC123_Z/groups').respond(mockGroups); $httpBackend.expectGET('/api/courses/1abcABC123-abcABC123_Z/assignments/1abcABC123-abcABC123_Z/answers?page=1&perPage=20').respond(mockAnswers); + $httpBackend.expectGET('/api/courses/1abcABC123-abcABC123_Z/users/instructionals').respond({ + "objects": [{ + "group_name": null, + "id": "1", + "name": "One, Instructor", + "role": "Instructor" + }] + }); $httpBackend.flush(); }); diff --git a/compair/static/modules/assignment/assignment-view-partial.html b/compair/static/modules/assignment/assignment-view-partial.html index db12a982d..d0c894bfe 100644 --- a/compair/static/modules/assignment/assignment-view-partial.html +++ b/compair/static/modules/assignment/assignment-view-partial.html @@ -97,7 +97,7 @@

All answers
@@ -171,7 +171,7 @@

All answers
-
+
@@ -180,6 +180,9 @@

All answers said on {{answer.created | amDateFormat: 'MMM D @ h:mm a'}}:
{{instructors[answer.user_id]}} Response + + Score: {{answer.score.normalized_score}}% + @@ -187,6 +190,18 @@

All answers Edit

+
+ + + + Students Ranked: #{{answer.score.rank}} (tied) + + + Students Ranked: N/A + + + +

diff --git a/compair/static/modules/comparison/comparison-module.js b/compair/static/modules/comparison/comparison-module.js index 91f086423..85ab90591 100644 --- a/compair/static/modules/comparison/comparison-module.js +++ b/compair/static/modules/comparison/comparison-module.js @@ -417,6 +417,17 @@ module.controller( } ); + CourseResource.getInstructionals({'id': $scope.courseId}).$promise.then( + function(ret) { + $scope.allInstructionals = ret.objects; + instructionalIds = $scope.getUserIds(ret.objects); + } + ); + + $scope.isInstructional = function(user_id) { + return user_id in instructionalIds; + } + $scope.loadAnswerByAuthor = function(author_id) { if (_.find($scope.answers, {user_id: author_id})) return; AnswerResource.get({'courseId': $scope.courseId, 'assignmentId': $scope.assignmentId, 'author': author_id}, function(response) { diff --git a/compair/static/modules/comparison/comparison-view-partial.html b/compair/static/modules/comparison/comparison-view-partial.html index 03852e03f..95786d33f 100644 --- a/compair/static/modules/comparison/comparison-view-partial.html +++ b/compair/static/modules/comparison/comparison-view-partial.html @@ -17,8 +17,8 @@

All comparisons @@ -50,7 +50,8 @@

- Show student's answer + Show instructor/TA's answer + Show student's answer


diff --git a/compair/static/modules/course/course-module.js b/compair/static/modules/course/course-module.js index e228c3eea..62b80a070 100644 --- a/compair/static/modules/course/course-module.js +++ b/compair/static/modules/course/course-module.js @@ -235,7 +235,8 @@ module.factory('CourseResource', 'createDuplicate': {method: 'POST', url: '/api/courses/:id/duplicate'}, 'getCurrentUserStatus': {url: '/api/courses/:id/assignments/status'}, 'getInstructorsLabels': {url: '/api/courses/:id/users/instructors/labels'}, - 'getStudents': {url: '/api/courses/:id/users/students'} + 'getStudents': {url: '/api/courses/:id/users/students'}, + 'getInstructionals': {url: '/api/courses/:id/users/instructionals'} } ); ret.MODEL = "Course"; // add constant to identify the model diff --git a/compair/tests/api/test_answers.py b/compair/tests/api/test_answers.py index 7959fba0b..5105c2c58 100644 --- a/compair/tests/api/test_answers.py +++ b/compair/tests/api/test_answers.py @@ -197,6 +197,21 @@ def test_get_all_answers(self): self.assertEqual(len(result), 1) self.assertEqual(result[0]['user_id'], self.fixtures.students[0].uuid) + # test data retrieve before answer period ended with non-privileged user + self.fixtures.assignment.answer_end = datetime.datetime.now() + datetime.timedelta(days=2) + db.session.add(self.fixtures.assignment) + db.session.commit() + rv = self.client.get(self.base_url) + self.assert200(rv) + actual_answers = rv.json['objects'] + self.assertEqual(1, len(actual_answers)) + self.assertEqual(1, rv.json['page']) + self.assertEqual(1, rv.json['pages']) + self.assertEqual(20, rv.json['per_page']) + self.assertEqual(1, rv.json['total']) + + # test data retrieve before answer period ended with privileged user + with self.login(self.fixtures.instructor.username): # add instructor answer answer = AnswerFactory( assignment=self.fixtures.assignment, @@ -215,21 +230,6 @@ def test_get_all_answers(self): for dropped_student in self.fixtures.dropped_students: self.assertNotIn(dropped_student.uuid, user_uuids) - # test data retrieve before answer period ended with non-privileged user - self.fixtures.assignment.answer_end = datetime.datetime.now() + datetime.timedelta(days=2) - db.session.add(self.fixtures.assignment) - db.session.commit() - rv = self.client.get(self.base_url) - self.assert200(rv) - actual_answers = rv.json['objects'] - self.assertEqual(1, len(actual_answers)) - self.assertEqual(1, rv.json['page']) - self.assertEqual(1, rv.json['pages']) - self.assertEqual(20, rv.json['per_page']) - self.assertEqual(1, rv.json['total']) - - # test data retrieve before answer period ended with privileged user - with self.login(self.fixtures.instructor.username): rv = self.client.get(self.base_url) self.assert200(rv) actual_answers = rv.json['objects'] @@ -281,6 +281,23 @@ def test_create_answer(self, mocked_update_assignment_grades_run, mocked_update_ data=json.dumps(expected_answer), content_type='application/json') self.assert404(rv) + # student answers can only be comparable + for comparable in [True, False, None]: + self.fixtures.add_students(1) + with self.login(self.fixtures.students[-1].username): + comp_ans = expected_answer.copy() + if comparable is not None: + comp_ans['comparable'] = comparable + rv = self.client.post( + self.base_url, + data=json.dumps(comp_ans), + content_type='application/json') + self.assert200(rv) + # retrieve again and verify + actual_answer = Answer.query.filter_by(uuid=rv.json['id']).one() + self.assertEqual(comp_ans['content'], actual_answer.content) + self.assertTrue(actual_answer.comparable) + # test create successful with self.login(self.fixtures.instructor.username): rv = self.client.post( @@ -292,6 +309,20 @@ def test_create_answer(self, mocked_update_assignment_grades_run, mocked_update_ actual_answer = Answer.query.filter_by(uuid=rv.json['id']).one() self.assertEqual(expected_answer['content'], actual_answer.content) + # test comparable instructor answers + for comparable in [True, False]: + comp_ans = expected_answer.copy() + comp_ans['comparable'] = comparable + rv = self.client.post( + self.base_url, + data=json.dumps(comp_ans), + content_type='application/json') + self.assert200(rv) + # retrieve again and verify + actual_answer = Answer.query.filter_by(uuid=rv.json['id']).one() + self.assertEqual(comp_ans['content'], actual_answer.content) + self.assertEqual(comparable, actual_answer.comparable) + # user should not have grades new_course_grade = CourseGrade.get_user_course_grade( self.fixtures.course, self.fixtures.instructor) new_assignment_grade = AssignmentGrade.get_user_assignment_grade(self.fixtures.assignment, self.fixtures.instructor) diff --git a/compair/tests/api/test_assignment.py b/compair/tests/api/test_assignment.py index 6c6d8b037..411a69ebb 100644 --- a/compair/tests/api/test_assignment.py +++ b/compair/tests/api/test_assignment.py @@ -552,11 +552,14 @@ def _submit_all_possible_comparisons_for_user(self, user_id): # self.login(username) # calculate number of comparisons to do before user has compared all the pairs it can num_eligible_answers = 0 # need to minus one to exclude the logged in user's own answer - for answer in self.data.get_student_answers(): + for answer in self.data.get_comparable_answers(): if answer.assignment_id == self.assignment.id and answer.user_id != user_id: num_eligible_answers += 1 - # n(n-1)/2 possible pairs before all answers have been compared - num_possible_comparisons = int(num_eligible_answers * (num_eligible_answers - 1) / 2) + # n(n-1)/2 possible pairs before all answers have been compared. + # don't compare more than the assignment required (minus example done). + num_possible_comparisons = min( + int(num_eligible_answers * (num_eligible_answers - 1) / 2), \ + self.assignment.total_comparisons_required - submit_count) for i in range(num_possible_comparisons): comparison = Comparison.create_new_comparison(self.assignment.id, user_id, False) comparison.completed = True @@ -661,11 +664,14 @@ def _submit_all_possible_comparisons_for_user(self, user_id): # self.login(username) # calculate number of comparisons to do before user has compared all the pairs it can num_eligible_answers = 0 # need to minus one to exclude the logged in user's own answer - for answer in self.data.get_student_answers(): + for answer in self.data.get_comparable_answers(): if answer.assignment_id == self.assignment.id and answer.user_id != user_id: num_eligible_answers += 1 - # n(n-1)/2 possible pairs before all answers have been compared - num_possible_comparisons = int(num_eligible_answers * (num_eligible_answers - 1) / 2) + # n(n-1)/2 possible pairs before all answers have been compared. + # don't compare more than the assignment required (minus example done). + num_possible_comparisons = min( + int(num_eligible_answers * (num_eligible_answers - 1) / 2), \ + self.assignment.total_comparisons_required - submit_count) for i in range(num_possible_comparisons): comparison = Comparison.create_new_comparison(self.assignment.id, user_id, False) comparison.completed = True @@ -714,7 +720,7 @@ def test_get_all_status(self): self.assertEqual(status['comparisons']['left'], assignment.total_comparisons_required) self.assertFalse(status['comparisons']['has_draft']) self.assertTrue(status['answers']['answered']) - self.assertEqual(status['answers']['count'], 1) + self.assertEqual(status['answers']['count'], 2) # one comparable, one non-comparable self.assertEqual(status['answers']['feedback'], 0) elif assignments[1].id == assignment.id: self.assertTrue(status['comparisons']['available']) @@ -722,7 +728,7 @@ def test_get_all_status(self): self.assertEqual(status['comparisons']['left'], assignment.total_comparisons_required) self.assertFalse(status['comparisons']['has_draft']) self.assertTrue(status['answers']['answered']) - self.assertEqual(status['answers']['count'], 1) + self.assertEqual(status['answers']['count'], 2) # one comparable, one non-comparable self.assertEqual(status['answers']['feedback'], 0) else: self.assertFalse(status['comparisons']['available']) @@ -774,11 +780,14 @@ def test_get_all_status(self): self.assertTrue(assignment.uuid in rv.json['statuses']) status = rv.json['statuses'][assignment.uuid] if assignments[0].id == assignment.id: - self.assertFalse(status['comparisons']['available']) + # we have more available comparison pairs than required by the assignment + self.assertTrue(status['comparisons']['available']) self.assertEqual(status['comparisons']['count'], compare_count_result) self.assertEqual(status['comparisons']['left'], assignment.total_comparisons_required - compare_count_result) self.assertFalse(status['comparisons']['has_draft']) + # student should completed all required comparison unless no more available pairs + self.assertFalse(status['comparisons']['left'] > 0 and status['comparisons']['available']) self.assertTrue(status['answers']['answered']) self.assertEqual(status['answers']['count'], 1) self.assertEqual(status['answers']['feedback'], 0) @@ -814,11 +823,12 @@ def test_get_all_status(self): if assignments[0].id == assignment.id: self.assertFalse(status['comparisons']['self_evaluation_completed']) self.assertFalse(status['comparisons']['self_evaluation_draft']) - self.assertFalse(status['comparisons']['available']) + self.assertTrue(status['comparisons']['available']) self.assertEqual(status['comparisons']['count'], compare_count_result) self.assertEqual(status['comparisons']['left'], assignment.total_comparisons_required - compare_count_result) self.assertFalse(status['comparisons']['has_draft']) + self.assertFalse(status['comparisons']['left'] > 0 and status['comparisons']['available']) self.assertTrue(status['answers']['answered']) self.assertEqual(status['answers']['count'], 1) self.assertEqual(status['answers']['feedback'], 0) @@ -868,11 +878,12 @@ def test_get_all_status(self): if assignments[0].id == assignment.id: self.assertFalse(status['comparisons']['self_evaluation_completed']) self.assertTrue(status['comparisons']['self_evaluation_draft']) - self.assertFalse(status['comparisons']['available']) + self.assertTrue(status['comparisons']['available']) self.assertEqual(status['comparisons']['count'], compare_count_result) self.assertEqual(status['comparisons']['left'], assignment.total_comparisons_required - compare_count_result) self.assertFalse(status['comparisons']['has_draft']) + self.assertFalse(status['comparisons']['left'] > 0 and status['comparisons']['available']) self.assertTrue(status['answers']['answered']) self.assertEqual(status['answers']['count'], 1) self.assertEqual(status['answers']['feedback'], 0) @@ -910,11 +921,12 @@ def test_get_all_status(self): if assignments[0].id == assignment.id: self.assertTrue(status['comparisons']['self_evaluation_completed']) self.assertFalse(status['comparisons']['self_evaluation_draft']) - self.assertFalse(status['comparisons']['available']) + self.assertTrue(status['comparisons']['available']) self.assertEqual(status['comparisons']['count'], compare_count_result) self.assertEqual(status['comparisons']['left'], assignment.total_comparisons_required - compare_count_result) self.assertFalse(status['comparisons']['has_draft']) + self.assertFalse(status['comparisons']['left'] > 0 and status['comparisons']['available']) self.assertTrue(status['answers']['answered']) self.assertEqual(status['answers']['count'], 1) self.assertEqual(status['answers']['feedback'], 1) @@ -962,11 +974,12 @@ def test_get_all_status(self): if assignments[0].id == assignment.id: self.assertTrue(status['comparisons']['self_evaluation_completed']) self.assertFalse(status['comparisons']['self_evaluation_draft']) - self.assertFalse(status['comparisons']['available']) + self.assertTrue(status['comparisons']['available']) self.assertEqual(status['comparisons']['count'], compare_count_result) self.assertEqual(status['comparisons']['left'], assignment.total_comparisons_required - compare_count_result) self.assertFalse(status['comparisons']['has_draft']) + self.assertFalse(status['comparisons']['left'] > 0 and status['comparisons']['available']) self.assertTrue(status['answers']['answered']) self.assertEqual(status['answers']['count'], 1) self.assertEqual(status['answers']['feedback'], 4) @@ -1064,7 +1077,7 @@ def test_get_status(self): self.assertTrue(status['comparisons']['available']) self.assertFalse(status['comparisons']['has_draft']) self.assertTrue(status['answers']['answered']) - self.assertEqual(status['answers']['count'], 1) + self.assertEqual(status['answers']['count'], 2) # one comparable, one not comparable self.assertEqual(status['answers']['feedback'], 0) with self.login(self.data.get_authorized_student().username): @@ -1088,8 +1101,9 @@ def test_get_status(self): self.assertEqual(status['comparisons']['count'], compare_count_result) self.assertEqual(status['comparisons']['left'], self.assignment.total_comparisons_required - compare_count_result) - self.assertFalse(status['comparisons']['available']) + self.assertTrue(status['comparisons']['available']) self.assertFalse(status['comparisons']['has_draft']) + self.assertFalse(status['comparisons']['left'] > 0 and status['comparisons']['available']) self.assertTrue(status['answers']['answered']) self.assertEqual(status['answers']['count'], 1) self.assertEqual(status['answers']['feedback'], 0) @@ -1106,8 +1120,9 @@ def test_get_status(self): self.assertEqual(status['comparisons']['count'], compare_count_result) self.assertEqual(status['comparisons']['left'], self.assignment.total_comparisons_required - compare_count_result) - self.assertFalse(status['comparisons']['available']) + self.assertTrue(status['comparisons']['available']) self.assertFalse(status['comparisons']['has_draft']) + self.assertFalse(status['comparisons']['left'] > 0 and status['comparisons']['available']) self.assertTrue(status['answers']['answered']) self.assertEqual(status['answers']['count'], 1) self.assertEqual(status['answers']['feedback'], 0) @@ -1133,7 +1148,8 @@ def test_get_status(self): self.assertEqual(status['comparisons']['count'], compare_count_result) self.assertEqual(status['comparisons']['left'], self.assignment.total_comparisons_required - compare_count_result) - self.assertFalse(status['comparisons']['available']) + self.assertTrue(status['comparisons']['available']) + self.assertFalse(status['comparisons']['left'] > 0 and status['comparisons']['available']) self.assertFalse(status['comparisons']['has_draft']) self.assertTrue(status['answers']['answered']) self.assertEqual(status['answers']['count'], 1) @@ -1151,8 +1167,9 @@ def test_get_status(self): self.assertEqual(status['comparisons']['count'], compare_count_result) self.assertEqual(status['comparisons']['left'], self.assignment.total_comparisons_required - compare_count_result) - self.assertFalse(status['comparisons']['available']) + self.assertTrue(status['comparisons']['available']) self.assertFalse(status['comparisons']['has_draft']) + self.assertFalse(status['comparisons']['left'] > 0 and status['comparisons']['available']) self.assertTrue(status['answers']['answered']) self.assertEqual(status['answers']['count'], 1) self.assertEqual(status['answers']['feedback'], 1) @@ -1173,8 +1190,9 @@ def test_get_status(self): self.assertEqual(status['comparisons']['count'], compare_count_result) self.assertEqual(status['comparisons']['left'], self.assignment.total_comparisons_required - compare_count_result) - self.assertFalse(status['comparisons']['available']) + self.assertTrue(status['comparisons']['available']) self.assertFalse(status['comparisons']['has_draft']) + self.assertFalse(status['comparisons']['left'] > 0 and status['comparisons']['available']) self.assertTrue(status['answers']['answered']) self.assertEqual(status['answers']['count'], 1) self.assertEqual(status['answers']['feedback'], 4) diff --git a/compair/tests/api/test_classlist.py b/compair/tests/api/test_classlist.py index b120dcedc..c7dfb9202 100644 --- a/compair/tests/api/test_classlist.py +++ b/compair/tests/api/test_classlist.py @@ -242,6 +242,77 @@ def test_get_students_course(self): self.assertEqual(students[0]['id'], expected['id']) self.assertEqual(students[0]['name'], expected['name'] + ' (You)') + def test_get_instructional_course(self): + url = self.url + "/instructionals" + + # test login required + rv = self.client.get(url) + self.assert401(rv) + + # test dropped instructor - unauthorized + with self.login(self.data.get_dropped_instructor().username): + rv = self.client.get(url) + self.assert403(rv) + + # test unauthorized instructor + with self.login(self.data.get_unauthorized_instructor().username): + rv = self.client.get(url) + self.assert403(rv) + + with self.login(self.data.get_authorized_instructor().username): + # test invalid course id + rv = self.client.get('/api/courses/999/users/instructionals') + self.assert404(rv) + + # test success - instructor + rv = self.client.get(url) + self.assert200(rv) + instructionals = rv.json['objects'] + expected = [ + { + 'role': 'Teaching Assistant', + 'id': self.data.get_authorized_ta().uuid, + 'name': self.data.get_authorized_ta().fullname_sortable + }, + { + 'role': 'Instructor', + 'id': self.data.get_authorized_instructor().uuid, + 'name': self.data.get_authorized_instructor().fullname_sortable + } + ] + + self.assertEqual(len(instructionals), len(expected)) + for expect in expected: + actual = next((x for x in instructionals if x['id'] == expect['id']), None) + self.assertIsNotNone(actual) + for key in expect: + self.assertEqual(actual[key], expect[key]) + + # test success - student + with self.login(self.data.get_authorized_student().username): + rv = self.client.get(url) + self.assert200(rv) + instructionals = rv.json['objects'] + expected = [ + { + 'role': 'Teaching Assistant', + 'id': self.data.get_authorized_ta().uuid, + 'name': self.data.get_authorized_ta().displayname + }, + { + 'role': 'Instructor', + 'id': self.data.get_authorized_instructor().uuid, + 'name': self.data.get_authorized_instructor().displayname + } + ] + + self.assertEqual(len(instructionals), len(expected)) + for expect in expected: + actual = next((x for x in instructionals if x['id'] == expect['id']), None) + self.assertIsNotNone(actual) + for key in expect: + self.assertEqual(actual[key], expect[key]) + def test_enrol_instructor(self): url = self._create_enrol_url(self.url, self.data.get_dropped_instructor().uuid) role = {'course_role': 'Instructor'} # defaults to Instructor diff --git a/compair/tests/api/test_comparisons.py b/compair/tests/api/test_comparisons.py index b07f1d85d..666e18f96 100644 --- a/compair/tests/api/test_comparisons.py +++ b/compair/tests/api/test_comparisons.py @@ -230,16 +230,16 @@ def test_get_and_submit_comparison(self, mocked_update_assignment_grades_run, mo users = [self.data.get_authorized_student(), self.data.get_authorized_instructor(), self.data.get_authorized_ta()] for user in users: max_comparisons = self.assignment.number_of_comparisons - other_student_answers = 0 + other_people_answers = 0 valid_answer_uuids = set() - for answer in self.data.get_student_answers(): + for answer in self.data.get_comparable_answers(): if answer.assignment.id == self.assignment.id and answer.user_id != user.id: - other_student_answers += 1 + other_people_answers += 1 valid_answer_uuids.add(answer.uuid) # instructors and tas can compare every possible pair if user.id in [self.data.get_authorized_instructor().id, self.data.get_authorized_ta().id]: - max_comparisons = int(other_student_answers * (other_student_answers - 1) / 2) + max_comparisons = int(other_people_answers * (other_people_answers - 1) / 2) if user.id == self.data.get_authorized_student().id: for comparison_example in self.data.comparisons_examples: @@ -277,6 +277,14 @@ def test_get_and_submit_comparison(self, mocked_update_assignment_grades_run, mo self.assertNotEqual(actual_answer1_uuid, actual_answer2_uuid) self.assertTrue(rv.json['new_pair']) self.assertEqual(rv.json['current'], current) + # no user info should be included in the answer + self.assertFalse('user' in rv.json['comparison']['answer1'] or + 'user_id' in rv.json['comparison']['answer1']) + self.assertFalse('user' in rv.json['comparison']['answer2'] or + 'user_id' in rv.json['comparison']['answer2']) + # no score info should be included in the answer + self.assertFalse('score' in rv.json['comparison']['answer1']) + self.assertFalse('score' in rv.json['comparison']['answer2']) # fetch again rv = self.client.get(self.base_url) @@ -317,7 +325,7 @@ def test_get_and_submit_comparison(self, mocked_update_assignment_grades_run, mo actual_comparison = rv.json['comparison'] self._validate_comparison_submit(comparison_submit, actual_comparison, expected_comparison) - # grades should increase for every comparison + # grades should increase for every comparison, unless student has done more than required by assignment if user.id == self.data.get_authorized_student().id: new_course_grade = CourseGrade.get_user_course_grade(self.course, user) new_assignment_grade = AssignmentGrade.get_user_assignment_grade(self.assignment, user) @@ -391,6 +399,7 @@ def _validate_comparison_submit(self, comparison_submit, actual_comparison, expe def _submit_all_possible_comparisons_for_user(self, user_id): example_winner_ids = [] example_loser_ids = [] + submit_count = 0 for comparison_example in self.data.comparisons_examples: if comparison_example.assignment_id == self.assignment.id: @@ -406,6 +415,7 @@ def _submit_all_possible_comparisons_for_user(self, user_id): comparison.winner = WinningAnswer.answer1 if comparison.answer1_id < comparison.answer2_id else WinningAnswer.answer2 for comparison_criterion in comparison.comparison_criteria: comparison_criterion.winner = comparison.winner + submit_count += 1 db.session.add(comparison) db.session.commit() @@ -413,11 +423,14 @@ def _submit_all_possible_comparisons_for_user(self, user_id): # self.login(username) # calculate number of comparisons to do before user has compared all the pairs it can num_eligible_answers = 0 # need to minus one to exclude the logged in user's own answer - for answer in self.data.get_student_answers(): + for answer in self.data.get_comparable_answers(): if answer.assignment_id == self.assignment.id and answer.user_id != user_id: num_eligible_answers += 1 # n(n-1)/2 possible pairs before all answers have been compared - num_possible_comparisons = int(num_eligible_answers * (num_eligible_answers - 1) / 2) + # don't compare more than the assignment required (minus example done). + num_possible_comparisons = min( + int(num_eligible_answers * (num_eligible_answers - 1) / 2), \ + self.assignment.total_comparisons_required - submit_count) winner_ids = [] loser_ids = [] for i in range(num_possible_comparisons): @@ -495,7 +508,6 @@ def _submit_all_possible_comparisons_for_user(self, user_id): self.assertEqual(min_score.rounds, min_stats[criterion_id]['rounds']+1) max_score = AnswerCriterionScore.query.filter_by(answer_id=max_id, criterion_id=criterion_id).first() - print(max_score) self.assertEqual(max_score.wins, max_stats[criterion_id]['wins']) self.assertEqual(max_score.loses, max_stats[criterion_id]['loses']+1) self.assertIn(max_score.opponents, [max_stats[criterion_id]['opponents'], max_stats[criterion_id]['opponents']+1]) @@ -534,7 +546,7 @@ def test_score_calculation(self, mock_shuffle): num_wins_by_id[winner_id] = num_wins + 1 # Get the actual score calculated for each answer - answers = self.data.get_student_answers() + answers = self.data.get_comparable_answers() answer_scores = {} for answer in answers: if answer.assignment.id == self.assignment.id: @@ -542,14 +554,46 @@ def test_score_calculation(self, mock_shuffle): # Check that ranking by score and by wins match, this only works for low number of # comparisons - sorted_expect_ranking = sorted(num_wins_by_id.items(), key=operator.itemgetter(0)) - sorted_expect_ranking = sorted(sorted_expect_ranking, key=operator.itemgetter(1)) - expected_ranking_by_wins = [answer_id for (answer_id, wins) in sorted_expect_ranking] + id_by_win = {} + for the_id in num_wins_by_id: + id_by_win.setdefault(num_wins_by_id[the_id], []).append(the_id) + # list of lists of id, ordered by # of wins. id with same # of wins grouped in the same sub-list + expected_ranking_by_wins = [i[1] for i in sorted(id_by_win.items(), key=lambda x: x[0])] sorted_actual_ranking = sorted(answer_scores.items(), key=operator.itemgetter(1)) actual_ranking_by_scores = [answer_id for (answer_id, score) in sorted_actual_ranking] - self.assertSequenceEqual(actual_ranking_by_scores, expected_ranking_by_wins) + for ids in expected_ranking_by_wins: + while len(ids) > 0: + actual_id = actual_ranking_by_scores.pop(0) + self.assertIn(actual_id, ids) + ids.remove(actual_id) + + self.assertFalse(len(actual_ranking_by_scores)) + + # test score and rank are in the same order + @mock.patch('random.shuffle') + def test_score_rank_order(self, mock_shuffle): + # Make sure all answers are compared first + comparisons_auth = self._submit_all_possible_comparisons_for_user( + self.data.get_authorized_student().id) + comparisons_secondary = self._submit_all_possible_comparisons_for_user( + self.data.get_secondary_authorized_student().id) + + # Get the actual score calculated and the rank for each answer + answers = self.data.get_comparable_answers() + answer_scores = {} + answer_ranks = {} + for answer in answers: + if answer.assignment.id == self.assignment.id: + answer_scores[answer.id] = answer.score.score + answer_ranks[answer.id] = answer.score.rank + + # Answer id sorted by scores and ranks. Highest score / lowest rank first + sorted_by_scores = sorted(answer_scores, key=answer_scores.get, reverse=True) + sorted_by_ranks = sorted(answer_ranks, key=answer_ranks.get) + + self.assertSequenceEqual(sorted_by_scores, sorted_by_ranks) def test_comparison_count_matched_pairing(self): # Make sure all answers are compared first diff --git a/compair/tests/api/test_report.py b/compair/tests/api/test_report.py index 9a5d4874b..69518e0d4 100644 --- a/compair/tests/api/test_report.py +++ b/compair/tests/api/test_report.py @@ -236,15 +236,17 @@ def test_generate_report(self): overall_stats = {} + course_users = sorted(self.fixtures.students + [self.fixtures.instructor, self.fixtures.ta], + key=lambda u: (u.lastname, u.firstname, u.id)) for assignment in assignments: - for student in self.fixtures.students: + for user in course_users: next_row = next(reader) - user_stats = self._check_participation_stat_report_user_row(assignment, student, next_row, overall_stats) + user_stats = self._check_participation_stat_report_user_row(assignment, user, next_row, overall_stats) # overall - for student in self.fixtures.students: + for user in course_users: next_row = next(reader) - self._check_participation_stat_report_user_overall_row(student, next_row, overall_stats) + self._check_participation_stat_report_user_overall_row(user, next_row, overall_stats) # test authorized user one assignment single_assignment_input = input.copy() @@ -264,9 +266,12 @@ def test_generate_report(self): overall_stats = {} - for student in self.fixtures.students: + course_users = sorted(self.fixtures.students + [self.fixtures.instructor, self.fixtures.ta], + key=lambda u: (u.lastname, u.firstname, u.id)) + + for user in course_users: next_row = next(reader) - user_stats = self._check_participation_stat_report_user_row(self.fixtures.assignments[0], student, next_row, overall_stats) + user_stats = self._check_participation_stat_report_user_row(self.fixtures.assignments[0], user, next_row, overall_stats) # test authorized user entire course with group_name filter group_name_input = input.copy() @@ -287,19 +292,18 @@ def test_generate_report(self): overall_stats = {} + group_members = [u for u in self.fixtures.students if u.user_courses[0].group_name == self.fixtures.groups[0]] + group_members = sorted(group_members, key=lambda m: (m.lastname, m.firstname, m.id)) + for assignment in assignments: - for student in self.fixtures.students: - if student.user_courses[0].group_name != self.fixtures.groups[0]: - continue + for member in group_members: next_row = next(reader) - user_stats = self._check_participation_stat_report_user_row(assignment, student, next_row, overall_stats) + user_stats = self._check_participation_stat_report_user_row(assignment, member, next_row, overall_stats) # overall - for student in self.fixtures.students: - if student.user_courses[0].group_name != self.fixtures.groups[0]: - continue + for member in group_members: next_row = next(reader) - self._check_participation_stat_report_user_overall_row(student, next_row, overall_stats) + self._check_participation_stat_report_user_overall_row(member, next_row, overall_stats) # test authorized user one assignment group_name_input = input.copy() @@ -320,11 +324,12 @@ def test_generate_report(self): overall_stats = {} - for student in self.fixtures.students: - if student.user_courses[0].group_name != self.fixtures.groups[0]: - continue + group_members = [u for u in self.fixtures.students if u.user_courses[0].group_name == self.fixtures.groups[0]] + group_members = sorted(group_members, key=lambda m: (m.lastname, m.firstname, m.id)) + + for member in group_members: next_row = next(reader) - user_stats = self._check_participation_stat_report_user_row(self.fixtures.assignments[0], student, next_row, overall_stats) + user_stats = self._check_participation_stat_report_user_row(self.fixtures.assignments[0], member, next_row, overall_stats) # peer_feedback with valid instructor with self.login(self.fixtures.instructor.username): @@ -499,14 +504,16 @@ def test_generate_report(self): self.files_to_cleanup.append(file_name) def _check_participation_stat_report_heading_rows(self, heading): - expected_heading = ['Assignment', 'User UUID', 'Last Name', 'First Name', - 'Answer Submitted', 'Answer ID', 'Evaluations Submitted', 'Evaluations Required', - 'Evaluation Requirements Met', 'Replies Submitted'] + expected_heading = [ + 'Assignment', 'User UUID', 'Last Name', 'First Name', 'Answer Submitted', 'Answer ID', + 'Answer', 'Students Ranked', 'Overall Score', + 'Evaluations Submitted', 'Evaluations Required', 'Evaluation Requirements Met', + 'Replies Submitted'] self.assertEqual(expected_heading, heading) def _check_participation_stat_report_user_overall_row(self, student, row, overall_stats): - excepted_row = [] + expected_row = [] overall_stats.setdefault(student.id, { 'answers_submitted': 0, @@ -517,22 +524,25 @@ def _check_participation_stat_report_user_overall_row(self, student, row, overal }) user_stats = overall_stats[student.id] - excepted_row.append("(Overall in Course)") - excepted_row.append(student.uuid) - excepted_row.append(student.lastname) - excepted_row.append(student.firstname) - excepted_row.append(str(user_stats["answers_submitted"])) - excepted_row.append("(Overall in Course)") - excepted_row.append(str(user_stats["evaluations_submitted"])) - excepted_row.append(str(user_stats["evaluations_required"])) - excepted_row.append("Yes" if user_stats["evaluation_requirements_met"] else "No") - excepted_row.append(str(user_stats["replies_submitted"])) + expected_row.append("(Overall in Course)") + expected_row.append(student.uuid) + expected_row.append(student.lastname) + expected_row.append(student.firstname) + expected_row.append(str(user_stats["answers_submitted"])) + expected_row.append("(Overall in Course)") + expected_row.append("(Overall in Course)") + expected_row.append("") + expected_row.append("") + expected_row.append(str(user_stats["evaluations_submitted"])) + expected_row.append(str(user_stats["evaluations_required"])) + expected_row.append("Yes" if user_stats["evaluation_requirements_met"] else "No") + expected_row.append(str(user_stats["replies_submitted"])) - self.assertEqual(row, excepted_row) + self.assertEqual(row, expected_row) def _check_participation_stat_report_user_row(self, assignment, student, row, overall_stats): - excepted_row = [] + expected_row = [] overall_stats.setdefault(student.id, { 'answers_submitted': 0, @@ -543,10 +553,10 @@ def _check_participation_stat_report_user_row(self, assignment, student, row, ov }) user_stats = overall_stats[student.id] - excepted_row.append(assignment.name) - excepted_row.append(student.uuid) - excepted_row.append(student.lastname) - excepted_row.append(student.firstname) + expected_row.append(assignment.name) + expected_row.append(student.uuid) + expected_row.append(student.lastname) + expected_row.append(student.firstname) answer = Answer.query \ .filter_by( @@ -560,11 +570,21 @@ def _check_participation_stat_report_user_row(self, assignment, student, row, ov if answer: user_stats["answers_submitted"] += 1 - excepted_row.append("1") - excepted_row.append(answer.uuid) + expected_row.append("1") + expected_row.append(answer.uuid) + expected_row.append(self._snippet(answer.content)) + else: + expected_row.append("0") + expected_row.append("N/A") + expected_row.append("N/A") + + if answer and answer.score: + expected_row.append(str(answer.score.rank)) + expected_row.append(str(round(answer.score.normalized_score, 3))) # round the floating point value for comparison + row[8] = str(round(float(row[8]), 3)) else: - excepted_row.append("0") - excepted_row.append("N/A") + expected_row.append("Not Evaluated") + expected_row.append("Not Evaluated") comparisons = Comparison.query \ .filter( @@ -575,16 +595,16 @@ def _check_participation_stat_report_user_row(self, assignment, student, row, ov evaluations_submitted = len(comparisons) user_stats["evaluations_submitted"] += evaluations_submitted - excepted_row.append(str(evaluations_submitted)) + expected_row.append(str(evaluations_submitted)) user_stats["evaluations_required"] += assignment.total_comparisons_required - excepted_row.append(str(assignment.total_comparisons_required)) + expected_row.append(str(assignment.total_comparisons_required)) if assignment.total_comparisons_required > evaluations_submitted: user_stats["evaluation_requirements_met"] = False - excepted_row.append("No") + expected_row.append("No") else: - excepted_row.append("Yes") + expected_row.append("Yes") answer_comments = AnswerComment.query \ .filter( @@ -596,9 +616,9 @@ def _check_participation_stat_report_user_row(self, assignment, student, row, ov replies_submitted = len(answer_comments) user_stats["replies_submitted"] += replies_submitted - excepted_row.append(str(replies_submitted)) + expected_row.append(str(replies_submitted)) - self.assertEqual(row, excepted_row) + self.assertEqual(row, expected_row) def _check_participation_report_heading_rows(self, assignments, heading1, heading2): @@ -742,4 +762,14 @@ def _strip_html(self, text): text = text.replace('>', '>') text = text.replace('"', '"') text = text.replace(''', '\'') - return text \ No newline at end of file + return text + + def _snippet(self, content, length=100, suffix='...'): + if content == None: + return "" + content = self._strip_html(content) + content = content.replace('\n', ' ').replace('\r', '').strip() + if len(content) <= length: + return content + else: + return ' '.join(content[:length+1].split(' ')[:-1]) + suffix \ No newline at end of file diff --git a/data/fixtures/test_data.py b/data/fixtures/test_data.py index 983b6e117..e4e0fc333 100644 --- a/data/fixtures/test_data.py +++ b/data/fixtures/test_data.py @@ -452,6 +452,7 @@ def __init__(self): self.authorized_student_with_no_answers = self.create_normal_user() self.enrol_student(self.authorized_student_with_no_answers, self.get_course()) self.student_answers = copy.copy(self.answers) + self.comparable_answers = copy.copy(self.answers) self.comparisons_examples = [] for assignment in self.get_assignments(): # make sure we're allowed to compare existing assignments @@ -459,15 +460,28 @@ def __init__(self): answer = self.create_answer(assignment, self.secondary_authorized_student) self.answers.append(answer) self.student_answers.append(answer) + self.comparable_answers.append(answer) self.answers.append(answer) answer = self.create_answer(assignment, self.get_authorized_student()) self.answers.append(answer) self.student_answers.append(answer) - # add a TA and Instructor answer + self.comparable_answers.append(answer) + # add a TA and Instructor answer - not comparable answer = self.create_answer(assignment, self.get_authorized_ta()) + answer.comparable = False self.answers.append(answer) answer = self.create_answer(assignment, self.get_authorized_instructor()) + answer.comparable = False self.answers.append(answer) + # add a TA and Instructor answer - comparable + answer = self.create_answer(assignment, self.get_authorized_ta()) + answer.comparable = True + self.answers.append(answer) + self.comparable_answers.append(answer) + answer = self.create_answer(assignment, self.get_authorized_instructor()) + answer.comparable = True + self.answers.append(answer) + self.comparable_answers.append(answer) # add a comparison example answer1 = self.create_answer(assignment, self.get_authorized_ta()) self.answers.append(answer) @@ -495,6 +509,9 @@ def create_comparison_example(self, assignment, answer1, answer2): def get_student_answers(self): return self.student_answers + def get_comparable_answers(self): + return self.comparable_answers + def get_assignment_in_answer_period(self): return self.answer_period_assignment