From 0054a1fa6d4ef2d35196b645375505c358b0a881 Mon Sep 17 00:00:00 2001 From: Kyle Raftogianis Date: Tue, 19 Jan 2016 18:59:58 -0800 Subject: [PATCH 1/9] Participant -> Enrollment --- manage.py | 8 ++++---- server/controllers/admin.py | 14 +++++++------- server/controllers/api.py | 2 +- server/models.py | 14 ++++++++------ tests/test_group.py | 4 ++-- 5 files changed, 22 insertions(+), 20 deletions(-) diff --git a/manage.py b/manage.py index d333c89ba..1ea44ed54 100755 --- a/manage.py +++ b/manage.py @@ -7,7 +7,7 @@ from flask.ext.script.commands import ShowUrls, Clean from flask.ext.migrate import Migrate, MigrateCommand from server import create_app -from server.models import db, User, Course, Assignment, Participant, \ +from server.models import db, User, Course, Assignment, Enrollment, \ Backup, Submission, Message # default to dev config because no one should use this in @@ -83,14 +83,14 @@ def seed(): make_backup(staff_member, assign2, time, messages) db.session.commit() - staff = Participant(user_id=staff_member.id, course_id=courses[0].id, + staff = Enrollment(user_id=staff_member.id, course_id=courses[0].id, role="staff") db.session.add(staff) - staff_also_student = Participant(user_id=staff_member.id, + staff_also_student = Enrollment(user_id=staff_member.id, course_id=courses[1].id, role="student") db.session.add(staff_also_student) - student_enrollment = [Participant(user_id=student.id, role="student", + student_enrollment = [Enrollment(user_id=student.id, role="student", course_id=courses[0].id) for student in students] db.session.add_all(student_enrollment) diff --git a/server/controllers/admin.py b/server/controllers/admin.py index 7c3a47f21..10657dc5e 100644 --- a/server/controllers/admin.py +++ b/server/controllers/admin.py @@ -5,7 +5,7 @@ import pytz import csv -from server.models import User, Course, Assignment, Participant, db +from server.models import User, Course, Assignment, Enrollment, db from server.constants import STAFF_ROLES, VALID_ROLES, STUDENT_ROLE import server.forms as forms @@ -139,7 +139,7 @@ def enrollment(cid): single_form = forms.EnrollmentForm(prefix="single") if single_form.validate_on_submit(): email, role = single_form.email.data, single_form.role.data - Participant.enroll_from_form(cid, single_form) + Enrollment.enroll_from_form(cid, single_form) flash("Added {email} as {role}".format(email=email, role=role), "success") query = request.args.get('query', '').strip() @@ -150,15 +150,15 @@ def enrollment(cid): find_student = User.query.filter_by(email=query) student = find_student.first() if student: - students = Participant.query.filter_by(course_id=cid, role=STUDENT_ROLE, + students = Enrollment.query.filter_by(course_id=cid, role=STUDENT_ROLE, user_id=student.id).paginate(page=page, per_page=1) else: flash("No student found with email {}".format(query), "warning") if not students: - students = Participant.query.filter_by(course_id=cid, + students = Enrollment.query.filter_by(course_id=cid, role=STUDENT_ROLE).paginate(page=page, per_page=5) - staff = Participant.query.filter(Participant.course_id == cid, - Participant.role.in_(STAFF_ROLES)).all() + staff = Enrollment.query.filter(Enrollment.course_id == cid, + Enrollment.role.in_(STAFF_ROLES)).all() return render_template('staff/course/enrollment.html', enrollments=students, staff=staff, query=query, @@ -174,7 +174,7 @@ def batch_enroll(cid): courses, current_course = get_courses(cid) batch_form = forms.BatchEnrollmentForm() if batch_form.validate_on_submit(): - new, updated = Participant.enroll_from_csv(cid, batch_form) + new, updated = Enrollment.enroll_from_csv(cid, batch_form) msg = "Added {new}, Updated {old} students".format(new=new, old=updated) flash(msg, "success") return redirect(url_for(".enrollment", cid=cid)) diff --git a/server/controllers/api.py b/server/controllers/api.py index 396d1cb40..26819067a 100644 --- a/server/controllers/api.py +++ b/server/controllers/api.py @@ -387,7 +387,7 @@ class Enrollment(PublicResource): Authenticated. Permissions: >= User Used by: Ok Client Auth """ - model = models.Participant + model = models.Enrollment schema = EnrollmentSchema() @marshal_with(schema.get_fields) diff --git a/server/models.py b/server/models.py index 71b61d67c..6ac2f3c73 100644 --- a/server/models.py +++ b/server/models.py @@ -71,6 +71,7 @@ def lookup(email): """Get a User with the given email address, or None.""" return User.query.filter_by(email=email).one_or_none() + class Course(db.Model, TimestampMixin): id = db.Column(db.Integer(), primary_key=True) offering = db.Column(db.String(), unique=True) @@ -84,7 +85,7 @@ def __repr__(self): return '' % self.offering def is_enrolled(self, user): - return Participant.query.filter_by( + return Enrollment.query.filter_by( user=user, course=self ).count() > 0 @@ -151,7 +152,7 @@ def submission_time(self, usr_id): return most_recent.backup.client_time -class Participant(db.Model, TimestampMixin): +class Enrollment(db.Model, TimestampMixin): id = db.Column(db.Integer(), primary_key=True) user_id = db.Column(db.ForeignKey("user.id"), index=True, nullable=False) course_id = db.Column(db.ForeignKey("course.id"), index=True, @@ -182,7 +183,7 @@ def enroll_from_form(cid, form): db.session.add(usr) db.session.commit() role = form.role.data - Participant.create(cid, [usr.id], role) + Enrollment.create(cid, [usr.id], role) @staticmethod @transaction @@ -206,7 +207,7 @@ def enroll_from_csv(cid, form): db.session.add_all(new_users) db.session.commit() user_ids = [u.id for u in new_users] + existing_uids - Participant.create(cid, user_ids, STUDENT_ROLE) + Enrollment.create(cid, user_ids, STUDENT_ROLE) return len(new_users), len(existing_uids) @@ -215,15 +216,16 @@ def enroll_from_csv(cid, form): def create(cid, usr_ids=[], role=STUDENT_ROLE): new_records = [] for usr_id in usr_ids: - record = Participant.query.filter_by(user_id=usr_id, + record = Enrollment.query.filter_by(user_id=usr_id, course_id=cid).one_or_none() if record: record.role = role else: - record = Participant(course_id=cid, user_id=usr_id, role=role) + record = Enrollment(course_id=cid, user_id=usr_id, role=role) new_records.append(record) db.session.add_all(new_records) + class Message(db.Model, TimestampMixin): id = db.Column(db.Integer(), primary_key=True) backup = db.Column(db.ForeignKey("backup.id"), index=True) diff --git a/tests/test_group.py b/tests/test_group.py index 31532bc3f..e05cd9923 100644 --- a/tests/test_group.py +++ b/tests/test_group.py @@ -1,7 +1,7 @@ import datetime from werkzeug.exceptions import BadRequest -from server.models import db, Assignment, Course, Group, Participant, User +from server.models import db, Assignment, Course, Group, Enrollment, User from .helpers import OkTestCase @@ -21,7 +21,7 @@ def setUp(self): def make_student(n): user = User(email='student{0}@aol.com'.format(n)) - participant = Participant( + participant = Enrollment( user=user, course=self.course) db.session.add(participant) From f30577019dfb6e405a6087ebb122bd6433201fe4 Mon Sep 17 00:00:00 2001 From: Kyle Raftogianis Date: Wed, 20 Jan 2016 13:28:16 -0800 Subject: [PATCH 2/9] Api tests --- tests/test_api.py | 68 +++++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 68 insertions(+) create mode 100644 tests/test_api.py diff --git a/tests/test_api.py b/tests/test_api.py new file mode 100644 index 000000000..0dd39dec5 --- /dev/null +++ b/tests/test_api.py @@ -0,0 +1,68 @@ +import datetime +import json +from server.models import db, Assignment, Course, User + +from .helpers import OkTestCase + +class TestAuth(OkTestCase): + def test_submit(self): + email = 'student@okpy.org' + self.login(email) + user = User.lookup(email) + + course = Course(offering='cal/cs61a/sp16') + assignment = Assignment( + name='cal/cs61a/sp16/proj1', + course=course, + display_name='Hog', + due_date=datetime.datetime.now(), + lock_date=datetime.datetime.now() + datetime.timedelta(days=1), + max_group_size=4) + db.session.add(assignment) + db.session.commit() + + data = { + 'assignment': assignment.name, + 'messages': { + 'file_contents': { + 'hog.py': 'print "Hello world!"' + } + } + } + + response = self.client.post('/api/v3/backups/', data=json.dumps(data), + headers=[('Content-Type', 'application/json')]) + backup = assignment.backups(user).first() + assert backup is not None + + self.assert_200(response) + assert response.json['data'] == { + 'email': email, + 'key': backup.id, + 'course': course.id, + 'assignment': assignment.id + } + + assert backup.assignment == assignment + assert backup.submitter_id == user.id + assert len(backup.messages) == len(data['messages']) + assert not backup.submit + + data['submit'] = True + response = self.client.post('/api/v3/backups/', data=json.dumps(data), + headers=[('Content-Type', 'application/json')]) + submission = assignment.submissions(user).first() + assert submission is not None + + self.assert_200(response) + assert response.json['data'] == { + 'email': email, + 'key': submission.id, + 'course': course.id, + 'assignment': assignment.id + } + + assert submission.assignment == assignment + assert submission.submitter_id == user.id + assert len(submission.messages) == len(data['messages']) + assert submission.submit From fa445ab47e0db535703fd3b637561c91a6e450d8 Mon Sep 17 00:00:00 2001 From: Kyle Raftogianis Date: Wed, 20 Jan 2016 13:28:42 -0800 Subject: [PATCH 3/9] Unify backups and submissions --- manage.py | 14 +--- server/controllers/api.py | 71 +++---------------- server/controllers/student.py | 2 +- server/models.py | 43 ++++------- .../student/assignment/_tablehelper.html | 16 ++--- 5 files changed, 37 insertions(+), 109 deletions(-) diff --git a/manage.py b/manage.py index 1ea44ed54..780e42ef3 100755 --- a/manage.py +++ b/manage.py @@ -8,7 +8,7 @@ from flask.ext.migrate import Migrate, MigrateCommand from server import create_app from server.models import db, User, Course, Assignment, Enrollment, \ - Backup, Submission, Message + Backup, Message # default to dev config because no one should use this in # production anyway. @@ -34,22 +34,14 @@ def make_shell_context(): def make_backup(user, assign, time, messages, submit=True): backup = Backup(client_time=time, - submitter=user.id, - assignment=assign.id, submit=submit) + submitter=user, + assignment=assign, submit=submit) messages = [Message(kind=k, backup=backup, contents=m) for k, m in messages.items()] backup.messages = messages db.session.add_all(messages) db.session.add(backup) - if submit: - subm = Submission(submitter=user.id, - assignment=assign.id, backup=backup) - db.session.add(subm) - - - - @manager.command def seed(): """ Create default records for development. diff --git a/server/controllers/api.py b/server/controllers/api.py index 26819067a..a7425a422 100644 --- a/server/controllers/api.py +++ b/server/controllers/api.py @@ -166,15 +166,14 @@ def get(self): # TODO Permsisions for API actions -def make_backup(user, assignment, messages, submit): +def make_backup(user, assignment_id, messages, submit): """ Create backup with message objects. :param user: (object) caller - :param assignment: (int) Assignment ID + :param assignment_id: (int) Assignment ID :param messages: Data content of backup/submission :param submit: Whether this backup is a submission to be graded - :param submitter: (object) caller or submitter :return: (Backup) backup """ @@ -187,8 +186,8 @@ def make_backup(user, assignment, messages, submit): else: client_time = datetime.datetime.now() - backup = models.Backup(client_time=client_time, submitter=user.id, - assignment=assignment, submit=submit) + backup = models.Backup(client_time=client_time, submitter=user, + assignment_id=assignment_id, submit=submit) messages = [models.Message(kind=k, backup=backup, contents=m) for k, m in messages.items()] backup.messages = messages @@ -239,7 +238,7 @@ class BackupSchema(APISchema): } post_fields = { - 'email': fields.Integer, + 'email': fields.String, 'key': fields.String, 'course': fields.String, 'assign': fields.String, @@ -251,11 +250,8 @@ def __init__(self): help='Name of Assignment') self.parser.add_argument('messages', type=dict, required=True, help='Backup Contents as JSON') - - # Optional - probably not needed now that there are two endpoints - self.parser.add_argument('submit', type=bool, + self.parser.add_argument('submit', type=bool, default=False, help='Flagged as a submission') - self.parser.add_argument('submitter', help='Name of Assignment') def store_backup(self, user): args = self.parse_args() @@ -266,18 +262,6 @@ def store_backup(self, user): return backup -class SubmissionSchema(BackupSchema): - - get_fields = { - 'id': fields.Integer, - 'assignment': fields.Integer, - 'submitter': fields.Integer, - 'backup': fields.Nested(BackupSchema.get_fields), - 'flagged': fields.Boolean, - 'revision': fields.Boolean, - 'created': fields.DateTime(dt_format='rfc822') - } - class CourseSchema(APISchema): get_fields = { 'id': fields.Integer, @@ -301,6 +285,7 @@ class EnrollmentSchema(APISchema): } +# TODO: should be two classes, one for /backups/ and one for /backups// class Backup(Resource): """ Submission retreival resource. Authenticated. Permissions: >= User/Staff @@ -326,43 +311,8 @@ def post(self, user, key=None): return { 'email': current_user.email, 'key': backup.id, - 'course': backup.assign.course_id, - 'assign': backup.assignment - } - - - -class Submission(Resource): - """ Submission resource. - Authenticated. Permissions: >= Student/Staff - Used by: Ok Client Submission. - """ - model = models.Submission - schema = SubmissionSchema() - - @marshal_with(schema.get_fields) - def get(self, user, key=None): - if key is None: - restful.abort(405) - submission = self.model.query.filter_by(id=key).first() - # TODO CHECK .user obj - if submission.user != user or not user.is_admin: - restful.abort(403) - return submission - - def post(self, user, key=None): - if key is not None: - restful.abort(405) - backup = self.schema.store_backup(user) - subm = models.Submission(backup_id=backup.id, assignment=backup.assignment, - submitter=user.id) - models.db.session.add(subm) - models.db.session.commit() - return { - 'email': current_user.email, - 'key': backup.id, - 'course': backup.assign.course_id, - 'assign': backup.assignment + 'course': backup.assignment.course_id, + 'assignment': backup.assignment_id } class Score(Resource): @@ -400,7 +350,6 @@ def get(self, email): api.add_resource(v3Info, '/v3') -api.add_resource(Submission, '/v3/submission', '/v3/submission/') -api.add_resource(Backup, '/v3/backup', '/v3/backup/') +api.add_resource(Backup, '/v3/backups/', '/v3/backups//') api.add_resource(Score, '/v3/score') api.add_resource(Enrollment, '/v3/enrollment/') diff --git a/server/controllers/student.py b/server/controllers/student.py index a63d7ffff..53bbe1e04 100644 --- a/server/controllers/student.py +++ b/server/controllers/student.py @@ -99,7 +99,7 @@ def code(cid, aid, bid): group = Group.lookup(current_user, assgn) backup = Backup.query.get(bid) if backup and backup.can_view(current_user, group, assgn): - submitter = User.query.get(backup.submitter) + submitter = User.query.get(backup.submitter_id) file_contents = [m for m in backup.messages if m.kind == "file_contents"] if file_contents: diff --git a/server/models.py b/server/models.py index 6ac2f3c73..1c6d852c1 100644 --- a/server/models.py +++ b/server/models.py @@ -132,24 +132,25 @@ def backups(self, usr_id): assignment. """ return Backup.query.filter( - Backup.submitter == usr_id, - Backup.assignment == self.id + Backup.submitter_id == usr_id, + Backup.assignment == self ).order_by(Backup.client_time.desc()) def submissions(self, usr_id): """Returns a query for the submission that the current user has for this assignment. """ - return Submission.query.filter( - Submission.submitter == usr_id, - Submission.assignment == self.id - ).order_by(Submission.created.desc()) # TODO: client time + return Backup.query.filter( + Backup.submitter_id == usr_id, + Backup.assignment == self, + Backup.submit == True + ).order_by(Backup.client_time.desc()) def submission_time(self, usr_id): """Returns the time of the most recent submission, or None.""" most_recent = self.submissions(usr_id).first() if most_recent: - return most_recent.backup.client_time + return most_recent.client_time class Enrollment(db.Model, TimestampMixin): @@ -244,11 +245,14 @@ class Backup(db.Model, TimestampMixin): assign = db.relationship("Assignment") client_time = db.Column(db.DateTime()) - submitter = db.Column(db.ForeignKey("user.id"), nullable=False) - assignment = db.Column(db.ForeignKey("assignment.id"), nullable=False) + submitter_id = db.Column(db.ForeignKey("user.id"), nullable=False) + assignment_id = db.Column(db.ForeignKey("assignment.id"), nullable=False) submit = db.Column(db.Boolean(), default=False) flagged = db.Column(db.Boolean(), default=False) + submitter = db.relationship("User") + assignment = db.relationship("Assignment") + db.Index('idx_usrBackups', 'assignment', 'submitter', 'submit', 'flagged') db.Index('idx_usrFlagged', 'assignment', 'submitter', 'flagged') db.Index('idx_submittedBacks', 'assignment', 'submit') @@ -260,7 +264,7 @@ def as_dict(self): def can_view(self, user, group=None, assign=None): if user.is_admin: return True - if user.id == self.submitter: + if user.id == self.submitter_id: return True # Allow group members to view @@ -269,7 +273,7 @@ def can_view(self, user, group=None, assign=None): if group: member_ids = group.users() - if self.submitter in member_ids: + if self.submitter_id in member_ids: return True if not assign or assign.id != self.assignment_id: @@ -293,23 +297,6 @@ def statistics(self): ORDER BY date_trunc('hour', backup.created)""")).all() -class Submission(db.Model, TimestampMixin): - """ A submission is created from --submit or when a backup is flagged for - grading. - - **This model may be removed. Do not depend on it for features.** - """ - id = db.Column(db.Integer(), primary_key=True) - backup_id = db.Column(db.ForeignKey("backup.id"), nullable=False) - assignment = db.Column(db.ForeignKey("assignment.id"), nullable=False) - submitter = db.Column(db.ForeignKey("user.id"), nullable=False) - flagged = db.Column(db.Boolean(), default=False) - backup = db.relationship("Backup") - user = db.relationship("User") - - db.Index('idx_flaggedSubms', 'assignment', 'submitter', 'flagged'), - - class Score(db.Model, TimestampMixin): id = db.Column(db.Integer(), primary_key=True) backup = db.Column(db.ForeignKey("backup.id"), nullable=False) diff --git a/server/templates/student/assignment/_tablehelper.html b/server/templates/student/assignment/_tablehelper.html index 46eb88cc8..610be0f37 100644 --- a/server/templates/student/assignment/_tablehelper.html +++ b/server/templates/student/assignment/_tablehelper.html @@ -36,7 +36,7 @@

