From cf6d6089c2d964011a2dd4bb98a0e7b8f4c68fe5 Mon Sep 17 00:00:00 2001 From: Silas Santini <70163606+pancakereport@users.noreply.github.com> Date: Fri, 9 Oct 2026 12:22:59 -0700 Subject: [PATCH 1/2] One User per person across all offerings, like seating Replace the Account table and the per-offering user rows with a single `users` table: one row per person, with their roles in each offering as sets of offering keys (seating stores Canvas IDs; keys also cover offerings not linked to Canvas). Inside an offering, is_staff and is_admin answer for that offering, and login syncs the role from Canvas as before. Since a person's sections and attendance now span offerings, every operation that reads or changes them is limited to the current offering: refresh, join/switch, leave all, remove students, reset, exports, history. Add Flask-Migrate. 0001 is the schema as of #14 (a no-op on existing databases); 0002 merges per-offering rows by email, re-points enrollments, attendance and section staff, and drops account, the old user table and the unused course, slot, survey and user_section_junction tables. Deploys with RUN_MIGRATIONS=true run `flask db upgrade` on start, under a Postgres advisory lock; PR previews never migrate the shared staging database. Co-Authored-By: Claude Opus 5.5 --- .env.example | 5 + Dockerfile | 7 +- README.md | 16 +- server/import_sheet.py | 37 ++-- server/login.py | 75 ++++--- server/main.py | 7 +- server/migrations/README | 1 + server/migrations/alembic.ini | 50 +++++ server/migrations/env.py | 113 ++++++++++ server/migrations/script.py.mako | 24 +++ server/migrations/versions/0001_baseline.py | 141 +++++++++++++ .../migrations/versions/0002_single_user.py | 159 ++++++++++++++ server/models.py | 190 ++++++++++++----- server/offerings.py | 12 +- server/requirements.txt | 2 + server/seed.py | 15 +- server/state.py | 111 +++++----- server/test_canvas_login.py | 194 +++++++++++++----- server/test_migrations.py | 103 ++++++++++ 19 files changed, 1019 insertions(+), 243 deletions(-) create mode 100644 server/migrations/README create mode 100644 server/migrations/alembic.ini create mode 100644 server/migrations/env.py create mode 100644 server/migrations/script.py.mako create mode 100644 server/migrations/versions/0001_baseline.py create mode 100644 server/migrations/versions/0002_single_user.py create mode 100644 server/test_migrations.py diff --git a/.env.example b/.env.example index 9336ade..b2ab0b9 100644 --- a/.env.example +++ b/.env.example @@ -14,6 +14,11 @@ DB_NAME= DATABASE_URL= # 3. Neither set, in development: server/app.db (SQLite). +# Apply database migrations when the server starts. Set on staging and production +# only, never on PR previews (they share the staging database). Locally, run +# `flask --app main db upgrade` yourself instead. +RUN_MIGRATIONS= + # Canvas developer key for this app. Locally, use the UC Berkeley sandbox key: # https://ucberkeleysandbox.instructure.com/ CANVAS_SERVER_URL=https://ucberkeleysandbox.instructure.com/ diff --git a/Dockerfile b/Dockerfile index 86f8855..3901735 100644 --- a/Dockerfile +++ b/Dockerfile @@ -62,7 +62,8 @@ EXPOSE 8080 HEALTHCHECK --interval=30s --timeout=5s --start-period=20s --retries=3 \ CMD python -c 'import os, urllib.request; urllib.request.urlopen("http://127.0.0.1:" + os.environ.get("PORT", "8080") + "/health")' +# With RUN_MIGRATIONS=true (set on staging and production, never on PR previews, +# which share the staging database), apply database migrations before starting. # `exec` keeps gunicorn as PID 1 so it gets SIGTERM directly on redeploy. -# One worker process with threads, as in seating: Cloud Run's default instance -# is 1 vCPU, and a single worker also keeps db.create_all() at startup from racing. -CMD ["sh", "-c", "exec gunicorn -b 0.0.0.0:${PORT:-8080} main:app --workers ${WEB_CONCURRENCY:-1} --threads ${GUNICORN_THREADS:-8} --timeout ${GUNICORN_TIMEOUT:-120} --access-logfile -"] +# One worker process with threads, as in seating: Cloud Run's default instance is 1 vCPU. +CMD ["sh", "-c", "if [ \"$RUN_MIGRATIONS\" = true ]; then flask --app main db upgrade || exit 1; fi; exec gunicorn -b 0.0.0.0:${PORT:-8080} main:app --workers ${WEB_CONCURRENCY:-1} --threads ${GUNICORN_THREADS:-8} --timeout ${GUNICORN_TIMEOUT:-120} --access-logfile -"] diff --git a/README.md b/README.md index 894c2f8..243ed08 100644 --- a/README.md +++ b/README.md @@ -13,8 +13,9 @@ Requires Python 3.14 and Node 24 (`mise install` picks both up from `mise.toml`) pip install -r server/requirements.txt ``` 3. Install the frontend dependencies with `yarn install`. -4. Run both with `yarn dev`, or separately: the backend with `python3 main.py` from `server/` (port 8000) and the frontend with `yarn start` (the Vite dev server on port 3000, which forwards API calls, the login routes and `/offerings` to port 8000). -5. Run the backend tests from the repository root with `python -m unittest discover -s server -p "test_*.py"`. +4. Create or update the local database with `flask --app main db upgrade` from `server/`. +5. Run both with `yarn dev`, or separately: the backend with `python3 main.py` from `server/` (port 8000) and the frontend with `yarn start` (the Vite dev server on port 3000, which forwards API calls, the login routes and `/offerings` to port 8000). +6. Run the backend tests from the repository root with `python -m unittest discover -s server -p "test_*.py"`. ## Authentication @@ -50,6 +51,17 @@ python3 import_locally.py --course 157 --type enrollment --file test_csvs/lab_en 4. Open `localhost:3000/offerings/157/`; you should see the example sections. +## Database Migrations + +The schema is managed with Flask-Migrate (Alembic); migrations live in `server/migrations/versions`. To change the schema, edit `server/models.py`, then from `server/`: + +``` +flask --app main db migrate -m "describe the change" # generates a migration; review and edit it +flask --app main db upgrade # applies it to your local database +``` + +Commit the migration with the model change. Deployments with `RUN_MIGRATIONS=true` (staging and production) run `flask db upgrade` when they start; PR previews never do, because they share the staging database. + ## Import Sections from GCP 1. Download the sections database to obtain a `.sql` file. Relevant Links [1](https://cloud.google.com/sql/docs/mysql/import-export/import-export-sql) and [2](https://cloud.google.com/storage/docs/downloading-objects). 2. Likely the export uses MySQL syntax, so update it to SQLite. diff --git a/server/import_sheet.py b/server/import_sheet.py index c60b965..68587e5 100644 --- a/server/import_sheet.py +++ b/server/import_sheet.py @@ -8,7 +8,7 @@ from offering import get_course from google_sheets import read_spreadsheet from dataclasses import asdict, dataclass, field, fields -from models import Failure, Section, User, db, user_section +from models import Failure, Section, db, find_or_add_person, offering_member, user_section # Sample Spreadsheet Link: https://docs.google.com/spreadsheets/d/1WL7gXiBxPe6aUFBKfEOVS6iY27ITy9mARupLoJX-ouE/edit?usp=sharing pst = ZoneInfo("US/Pacific") @@ -95,16 +95,10 @@ def import_sections(data: Iterator): raise Failure(f"Unknown boolean value: {can_self_enroll}") can_self_enroll = can_self_enroll == "true" - staff = User.query.filter_by( - email=email, - course=get_course(), - ).one_or_none() or User( - email=email, - name=name, - is_staff=True, - is_admin=False, - course=get_course(), - ) + staff = offering_member(email=email) + if staff is None: + staff = find_or_add_person(email, name) + staff.set_role(get_course(), is_staff=True) section_type = row[header.type_index] location = row[header.location_index] @@ -156,21 +150,14 @@ def import_enrollment(data: Iterator): start_time = parse_time_string(row[header.day_index], row[header.start_index]) section_type = row[header.type_index] - student = User.query.filter_by( - email=student_email, - course=get_course() - ).one_or_none() or User( - email=student_email, - name=student_name, - is_staff=False, - is_admin=False, - course=get_course() - ) + student = offering_member(email=student_email) + if student is None: + student = find_or_add_person(student_email, student_name) + student.set_role(get_course(), is_staff=False) - staff = User.query.filter_by( - email=staff_email, - course=get_course(), - ).one() + staff = offering_member(email=staff_email) + if staff is None: + raise Failure(f"Unable to import enrollment data for {student_email}! {staff_email} isn't on staff.") # Assumes that each section can be uniquely identified by the below parameters section = Section.query.filter_by( diff --git a/server/login.py b/server/login.py index e1aa535..906ce40 100644 --- a/server/login.py +++ b/server/login.py @@ -8,8 +8,8 @@ from markupsafe import escape import canvas_service -from offering import current_offering, get_course, in_offering -from models import Account, User, db +from offering import current_offering, in_offering +from models import User, db AFTER_LOGIN_KEY = "after_login" @@ -22,8 +22,8 @@ def _safe_next(target): def complete_login(token: dict): - """Create or update the signed-in Account from a Canvas OAuth token response.""" - canvas_user_id = token["user"]["id"] + """Create or update the signed-in User from a Canvas OAuth token response.""" + canvas_user_id = str(token["user"]["id"]) access_token = token["access_token"] profile = canvas_service.get_profile(canvas_user_id, access_token) email = profile.get("primary_email") or profile.get("login_id") @@ -31,50 +31,50 @@ def complete_login(token: dict): courses = canvas_service.get_user_courses(canvas_user_id, access_token) override_id = current_app.config.get("ADMIN_OVERRIDE_CANVAS_COURSE_ID") - account = Account.query.filter_by(canvas_id=str(canvas_user_id)).one_or_none() - if account is None: - account = Account(canvas_id=str(canvas_user_id)) - db.session.add(account) - account.email = email - account.name = name - account.canvas_courses = courses - account.is_global_admin = override_id is not None and str(override_id) in courses + # People added by staff or imported from a spreadsheet have a User (found + # by email) before they ever log in. + user = ( + User.query.filter_by(canvas_id=canvas_user_id).one_or_none() + or User.query.filter_by(email=email).one_or_none() + ) + if user is None: + user = User(email=email) + db.session.add(user) + user.canvas_id = canvas_user_id + user.email = email + user.name = name + user.canvas_courses = courses + user.is_global_admin = override_id is not None and str(override_id) in courses db.session.commit() - login_user(account, remember=True) + login_user(user, remember=True) session.permanent = True # Keep only a short-lived access token, and only for staff, who need it to # look up students; reauthorize when it expires. session.pop("canvas_access_token", None) session.pop("canvas_token_expires_at", None) - if account.is_global_admin or any(c["is_staff"] for c in courses.values()): + if user.is_global_admin or any(c["is_staff"] for c in courses.values()): session["canvas_access_token"] = access_token session["canvas_token_expires_at"] = time.time() + token.get("expires_in", 3600) -def course_user(account: Account): - """The account's User row in the current course, created on first visit. +def sync_offering_role(user: User) -> bool: + """Bring the user's role in the current offering in line with Canvas. - Staff and admin flags follow the account's Canvas roles from its last - sign-in. Returns None if the account isn't enrolled in the course. + Returns False if the user isn't enrolled in the offering's Canvas course. """ - roles = account.roles_in(current_offering().canvas_id) + if user.is_global_admin: + return True + offering = current_offering() + roles = user.canvas_roles_in(offering.canvas_id) if roles is None: - return None + return False is_staff, is_admin = roles - course = get_course() - user = User.query.filter_by(email=account.email, course=course).one_or_none() - if user is None: - user = User(email=account.email, name=account.name, is_staff=is_staff, - is_admin=is_admin, course=course) - db.session.add(user) - elif (user.name, user.is_staff, user.is_admin) == (account.name, is_staff, is_admin): - return user - user.name = account.name - user.is_staff = is_staff - user.is_admin = is_admin - db.session.commit() - return user + if (offering.key in user.staff_offerings, offering.key in user.admin_offerings, + user.is_member_of(offering.key)) != (is_staff, is_admin, True): + user.set_role(offering.key, is_staff=is_staff, is_admin=is_admin) + db.session.commit() + return True def create_login_client(app: flask.Flask): @@ -98,12 +98,9 @@ def create_login_client(app: flask.Flask): @login_manager.user_loader def load_user(session_id): - account_id = Account.parse_session_id(session_id) - account = db.session.get(Account, account_id) if account_id else None - if account is None or not in_offering(): - return account - user = course_user(account) - if user is None: + user_id = User.parse_session_id(session_id) + user = db.session.get(User, user_id) if user_id else None + if user is not None and in_offering() and not sync_offering_role(user): abort(403, "You aren't enrolled in this course on bCourses. If you just " "enrolled, sign out and sign in again.") return user diff --git a/server/main.py b/server/main.py index 670155e..97ce78c 100644 --- a/server/main.py +++ b/server/main.py @@ -1,6 +1,7 @@ import os from flask import Flask +from flask_migrate import Migrate from jinja2 import ChoiceLoader, FileSystemLoader from werkzeug.middleware.proxy_fix import ProxyFix @@ -24,6 +25,8 @@ load_config(app) db.init_app(app) +# Schema changes are Alembic migrations in server/migrations; `flask db upgrade` applies them. +Migrate(app, db, directory=os.path.join(os.path.dirname(os.path.abspath(__file__)), "migrations")) create_state_client(app) create_login_client(app) create_offering_pages(app) @@ -34,9 +37,5 @@ def health(): return "ok" -with app.app_context(): - db.create_all() - - if __name__ == "__main__": app.run(host="127.0.0.1", port=8000, debug=True) diff --git a/server/migrations/README b/server/migrations/README new file mode 100644 index 0000000..0e04844 --- /dev/null +++ b/server/migrations/README @@ -0,0 +1 @@ +Single-database configuration for Flask. diff --git a/server/migrations/alembic.ini b/server/migrations/alembic.ini new file mode 100644 index 0000000..ec9d45c --- /dev/null +++ b/server/migrations/alembic.ini @@ -0,0 +1,50 @@ +# A generic, single database configuration. + +[alembic] +# template used to generate migration files +# file_template = %%(rev)s_%%(slug)s + +# set to 'true' to run the environment during +# the 'revision' command, regardless of autogenerate +# revision_environment = false + + +# Logging configuration +[loggers] +keys = root,sqlalchemy,alembic,flask_migrate + +[handlers] +keys = console + +[formatters] +keys = generic + +[logger_root] +level = WARN +handlers = console +qualname = + +[logger_sqlalchemy] +level = WARN +handlers = +qualname = sqlalchemy.engine + +[logger_alembic] +level = INFO +handlers = +qualname = alembic + +[logger_flask_migrate] +level = INFO +handlers = +qualname = flask_migrate + +[handler_console] +class = StreamHandler +args = (sys.stderr,) +level = NOTSET +formatter = generic + +[formatter_generic] +format = %(levelname)-5.5s [%(name)s] %(message)s +datefmt = %H:%M:%S diff --git a/server/migrations/env.py b/server/migrations/env.py new file mode 100644 index 0000000..559948e --- /dev/null +++ b/server/migrations/env.py @@ -0,0 +1,113 @@ +import logging +from logging.config import fileConfig + +from flask import current_app + +from alembic import context +from sqlalchemy import text + +# this is the Alembic Config object, which provides +# access to the values within the .ini file in use. +config = context.config + +# Interpret the config file for Python logging. +# This line sets up loggers basically. +fileConfig(config.config_file_name) +logger = logging.getLogger('alembic.env') + + +def get_engine(): + return current_app.extensions['migrate'].db.engine + + +def get_engine_url(): + try: + return get_engine().url.render_as_string(hide_password=False).replace( + '%', '%%') + except AttributeError: + return str(get_engine().url).replace('%', '%%') + + +# add your model's MetaData object here +# for 'autogenerate' support +# from myapp import mymodel +# target_metadata = mymodel.Base.metadata +config.set_main_option('sqlalchemy.url', get_engine_url()) +target_db = current_app.extensions['migrate'].db + +# other values from the config, defined by the needs of env.py, +# can be acquired: +# my_important_option = config.get_main_option("my_important_option") +# ... etc. + + +def get_metadata(): + if hasattr(target_db, 'metadatas'): + return target_db.metadatas[None] + return target_db.metadata + + +def run_migrations_offline(): + """Run migrations in 'offline' mode. + + This configures the context with just a URL + and not an Engine, though an Engine is acceptable + here as well. By skipping the Engine creation + we don't even need a DBAPI to be available. + + Calls to context.execute() here emit the given string to the + script output. + + """ + url = config.get_main_option("sqlalchemy.url") + context.configure( + url=url, target_metadata=get_metadata(), literal_binds=True + ) + + with context.begin_transaction(): + context.run_migrations() + + +def run_migrations_online(): + """Run migrations in 'online' mode. + + In this scenario we need to create an Engine + and associate a connection with the context. + + """ + + # this callback is used to prevent an auto-migration from being generated + # when there are no changes to the schema + # reference: http://alembic.zzzcomputing.com/en/latest/cookbook.html + def process_revision_directives(context, revision, directives): + if getattr(config.cmd_opts, 'autogenerate', False): + script = directives[0] + if script.upgrade_ops.is_empty(): + directives[:] = [] + logger.info('No changes in schema detected.') + + conf_args = current_app.extensions['migrate'].configure_args + if conf_args.get("process_revision_directives") is None: + conf_args["process_revision_directives"] = process_revision_directives + + connectable = get_engine() + + with connectable.connect() as connection: + context.configure( + connection=connection, + target_metadata=get_metadata(), + **conf_args + ) + + with context.begin_transaction(): + if connection.dialect.name == "postgresql": + # Several Cloud Run instances can start at once; the first takes + # this lock and migrates, and the rest wait, then find nothing to do. + connection.execute(text("SELECT pg_advisory_xact_lock(724501)")) + context.run_migrations() + + +if context.is_offline_mode(): + run_migrations_offline() +else: + run_migrations_online() diff --git a/server/migrations/script.py.mako b/server/migrations/script.py.mako new file mode 100644 index 0000000..2c01563 --- /dev/null +++ b/server/migrations/script.py.mako @@ -0,0 +1,24 @@ +"""${message} + +Revision ID: ${up_revision} +Revises: ${down_revision | comma,n} +Create Date: ${create_date} + +""" +from alembic import op +import sqlalchemy as sa +${imports if imports else ""} + +# revision identifiers, used by Alembic. +revision = ${repr(up_revision)} +down_revision = ${repr(down_revision)} +branch_labels = ${repr(branch_labels)} +depends_on = ${repr(depends_on)} + + +def upgrade(): + ${upgrades if upgrades else "pass"} + + +def downgrade(): + ${downgrades if downgrades else "pass"} diff --git a/server/migrations/versions/0001_baseline.py b/server/migrations/versions/0001_baseline.py new file mode 100644 index 0000000..588ffe4 --- /dev/null +++ b/server/migrations/versions/0001_baseline.py @@ -0,0 +1,141 @@ +"""Baseline: the schema as of PR #14, before migrations existed. + +Revision ID: 0001_baseline +Revises: +Create Date: 2026-10-09 12:09:13.121880 + +Databases created before this (staging, whose data came from the monorepo) +already have these tables, so upgrading them only records this revision. +""" +from alembic import op +import sqlalchemy as sa + + +# revision identifiers, used by Alembic. +revision = '0001_baseline' +down_revision = None +branch_labels = None +depends_on = None + + +def upgrade(): + if sa.inspect(op.get_bind()).has_table("offerings"): + return + op.create_table('account', + sa.Column('id', sa.Integer(), nullable=False), + sa.Column('canvas_id', sa.String(length=255), nullable=False), + sa.Column('email', sa.String(length=255), nullable=False), + sa.Column('name', sa.String(length=255), nullable=False), + sa.Column('is_global_admin', sa.Boolean(), nullable=False), + sa.Column('canvas_courses', sa.JSON(), nullable=False), + sa.PrimaryKeyConstraint('id') + ) + with op.batch_alter_table('account', schema=None) as batch_op: + batch_op.create_index(batch_op.f('ix_account_canvas_id'), ['canvas_id'], unique=True) + batch_op.create_index(batch_op.f('ix_account_email'), ['email'], unique=False) + + op.create_table('course_config', + sa.Column('id', sa.Integer(), nullable=False), + sa.Column('course', sa.String(length=255), nullable=True), + sa.Column('can_students_join_lab', sa.Boolean(), nullable=True), + sa.Column('can_students_change_lab', sa.Boolean(), nullable=True), + sa.Column('can_tutors_change_lab', sa.Boolean(), nullable=True), + sa.Column('can_tutors_reassign_lab', sa.Boolean(), nullable=True), + sa.Column('can_students_join_disc', sa.Boolean(), nullable=True), + sa.Column('can_students_change_disc', sa.Boolean(), nullable=True), + sa.Column('can_tutors_change_disc', sa.Boolean(), nullable=True), + sa.Column('can_tutors_reassign_disc', sa.Boolean(), nullable=True), + sa.Column('can_students_join_tutoring', sa.Boolean(), nullable=True), + sa.Column('can_students_change_tutoring', sa.Boolean(), nullable=True), + sa.Column('can_tutors_change_tutoring', sa.Boolean(), nullable=True), + sa.Column('can_tutors_reassign_tutoring', sa.Boolean(), nullable=True), + sa.Column('message', sa.String(length=1024), nullable=True), + sa.PrimaryKeyConstraint('id') + ) + with op.batch_alter_table('course_config', schema=None) as batch_op: + batch_op.create_index(batch_op.f('ix_course_config_course'), ['course'], unique=False) + + op.create_table('offerings', + sa.Column('id', sa.Integer(), nullable=False), + sa.Column('canvas_id', sa.Integer(), nullable=False), + sa.Column('key', sa.String(length=255), nullable=False), + sa.Column('name', sa.String(length=255), nullable=False), + sa.Column('code', sa.String(length=255), nullable=True), + sa.Column('start_at', sa.String(length=255), nullable=True), + sa.PrimaryKeyConstraint('id'), + sa.UniqueConstraint('key') + ) + with op.batch_alter_table('offerings', schema=None) as batch_op: + batch_op.create_index(batch_op.f('ix_offerings_canvas_id'), ['canvas_id'], unique=True) + + op.create_table('user', + sa.Column('id', sa.Integer(), nullable=False), + sa.Column('course', sa.String(length=255), nullable=True), + sa.Column('email', sa.String(length=255), nullable=True), + sa.Column('name', sa.String(length=255), nullable=True), + sa.Column('is_staff', sa.Boolean(), nullable=True), + sa.Column('is_admin', sa.Boolean(), nullable=True), + sa.PrimaryKeyConstraint('id') + ) + with op.batch_alter_table('user', schema=None) as batch_op: + batch_op.create_index(batch_op.f('ix_user_course'), ['course'], unique=False) + batch_op.create_index(batch_op.f('ix_user_email'), ['email'], unique=False) + + op.create_table('section', + sa.Column('id', sa.Integer(), nullable=False), + sa.Column('course', sa.String(length=255), nullable=True), + sa.Column('description', sa.String(length=255), nullable=True), + sa.Column('capacity', sa.Integer(), nullable=True), + sa.Column('can_self_enroll', sa.Boolean(), nullable=True), + sa.Column('enrollment_code', sa.String(length=255), nullable=True), + sa.Column('staff_id', sa.Integer(), nullable=True), + sa.Column('tag_string', sa.String(length=255), nullable=False), + sa.Column('name', sa.String(length=255), nullable=True), + sa.Column('start_time', sa.Integer(), nullable=True), + sa.Column('end_time', sa.Integer(), nullable=True), + sa.Column('location', sa.String(length=255), nullable=False), + sa.Column('call_link', sa.String(length=255), nullable=True), + sa.ForeignKeyConstraint(['staff_id'], ['user.id'], ), + sa.PrimaryKeyConstraint('id') + ) + with op.batch_alter_table('section', schema=None) as batch_op: + batch_op.create_index(batch_op.f('ix_section_course'), ['course'], unique=False) + + op.create_table('session', + sa.Column('id', sa.Integer(), nullable=False), + sa.Column('course', sa.String(length=255), nullable=True), + sa.Column('start_time', sa.Integer(), nullable=True), + sa.Column('section_id', sa.Integer(), nullable=True), + sa.ForeignKeyConstraint(['section_id'], ['section.id'], ), + sa.PrimaryKeyConstraint('id') + ) + with op.batch_alter_table('session', schema=None) as batch_op: + batch_op.create_index(batch_op.f('ix_session_course'), ['course'], unique=False) + batch_op.create_index(batch_op.f('ix_session_section_id'), ['section_id'], unique=False) + + op.create_table('user_section', + sa.Column('user_id', sa.Integer(), nullable=False), + sa.Column('section_id', sa.Integer(), nullable=False), + sa.ForeignKeyConstraint(['section_id'], ['section.id'], ), + sa.ForeignKeyConstraint(['user_id'], ['user.id'], ), + sa.PrimaryKeyConstraint('user_id', 'section_id') + ) + op.create_table('attendance', + sa.Column('id', sa.Integer(), nullable=False), + sa.Column('course', sa.String(length=255), nullable=True), + sa.Column('status', sa.Enum('present', 'excused', 'absent', name='attendance_status'), nullable=True), + sa.Column('session_id', sa.Integer(), nullable=True), + sa.Column('student_id', sa.Integer(), nullable=True), + sa.ForeignKeyConstraint(['session_id'], ['session.id'], ), + sa.ForeignKeyConstraint(['student_id'], ['user.id'], ), + sa.PrimaryKeyConstraint('id') + ) + with op.batch_alter_table('attendance', schema=None) as batch_op: + batch_op.create_index(batch_op.f('ix_attendance_course'), ['course'], unique=False) + batch_op.create_index(batch_op.f('ix_attendance_session_id'), ['session_id'], unique=False) + batch_op.create_index(batch_op.f('ix_attendance_student_id'), ['student_id'], unique=False) + + +def downgrade(): + # Dropping the baseline would drop every table, including pre-existing data. + raise NotImplementedError("The baseline migration can't be downgraded.") diff --git a/server/migrations/versions/0002_single_user.py b/server/migrations/versions/0002_single_user.py new file mode 100644 index 0000000..0a4d5c0 --- /dev/null +++ b/server/migrations/versions/0002_single_user.py @@ -0,0 +1,159 @@ +"""One User per person across all offerings, like seating. + +Revision ID: 0002_single_user +Revises: 0001_baseline +Create Date: 2026-10-09 + +Before: a `user` row per person per offering (with that offering's is_staff and +is_admin), plus an `account` row per person who has logged in. +After: one `users` row per person (matched by email), with their roles in each +offering as sets of offering keys. Enrollments, attendance and section staff +are re-pointed at the merged rows. + +Also drops tables nothing reads: `account`, the old `user`, `course` (left over +from renaming offerings), and `slot`, `survey` and `user_section_junction` (from +the monorepo), along with `session.slot_id`, the monorepo's link to `slot`. +""" +from alembic import op +import sqlalchemy as sa + + +revision = "0002_single_user" +down_revision = "0001_baseline" +branch_labels = None +depends_on = None + +# Tables with a foreign key to the old `user` table, and the column holding it. +USER_REFERENCES = [("attendance", "student_id"), ("section", "staff_id"), ("user_section", "user_id")] +# Tables from the monorepo (and the pre-rename `course`) that nothing reads. +LEFTOVER_TABLES = ("course", "slot", "survey") +# Gives SQLite's unnamed foreign keys a name, so batch mode can drop them. +FK_NAMING = {"fk": "fk_%(table_name)s_%(column_0_name)s_%(referred_table_name)s"} + + +def _people(bind): + """One dict per person, merged by email, from the old user and account rows.""" + meta = sa.MetaData() + old_user = sa.Table("user", meta, autoload_with=bind) + people = {} + for row in bind.execute(sa.select(old_user).order_by(old_user.c.id)): + person = people.setdefault(row.email.lower(), { + "email": row.email, "name": row.name or row.email, "canvas_id": None, + "is_global_admin": False, "canvas_courses": {}, + "staff": set(), "admin": set(), "student": set(), "old_ids": [], + }) + person["old_ids"].append(row.id) + if row.name: + person["name"] = row.name # rows are in id order, so the newest name wins + if row.is_staff: + person["staff"].add(row.course) + else: + person["student"].add(row.course) + if row.is_admin: + person["admin"].add(row.course) + + if sa.inspect(bind).has_table("account"): + account = sa.Table("account", meta, autoload_with=bind) + for row in bind.execute(sa.select(account)): + person = people.setdefault(row.email.lower(), { + "email": row.email, "name": row.name, "staff": set(), "admin": set(), + "student": set(), "old_ids": [], + }) + # Logged-in people get their Canvas name and details. + person.update(name=row.name, canvas_id=row.canvas_id, + is_global_admin=bool(row.is_global_admin), + canvas_courses=row.canvas_courses or {}) + return people + + +def _drop_foreign_keys_to(table, referred): + fks = [fk for fk in sa.inspect(op.get_bind()).get_foreign_keys(table) + if fk["referred_table"] == referred] + if not fks: + return + with op.batch_alter_table(table, naming_convention=FK_NAMING) as batch: + for fk in fks: + name = fk["name"] or f"fk_{table}_{fk['constrained_columns'][0]}_{referred}" + batch.drop_constraint(name, type_="foreignkey") + + +def upgrade(): + bind = op.get_bind() + + users = op.create_table( + "users", + sa.Column("id", sa.Integer(), nullable=False), + sa.Column("email", sa.String(length=255), nullable=False), + sa.Column("name", sa.String(length=255), nullable=False), + sa.Column("canvas_id", sa.String(length=255), nullable=True), + sa.Column("is_global_admin", sa.Boolean(), nullable=False, server_default=sa.false()), + sa.Column("canvas_courses", sa.JSON(), nullable=False), + sa.Column("staff_offerings", sa.Text(), nullable=False, server_default=""), + sa.Column("admin_offerings", sa.Text(), nullable=False, server_default=""), + sa.Column("student_offerings", sa.Text(), nullable=False, server_default=""), + sa.PrimaryKeyConstraint("id"), + ) + op.create_index("ix_users_email", "users", ["email"], unique=True) + op.create_index("ix_users_canvas_id", "users", ["canvas_id"], unique=True) + + people = _people(bind) + for person in people.values(): + person["id"] = bind.execute( + sa.insert(users).returning(users.c.id), + { + "email": person["email"], "name": person["name"], + "canvas_id": person.get("canvas_id"), + "is_global_admin": person.get("is_global_admin", False), + "canvas_courses": person.get("canvas_courses") or {}, + "staff_offerings": ",".join(sorted(person["staff"])), + "admin_offerings": ",".join(sorted(person["admin"])), + "student_offerings": ",".join(sorted(person["student"])), + }, + ).scalar_one() + + # Map every old per-offering row to its person, then re-point references. + id_map = op.create_table( + "user_id_map", + sa.Column("old_id", sa.Integer(), primary_key=True), + sa.Column("new_id", sa.Integer(), nullable=False), + ) + mappings = [{"old_id": old_id, "new_id": person["id"]} + for person in people.values() for old_id in person["old_ids"]] + if mappings: + op.bulk_insert(id_map, mappings) + + if sa.inspect(bind).has_table("user_section_junction"): + op.drop_table("user_section_junction") + for table, column in USER_REFERENCES: + _drop_foreign_keys_to(table, "user") + # Two steps, through negative ids: new ids overlap old ones, and + # user_section's primary key is checked row by row during an UPDATE. + bind.execute(sa.text( + f"UPDATE {table} SET {column} = " + f"-(SELECT new_id FROM user_id_map WHERE old_id = {table}.{column}) " + f"WHERE {column} IS NOT NULL" + )) + bind.execute(sa.text(f"UPDATE {table} SET {column} = -{column} WHERE {column} < 0")) + with op.batch_alter_table(table) as batch: + batch.create_foreign_key(f"fk_{table}_{column}_users", "users", [column], ["id"]) + op.drop_table("user_id_map") + + # Databases from the monorepo still link sessions to the old slot table. + inspector = sa.inspect(bind) + for table in inspector.get_table_names(): + if any(fk["referred_table"] in LEFTOVER_TABLES for fk in inspector.get_foreign_keys(table)): + for referred in LEFTOVER_TABLES: + _drop_foreign_keys_to(table, referred) + if "slot_id" in {c["name"] for c in sa.inspect(bind).get_columns("session")}: + with op.batch_alter_table("session") as batch: + batch.drop_column("slot_id") + + inspector = sa.inspect(bind) + for table in ("account", "user") + LEFTOVER_TABLES: + if inspector.has_table(table): + op.drop_table(table) + + +def downgrade(): + # Per-offering rows can't be recovered once merged. + raise NotImplementedError("Merging users into one row per person can't be undone.") diff --git a/server/models.py b/server/models.py index a19f00e..93ea0a6 100644 --- a/server/models.py +++ b/server/models.py @@ -3,8 +3,10 @@ from typing import List from urllib.parse import quote +from flask import g from flask_login import UserMixin, current_user from flask_sqlalchemy import SQLAlchemy +from sqlalchemy import literal, types from sqlalchemy.orm import DeclarativeBase, joinedload @@ -15,9 +17,32 @@ class Base(DeclarativeBase): db = SQLAlchemy(model_class=Base) + +class StringSet(types.TypeDecorator): + """A set of strings stored as comma-separated text, as in seating.""" + + impl = types.Text + cache_ok = True + + def process_bind_param(self, value, dialect): + return ",".join(sorted(set(value or ()))) + + def process_result_value(self, value, dialect): + return set(value.split(",")) if value else set() + + +def set_contains(column, item: str): + """SQL condition: the StringSet ``column`` contains ``item``.""" + return (literal(",") + db.func.coalesce(column, "") + literal(",")).like(f"%,{item},%") + + +def current_course_key(): + """The current offering's key, or None outside an offering's pages.""" + return g.offering.key if "offering" in g else None + # Association Table for User - Section Pairs because each user can have multiple sections user_section = db.Table('user_section', - db.Column('user_id', db.Integer, db.ForeignKey('user.id'), primary_key=True), + db.Column('user_id', db.Integer, db.ForeignKey('users.id'), primary_key=True), db.Column('section_id', db.Integer, db.ForeignKey('section.id'), primary_key=True) ) @@ -29,7 +54,7 @@ class Section(db.Model): capacity: int = db.Column(db.Integer) can_self_enroll: bool = db.Column(db.Boolean) enrollment_code: str = db.Column(db.String(255), nullable=True) - staff_id: int = db.Column(db.Integer, db.ForeignKey("user.id"), nullable=True) + staff_id: int = db.Column(db.Integer, db.ForeignKey("users.id"), nullable=True) staff: "User" = db.relationship( "User", backref=db.backref("sections_taught", lazy="joined"), @@ -161,7 +186,7 @@ class Attendance(db.Model): lazy="joined", innerjoin=True, ) - student_id: int = db.Column(db.Integer, db.ForeignKey("user.id"), index=True) + student_id: int = db.Column(db.Integer, db.ForeignKey("users.id"), index=True) student: "User" = db.relationship( lambda: User, backref=db.backref("attendances"), lazy="joined", innerjoin=True ) @@ -182,22 +207,97 @@ def full_json(self): class User(db.Model, UserMixin): - # just here to make PyCharm stop complaining - def __init__(self, email: str, name: str, is_staff: bool, course: str, is_admin: bool): - # noinspection PyArgumentList - super().__init__(email=email, name=name, is_staff=is_staff, course=course, is_admin=is_admin) + """One person, across all offerings, like seating's User. + + Roles are per offering, in sets of offering keys (the value of each row's + `course` column) rather than seating's Canvas IDs, because offerings moved + from the monorepo aren't all linked to Canvas. Inside an offering's pages, + `is_staff` and `is_admin` answer for that offering. + """ + + __tablename__ = "users" id: int = db.Column(db.Integer, primary_key=True) - course: str = db.Column(db.String(255), index=True) - email: str = db.Column(db.String(255), index=True) - name: str = db.Column(db.String(255)) - is_staff: bool = db.Column(db.Boolean) - is_admin: bool = db.Column(db.Boolean) + email: str = db.Column(db.String(255), unique=True, index=True, nullable=False) + name: str = db.Column(db.String(255), nullable=False) + # Set when the person first logs in; people added by staff or imported from + # a spreadsheet may not have one. + canvas_id: str = db.Column(db.String(255), unique=True, index=True, nullable=True) + # Staff and admin in every offering (member of ADMIN_OVERRIDE_CANVAS_COURSE_ID). + is_global_admin: bool = db.Column(db.Boolean, nullable=False, default=False) + # Active Canvas courses at last login, keyed by str(canvas course id): + # {"name", "code", "start_at", "is_staff", "is_admin", "is_student"}. + canvas_courses: dict = db.Column(db.JSON, nullable=False, default=dict) + staff_offerings: set = db.Column(StringSet, nullable=False, default=set) + admin_offerings: set = db.Column(StringSet, nullable=False, default=set) + student_offerings: set = db.Column(StringSet, nullable=False, default=set) sections: List["Section"] = db.relationship( 'Section', secondary=user_section, back_populates='students', lazy='joined' ) # attendances: List[Attendance] is a backref defined on Attendance + def __init__(self, **kwargs): + # Column defaults only apply on insert; set_role needs real sets before then. + for field in ("staff_offerings", "admin_offerings", "student_offerings"): + kwargs.setdefault(field, set()) + kwargs.setdefault("canvas_courses", {}) + kwargs.setdefault("is_global_admin", False) + super().__init__(**kwargs) + + # Session ids are prefixed so cookies from before this table existed, which + # hold the id of a different table's row, can't be mistaken for a user id. + SESSION_ID_PREFIX = "user-" + + def get_id(self): + return f"{self.SESSION_ID_PREFIX}{self.id}" + + @classmethod + def parse_session_id(cls, session_id: str): + prefix, _, user_id = session_id.partition(cls.SESSION_ID_PREFIX) + return int(user_id) if not prefix and user_id.isdigit() else None + + @property + def is_staff(self) -> bool: + key = current_course_key() + return key is not None and (self.is_global_admin or key in self.staff_offerings) + + @property + def is_admin(self) -> bool: + key = current_course_key() + return key is not None and (self.is_global_admin or key in self.admin_offerings) + + def is_member_of(self, key: str) -> bool: + return key in self.staff_offerings or key in self.student_offerings + + def set_role(self, key: str, *, is_staff: bool, is_admin: bool = False): + """Make this person staff (optionally admin) or a student in an offering.""" + if is_staff: + self.staff_offerings = self.staff_offerings | {key} + self.student_offerings = self.student_offerings - {key} + else: + self.student_offerings = self.student_offerings | {key} + self.staff_offerings = self.staff_offerings - {key} + if is_admin: + self.admin_offerings = self.admin_offerings | {key} + else: + self.admin_offerings = self.admin_offerings - {key} + + def remove_from_offering(self, key: str): + self.staff_offerings = self.staff_offerings - {key} + self.admin_offerings = self.admin_offerings - {key} + self.student_offerings = self.student_offerings - {key} + + def sections_in(self, key: str) -> List["Section"]: + return [section for section in self.sections if section.course == key] + + def canvas_roles_in(self, canvas_course_id: int): + """``(is_staff, is_admin)`` in a Canvas course from the last login, or + None if not enrolled there.""" + info = (self.canvas_courses or {}).get(str(canvas_course_id)) + if info is None or not (info["is_staff"] or info["is_student"]): + return None + return info["is_staff"], info["is_admin"] + @property def identity_json(self): return { @@ -230,7 +330,7 @@ def json(self): @property def full_json(self): attendances = ( - Attendance.query.filter_by(student_id=self.id) + Attendance.query.filter_by(student_id=self.id, course=current_course_key()) .options( joinedload(Attendance.session, innerjoin=True) .joinedload(Session.section) @@ -252,7 +352,7 @@ def full_json(self): @property def simple_json(self): attendances = ( - Attendance.query.filter_by(student_id=self.id) + Attendance.query.filter_by(student_id=self.id, course=current_course_key()) .options( joinedload(Attendance.session, innerjoin=True) .joinedload(Session.section) @@ -286,6 +386,29 @@ def simple_json(self): } +def offering_members(key: str): + """Query for everyone (staff or student) in an offering.""" + return User.query.filter( + db.or_(set_contains(User.staff_offerings, key), set_contains(User.student_offerings, key)) + ) + + +def offering_member(**filters): + """The person matching ``filters`` (e.g. email=...) if they're in the + current offering, otherwise None.""" + user = User.query.filter_by(**filters).one_or_none() + return user if user is not None and user.is_member_of(current_course_key()) else None + + +def find_or_add_person(email: str, name: str) -> User: + """The person with this email, created (not yet in any offering) if new.""" + user = User.query.filter_by(email=email).one_or_none() + if user is None: + user = User(email=email, name=name) + db.session.add(user) + return user + + class CourseConfig(db.Model): id: int = db.Column(db.Integer, primary_key=True) course: str = db.Column(db.String(255), index=True) @@ -356,44 +479,5 @@ def __repr__(self): return f"" -class Account(db.Model, UserMixin): - """Someone signed in with Canvas, across all of their courses. - - Flask-Login keeps the account's id in the session. Inside a course, the - login manager resolves it to that course's User row (see login.py). - """ - - id: int = db.Column(db.Integer, primary_key=True) - canvas_id: str = db.Column(db.String(255), unique=True, index=True, nullable=False) - email: str = db.Column(db.String(255), index=True, nullable=False) - name: str = db.Column(db.String(255), nullable=False) - # Staff and admin in every course (member of ADMIN_OVERRIDE_CANVAS_COURSE_ID). - is_global_admin: bool = db.Column(db.Boolean, nullable=False, default=False) - # Active Canvas courses at last sign-in, keyed by str(canvas course id): - # {"name", "code", "start_at", "is_staff", "is_admin", "is_student"}. - canvas_courses: dict = db.Column(db.JSON, nullable=False, default=dict) - - # Session ids are prefixed so cookies from before accounts existed, which - # hold a per-course User id, can't be mistaken for an account id. - SESSION_ID_PREFIX = "account-" - - def get_id(self): - return f"{self.SESSION_ID_PREFIX}{self.id}" - - @classmethod - def parse_session_id(cls, session_id: str): - prefix, _, account_id = session_id.partition(cls.SESSION_ID_PREFIX) - return int(account_id) if not prefix and account_id.isdigit() else None - - def roles_in(self, canvas_course_id: int): - """``(is_staff, is_admin)`` in the course, or None if not enrolled.""" - if self.is_global_admin: - return True, True - info = self.canvas_courses.get(str(canvas_course_id)) - if info is None or not (info["is_staff"] or info["is_student"]): - return None - return info["is_staff"], info["is_admin"] - - class Failure(Exception): pass diff --git a/server/offerings.py b/server/offerings.py index 4f9044f..b8bb6bb 100644 --- a/server/offerings.py +++ b/server/offerings.py @@ -8,24 +8,24 @@ from models import Offering, db -def _categorized_offerings(account): - """Set-up offerings the account is staff in, a student in, or otherwise +def _categorized_offerings(user): + """Set-up offerings the user is staff in, a student in, or otherwise enrolled in, like seating's offerings page.""" - canvas_ids = [int(i) for i in account.canvas_courses] + canvas_ids = [int(i) for i in user.canvas_courses] existing = Offering.query.filter(Offering.canvas_id.in_(canvas_ids)).all() staff, students, other = [], [], [] for offering in existing: - info = account.canvas_courses[str(offering.canvas_id)] + info = user.canvas_courses[str(offering.canvas_id)] # Offerings linked from the monorepo start without Canvas details. if offering.code is None: offering.name, offering.code, offering.start_at = info["name"], info["code"], info["start_at"] - if info["is_staff"] or account.is_global_admin: + if info["is_staff"] or user.is_global_admin: staff.append(offering) elif info["is_student"]: students.append(offering) else: other.append(offering) - if account.is_global_admin: + if user.is_global_admin: staff += Offering.query.filter(Offering.canvas_id.notin_(canvas_ids)).all() db.session.commit() diff --git a/server/requirements.txt b/server/requirements.txt index 87e386d..d6da3ad 100644 --- a/server/requirements.txt +++ b/server/requirements.txt @@ -3,6 +3,8 @@ canvasapi==3.6.0 cloud-sql-python-connector[pg8000]==1.22.0 Flask==3.1.3 Flask_Login==0.6.3 +Flask-Migrate==4.1.0 +alembic==1.20.0 Flask_SQLAlchemy==3.1.1 google_api_python_client==2.200.0 google-auth==2.60.0 diff --git a/server/seed.py b/server/seed.py index f2a28b8..4c97702 100644 --- a/server/seed.py +++ b/server/seed.py @@ -4,8 +4,10 @@ from zoneinfo import ZoneInfo +from flask_migrate import upgrade + from main import app -from models import Offering, Section, User, db, user_section +from models import Offering, Section, db, find_or_add_person def seed(canvas_course_id: int): @@ -13,7 +15,7 @@ def seed(canvas_course_id: int): if app.config["APP_ENV"] != "development": return with app.app_context(): - db.create_all() + upgrade() if Offering.query.filter_by(canvas_id=canvas_course_id).one_or_none() is None: db.session.add(Offering(canvas_id=canvas_course_id, key=str(canvas_course_id), name="Demo 101")) course = str(canvas_course_id) @@ -57,13 +59,8 @@ def seed(canvas_course_id: int): for i in range(section_count * 2): discussion = choice(discussions) lab = choice(labs) - user = User( - course=course, - email=f"gobears{i}@berkeley.edu", - name=f"Oski {i}th of his name", - is_staff=False, - is_admin=False, - ) + user = find_or_add_person(f"gobears{i}@berkeley.edu", f"Oski {i}th of his name") + user.set_role(course, is_staff=False) user.sections = [discussion, lab] db.session.add(user) users.append(user) diff --git a/server/state.py b/server/state.py index 9c3833e..5237960 100644 --- a/server/state.py +++ b/server/state.py @@ -31,6 +31,10 @@ Session, User, db, + find_or_add_person, + offering_member, + offering_members, + set_contains, user_section, ) @@ -95,11 +99,13 @@ def is_valid_api_secret(secret) -> bool: def add_student_helper(student: User, target_section: Section): - if len(set([s.name for s in student.sections])) != len(student.sections): + # Only the target's offering matters; the person may be in other offerings too. + sections = student.sections_in(target_section.course) + if len(set([s.name for s in sections])) != len(sections): raise Failure("Student has multiple sections of the same type") # If user is already in a section of the same type, remove it and add target section - for s in student.sections: + for s in sections: if s.name == target_section.name: student.sections.remove(s) break @@ -144,7 +150,9 @@ def sudo_wrapped(): if not is_valid_api_secret(secret): abort(401) - user = User.query.filter_by(course=get_course(), email=email).one() + user = offering_member(email=email) + if user is None: + abort(404) # Act as this user for this request only, without touching the session. g._login_user = user try: @@ -208,8 +216,8 @@ def refresh_state(): if current_user.is_authenticated: course = get_course() - enrolled_sections = list(current_user.sections) - taught_sections = list(current_user.sections_taught) + enrolled_sections = current_user.sections_in(course) + taught_sections = [s for s in current_user.sections_taught if s.course == course] current_section_ids = {section.id for section in enrolled_sections} counts_started_at = time.perf_counter() @@ -316,23 +324,23 @@ def join_section(target_section_id: str, enrollment_code: str = ""): # TODO: Really shouldn't be hard coded, but has to be with the code right now config = get_config() if target_section.name == "Discussion" and not config.can_students_join_disc: - if any(current_user.sections): + if any(current_user.sections_in(get_course())): raise Failure("Students cannot change their enrolled discussion!") else: raise Failure("Students cannot add themselves themselves to discussions!") if target_section.name == "Lab" and not config.can_students_join_lab: - if any(current_user.sections): + if any(current_user.sections_in(get_course())): raise Failure("Students cannot change their enrolled lab!") else: raise Failure("Students cannot add themselves themselves to labs!") if target_section.name == "Tutoring" and not config.can_students_join_tutoring: - if any(current_user.sections): + if any(current_user.sections_in(get_course())): raise Failure("Students cannot change their enrolled tutorial!") else: raise Failure("Students cannot add themselves themselves to tutorials!") # If student is already enrolled in a section of the same type/name (Lab/Disc/etc) - if any(map(lambda section: section.name == target_section.name, current_user.sections)): + if any(map(lambda section: section.name == target_section.name, current_user.sections_in(get_course()))): if target_section.name == "Discussion" and not config.can_students_change_disc: raise Failure("Students cannot change their enrolled discussion!") if target_section.name == "Lab" and not config.can_students_change_lab: @@ -390,10 +398,11 @@ def leave_all_sections(): "Discussion": config.can_students_change_disc, "Tutoring": config.can_students_change_tutoring, } - if not all(can_change.get(s.name, True) for s in current_user.sections): + course_sections = current_user.sections_in(get_course()) + if not all(can_change.get(s.name, True) for s in course_sections): raise Failure("Students cannot remove themselves from sections!") - current_user.sections = [] + current_user.sections = [s for s in current_user.sections if s not in course_sections] db.session.commit() return refresh_state() @@ -520,9 +529,7 @@ def set_attendance(session_id: str, students: str, status: Optional[str]): session = Session.query.filter_by(id=session_id, course=get_course()).one() status = AttendanceStatus[status] for email in parse_emails(students): - student = User.query.filter_by( - email=email, course=get_course() - ).one_or_none() + student = offering_member(email=email) if student is None: raise Failure(f"Student {email} is not enrolled") Attendance.query.filter_by(session_id=session_id, student=student).delete() @@ -551,7 +558,9 @@ def update_config(**kwargs): @staff_required def remove_student(student: str, section_id: str): section_id = int(section_id) - student = User.query.filter_by(email=student, course=get_course()).one() + student = offering_member(email=student) + if student is None: + raise Failure("That student is not enrolled in this course") section = Section.query.filter_by(id=section_id, course=get_course()).one() student.sections.remove(section) db.session.commit() @@ -561,9 +570,10 @@ def remove_student(student: str, section_id: str): @admin_required def remove_students(students: str): for s in parse_emails(students): - student = User.query.filter_by(email=s, course=get_course()).one_or_none() + student = offering_member(email=s) if student: - student.sections = [] + # Only this offering's sections; the person may be in others. + student.sections = [sec for sec in student.sections if sec.course != get_course()] db.session.commit() return refresh_state() @@ -571,9 +581,12 @@ def remove_students(students: str): @admin_required def remove_students_from_tutoring(students: str): for s in parse_emails(students): - student = User.query.filter_by(email=s, course=get_course()).one_or_none() + student = offering_member(email=s) if student: - student.sections = [sec for sec in student.sections if sec.name != "Tutoring"] + student.sections = [ + sec for sec in student.sections + if not (sec.course == get_course() and sec.name == "Tutoring") + ] db.session.commit() return refresh_state() @@ -583,9 +596,10 @@ def add_student(email: str, section_id: str): #this function is never called!! use add_students instead section_id = int(section_id) section = Section.query.filter_by(id=section_id, course=get_course()).one() - student = User.query.filter_by(email=email, course=get_course()).one_or_none() + student = offering_member(email=email) if student is None: - student = User(email=email, name=email, is_staff=False, is_admin=False, course=get_course()) + student = find_or_add_person(email, email) + student.set_role(get_course(), is_staff=False) # for decoupling add_student_helper(student, section) @@ -600,9 +614,7 @@ def add_students(emails: str, section_id: str): section = Section.query.filter_by(id=section_id, course=get_course()).one() access_token = session.get("canvas_access_token") for email in parse_emails(emails): - student = User.query.filter_by( - email=email, course=get_course() - ).one_or_none() + student = offering_member(email=email) if student is not None and student.is_staff: raise Failure("Attempted to add staff: {student.name}") if student is None: @@ -629,9 +641,8 @@ def add_students(emails: str, section_id: str): if (not canvasname): raise Failure("Could not find email that belongs to this class") - student = User( - email=email, name=canvasname, is_staff=False, is_admin=False, course=get_course() - ) + student = find_or_add_person(email, canvasname) + student.set_role(get_course(), is_staff=False) add_student_helper(student, section) db.session.commit() return fetch_section(section_id=section_id) @@ -663,13 +674,14 @@ def export_helper(): stringify = dumps attendances = dict() emails = set() + course = get_course() for user in ( - User.query.filter_by(is_staff=False, course=get_course()) + User.query.filter(set_contains(User.student_offerings, course)) .options(joinedload(User.attendances).joinedload(Attendance.session)) .all() ): emails.add(user.email) - for attendance in user.attendances: + for attendance in [a for a in user.attendances if a.course == course]: try: section_name = attendance.session.section.name except AttributeError: @@ -751,7 +763,7 @@ def export_rosters(): @staff_required def fetch_user(user_id: str): user_id = int(user_id) - user = User.query.filter_by(id=user_id, course=get_course()).one_or_none() + user = offering_member(id=user_id) if user is None: raise Failure(f"No user found with id {user_id}") return user.simple_json @@ -759,7 +771,7 @@ def fetch_user(user_id: str): @api @staff_required def get_userid(email: str): - user = User.query.filter_by(email=email, course=get_course()).one_or_none() + user = offering_member(email=email) if user is None: raise Failure(f"No user found with email {email}") return user.id @@ -785,10 +797,12 @@ def reset_sections(): Attendance.query.filter_by(course=course).delete() Session.query.filter_by(course=course).delete() - for user in User.query.filter_by(course=course).all(): - user.sections.clear() + # People can be in other offerings, so take them out of this one rather + # than deleting them; staff get their role back from Canvas on their next visit. + for user in offering_members(course).all(): + user.sections = [s for s in user.sections if s.course != course] + user.remove_from_offering(course) - User.query.filter_by(course=course).delete() Section.query.filter_by(course=course).delete() db.session.commit() return refresh_state() @@ -797,16 +811,17 @@ def reset_sections(): @admin_required def fetch_to_drop(): students = "" + course = get_course() for student in ( - User.query.filter_by(is_staff=False, course=get_course()) - .filter(User.sections.any()) + User.query.filter(set_contains(User.student_offerings, course)) + .filter(User.sections.any(Section.course == course)) .all() ): tutoring_section = None absences = [] excused = [] if student: - for section in student.sections: + for section in student.sections_in(course): if section.name == 'Tutoring': tutoring_section = section if tutoring_section: @@ -839,22 +854,16 @@ def fetch_to_drop(): @api @admin_required def get_student_section_ids(email: str): - student = User.query.filter_by( - email= email, - course=get_course() - ).one_or_none() + student = offering_member(email=email) if (student is None): return {email: []} - section_ids = [section.id for section in student.sections] + section_ids = [section.id for section in student.sections_in(get_course())] return {email: section_ids} @api @admin_required def get_student_discussion_attendance(email:str): - student = User.query.filter_by( - email = email, - course=get_course() - ).one_or_none() + student = offering_member(email=email) discussion_present_days = [] if student: attendances = (Attendance.query.join(Attendance.session).join(Session.section) @@ -874,10 +883,7 @@ def get_student_discussion_attendance(email:str): @api @admin_required def get_student_lab_attendance(email:str): - student = User.query.filter_by( - email = email, - course=get_course() - ).one_or_none() + student = offering_member(email=email) lab_present_days = [] if student: attendances = (Attendance.query.join(Attendance.session).join(Session.section) @@ -896,10 +902,7 @@ def get_student_lab_attendance(email:str): @api @admin_required def get_student_tutoring_attendance(email:str): - student = User.query.filter_by( - email = email, - course=get_course() - ).one_or_none() + student = offering_member(email=email) tutoring_present_days = [] if student: attendances = (Attendance.query.join(Attendance.session).join(Session.section) diff --git a/server/test_canvas_login.py b/server/test_canvas_login.py index a8aa9ca..ad9df00 100644 --- a/server/test_canvas_login.py +++ b/server/test_canvas_login.py @@ -8,10 +8,10 @@ from offerings import create_offering_pages from login import complete_login, create_login_client -from models import Account, Offering, Section, User, db +from models import Attendance, AttendanceStatus, Offering, Section, Session, User, db from state import create_state_client -# Course 101 was moved from the monorepo, so its rows use the old key "cs61a". +# Offering 101 was moved from the monorepo, so its rows use the old key "cs61a". CS61A, DATA8, UNSET_UP = 101, 202, 303 @@ -20,6 +20,11 @@ def canvas_course(code, *, staff=False, admin=False, student=False): "is_staff": staff, "is_admin": admin, "is_student": student} +def lab(course, location="Room"): + return Section(course=course, name="Lab", location=location, description="", + capacity=30, can_self_enroll=True, start_time=0, end_time=3600) + + class CanvasLoginTests(unittest.TestCase): def setUp(self): self.app = Flask(__name__) @@ -41,39 +46,36 @@ def setUp(self): # Linked from the monorepo: no Canvas details until a member signs in. Offering(canvas_id=CS61A, key="cs61a", name="CS 61A"), Offering(canvas_id=DATA8, key=str(DATA8), name="DATA 8 - LEC 001", code="DATA 8", - start_at="2026-08-19T07:00:00Z"), + start_at="2026-08-19T07:00:00Z"), ]) - self.staff_account = Account(canvas_id="1", email="staff@test", name="Staff", canvas_courses={ + self.staff = User(email="staff@test", name="Staff", canvas_id="1", canvas_courses={ str(CS61A): canvas_course("CS 61A", staff=True), str(UNSET_UP): canvas_course("CS 70", staff=True, admin=True), }) - self.known_account = Account(canvas_id="2", email="known@test", name="Known", canvas_courses={ + self.staff.set_role("cs61a", is_staff=True) + self.known = User(email="known@test", name="Known", canvas_id="2", canvas_courses={ str(CS61A): canvas_course("CS 61A", student=True), str(UNSET_UP): canvas_course("CS 70", student=True), }) - staff = User(email="staff@test", name="Staff", is_staff=True, is_admin=False, course="cs61a") - known = User(email="known@test", name="Known", is_staff=False, is_admin=False, course="cs61a") - section = Section(course="cs61a", name="Lab", location="Room", description="", - capacity=30, can_self_enroll=True, start_time=0, end_time=3600) - data8_section = Section(course=str(DATA8), name="Lab", location="Elsewhere", - description="", capacity=30, can_self_enroll=True, - start_time=0, end_time=3600) - db.session.add_all([self.staff_account, self.known_account, staff, known, section, data8_section]) + self.known.set_role("cs61a", is_staff=False) + section = lab("cs61a") + data8_section = lab(str(DATA8), location="Elsewhere") + db.session.add_all([self.staff, self.known, section, data8_section]) db.session.commit() - self.staff_id = staff.id - self.known_id = known.id self.section_id = section.id + self.data8_section_id = data8_section.id + self.staff_id, self.known_id = self.staff.id, self.known.id self.client = self.app.test_client() - self.sign_in(self.staff_account) + self.sign_in(self.staff) def tearDown(self): db.session.remove() db.drop_all() self.context.pop() - def sign_in(self, account): + def sign_in(self, user): with self.client.session_transaction() as browser: - browser["_user_id"] = account.get_id() + browser["_user_id"] = user.get_id() def get(self, path): self.clear_request_globals() @@ -104,6 +106,15 @@ def authorize(self, expires_in=3600): browser["canvas_access_token"] = "access" browser["canvas_token_expires_at"] = time.time() + expires_in + def enroll_known_in_data8(self): + """Make "known" a student in DATA 8 too, enrolled in its lab.""" + known = db.session.get(User, self.known_id) + known.canvas_courses = {**known.canvas_courses, str(DATA8): canvas_course("DATA 8", student=True)} + known.set_role(str(DATA8), is_staff=False) + known.sections.append(db.session.get(Section, self.data8_section_id)) + db.session.commit() + return known + def test_existing_students_need_no_canvas_token(self): with patch("state.canvas_service.get_student_from_email") as lookup: self.assertTrue(self.add_students("known@test")["success"]) @@ -129,7 +140,7 @@ def test_fresh_token_is_passed_to_canvas(self): lookup.assert_called_once_with("new@test", "access") db.session.remove() new = User.query.filter_by(email="new@test").one() - self.assertEqual(new.course, "cs61a") + self.assertEqual(new.student_offerings, {"cs61a"}) self.assertEqual([s.id for s in new.sections], [self.section_id]) def test_revoked_token_requests_reauthentication(self): @@ -155,14 +166,30 @@ def test_login_retains_only_staff_access_token_and_logout_clears_it(self): self.assertEqual(session.get("canvas_access_token"), "access" if is_staff else None) self.assertNotIn("refresh_token", session) self.assertTrue(session.permanent) - self.assertEqual(session["_user_id"], self.staff_account.get_id()) - self.assertEqual(db.session.get(Account, self.staff_account.id).canvas_courses, courses) + self.assertEqual(session["_user_id"], self.staff.get_id()) + self.assertEqual(db.session.get(User, self.staff_id).canvas_courses, courses) self.authorize() self.get("/logout/") with self.client.session_transaction() as browser: self.assertNotIn("canvas_access_token", browser) self.assertNotIn("canvas_token_expires_at", browser) + def test_first_login_claims_the_imported_person_with_that_email(self): + imported = User(email="tutor@test", name="Imported Name") + imported.set_role("cs61a", is_staff=True) + db.session.add(imported) + db.session.commit() + with self.app.test_request_context(), \ + patch("login.canvas_service.get_profile", + return_value={"primary_email": "tutor@test", "short_name": "Canvas Name"}), \ + patch("login.canvas_service.get_user_courses", + return_value={str(CS61A): canvas_course("CS 61A", staff=True)}): + complete_login({"user": {"id": 77}, "access_token": "access"}) + self.assertEqual(User.query.filter_by(email="tutor@test").count(), 1) + tutor = db.session.get(User, imported.id) + self.assertEqual((tutor.canvas_id, tutor.name), ("77", "Canvas Name")) + self.assertEqual(tutor.staff_offerings, {"cs61a"}) + def test_admin_override_course_makes_global_admin(self): self.app.config["ADMIN_OVERRIDE_CANVAS_COURSE_ID"] = 999 with self.app.test_request_context(), \ @@ -171,7 +198,7 @@ def test_admin_override_course_makes_global_admin(self): return_value={"999": canvas_course("Override", student=True)}): complete_login({"user": {"id": 9}, "access_token": "access"}) self.assertIsNotNone(session.get("canvas_access_token")) - boss = Account.query.filter_by(canvas_id="9").one() + boss = User.query.filter_by(canvas_id="9").one() self.assertTrue(boss.is_global_admin) self.sign_in(boss) user = self.api("refresh_state", DATA8).get_json()["data"]["currentUser"] @@ -181,9 +208,10 @@ def test_admin_override_course_makes_global_admin(self): def test_student_refresh_uses_summaries_and_keeps_enrolled_roster(self): known = db.session.get(User, self.known_id) section = db.session.get(Section, self.section_id) - peer = User(email="peer@test", name="Peer", is_staff=False, is_admin=False, course="cs61a") - unrelated = User(email="unrelated@test", name="Unrelated", is_staff=False, - is_admin=False, course="cs61a") + peer = User(email="peer@test", name="Peer") + unrelated = User(email="unrelated@test", name="Unrelated") + for person in (peer, unrelated): + person.set_role("cs61a", is_staff=False) other_section = Section(course="cs61a", name="Discussion", location="Other Room", description="", capacity=25, can_self_enroll=True, start_time=7200, end_time=10800) @@ -192,7 +220,7 @@ def test_student_refresh_uses_summaries_and_keeps_enrolled_roster(self): db.session.add_all([peer, unrelated, other_section]) db.session.commit() - self.sign_in(self.known_account) + self.sign_in(self.known) response = self.api("refresh_state") self.assertEqual(response.status_code, 200) @@ -223,41 +251,109 @@ def test_courses_are_isolated(self): state = self.api("refresh_state").get_json()["data"] self.assertEqual([s["location"] for s in state["sections"]], ["Room"]) # Sections from another course can't be reached through this one. - data8_section = Section.query.filter_by(course=str(DATA8)).one() with self.assertRaises(NoResultFound): # a 500 outside tests - self.api("claim_section", section_id=str(data8_section.id)) - self.assertIsNone(db.session.get(Section, data8_section.id).staff_id) + self.api("claim_section", section_id=str(self.data8_section_id)) + self.assertIsNone(db.session.get(Section, self.data8_section_id).staff_id) # Not enrolled in DATA 8, so its pages and APIs are forbidden. self.assertEqual(self.api("refresh_state", DATA8).status_code, 403) self.assertEqual(self.get(f"/offerings/{DATA8}/").status_code, 403) + def test_one_person_in_two_offerings_sees_each_separately(self): + known = self.enroll_known_in_data8() + known.sections.append(db.session.get(Section, self.section_id)) + db.session.commit() + self.sign_in(known) + for canvas_id, location in ((CS61A, "Room"), (DATA8, "Elsewhere")): + state = self.api("refresh_state", canvas_id).get_json()["data"] + self.assertEqual([s["location"] for s in state["enrolledSections"]], [location]) + + def test_joining_a_lab_keeps_the_lab_in_another_offering(self): + known = self.enroll_known_in_data8() + self.sign_in(known) + self.api("refresh_state") # the app loads state (and the course config) first + result = self.api("join_section", target_section_id=str(self.section_id)).get_json() + self.assertTrue(result["success"], result) + db.session.remove() + sections = {s.id for s in db.session.get(User, self.known_id).sections} + self.assertEqual(sections, {self.section_id, self.data8_section_id}) + + def test_leaving_and_removal_only_affect_this_offering(self): + known = self.enroll_known_in_data8() + known.sections.append(db.session.get(Section, self.section_id)) + db.session.commit() + self.sign_in(known) + self.api("refresh_state") # the app loads state (and the course config) first + self.assertTrue(self.api("leave_all_sections").get_json()["success"]) + db.session.remove() + self.assertEqual([s.id for s in db.session.get(User, self.known_id).sections], + [self.data8_section_id]) + + # Admins removing a student also leave their other offerings alone. + known = db.session.get(User, self.known_id) + known.sections.append(db.session.get(Section, self.section_id)) + staff = db.session.get(User, self.staff_id) + staff.canvas_courses = {str(CS61A): canvas_course("CS 61A", staff=True, admin=True)} + db.session.commit() + self.sign_in(staff) + self.assertTrue(self.api("remove_students", students="known@test").get_json()["success"]) + db.session.remove() + self.assertEqual([s.id for s in db.session.get(User, self.known_id).sections], + [self.data8_section_id]) + + def test_attendance_history_is_per_offering(self): + known = self.enroll_known_in_data8() + data8_session = Session(course=str(DATA8), start_time=0, section_id=self.data8_section_id) + db.session.add(data8_session) + db.session.add(Attendance(course=str(DATA8), status=AttendanceStatus.present, + session=data8_session, student=known)) + db.session.commit() + self.sign_in(known) + for canvas_id, expected in ((CS61A, 0), (DATA8, 1)): + user = self.api("refresh_state", canvas_id).get_json()["data"]["currentUser"] + self.assertEqual(len(user["attendanceHistory"]), expected) + + def test_reset_removes_people_from_the_offering_but_keeps_them(self): + known = self.enroll_known_in_data8() + known.sections.append(db.session.get(Section, self.section_id)) + staff = db.session.get(User, self.staff_id) + staff.canvas_courses = {str(CS61A): canvas_course("CS 61A", staff=True, admin=True)} + db.session.commit() + self.assertTrue(self.api("reset_sections").get_json()["success"]) + db.session.remove() + known = db.session.get(User, self.known_id) + self.assertEqual(known.student_offerings, {str(DATA8)}) + self.assertEqual([s.id for s in known.sections], [self.data8_section_id]) + self.assertIsNone(db.session.get(Section, self.section_id)) + def test_course_that_is_not_set_up_is_not_found(self): self.assertEqual(self.api("refresh_state", UNSET_UP).status_code, 404) - def test_first_visit_creates_course_user_and_roles_follow_canvas(self): - self.staff_account.canvas_courses = { - **self.staff_account.canvas_courses, + def test_roles_follow_canvas(self): + staff = db.session.get(User, self.staff_id) + staff.canvas_courses = { + **staff.canvas_courses, str(DATA8): canvas_course("DATA 8", staff=True, admin=True), } db.session.commit() user = self.api("refresh_state", DATA8).get_json()["data"]["currentUser"] self.assertTrue(user["isAdmin"]) - self.assertEqual(User.query.filter_by(email="staff@test", course=str(DATA8)).count(), 1) + self.assertIn(str(DATA8), db.session.get(User, self.staff_id).admin_offerings) - # Dropped from staff in Canvas: the next sign-in updates the account, - # and the course user follows. - self.staff_account.canvas_courses = {str(CS61A): canvas_course("CS 61A", student=True)} + # Dropped from staff in Canvas: the next login updates canvas_courses, + # and the offering role follows. + staff = db.session.get(User, self.staff_id) + staff.canvas_courses = {str(CS61A): canvas_course("CS 61A", student=True)} db.session.commit() user = self.api("refresh_state").get_json()["data"]["currentUser"] self.assertEqual(user["id"], self.staff_id) self.assertFalse(user["isStaff"]) def test_staff_import_offerings_and_students_cannot(self): - self.sign_in(self.known_account) + self.sign_in(self.known) self.post("/offerings/new", data={"offerings": [str(UNSET_UP)]}) self.assertIsNone(Offering.query.filter_by(canvas_id=UNSET_UP).one_or_none()) - self.sign_in(self.staff_account) + self.sign_in(self.staff) page = self.get("/offerings/new").get_data(as_text=True) self.assertIn(f'value="{UNSET_UP}"', page) self.assertIn("2026-08 | CS 70 | CS 70 - LEC 001", page) @@ -265,8 +361,8 @@ def test_staff_import_offerings_and_students_cannot(self): # Only courses the user is staff in can be imported. response = self.post("/offerings/new", data={"offerings": [str(UNSET_UP), str(DATA8), "999"]}) self.assertTrue(response.headers["Location"].endswith("/offerings")) - course = Offering.query.filter_by(canvas_id=UNSET_UP).one() - self.assertEqual((course.key, course.code, course.start_month), (str(UNSET_UP), "CS 70", "2026-08")) + offering = Offering.query.filter_by(canvas_id=UNSET_UP).one() + self.assertEqual((offering.key, offering.code, offering.start_month), (str(UNSET_UP), "CS 70", "2026-08")) self.assertIsNone(Offering.query.filter_by(canvas_id=999).one_or_none()) self.assertTrue(self.api("refresh_state", UNSET_UP).get_json()["success"]) @@ -276,12 +372,12 @@ def test_offerings_page_matches_seating(self): self.assertIn("Staff Course Offerings", page) self.assertIn(f'href="/offerings/{CS61A}/"', page) self.assertNotIn(f"/offerings/{DATA8}/", page) - # The linked course picked up its Canvas details. + # The linked offering picked up its Canvas details. self.assertIn("CS 61A - LEC 001", page) self.assertEqual(Offering.query.filter_by(canvas_id=CS61A).one().code, "CS 61A") self.assertEqual(self.api("refresh_state").get_json()["data"]["course"], "CS 61A") - self.sign_in(self.known_account) + self.sign_in(self.known) page = self.get("/offerings").get_data(as_text=True) student_part = page[page.index("Student Course Offerings"):page.index("Other Course Offerings")] self.assertIn(f'href="/offerings/{CS61A}/"', student_part) @@ -293,12 +389,14 @@ def test_logged_out_pages_go_to_login_and_apis_return_401(self): self.assertIn("/login/", self.get("/offerings").headers["Location"]) self.assertEqual(self.api("join_section", target_section_id=str(self.section_id)).status_code, 401) - def test_session_from_before_accounts_is_ignored(self): - # Sessions from the single-course app hold a per-course User id. - with self.client.session_transaction() as browser: - browser["_user_id"] = str(self.staff_account.id) - state = self.api("refresh_state").get_json()["data"] - self.assertIsNone(state["currentUser"]) + def test_sessions_from_before_this_change_are_ignored(self): + # Older cookies hold a per-course row id or an "account-" id. + for old_id in (str(self.staff_id), f"account-{self.staff_id}"): + with self.subTest(old_id=old_id): + with self.client.session_transaction() as browser: + browser["_user_id"] = old_id + state = self.api("refresh_state").get_json()["data"] + self.assertIsNone(state["currentUser"]) if __name__ == "__main__": diff --git a/server/test_migrations.py b/server/test_migrations.py new file mode 100644 index 0000000..2fa603a --- /dev/null +++ b/server/test_migrations.py @@ -0,0 +1,103 @@ +import os +import tempfile +import unittest + +import sqlalchemy as sa +from flask import Flask +from flask_migrate import Migrate, upgrade + +from models import db + +MIGRATIONS = os.path.join(os.path.dirname(os.path.abspath(__file__)), "migrations") + + +class SingleUserMigrationTests(unittest.TestCase): + """0002_single_user merges per-offering user rows into one user per person.""" + + def setUp(self): + handle, self.path = tempfile.mkstemp(suffix=".db") + os.close(handle) + self.app = Flask(__name__) + self.app.config["SQLALCHEMY_DATABASE_URI"] = f"sqlite:///{self.path}" + db.init_app(self.app) + Migrate(self.app, db, directory=MIGRATIONS) + self.context = self.app.app_context() + self.context.push() + + def tearDown(self): + db.session.remove() + db.engine.dispose() + self.context.pop() + os.remove(self.path) + + def sql(self, statement, **params): + with db.engine.begin() as conn: + return conn.execute(sa.text(statement), params).all() if statement.lstrip().upper().startswith("SELECT") \ + else conn.execute(sa.text(statement), params) + + def test_merges_people_and_repoints_references(self): + upgrade(revision="0001_baseline") + self.sql("""INSERT INTO "user" (id, course, email, name, is_staff, is_admin) VALUES + (10, 'cs61a', 'ada@test', 'Ada', 0, 0), + (11, 'data8', 'ada@test', 'Ada L.', 1, 1), + (12, 'cs61a', 'tutor@test', 'Tutor', 1, 0), + (13, 'eecs16b', 'bob@test', 'Bob', NULL, NULL)""") + self.sql("""INSERT INTO account (id, canvas_id, email, name, is_global_admin, canvas_courses) + VALUES (1, '555', 'ada@test', 'Ada Lovelace', 0, '{"101": {"is_staff": false}}'), + (2, '556', 'new@test', 'Only Logged In', 1, '{}')""") + self.sql("""INSERT INTO section (id, course, staff_id, tag_string, location, name) VALUES + (1, 'cs61a', 12, '', 'Soda', 'Lab'), (2, 'data8', 11, '', 'Evans', 'Lab')""") + self.sql("INSERT INTO user_section (user_id, section_id) VALUES (10, 1), (13, 1)") + self.sql("INSERT INTO session (id, course, start_time, section_id) VALUES (1, 'cs61a', 0, 1)") + self.sql("""INSERT INTO attendance (id, course, status, session_id, student_id) + VALUES (1, 'cs61a', 'present', 1, 10)""") + self.sql("INSERT INTO offerings (id, canvas_id, key, name) VALUES (1, 101, 'cs61a', 'CS 61A')") + self.sql("CREATE TABLE course (id INTEGER PRIMARY KEY)") + self.sql("CREATE TABLE slot (id INTEGER PRIMARY KEY)") + # Monorepo databases link sessions to slots. + self.sql("ALTER TABLE session ADD COLUMN slot_id INTEGER REFERENCES slot(id)") + self.sql("CREATE TABLE survey (id INTEGER PRIMARY KEY)") + self.sql("CREATE TABLE user_section_junction (user_id INTEGER REFERENCES \"user\"(id), section_id INTEGER)") + + upgrade() + + self.assertNotIn("slot_id", {c["name"] for c in sa.inspect(db.engine).get_columns("session")}) + tables = set(sa.inspect(db.engine).get_table_names()) + self.assertIn("users", tables) + for gone in ("user", "account", "course", "slot", "survey", "user_section_junction", "user_id_map"): + self.assertNotIn(gone, tables) + + people = {row.email: row for row in self.sql("SELECT * FROM users")} + self.assertEqual(set(people), {"ada@test", "tutor@test", "bob@test", "new@test"}) + ada = people["ada@test"] + # Logged-in people keep their Canvas name and details. + self.assertEqual((ada.name, ada.canvas_id), ("Ada Lovelace", "555")) + self.assertEqual( + (ada.student_offerings, ada.staff_offerings, ada.admin_offerings), ("cs61a", "data8", "data8")) + self.assertEqual(people["bob@test"].student_offerings, "eecs16b") # unknown role counts as student + self.assertTrue(people["new@test"].is_global_admin) + + # Every reference now points at the merged person. + ids = {email: row.id for email, row in people.items()} + self.assertEqual(self.sql("SELECT staff_id FROM section ORDER BY id"), + [(ids["tutor@test"],), (ids["ada@test"],)]) + self.assertEqual(sorted(self.sql("SELECT user_id FROM user_section")), + sorted([(ids["ada@test"],), (ids["bob@test"],)])) + self.assertEqual(self.sql("SELECT student_id FROM attendance"), [(ids["ada@test"],)]) + foreign_keys = { + (table, fk["referred_table"]) + for table in ("attendance", "section", "user_section") + for fk in sa.inspect(db.engine).get_foreign_keys(table) + } + self.assertIn(("attendance", "users"), foreign_keys) + self.assertIn(("section", "users"), foreign_keys) + self.assertIn(("user_section", "users"), foreign_keys) + self.assertNotIn("user", {referred for _, referred in foreign_keys}) + + def test_fresh_database(self): + upgrade() + self.assertEqual(self.sql("SELECT count(*) FROM users"), [(0,)]) + + +if __name__ == "__main__": + unittest.main() From 55e22460303d66d4ce1edf12c5ca5d6fb8401911 Mon Sep 17 00:00:00 2001 From: Silas Santini <70163606+pancakereport@users.noreply.github.com> Date: Fri, 9 Oct 2026 12:40:07 -0700 Subject: [PATCH 2/2] Reset Sections Tool keeps people in the offering Reset now deletes the offering's sections, their sessions and attendance, and every student and staff assignment to them, but leaves everyone in the offering with their roles. Update the confirmation text to match. Co-Authored-By: Claude Opus 5.5 --- server/models.py | 12 ------------ server/state.py | 13 ++++++------- server/test_canvas_login.py | 18 ++++++++++++++---- src/ResetSectionsModal.tsx | 5 +++-- 4 files changed, 23 insertions(+), 25 deletions(-) diff --git a/server/models.py b/server/models.py index 93ea0a6..52fc908 100644 --- a/server/models.py +++ b/server/models.py @@ -282,11 +282,6 @@ def set_role(self, key: str, *, is_staff: bool, is_admin: bool = False): else: self.admin_offerings = self.admin_offerings - {key} - def remove_from_offering(self, key: str): - self.staff_offerings = self.staff_offerings - {key} - self.admin_offerings = self.admin_offerings - {key} - self.student_offerings = self.student_offerings - {key} - def sections_in(self, key: str) -> List["Section"]: return [section for section in self.sections if section.course == key] @@ -386,13 +381,6 @@ def simple_json(self): } -def offering_members(key: str): - """Query for everyone (staff or student) in an offering.""" - return User.query.filter( - db.or_(set_contains(User.staff_offerings, key), set_contains(User.student_offerings, key)) - ) - - def offering_member(**filters): """The person matching ``filters`` (e.g. email=...) if they're in the current offering, otherwise None.""" diff --git a/server/state.py b/server/state.py index 5237960..5c1e0da 100644 --- a/server/state.py +++ b/server/state.py @@ -33,7 +33,6 @@ db, find_or_add_person, offering_member, - offering_members, set_contains, user_section, ) @@ -797,12 +796,12 @@ def reset_sections(): Attendance.query.filter_by(course=course).delete() Session.query.filter_by(course=course).delete() - # People can be in other offerings, so take them out of this one rather - # than deleting them; staff get their role back from Canvas on their next visit. - for user in offering_members(course).all(): - user.sections = [s for s in user.sections if s.course != course] - user.remove_from_offering(course) - + # Remove the offering's sections and everyone's assignments to them, but + # keep the people and their roles in the offering. + course_section_ids = [s.id for s in Section.query.filter_by(course=course)] + db.session.execute( + user_section.delete().where(user_section.c.section_id.in_(course_section_ids)) + ) Section.query.filter_by(course=course).delete() db.session.commit() return refresh_state() diff --git a/server/test_canvas_login.py b/server/test_canvas_login.py index ad9df00..4616678 100644 --- a/server/test_canvas_login.py +++ b/server/test_canvas_login.py @@ -312,18 +312,28 @@ def test_attendance_history_is_per_offering(self): user = self.api("refresh_state", canvas_id).get_json()["data"]["currentUser"] self.assertEqual(len(user["attendanceHistory"]), expected) - def test_reset_removes_people_from_the_offering_but_keeps_them(self): + def test_reset_removes_sections_and_assignments_but_keeps_people(self): known = self.enroll_known_in_data8() - known.sections.append(db.session.get(Section, self.section_id)) + section = db.session.get(Section, self.section_id) + known.sections.append(section) + section.staff = db.session.get(User, self.staff_id) + session = Session(course="cs61a", start_time=0, section=section) + db.session.add(Attendance(course="cs61a", status=AttendanceStatus.present, + session=session, student=known)) staff = db.session.get(User, self.staff_id) staff.canvas_courses = {str(CS61A): canvas_course("CS 61A", staff=True, admin=True)} db.session.commit() + self.assertTrue(self.api("reset_sections").get_json()["success"]) db.session.remove() + self.assertIsNone(db.session.get(Section, self.section_id)) + self.assertEqual(Session.query.filter_by(course="cs61a").count(), 0) + self.assertEqual(Attendance.query.filter_by(course="cs61a").count(), 0) known = db.session.get(User, self.known_id) - self.assertEqual(known.student_offerings, {str(DATA8)}) + # Still in the course, and untouched in DATA 8. + self.assertEqual(known.student_offerings, {"cs61a", str(DATA8)}) self.assertEqual([s.id for s in known.sections], [self.data8_section_id]) - self.assertIsNone(db.session.get(Section, self.section_id)) + self.assertIn("cs61a", db.session.get(User, self.staff_id).staff_offerings) def test_course_that_is_not_set_up_is_not_found(self): self.assertEqual(self.api("refresh_state", UNSET_UP).status_code, 404) diff --git a/src/ResetSectionsModal.tsx b/src/ResetSectionsModal.tsx index 4216e9c..379d84f 100644 --- a/src/ResetSectionsModal.tsx +++ b/src/ResetSectionsModal.tsx @@ -14,8 +14,9 @@ export default function ResetSectionsModal({ show, onReset, onClose }: Props) { Reset Sections Tool - WARNING: This action will delete all attendance history, sessions, - sections, and users associated with your course. This is + WARNING: This action will delete all of your course's sections, their + sessions and attendance history, and every student and staff assignment + to them. Students and staff stay in the course. This is irreversible and should only be done in order to prepare sections tool for a new semester. Be extremely cautious when using this button. You have been warned.