From 267f3766fd0d35ae90775efd6eda599548cc0c6a Mon Sep 17 00:00:00 2001 From: "copilot-swe-agent[bot]" <198982749+Copilot@users.noreply.github.com> Date: Sat, 13 Dec 2025 16:39:20 +0000 Subject: [PATCH 1/6] Initial plan From be4fe3ea12b89af7a17ef59e8a99a33c2c5710fb Mon Sep 17 00:00:00 2001 From: "copilot-swe-agent[bot]" <198982749+Copilot@users.noreply.github.com> Date: Sat, 13 Dec 2025 16:50:12 +0000 Subject: [PATCH 2/6] Add logging for Assignment, Course, and Role events Co-authored-by: acbart <897227+acbart@users.noreply.github.com> --- models/assignment.py | 16 +++++++++++++++- models/course.py | 34 ++++++++++++++++++++++++++++------ models/role.py | 13 ++++++++++++- models/user.py | 12 +++++++++++- 4 files changed, 66 insertions(+), 9 deletions(-) diff --git a/models/assignment.py b/models/assignment.py index fc1cd68f..45762ed2 100644 --- a/models/assignment.py +++ b/models/assignment.py @@ -15,7 +15,7 @@ from common.databases import optional_encoded_field, make_copy, get_enum_values from common.maybe import maybe_int from common.text import make_flavored_uuid_generator -from models.enums import AssignmentStatus, AssignmentTypes +from models.enums import AssignmentStatus, AssignmentTypes, AssignmentLogEvent from models.generics.definitions import LatePolicy from models.generics.models import db, ma from models.generics.base import EnhancedBase, Base @@ -223,6 +223,10 @@ def new(owner_id, course_id, type="blockpy", name=None, level=None, url=None) -> type=type, name=level if type == 'maze' else name) db.session.add(assignment) db.session.commit() + # Log the assignment creation + models.AssignmentLog.new(assignment.id, assignment.version, assignment.course_id, + owner_id, AssignmentLogEvent.CREATE, "assignment", "", + "", "") return assignment def move_course(self, new_course_id: int): @@ -234,6 +238,12 @@ def move_course(self, new_course_id: int): @staticmethod def remove(assignment_id: int): """ Delete the assignment with the given ID. """ + # Log the assignment deletion before removing it + assignment = Assignment.query.get(assignment_id) + if assignment: + models.AssignmentLog.new(assignment.id, assignment.version, assignment.course_id, + assignment.owner_id, AssignmentLogEvent.DELETE, "assignment", "", + "", "") # TODO: Clear anyone's Fork that is me # TODO: Clear assignment tag membership # TODO: Clear submission sample @@ -391,6 +401,10 @@ def fork(self, new_owner_id: int, new_course_id: int, new_name=None, new_url=Non # TODO: Copy tags, sample_submissions, submissions, files db.session.add(assignment) db.session.commit() + # Log the fork event for the new assignment + models.AssignmentLog.new(assignment.id, assignment.version, assignment.course_id, + new_owner_id, AssignmentLogEvent.FORK, "forked_id", str(self.id), + "", "") return assignment def is_allowed(self, ip: str) -> bool: diff --git a/models/course.py b/models/course.py index 5c70cc36..3c870a98 100644 --- a/models/course.py +++ b/models/course.py @@ -9,7 +9,7 @@ import models from common.maybe import maybe_int from common.text import make_flavored_uuid_generator -from models.enums import CourseVisibility, CourseKind, CourseService +from models.enums import CourseVisibility, CourseKind, CourseService, CourseLogEvent, RoleLogEvent from models.generics.models import db, ma from models.generics.base import EnhancedBase, Base, VersionedBase from common.dates import datetime_to_string, string_to_datetime @@ -120,6 +120,11 @@ def get_public(): @staticmethod def remove(course_id, remove_linked=False): course_id = maybe_int(course_id) + # Get course info before deleting + course = Course.query.get(course_id) + if course: + # Log the course deletion + models.CourseLog.new(course_id, course.owner_id, CourseLogEvent.DELETE) Course.query.filter_by(id=course_id).delete() if remove_linked: for m in models.AssignmentGroupMembership.by_course(course_id): @@ -291,22 +296,33 @@ def rename(course_id, name=None): def edit(self, name=None, url=None, visibility=None, term=None, settings=None): modified = False - if name is not None: + changes = {} + if name is not None and self.name != name: + changes['name'] = name self.name = name modified = True - if url is not None: + if url is not None and self.url != url: + changes['url'] = url self.url = url modified = True - if visibility is not None: + if visibility is not None and self.visibility != visibility: + changes['visibility'] = visibility self.visibility = visibility modified = True - if term is not None: + if term is not None and self.term != term: + changes['term'] = term self.term = term modified = True - if settings is not None: + if settings is not None and self.settings != settings: + changes['settings'] = settings self.settings = settings modified = True db.session.commit() + # Log each field change + if modified: + for field, value in changes.items(): + models.CourseLog.new(self.id, self.owner_id, CourseLogEvent.EDIT, + field=field, value=str(value)) return modified @staticmethod @@ -324,7 +340,13 @@ def new(name, owner_id, visibility, term, url): db.session.flush() new_role = models.Role(name='instructor', user_id=owner_id, course_id=new_course.id) db.session.add(new_role) + db.session.flush() # Ensure the role gets an ID db.session.commit() + # Log the course creation + models.CourseLog.new(new_course.id, owner_id, CourseLogEvent.CREATE) + # Log the initial instructor role creation + models.RoleLog.new(new_role.id, new_course.id, owner_id, owner_id, + models.RoleLogEvent.GIVEN, 'instructor') return new_course @staticmethod diff --git a/models/role.py b/models/role.py index 708b78bb..74fb0fe2 100644 --- a/models/role.py +++ b/models/role.py @@ -5,7 +5,7 @@ from common.maybe import maybe_int from common.databases import get_enum_values -from models.enums import UserRoles +from models.enums import UserRoles, RoleLogEvent from models.generics.models import db, ma from models.generics.base import Base @@ -46,8 +46,13 @@ def encode_json(self, use_owner=True): def update_role(self, new_role): if new_role in [id for id, name in self.CHOICES]: + old_role = self.name self.name = new_role db.session.commit() + # Log the role change + import models + models.RoleLog.new(self.id, self.course_id, self.user_id, self.user_id, + RoleLogEvent.CHANGED, f"{old_role} -> {new_role}") return new_role return None @@ -56,6 +61,12 @@ def __str__(self): @staticmethod def remove(role_id): + import models + role = Role.query.get(role_id) + if role: + # Log the role removal + models.RoleLog.new(role.id, role.course_id, role.user_id, role.user_id, + RoleLogEvent.REMOVED, role.name) Role.query.filter_by(id=role_id).delete() db.session.commit() diff --git a/models/user.py b/models/user.py index 8d02415d..be2f4f2e 100644 --- a/models/user.py +++ b/models/user.py @@ -12,7 +12,7 @@ import models from common.maybe import maybe_int -from models.enums import RolePermissions, USER_DISPLAY_ROLES, UserRoles +from models.enums import RolePermissions, USER_DISPLAY_ROLES, UserRoles, RoleLogEvent from models.generics.models import db, ma from models.generics.base import Base @@ -239,6 +239,9 @@ def add_role(self, name, course_id): course_id=maybe_int(course_id)) db.session.add(new_role) db.session.commit() + # Log the role creation + models.RoleLog.new(new_role.id, maybe_int(course_id), self.id, self.id, + RoleLogEvent.GIVEN, name) return new_role return None @@ -247,12 +250,19 @@ def update_roles(self, new_roles, course_id): new_role_names = set(new_role_name.lower() for new_role_name in new_roles) for old_role in old_roles: if old_role.name.lower() not in new_role_names: + # Log the role removal + models.RoleLog.new(old_role.id, maybe_int(course_id), self.id, self.id, + RoleLogEvent.REMOVED, old_role.name) models.Role.query.filter(models.Role.id == old_role.id).delete() old_role_names = set(role.name.lower() for role in old_roles) for new_role_name in new_roles: if new_role_name.lower() not in old_role_names: new_role = models.Role(name=new_role_name.lower(), user_id=self.id, course_id=maybe_int(course_id)) db.session.add(new_role) + db.session.flush() # Ensure the role gets an ID + # Log the role creation + models.RoleLog.new(new_role.id, maybe_int(course_id), self.id, self.id, + RoleLogEvent.GIVEN, new_role_name.lower()) db.session.commit() def determine_role(self, assignments, submissions): From 1b52624977891b016fdd6ab44854e9197c3f1863 Mon Sep 17 00:00:00 2001 From: "copilot-swe-agent[bot]" <198982749+Copilot@users.noreply.github.com> Date: Sat, 13 Dec 2025 16:52:46 +0000 Subject: [PATCH 3/6] Add authorizer tracking for role operations Co-authored-by: acbart <897227+acbart@users.noreply.github.com> --- controllers/endpoints/courses.py | 10 +++++----- models/role.py | 14 ++++++++++---- models/user.py | 16 +++++++++++----- 3 files changed, 26 insertions(+), 14 deletions(-) diff --git a/controllers/endpoints/courses.py b/controllers/endpoints/courses.py index 1e42bf39..d02e7f5e 100644 --- a/controllers/endpoints/courses.py +++ b/controllers/endpoints/courses.py @@ -428,13 +428,13 @@ def add_users(course_id): new_role = add_form.role.data # TODO: Update this to reflect new enums if new_role in ('student', 'learner') and not new_user.is_student(course_id): - new_user.add_role('learner', course_id=course_id) + new_user.add_role('learner', course_id=course_id, authorizer_id=g.user.id) newly_added.append(new_user) elif new_role == 'teachingassistant' and not new_user.is_grader(course_id): - new_user.add_role('teachingassistant', course_id=course_id) + new_user.add_role('teachingassistant', course_id=course_id, authorizer_id=g.user.id) newly_added.append(new_user) elif new_role == 'instructor' and not new_user.is_instructor(course_id): - new_user.add_role('instructor', course_id=course_id) + new_user.add_role('instructor', course_id=course_id, authorizer_id=g.user.id) newly_added.append(new_user) # TODO: Add an invite for the course and that user # TODO: Send an email to reset their password @@ -455,7 +455,7 @@ def remove_role(role_id): is_instructor = g.user.is_instructor(course_id) if not is_instructor: return "You're not an instructor in this course!" - Role.remove(int(role_id)) + Role.remove(int(role_id), authorizer_id=g.user.id) return redirect(url_for('courses.manage_users', course_id=course_id)) @@ -475,7 +475,7 @@ def change_role(): is_instructor = g.user.is_instructor(course_id) if not is_instructor: return "You're not an instructor in this course!" - role.update_role(new_role) + role.update_role(new_role, authorizer_id=g.user.id) return ajax_success({"new_role": new_role}) diff --git a/models/role.py b/models/role.py index 74fb0fe2..f8a08b95 100644 --- a/models/role.py +++ b/models/role.py @@ -44,14 +44,17 @@ def encode_json(self, use_owner=True): 'course_id': self.course_id } - def update_role(self, new_role): + def update_role(self, new_role, authorizer_id=None): if new_role in [id for id, name in self.CHOICES]: old_role = self.name self.name = new_role db.session.commit() # Log the role change import models - models.RoleLog.new(self.id, self.course_id, self.user_id, self.user_id, + # If no authorizer specified, assume the user is changing their own role + if authorizer_id is None: + authorizer_id = self.user_id + models.RoleLog.new(self.id, self.course_id, self.user_id, authorizer_id, RoleLogEvent.CHANGED, f"{old_role} -> {new_role}") return new_role return None @@ -60,12 +63,15 @@ def __str__(self): return ''.format(self.user_id, self.name) @staticmethod - def remove(role_id): + def remove(role_id, authorizer_id=None): import models role = Role.query.get(role_id) if role: + # If no authorizer specified, assume the user is removing their own role + if authorizer_id is None: + authorizer_id = role.user_id # Log the role removal - models.RoleLog.new(role.id, role.course_id, role.user_id, role.user_id, + models.RoleLog.new(role.id, role.course_id, role.user_id, authorizer_id, RoleLogEvent.REMOVED, role.name) Role.query.filter_by(id=role_id).delete() db.session.commit() diff --git a/models/user.py b/models/user.py index be2f4f2e..7f033e71 100644 --- a/models/user.py +++ b/models/user.py @@ -233,25 +233,31 @@ def is_test_user(self, course_id=None): ### Adding and updating roles ### - def add_role(self, name, course_id): + def add_role(self, name, course_id, authorizer_id=None): if name in [id for id, _ in USER_DISPLAY_ROLES.items()]: new_role = models.Role(name=name, user_id=self.id, course_id=maybe_int(course_id)) db.session.add(new_role) db.session.commit() # Log the role creation - models.RoleLog.new(new_role.id, maybe_int(course_id), self.id, self.id, + # If no authorizer specified, assume self-authorization + if authorizer_id is None: + authorizer_id = self.id + models.RoleLog.new(new_role.id, maybe_int(course_id), self.id, authorizer_id, RoleLogEvent.GIVEN, name) return new_role return None - def update_roles(self, new_roles, course_id): + def update_roles(self, new_roles, course_id, authorizer_id=None): + # If no authorizer specified, assume self-authorization + if authorizer_id is None: + authorizer_id = self.id old_roles = [role for role in self.roles if role.course_id == maybe_int(course_id)] new_role_names = set(new_role_name.lower() for new_role_name in new_roles) for old_role in old_roles: if old_role.name.lower() not in new_role_names: # Log the role removal - models.RoleLog.new(old_role.id, maybe_int(course_id), self.id, self.id, + models.RoleLog.new(old_role.id, maybe_int(course_id), self.id, authorizer_id, RoleLogEvent.REMOVED, old_role.name) models.Role.query.filter(models.Role.id == old_role.id).delete() old_role_names = set(role.name.lower() for role in old_roles) @@ -261,7 +267,7 @@ def update_roles(self, new_roles, course_id): db.session.add(new_role) db.session.flush() # Ensure the role gets an ID # Log the role creation - models.RoleLog.new(new_role.id, maybe_int(course_id), self.id, self.id, + models.RoleLog.new(new_role.id, maybe_int(course_id), self.id, authorizer_id, RoleLogEvent.GIVEN, new_role_name.lower()) db.session.commit() From 1d1e06bcb64fc5e20d8f66ab74ce8b55fff7ba44 Mon Sep 17 00:00:00 2001 From: "copilot-swe-agent[bot]" <198982749+Copilot@users.noreply.github.com> Date: Sat, 13 Dec 2025 16:56:26 +0000 Subject: [PATCH 4/6] Fix code review issues: improve role deletion handling and use role.name consistently Co-authored-by: acbart <897227+acbart@users.noreply.github.com> --- models/course.py | 2 +- models/role.py | 5 +++-- 2 files changed, 4 insertions(+), 3 deletions(-) diff --git a/models/course.py b/models/course.py index 3c870a98..257248c4 100644 --- a/models/course.py +++ b/models/course.py @@ -346,7 +346,7 @@ def new(name, owner_id, visibility, term, url): models.CourseLog.new(new_course.id, owner_id, CourseLogEvent.CREATE) # Log the initial instructor role creation models.RoleLog.new(new_role.id, new_course.id, owner_id, owner_id, - models.RoleLogEvent.GIVEN, 'instructor') + models.RoleLogEvent.GIVEN, new_role.name) return new_course @staticmethod diff --git a/models/role.py b/models/role.py index f8a08b95..2c7bf3c4 100644 --- a/models/role.py +++ b/models/role.py @@ -73,8 +73,9 @@ def remove(role_id, authorizer_id=None): # Log the role removal models.RoleLog.new(role.id, role.course_id, role.user_id, authorizer_id, RoleLogEvent.REMOVED, role.name) - Role.query.filter_by(id=role_id).delete() - db.session.commit() + # Delete the role + Role.query.filter_by(id=role_id).delete() + db.session.commit() @staticmethod def by_course(course_id): From 5b14e8dcd5757e6c2d640ffb15af26a67f31cfb1 Mon Sep 17 00:00:00 2001 From: "copilot-swe-agent[bot]" <198982749+Copilot@users.noreply.github.com> Date: Sat, 13 Dec 2025 16:59:42 +0000 Subject: [PATCH 5/6] Optimize database operations and fix import patterns Co-authored-by: acbart <897227+acbart@users.noreply.github.com> --- models/course.py | 3 +-- models/role.py | 3 +-- models/user.py | 11 ++++++++--- 3 files changed, 10 insertions(+), 7 deletions(-) diff --git a/models/course.py b/models/course.py index 257248c4..f47184c4 100644 --- a/models/course.py +++ b/models/course.py @@ -337,10 +337,9 @@ def new(name, owner_id, visibility, term, url): return None new_course = Course(name=name, owner_id=owner_id, visibility=visibility, term=term, url=url) db.session.add(new_course) - db.session.flush() + db.session.flush() # Ensure the course gets an ID before creating the role new_role = models.Role(name='instructor', user_id=owner_id, course_id=new_course.id) db.session.add(new_role) - db.session.flush() # Ensure the role gets an ID db.session.commit() # Log the course creation models.CourseLog.new(new_course.id, owner_id, CourseLogEvent.CREATE) diff --git a/models/role.py b/models/role.py index 2c7bf3c4..8fc8acad 100644 --- a/models/role.py +++ b/models/role.py @@ -3,6 +3,7 @@ from sqlalchemy.orm import Mapped, mapped_column from sqlalchemy import Column, String, Integer, ForeignKey, Enum, Index +import models from common.maybe import maybe_int from common.databases import get_enum_values from models.enums import UserRoles, RoleLogEvent @@ -50,7 +51,6 @@ def update_role(self, new_role, authorizer_id=None): self.name = new_role db.session.commit() # Log the role change - import models # If no authorizer specified, assume the user is changing their own role if authorizer_id is None: authorizer_id = self.user_id @@ -64,7 +64,6 @@ def __str__(self): @staticmethod def remove(role_id, authorizer_id=None): - import models role = Role.query.get(role_id) if role: # If no authorizer specified, assume the user is removing their own role diff --git a/models/user.py b/models/user.py index 7f033e71..fe8737d0 100644 --- a/models/user.py +++ b/models/user.py @@ -261,14 +261,19 @@ def update_roles(self, new_roles, course_id, authorizer_id=None): RoleLogEvent.REMOVED, old_role.name) models.Role.query.filter(models.Role.id == old_role.id).delete() old_role_names = set(role.name.lower() for role in old_roles) + new_roles_to_add = [] for new_role_name in new_roles: if new_role_name.lower() not in old_role_names: new_role = models.Role(name=new_role_name.lower(), user_id=self.id, course_id=maybe_int(course_id)) db.session.add(new_role) - db.session.flush() # Ensure the role gets an ID - # Log the role creation + new_roles_to_add.append((new_role, new_role_name.lower())) + # Flush once to get IDs for all new roles + if new_roles_to_add: + db.session.flush() + # Log all new role creations + for new_role, role_name in new_roles_to_add: models.RoleLog.new(new_role.id, maybe_int(course_id), self.id, authorizer_id, - RoleLogEvent.GIVEN, new_role_name.lower()) + RoleLogEvent.GIVEN, role_name) db.session.commit() def determine_role(self, assignments, submissions): From b5302863ee93ce55fd32d9205271e485767930fe Mon Sep 17 00:00:00 2001 From: "copilot-swe-agent[bot]" <198982749+Copilot@users.noreply.github.com> Date: Sat, 13 Dec 2025 17:01:38 +0000 Subject: [PATCH 6/6] Improve code clarity with named parameters and consistent enum usage Co-authored-by: acbart <897227+acbart@users.noreply.github.com> --- models/assignment.py | 6 +++--- models/course.py | 2 +- 2 files changed, 4 insertions(+), 4 deletions(-) diff --git a/models/assignment.py b/models/assignment.py index 45762ed2..1d5d8d6a 100644 --- a/models/assignment.py +++ b/models/assignment.py @@ -226,7 +226,7 @@ def new(owner_id, course_id, type="blockpy", name=None, level=None, url=None) -> # Log the assignment creation models.AssignmentLog.new(assignment.id, assignment.version, assignment.course_id, owner_id, AssignmentLogEvent.CREATE, "assignment", "", - "", "") + client_timestamp="", client_timezone="") return assignment def move_course(self, new_course_id: int): @@ -243,7 +243,7 @@ def remove(assignment_id: int): if assignment: models.AssignmentLog.new(assignment.id, assignment.version, assignment.course_id, assignment.owner_id, AssignmentLogEvent.DELETE, "assignment", "", - "", "") + client_timestamp="", client_timezone="") # TODO: Clear anyone's Fork that is me # TODO: Clear assignment tag membership # TODO: Clear submission sample @@ -404,7 +404,7 @@ def fork(self, new_owner_id: int, new_course_id: int, new_name=None, new_url=Non # Log the fork event for the new assignment models.AssignmentLog.new(assignment.id, assignment.version, assignment.course_id, new_owner_id, AssignmentLogEvent.FORK, "forked_id", str(self.id), - "", "") + client_timestamp="", client_timezone="") return assignment def is_allowed(self, ip: str) -> bool: diff --git a/models/course.py b/models/course.py index f47184c4..2f3f45e5 100644 --- a/models/course.py +++ b/models/course.py @@ -345,7 +345,7 @@ def new(name, owner_id, visibility, term, url): models.CourseLog.new(new_course.id, owner_id, CourseLogEvent.CREATE) # Log the initial instructor role creation models.RoleLog.new(new_role.id, new_course.id, owner_id, owner_id, - models.RoleLogEvent.GIVEN, new_role.name) + RoleLogEvent.GIVEN, new_role.name) return new_course @staticmethod