{{ tname }}

{% endif %} - + @@ -68,7 +68,7 @@

{{ tname }}

- + View Code @@ -102,7 +102,7 @@

{{ tname }}

{% for backup in backups %} - + @@ -112,7 +112,7 @@

{{ tname }}

- + View Code @@ -138,7 +138,7 @@

{{tname}}

{% for subm in subms %}
-
+
Submitter: {{ subm.user.email }}
@@ -156,7 +156,7 @@

{{tname}}


- + View Code @@ -182,10 +182,10 @@

{{tname}}

{% for backup in backups %}
-
+
Submitter: {{ backup.user.email }}
- + View Code From 3729e510648f89713a9daeb8e4804f29625919b5 Mon Sep 17 00:00:00 2001 From: Kyle Raftogianis Date: Wed, 20 Jan 2016 13:59:07 -0800 Subject: [PATCH 4/9] Pass tests --- tests/test_api.py | 4 ++-- tests/test_auth.py | 6 +++--- 2 files changed, 5 insertions(+), 5 deletions(-) diff --git a/tests/test_api.py b/tests/test_api.py index 0dd39dec5..19b9842c5 100644 --- a/tests/test_api.py +++ b/tests/test_api.py @@ -32,7 +32,7 @@ def test_submit(self): response = self.client.post('/api/v3/backups/', data=json.dumps(data), headers=[('Content-Type', 'application/json')]) - backup = assignment.backups(user).first() + backup = assignment.backups(user.id).first() assert backup is not None self.assert_200(response) @@ -51,7 +51,7 @@ def test_submit(self): data['submit'] = True response = self.client.post('/api/v3/backups/', data=json.dumps(data), headers=[('Content-Type', 'application/json')]) - submission = assignment.submissions(user).first() + submission = assignment.submissions(user.id).first() assert submission is not None self.assert_200(response) diff --git a/tests/test_auth.py b/tests/test_auth.py index f11573efb..ac43dcef8 100644 --- a/tests/test_auth.py +++ b/tests/test_auth.py @@ -22,11 +22,11 @@ def test_testing_login_fail(self): self.assert_404(response) def test_restricted(self): - """User should see /restricted if logged in, but not if logged out.""" + """User should see /student/ if logged in, but not if logged out.""" self.login(self.email) - response = self.client.get('/restricted') + response = self.client.get('/student/') self.assert_200(response) self.client.get('/logout') - response = self.client.get('/restricted') + response = self.client.get('/student/') self.assert_redirects(response, '/login') From 831f080177495527b579f38e1a6d2957b7b9c280 Mon Sep 17 00:00:00 2001 From: Kyle Raftogianis Date: Wed, 20 Jan 2016 14:01:17 -0800 Subject: [PATCH 5/9] Add trailing slash to auth routes --- server/controllers/auth.py | 10 +++++----- tests/helpers.py | 2 +- tests/test_auth.py | 16 ++++++++-------- 3 files changed, 14 insertions(+), 14 deletions(-) diff --git a/server/controllers/auth.py b/server/controllers/auth.py index e0d1a2218..d44b18fcc 100644 --- a/server/controllers/auth.py +++ b/server/controllers/auth.py @@ -92,14 +92,14 @@ def use_testing_login(): return current_app.config.get('TESTING_LOGIN', False) and \ current_app.config.get('ENV') != 'prod' -@auth.route("/login") +@auth.route("/login/") def login(): """ Authenticates a user with an access token using Google APIs. """ return google_auth.authorize(callback=url_for('.authorized', _external=True)) -@auth.route('/login/authorized') +@auth.route('/login/authorized/') @google_auth.authorized_handler def authorized(resp): if resp is None or 'access_token' not in resp: @@ -118,20 +118,20 @@ def authorized(resp): # Backdoor log in if you want to impersonate a user. # Will not give you a Google auth token. # Requires that TESTING_LOGIN = True in the config and we must not be running in prod. -@auth.route('/testing-login') +@auth.route('/testing-login/') def testing_login(): if not use_testing_login(): abort(404) return render_template('testing-login.html', callback=url_for(".testing_authorized")) -@auth.route('/testing-login/authorized', methods=['POST']) +@auth.route('/testing-login/authorized/', methods=['POST']) def testing_authorized(): if not use_testing_login(): abort(404) user = user_from_email(request.form['email']) return authorize_user(user) -@auth.route("/logout") +@auth.route("/logout/") def logout(): logout_user() session.pop('google_token', None) diff --git a/tests/helpers.py b/tests/helpers.py index 702bce86c..9f905c6ce 100644 --- a/tests/helpers.py +++ b/tests/helpers.py @@ -16,7 +16,7 @@ def tearDown(self): def login(self, email): """ Log in as an email address """ - response = self.client.post('/testing-login/authorized', data={ + response = self.client.post('/testing-login/authorized/', data={ 'email': email }, follow_redirects=True) self.assert_200(response) diff --git a/tests/test_auth.py b/tests/test_auth.py index ac43dcef8..4da3bd2c6 100644 --- a/tests/test_auth.py +++ b/tests/test_auth.py @@ -4,21 +4,21 @@ class TestAuth(OkTestCase): email = 'martymcfly@aol.com' def test_login(self): - """GET /login should redirect to Google OAuth.""" - response = self.client.get('/login') + """GET /login/ should redirect to Google OAuth.""" + response = self.client.get('/login/') assert response.location.startswith('https://accounts.google.com/o/oauth2/auth') def test_testing_login(self): - """GET /testing-login should show a test login page.""" - response = self.client.get('/testing-login') + """GET /testing-login/ should show a test login page.""" + response = self.client.get('/testing-login/') self.assert_200(response) self.assert_template_used('testing-login.html') def test_testing_login_fail(self): - """GET /testing-login should 404 if TESTING_LOGIN config is not set.""" + """GET /testing-login/ should 404 if TESTING_LOGIN config is not set.""" app = self.create_app() app.config['TESTING_LOGIN'] = False - response = app.test_client().get('/testing-login') + response = app.test_client().get('/testing-login/') self.assert_404(response) def test_restricted(self): @@ -27,6 +27,6 @@ def test_restricted(self): response = self.client.get('/student/') self.assert_200(response) - self.client.get('/logout') + self.client.get('/logout/') response = self.client.get('/student/') - self.assert_redirects(response, '/login') + self.assert_redirects(response, '/login/') From b80004b81631a5213394df97916ff8656c750d0f Mon Sep 17 00:00:00 2001 From: Kyle Raftogianis Date: Wed, 20 Jan 2016 16:37:50 -0800 Subject: [PATCH 6/9] Consider group submissions --- server/controllers/student.py | 27 ++++++++++------ server/models.py | 60 +++++++++++++++++++++++++++-------- 2 files changed, 64 insertions(+), 23 deletions(-) diff --git a/server/controllers/student.py b/server/controllers/student.py index 53bbe1e04..4440c41e1 100644 --- a/server/controllers/student.py +++ b/server/controllers/student.py @@ -56,13 +56,19 @@ def wrapper(*args, **kwargs): @is_enrolled def course(cid): course = Course.query.get(cid) - # TODO : Should consider group submissions as well. user_id = current_user.id + def assignment_info(assignment): + group = Group.lookup(current_user, assignment) + if group: + final_submission = assignment.final_submission(group.id) + else: + final_submission = assignment.final_submission(user_id) + submission_time = final_submission and final_submission.client_time + return assignment, submission_time, group + assignments = { - 'active': [(a, a.submission_time(user_id), a.group(user_id)) \ - for a in course.assignments if a.active], - 'inactive': [(a, a.submission_time(user_id), a.group(user_id)) \ - for a in course.assignments if not a.active] + 'active': [assignment_info(a) for a in course.assignments if a.active], + 'inactive': [assignment_info(a) for a in course.assignments if not a.active] } return render_template('student/course/index.html', course=course, **assignments) @@ -76,11 +82,12 @@ def assignment(cid, aid): course = assgn.course group = Group.lookup(current_user, assgn) if group: - # usr_ids = [u.id for u in group.users()] - # TODO : Fetch backups from group. - pass - backups = assgn.backups(current_user.id).limit(5).all() - subms = assgn.submissions(current_user.id).limit(5).all() + backups = assgn.group_backups(group.id).limit(5).all() + subms = assgn.group_submissions(group.id).limit(5).all() + else: + backups = assgn.backups(current_user.id).limit(5).all() + subms = assgn.submissions(current_user.id).limit(5).all() + # TODO: this is confusing if the flag is more than 5 submissions back flagged = any([s.flagged for s in subms]) print(flagged) return render_template('student/assignment/index.html', course=course, diff --git a/server/models.py b/server/models.py index 1c6d852c1..60366463d 100644 --- a/server/models.py +++ b/server/models.py @@ -118,40 +118,74 @@ class Assignment(db.Model, TimestampMixin): def active(self): return dt.utcnow() < self.lock_date # TODO : Ensure all times are UTC - def group(self, usr_id): + def group(self, user_id): # TODO merge with group.lookup member = GroupMember.query.filter_by( - user_id=usr_id, + user_id=user_id, assignment_id=self.id ).one_or_none() if member: return member.group - def backups(self, usr_id): + def backups(self, user_id): """Returns a query for the backups that the list of usrs has for this assignment. """ return Backup.query.filter( - Backup.submitter_id == usr_id, - Backup.assignment == self + Backup.submitter_id == user_id, + Backup.assignment_id == self.id ).order_by(Backup.client_time.desc()) - def submissions(self, usr_id): + def submissions(self, user_id): """Returns a query for the submission that the current user has for this assignment. """ return Backup.query.filter( - Backup.submitter_id == usr_id, - Backup.assignment == self, + Backup.submitter_id == user_id, + Backup.assignment_id == self.id, Backup.submit == True ).order_by(Backup.client_time.desc()) - def submission_time(self, usr_id): - """Returns the time of the most recent submission, or None.""" - most_recent = self.submissions(usr_id).first() - if most_recent: - return most_recent.client_time + def final_submission(self, user_id): + """Return a final submission for a user, or None.""" + return Backup.query.filter( + Backup.submitter_id == user_id, + Backup.assignment_id == self.id, + Backup.submit == True + ).order_by(Backup.flagged.desc(), Backup.client_time.desc()).first() + + # TODO less copy-paste + def group_backups(self, group_id): + """Returns a query for the backups in a group.""" + return Backup.query.join( + GroupMember, + GroupMember.user_id == Backup.submitter_id + ).filter( + GroupMember.group_id == group_id, + GroupMember.assignment_id == self.id, + ).order_by(Backup.client_time.desc()) + def group_submissions(self, group_id): + """Returns a query for the submissions in a group.""" + return Backup.query.join( + GroupMember, + GroupMember.user_id == Backup.submitter_id + ).filter( + GroupMember.group_id == group_id, + GroupMember.assignment_id == self.id, + Backup.submit == True + ).order_by(Backup.client_time.desc()) + + def group_final_submission(self, group_id): + """Return a final submission for a group, or None.""" + return Backup.query.join( + GroupMember, + GroupMember.user_id == Backup.submitter_id + ).filter( + GroupMember.group_id == group_id, + GroupMember.assignment_id == self.id, + Backup.submit == True + ).order_by(Backup.flagged.desc(), Backup.client_time.desc()).first() class Enrollment(db.Model, TimestampMixin): id = db.Column(db.Integer(), primary_key=True) From fcc5be71dd4709c206dd3db1a526eaad313e4e87 Mon Sep 17 00:00:00 2001 From: Kyle Raftogianis Date: Wed, 20 Jan 2016 16:44:23 -0800 Subject: [PATCH 7/9] Use final submission to determine if any are flagged --- server/controllers/student.py | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-) diff --git a/server/controllers/student.py b/server/controllers/student.py index 4440c41e1..71f449861 100644 --- a/server/controllers/student.py +++ b/server/controllers/student.py @@ -81,15 +81,16 @@ def assignment(cid, aid): if assgn: course = assgn.course group = Group.lookup(current_user, assgn) + # TODO this is repetitive if group: backups = assgn.group_backups(group.id).limit(5).all() subms = assgn.group_submissions(group.id).limit(5).all() + final_submission = assgn.group_final_submission(group.id) else: backups = assgn.backups(current_user.id).limit(5).all() subms = assgn.submissions(current_user.id).limit(5).all() - # TODO: this is confusing if the flag is more than 5 submissions back - flagged = any([s.flagged for s in subms]) - print(flagged) + final_submission = assgn.final_submission(current_user.id) + flagged = final_submission and final_submission.flagged return render_template('student/assignment/index.html', course=course, assignment=assgn, backups=backups, subms=subms, flagged=flagged) else: From 2a71d6636795f94b909109a4dda9515fc0ae117a Mon Sep 17 00:00:00 2001 From: Kyle Raftogianis Date: Wed, 20 Jan 2016 20:00:45 -0800 Subject: [PATCH 8/9] Sort submissions by created, not client_time --- server/models.py | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/server/models.py b/server/models.py index 60366463d..57ea59e6c 100644 --- a/server/models.py +++ b/server/models.py @@ -144,7 +144,7 @@ def submissions(self, user_id): Backup.submitter_id == user_id, Backup.assignment_id == self.id, Backup.submit == True - ).order_by(Backup.client_time.desc()) + ).order_by(Backup.created.desc()) def final_submission(self, user_id): """Return a final submission for a user, or None.""" @@ -152,7 +152,7 @@ def final_submission(self, user_id): Backup.submitter_id == user_id, Backup.assignment_id == self.id, Backup.submit == True - ).order_by(Backup.flagged.desc(), Backup.client_time.desc()).first() + ).order_by(Backup.flagged.desc(), Backup.created.desc()).first() # TODO less copy-paste def group_backups(self, group_id): @@ -174,7 +174,7 @@ def group_submissions(self, group_id): GroupMember.group_id == group_id, GroupMember.assignment_id == self.id, Backup.submit == True - ).order_by(Backup.client_time.desc()) + ).order_by(Backup.created.desc()) def group_final_submission(self, group_id): """Return a final submission for a group, or None.""" @@ -185,7 +185,7 @@ def group_final_submission(self, group_id): GroupMember.group_id == group_id, GroupMember.assignment_id == self.id, Backup.submit == True - ).order_by(Backup.flagged.desc(), Backup.client_time.desc()).first() + ).order_by(Backup.flagged.desc(), Backup.created.desc()).first() class Enrollment(db.Model, TimestampMixin): id = db.Column(db.Integer(), primary_key=True) From cd558232f6147f2d20c51c7287db088d88824fbb Mon Sep 17 00:00:00 2001 From: Kyle Raftogianis Date: Sat, 23 Jan 2016 19:57:05 -0800 Subject: [PATCH 9/9] Pass active user ids when querying backups --- server/controllers/student.py | 30 +++--- server/models.py | 97 ++++++------------- .../student/course/_assigntable.html | 4 +- tests/helpers.py | 37 ++++++- tests/test_api.py | 32 ++---- tests/test_group.py | 28 +----- tests/test_submission.py | 24 +++++ 7 files changed, 114 insertions(+), 138 deletions(-) create mode 100644 tests/test_submission.py diff --git a/server/controllers/student.py b/server/controllers/student.py index 71f449861..d2d85a373 100644 --- a/server/controllers/student.py +++ b/server/controllers/student.py @@ -56,15 +56,13 @@ def wrapper(*args, **kwargs): @is_enrolled def course(cid): course = Course.query.get(cid) - user_id = current_user.id def assignment_info(assignment): - group = Group.lookup(current_user, assignment) - if group: - final_submission = assignment.final_submission(group.id) - else: - final_submission = assignment.final_submission(user_id) + # TODO does this make O(n) db queries? + # TODO need group info too + user_ids = assignment.active_user_ids(current_user.id) + final_submission = assignment.final_submission(user_ids) submission_time = final_submission and final_submission.client_time - return assignment, submission_time, group + return assignment, submission_time assignments = { 'active': [assignment_info(a) for a in course.assignments if a.active], @@ -80,16 +78,10 @@ def assignment(cid, aid): assgn = Assignment.query.filter_by(id=aid, course_id=cid).one_or_none() if assgn: course = assgn.course - group = Group.lookup(current_user, assgn) - # TODO this is repetitive - if group: - backups = assgn.group_backups(group.id).limit(5).all() - subms = assgn.group_submissions(group.id).limit(5).all() - final_submission = assgn.group_final_submission(group.id) - else: - backups = assgn.backups(current_user.id).limit(5).all() - subms = assgn.submissions(current_user.id).limit(5).all() - final_submission = assgn.final_submission(current_user.id) + user_ids = assgn.active_user_ids(current_user.id) + backups = assgn.backups(user_ids).limit(5).all() + subms = assgn.submissions(user_ids).limit(5).all() + final_submission = assgn.final_submission(user_ids) flagged = final_submission and final_submission.flagged return render_template('student/assignment/index.html', course=course, assignment=assgn, backups=backups, subms=subms, flagged=flagged) @@ -104,9 +96,9 @@ def code(cid, aid, bid): assgn = Assignment.query.filter_by(id=aid, course_id=cid).one_or_none() if assgn: course = assgn.course - group = Group.lookup(current_user, assgn) + user_ids = assgn.active_user_ids(current_user.id) backup = Backup.query.get(bid) - if backup and backup.can_view(current_user, group, assgn): + if backup and backup.can_view(current_user, user_ids, course): submitter = User.query.get(backup.submitter_id) file_contents = [m for m in backup.messages if m.kind == "file_contents"] diff --git a/server/models.py b/server/models.py index 57ea59e6c..396218c12 100644 --- a/server/models.py +++ b/server/models.py @@ -2,7 +2,7 @@ from sqlalchemy import PrimaryKeyConstraint, MetaData from sqlalchemy.dialects import postgresql as pg from sqlalchemy.ext.hybrid import hybrid_property -from sqlalchemy.orm import backref +from sqlalchemy.orm import aliased, backref from werkzeug.exceptions import BadRequest from flask.ext.login import UserMixin, AnonymousUserMixin @@ -118,75 +118,52 @@ class Assignment(db.Model, TimestampMixin): def active(self): return dt.utcnow() < self.lock_date # TODO : Ensure all times are UTC - def group(self, user_id): - # TODO merge with group.lookup - member = GroupMember.query.filter_by( - user_id=user_id, - assignment_id=self.id - ).one_or_none() - if member: - return member.group + def active_user_ids(self, user_id): + """Return a set of the ids of all users that are active in the same group + that our user is active in. If the user is not in a group, return just + that user's id (i.e. as if they were in a 1-person group). + """ + user_member = aliased(GroupMember) + members = GroupMember.query.join( + user_member, GroupMember.group_id == user_member.group_id + ).filter( + user_member.user_id == user_id, + user_member.assignment_id == self.id, + user_member.status == 'active', + GroupMember.status == 'active' + ).all() + if not members: + return {user_id} + else: + return {member.user_id for member in members} - def backups(self, user_id): - """Returns a query for the backups that the list of usrs has for this + def backups(self, user_ids): + """Return a query for the backups that the list of usrs has for this assignment. """ return Backup.query.filter( - Backup.submitter_id == user_id, + Backup.submitter_id.in_(user_ids), Backup.assignment_id == self.id ).order_by(Backup.client_time.desc()) - def submissions(self, user_id): - """Returns a query for the submission that the current user has for this + def submissions(self, user_ids): + """Return a query for the submission that the current user has for this assignment. """ return Backup.query.filter( - Backup.submitter_id == user_id, + Backup.submitter_id.in_(user_ids), Backup.assignment_id == self.id, Backup.submit == True ).order_by(Backup.created.desc()) - def final_submission(self, user_id): + def final_submission(self, user_ids): """Return a final submission for a user, or None.""" return Backup.query.filter( - Backup.submitter_id == user_id, + Backup.submitter_id.in_(user_ids), Backup.assignment_id == self.id, Backup.submit == True ).order_by(Backup.flagged.desc(), Backup.created.desc()).first() - # TODO less copy-paste - def group_backups(self, group_id): - """Returns a query for the backups in a group.""" - return Backup.query.join( - GroupMember, - GroupMember.user_id == Backup.submitter_id - ).filter( - GroupMember.group_id == group_id, - GroupMember.assignment_id == self.id, - ).order_by(Backup.client_time.desc()) - - def group_submissions(self, group_id): - """Returns a query for the submissions in a group.""" - return Backup.query.join( - GroupMember, - GroupMember.user_id == Backup.submitter_id - ).filter( - GroupMember.group_id == group_id, - GroupMember.assignment_id == self.id, - Backup.submit == True - ).order_by(Backup.created.desc()) - - def group_final_submission(self, group_id): - """Return a final submission for a group, or None.""" - return Backup.query.join( - GroupMember, - GroupMember.user_id == Backup.submitter_id - ).filter( - GroupMember.group_id == group_id, - GroupMember.assignment_id == self.id, - Backup.submit == True - ).order_by(Backup.flagged.desc(), Backup.created.desc()).first() - class Enrollment(db.Model, TimestampMixin): id = db.Column(db.Integer(), primary_key=True) user_id = db.Column(db.ForeignKey("user.id"), index=True, nullable=False) @@ -295,33 +272,19 @@ class Backup(db.Model, TimestampMixin): def as_dict(self): return {c.name: getattr(self, c.name) for c in self.__table__.columns} - def can_view(self, user, group=None, assign=None): + def can_view(self, user, member_ids, course): if user.is_admin: return True if user.id == self.submitter_id: return True # Allow group members to view - if not group or group.assignment_id != self.assignment_id: - group = Group.lookup(user, self.assignment_id) - - if group: - member_ids = group.users() - if self.submitter_id in member_ids: - return True - - if not assign or assign.id != self.assignment_id: - assign = Assignment.get(self.assignment_id) - if not assign: - return False + if self.submitter_id in member_ids: + return True # Allow staff members to view - course = assign.course return user.is_enrolled(course.id, STAFF_ROLES) - - - @staticmethod def statistics(self): db.session.query(Backup).from_statement( diff --git a/server/templates/student/course/_assigntable.html b/server/templates/student/course/_assigntable.html index 670bed118..04c662912 100644 --- a/server/templates/student/course/_assigntable.html +++ b/server/templates/student/course/_assigntable.html @@ -10,7 +10,7 @@

{{ tname }}

Due Date - {% for assgn, subm_time, group in assignments %} + {% for assgn, subm_time in assignments %} @@ -95,7 +95,7 @@

No {{ tname }}