Skip to content

Enhance documentation and scripts for GradeSync setup and deployment#2

Open
WeishuZ wants to merge 3 commits into
mainfrom
docs
Open

Enhance documentation and scripts for GradeSync setup and deployment#2
WeishuZ wants to merge 3 commits into
mainfrom
docs

Conversation

@WeishuZ

@WeishuZ WeishuZ commented Apr 15, 2026

Copy link
Copy Markdown
Collaborator
  • Updated config-and-settings.md to provide a clearer overview of source files and configuration structure.
  • Expanded dev-and-deploy.md with detailed instructions for local development and production deployment processes.
  • Improved gradesync.md by adding key source files and clarifying sync architecture and configuration fields.
  • Revised SETUP_DEMO.md to provide a comprehensive guide for creating demo courses, including prerequisites and usage instructions.
  • Simplified START_HERE.md to focus on GradeSync's functionality and quick start instructions.
  • Refined deploy_to_gcp.sh script for better clarity and usability, including detailed steps for VM setup and configuration.

Linear Ticket

Linear Ticket

Description

Type of Change

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Refactoring (non-breaking change)
  • Breaking change (fix or feature that would change existing functionality)

Changes

Testing

Checklist

  • My branch name matches the format: <ticket-id>/<brief-description-of-change>
  • My PR name matches the format: [<ticket-id>] <brief-description-of-change>
  • I have added doc-comments to all new functions (JSDoc for JS and Docstrings for Python)
  • I have reviewed all of my code

Screenshots/Video

Additional Notes

- Updated config-and-settings.md to provide a clearer overview of source files and configuration structure.
- Expanded dev-and-deploy.md with detailed instructions for local development and production deployment processes.
- Improved gradesync.md by adding key source files and clarifying sync architecture and configuration fields.
- Revised SETUP_DEMO.md to provide a comprehensive guide for creating demo courses, including prerequisites and usage instructions.
- Simplified START_HERE.md to focus on GradeSync's functionality and quick start instructions.
- Refined deploy_to_gcp.sh script for better clarity and usability, including detailed steps for VM setup and configuration.
Copilot AI review requested due to automatic review settings April 15, 2026 22:13
@WeishuZ WeishuZ self-assigned this Apr 15, 2026

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Updates onboarding + deployment documentation for GradeView/GradeSync and refines the GCP provisioning script to better explain setup, configuration, and operational workflow.

Changes:

  • Expanded root README.md and ARCHITECTURE.md with end-to-end architecture, setup, and deployment guidance.
  • Updated feature docs (auth, config, dev/deploy, gradesync) with “key files”, flows, and configuration references.
  • Refined scripts/deploy_to_gcp.sh to be more step-driven and include post-deploy instructions.

Reviewed changes

Copilot reviewed 9 out of 9 changed files in this pull request and generated 20 comments.

Show a summary per file
File Description
scripts/deploy_to_gcp.sh Improves GCP provisioning messaging and adds post-deploy instructions for VM/Cloud SQL setup.
gradesync/START_HERE.md Replaces legacy content with a concise GradeSync documentation map and quick-start steps.
gradesync/SETUP_DEMO.md Rewrites demo-data setup into an English guide with prerequisites, usage, and verification.
docs/features/gradesync.md Adds a clearer architecture/config overview and a key source files index for GradeSync.
docs/features/dev-and-deploy.md Adds structured local-dev and production-deploy guidance and points to key scripts/compose files.
docs/features/config-and-settings.md Documents config location/structure and intended loading behavior for API + GradeSync.
docs/features/auth-and-iam.md Documents auth flow, middleware behavior, and IAM roles.
README.md Major expansion: prerequisites, env/config references, database setup, run modes, and GCP deployment steps.
ARCHITECTURE.md Reframes architecture reference with diagrams, service breakdown, and onboarding checklist.
Comments suppressed due to low confidence (1)

scripts/deploy_to_gcp.sh:38

  • REGION is treated like a GCE region (e.g., us-central1) but is later used to build a zone via --zone=${REGION}-a. Passing an actual zone (e.g., us-central1-a) will produce an invalid zone (us-central1-a-a). Consider renaming the argument to ZONE and accepting a full zone, or keep REGION and derive/validate a zone separately.
REGION="${2:-us-central1}"
INSTANCE_NAME="gradeview-app"
MACHINE_TYPE="e2-standard-4"

echo "Deploying GradeView to project: $PROJECT_ID"
echo "Region: $REGION"
echo ""

# Step 0 — Ensure default VPC network exists
echo "[0/4] Checking VPC network..."
if ! gcloud compute networks describe default --project=$PROJECT_ID >/dev/null 2>&1; then
  echo "  Creating default network..."
  gcloud compute networks create default \
    --subnet-mode=auto \
    --project=$PROJECT_ID
  echo "  Default network created."
else
  echo "  Default network already exists."
fi

echo ""

# Step 1 — Create the GCE VM instance
echo "[1/4] Creating Compute Engine VM..."
gcloud compute instances create $INSTANCE_NAME \
  --project=$PROJECT_ID \
  --zone=${REGION}-a \
  --machine-type=$MACHINE_TYPE \

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment thread README.md Outdated
Comment on lines +476 to +488
### Step 4 — Apply database migrations

```bash
# Connect to Cloud SQL via the proxy (already in docker-compose.yml)
docker compose up -d cloud-sql-proxy
sleep 5

# Apply schema (first deploy only)
docker run --rm --network=db \
-e PGPASSWORD="${POSTGRES_PASSWORD}" \
postgres:16 \
psql -h cloud-sql-proxy -U "${POSTGRES_USER}" -d "${POSTGRES_DB}" \
-f /dev/stdin < docs/database/schema.sql

Copilot AI Apr 15, 2026

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The migration/apply-schema command relies on ${POSTGRES_*} being present in the shell environment, but at this stage those values are typically only in .env (used by Docker Compose) and won’t be exported for docker run. Consider using docker run --env-file .env ... or explicitly sourcing/exporting .env before running this command to avoid connecting with empty credentials.

Copilot uses AI. Check for mistakes.
Comment thread scripts/deploy_to_gcp.sh Outdated
Comment on lines 137 to 142
gcloud sql instances create gradeview-db \
--project=$PROJECT_ID \
--database-version=POSTGRES_15 \
--tier=db-f1-micro \
--region=$REGION \
--no-backup

Copilot AI Apr 15, 2026

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Cloud SQL is created with --no-backup, which disables automated backups. For a production deployment script this is a dangerous default; consider enabling backups (or at least documenting that backups are intentionally disabled and how to turn them on).

Copilot uses AI. Check for mistakes.
Comment thread scripts/deploy_to_gcp.sh Outdated
echo " - POSTGRES_PASSWORD (改为强密码)"
echo " - INSTANCE_CONNECTION_NAME 已自动填充为"
echo " eecs-gradeview:${REGION}:gradeview-db"
echo "4. Edit config.json and add your courses:"

Copilot AI Apr 15, 2026

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Step 4 says to edit /opt/gradeview/config.json, but the startup script never creates it. Add an explicit step to copy config.example.json to config.json before editing, otherwise users will end up editing a nonexistent file and services will start with an empty config.

Suggested change
echo "4. Edit config.json and add your courses:"
echo "4. Create and edit config.json, then add your courses:"
echo " cp /opt/gradeview/config.example.json /opt/gradeview/config.json"

Copilot uses AI. Check for mistakes.
Comment thread gradesync/START_HERE.md Outdated
Comment on lines 44 to 51
## Creating Demo Data

## 🎬 立即开始

最快的方式:
To create a full demo course with synthetic students and grades (no real student data):

```bash
cd gradesync
python3 create_demo_course.py --clean
python create_demo_course.py --clean --students 30
```

Copilot AI Apr 15, 2026

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This doc instructs running python create_demo_course.py, but there is no gradesync/create_demo_course.py in the repository. Either add the script (and ensure it’s included in the repo) or update the docs to point at the actual entry point under gradesync/scripts/ (or wherever it lives).

Copilot uses AI. Check for mistakes.
Comment thread docs/features/config-and-settings.md Outdated
Comment on lines +14 to +24
The unified runtime config is `config.json` at the **repository root**. It is volume-mounted read-only into both the `api` and `gradesync` containers:

## Notes
```yaml
# docker-compose.yml
api:
volumes:
- ./config.json:/api/config/default.json:ro
gradesync:
volumes:
- ./config.json:/app/config.json:ro
```

Copilot AI Apr 15, 2026

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The Compose snippet shows the API mounting ./config.json to /api/config/default.json, but docker-compose.yml does not mount config into the API container, and api/lib/unifiedConfig.mjs reads ../../config.json (i.e., /api/config.json in-container), not config/default.json. Please update this doc to match the actual mount path(s) and loader behavior.

Copilot uses AI. Check for mistakes.
Comment thread docs/features/config-and-settings.md Outdated
Comment on lines +41 to +44
1. `api/server.js` imports `unifiedConfig.mjs` at startup.
2. `unifiedConfig.mjs` reads `config/default.json` (which is the mounted `config.json`).
3. The config object is cached in memory for the lifetime of the process.
4. Routes that need config call `getConfig()` from `unifiedConfig.mjs`.

Copilot AI Apr 15, 2026

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The stated API loading order doesn’t match the code: api/server.js does not import unifiedConfig.mjs, and there is no getConfig() helper (routes call loadUnifiedConfig() directly). Update the steps here to reflect the actual call sites (e.g., api/v2/Routes/config/index.js) and function names.

Suggested change
1. `api/server.js` imports `unifiedConfig.mjs` at startup.
2. `unifiedConfig.mjs` reads `config/default.json` (which is the mounted `config.json`).
3. The config object is cached in memory for the lifetime of the process.
4. Routes that need config call `getConfig()` from `unifiedConfig.mjs`.
1. Route modules that need config import `loadUnifiedConfig()` from `api/lib/unifiedConfig.mjs` (for example, `api/v2/Routes/config/index.js`).
2. `loadUnifiedConfig()` reads `config/default.json` (which is the mounted `config.json`).
3. The config object is cached in memory for the lifetime of the process.
4. Subsequent route calls reuse that cached config via `loadUnifiedConfig()`.

Copilot uses AI. Check for mistakes.
Comment thread scripts/deploy_to_gcp.sh Outdated
echo " 方式C - 浏览器SSH(最简单):"
echo " https://console.cloud.google.com/compute/instances?project=$PROJECT_ID"
echo "3. Upload your GCP service account key:"
echo " scp key.json $INSTANCE_NAME:/opt/gradeview/secrets/key.json"

Copilot AI Apr 15, 2026

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The instructions use plain scp with the instance name (scp key.json $INSTANCE_NAME:...), which will usually fail unless the user has custom SSH config. Prefer gcloud compute scp ... --zone ... --project ..., and also ensure /opt/gradeview/secrets/ exists (and set restrictive perms) before uploading key.json.

Suggested change
echo " scp key.json $INSTANCE_NAME:/opt/gradeview/secrets/key.json"
echo " gcloud compute ssh $INSTANCE_NAME --zone=${REGION}-a --project=$PROJECT_ID --command='sudo mkdir -p /opt/gradeview/secrets && sudo chmod 700 /opt/gradeview/secrets'"
echo " gcloud compute scp key.json $INSTANCE_NAME:~/key.json --zone=${REGION}-a --project=$PROJECT_ID"
echo " gcloud compute ssh $INSTANCE_NAME --zone=${REGION}-a --project=$PROJECT_ID --command='sudo mv ~/key.json /opt/gradeview/secrets/key.json && sudo chown root:root /opt/gradeview/secrets/key.json && sudo chmod 600 /opt/gradeview/secrets/key.json'"

Copilot uses AI. Check for mistakes.
Comment thread gradesync/START_HERE.md
Comment on lines +18 to +27
# 1. Copy environment template and fill in credentials
cp api/.env.example api/.env
# Edit api/.env: set GRADESCOPE_EMAIL, GRADESCOPE_PASSWORD, PL_API_TOKEN, DATABASE_URL, etc.

详细的技术文档,包括:
- 创建的数据结构
- 命令行选项
- 数据库清理方法
- 故障排除
# 2. Copy config template and add at least one course
cp ../config.example.json ../config.json
# Edit config.json: add your course under gradesync.courses[]

---
# 3. Start the service
docker compose up --build

Copilot AI Apr 15, 2026

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The standalone Docker Compose file at gradesync/docker-compose.yml loads environment variables from gradesync/.env, but these instructions copy the template to gradesync/api/.env. As written, docker compose up from gradesync/ will ignore api/.env and run with missing credentials.

Copilot uses AI. Check for mistakes.
Comment thread README.md Outdated
Comment on lines +145 to +149
Provide either `GRADESYNC_DATABASE_URL` **or** the individual `POSTGRES_*` variables. The full URL takes precedence when both are set.

| Variable | Example | Description |
|----------|---------|-------------|
| `GRADESYNC_DATABASE_URL` | `postgresql://user:pass@host:5432/gradesync` | Full PostgreSQL DSN. Used by both the API and GradeSync services. Recommended for production. |

Copilot AI Apr 15, 2026

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This section says GRADESYNC_DATABASE_URL is used by both the API and GradeSync, but GradeSync actually reads DATABASE_URL (see gradesync/api/core/db.py). If users only set GRADESYNC_DATABASE_URL per this doc, GradeSync will still start with an empty/incorrect DATABASE_URL from docker-compose interpolation. Update the docs to mention DATABASE_URL (or update Compose to map GRADESYNC_DATABASE_URLDATABASE_URL).

Suggested change
Provide either `GRADESYNC_DATABASE_URL` **or** the individual `POSTGRES_*` variables. The full URL takes precedence when both are set.
| Variable | Example | Description |
|----------|---------|-------------|
| `GRADESYNC_DATABASE_URL` | `postgresql://user:pass@host:5432/gradesync` | Full PostgreSQL DSN. Used by both the API and GradeSync services. Recommended for production. |
Provide either a full database URL (`DATABASE_URL` for GradeSync, and `GRADESYNC_DATABASE_URL` where the API expects that name) **or** the individual `POSTGRES_*` variables. The full URL takes precedence when both are set.
| Variable | Example | Description |
|----------|---------|-------------|
| `DATABASE_URL` | `postgresql://user:pass@host:5432/gradesync` | Full PostgreSQL DSN used by the GradeSync service. Set this when running GradeSync via Docker/Compose so it receives the correct connection string. |
| `GRADESYNC_DATABASE_URL` | `postgresql://user:pass@host:5432/gradesync` | Full PostgreSQL DSN used by the API where that variable name is expected. If you deploy both services, set this alongside `DATABASE_URL` unless your Compose/setup explicitly maps one to the other. |

Copilot uses AI. Check for mistakes.
Comment thread docs/features/gradesync.md Outdated
| `gradesync/api/app.py` | FastAPI app factory, lifespan hooks, route registration |
| `gradesync/api/config_manager.py` | Reads `config.json` mounted at `/app/config.json` |
| `gradesync/api/schemas.py` | Pydantic models for all request/response bodies |
| `gradesync/api/core/db.py` | SQLAlchemy engine + async session factory |

Copilot AI Apr 15, 2026

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

gradesync/api/core/db.py uses synchronous SQLAlchemy (create_engine, sessionmaker) and does not create an async session factory. Update this row to avoid implying the code is async.

Suggested change
| `gradesync/api/core/db.py` | SQLAlchemy engine + async session factory |
| `gradesync/api/core/db.py` | SQLAlchemy engine + session factory |

Copilot uses AI. Check for mistakes.
@WeishuZ

WeishuZ commented Apr 15, 2026

Copy link
Copy Markdown
Collaborator Author

@copilot apply changes based on the comments in this thread

Copilot AI commented Apr 15, 2026

Copy link
Copy Markdown

@copilot apply changes based on the comments in this thread

Implemented the requested thread updates in commit 7ab40d1.

This addresses the documentation/script review items: env-file usage for schema apply, Cloud SQL backups, config.json creation step, corrected config mount/auth/JWT docs, Docker Compose/plugin and login-user updates in GCP deploy script, IAP-restricted SSH guidance, secure gcloud compute scp flow, corrected GradeSync env/setup commands, and removal of references to the missing create_demo_course.py command.

No UI files were changed in this update, so there is no UI screenshot to provide.

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 9 out of 9 changed files in this pull request and generated 9 comments.


💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment on lines +83 to +90
## Demo / Testing Data

To populate synthetic grade data without real students, use the demo-course script:

```bash
cd gradesync
python create_demo_course.py --clean --students 30
```

Copilot AI Apr 15, 2026

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This doc instructs running python create_demo_course.py ..., but there is no create_demo_course.py in the repository (searching the repo returns no matches). Either add/restore the script, or update the documentation to reference the actual demo data workflow that exists today (e.g., manual sync against a sandbox course).

Copilot uses AI. Check for mistakes.
Comment thread scripts/deploy_to_gcp.sh Outdated
Comment on lines 62 to 65
# Clone the project
cd /opt
git clone https://github.com/your-org/gradeview.git
cd gradeview

Copilot AI Apr 15, 2026

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The startup script clones https://github.com/your-org/gradeview.git, which appears to be a placeholder and will break deployment unless the user edits the script. Recommend parameterizing the repo URL (e.g., env var / arg) or replacing it with this repository’s actual clone URL.

Copilot uses AI. Check for mistakes.
Comment thread docs/features/config-and-settings.md Outdated
Comment on lines +36 to +41
## Loading Order (API)

1. Route modules that need config import `loadUnifiedConfig()` from `api/lib/unifiedConfig.mjs` (for example, `api/v2/Routes/config/index.js`).
2. `loadUnifiedConfig()` reads `config.json` from the API container root path (`/api/config.json`).
3. The config object is cached in memory for the lifetime of the process.
4. Subsequent config reads go through `loadUnifiedConfig()`/helper accessors in `unifiedConfig.mjs`.

Copilot AI Apr 15, 2026

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

api/lib/unifiedConfig.mjs does not currently cache the parsed config object in memory; loadUnifiedConfig() reads and parses /api/config.json on every call. The “Loads, caches…” wording (and the step claiming it’s cached for process lifetime) should be adjusted to match the implementation, or the code should be updated to actually cache if that’s desired.

Copilot uses AI. Check for mistakes.
Comment thread README.md
## Quick-start (local Dev)

```bash
# 1. Clone the repo

Copilot AI Apr 15, 2026

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The quick-start clone command uses a placeholder repo URL (https://github.com/your-org/gradeview.git), which will fail for most users. Recommend either replacing it with the real repository URL for this project, or explicitly marking it as a placeholder and instructing readers to substitute their fork/org.

Suggested change
# 1. Clone the repo
# 1. Clone the repo
# Replace `your-org` below with your actual GitHub organization or fork owner.

Copilot uses AI. Check for mistakes.
Comment thread README.md Outdated
Comment on lines +560 to +566
| `super_admin` | Global | Full access to all courses, settings, and GradeSync admin UI. Defined in `gradeview.admins` in `config.json`. |
| `course_admin` | Per-course | Manage GradeSync sync jobs and course config for their bound courses. |
| `instructor` | Per-course | View class roster and grade data. Cannot access GradeSync admin. |
| `ta` | Per-course | Same view permissions as instructor. |
| `student` | Per-course | View own grades only, scoped to enrolled courses. |

Permissions are enforced primarily via the `users` and `course_permissions` database tables. Config file role lists (`admins`, `instructors`, `tas`) should be treated as **migration targets** only — migrate them into the DB tables before relying solely on DB-driven auth.

Copilot AI Apr 15, 2026

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The roles table states that super_admin is defined in gradeview.admins in config.json, but the current IAM implementation treats super-admin as a hard-coded email (SUPER_ADMIN_EMAIL in api/lib/iam.mjs / api/lib/unifiedConfig.mjs). This section should be updated to match the actual source of truth (hard-coded super-admin vs DB roles), otherwise operators will assume adding an email to config.json grants super-admin privileges when it won’t.

Suggested change
| `super_admin` | Global | Full access to all courses, settings, and GradeSync admin UI. Defined in `gradeview.admins` in `config.json`. |
| `course_admin` | Per-course | Manage GradeSync sync jobs and course config for their bound courses. |
| `instructor` | Per-course | View class roster and grade data. Cannot access GradeSync admin. |
| `ta` | Per-course | Same view permissions as instructor. |
| `student` | Per-course | View own grades only, scoped to enrolled courses. |
Permissions are enforced primarily via the `users` and `course_permissions` database tables. Config file role lists (`admins`, `instructors`, `tas`) should be treated as **migration targets** only — migrate them into the DB tables before relying solely on DB-driven auth.
| `super_admin` | Global | Full access to all courses, settings, and GradeSync admin UI. Defined by the API's hard-coded `SUPER_ADMIN_EMAIL` configuration, not by `gradeview.admins` in `config.json`. |
| `course_admin` | Per-course | Manage GradeSync sync jobs and course config for their bound courses. |
| `instructor` | Per-course | View class roster and grade data. Cannot access GradeSync admin. |
| `ta` | Per-course | Same view permissions as instructor. |
| `student` | Per-course | View own grades only, scoped to enrolled courses. |
Permissions for `course_admin`, `instructor`, `ta`, and `student` are enforced primarily via the `users` and `course_permissions` database tables. Config file role lists (`admins`, `instructors`, `tas`) should be treated as **migration targets** only — adding an email there does not grant `super_admin` access unless the IAM implementation's hard-coded `SUPER_ADMIN_EMAIL` is also updated.

Copilot uses AI. Check for mistakes.
Comment thread scripts/deploy_to_gcp.sh Outdated
Comment on lines 34 to 37
echo "[1/4] Creating Compute Engine VM..."
gcloud compute instances create $INSTANCE_NAME \
--project=$PROJECT_ID \
--zone=${REGION}-a \

Copilot AI Apr 15, 2026

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

gcloud compute instances create will hard-fail if the instance already exists, which makes the script non-idempotent and harder to rerun safely. Consider checking for an existing instance first (and either skipping, prompting to delete/recreate, or supporting --instance-name/--force behavior).

Copilot uses AI. Check for mistakes.
Comment thread scripts/deploy_to_gcp.sh Outdated
Comment on lines +118 to +125
# Restrict SSH to Google Cloud IAP TCP forwarding range.
gcloud compute firewall-rules create allow-ssh \
--project=$PROJECT_ID \
--network=default \
--allow=tcp:22 \
--source-ranges=0.0.0.0/0 \
--source-ranges=35.235.240.0/20 \
--target-tags=gradeview-app \
2>/dev/null || echo " SSH防火墙规则已存在"
2>/dev/null || echo " SSH firewall rule already exists."

Copilot AI Apr 15, 2026

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The script creates a project-wide firewall rule named allow-ssh and treats “already exists” as success. If a pre-existing allow-ssh rule allows 0.0.0.0/0, the script will still print that SSH is restricted to the IAP range even though it isn’t. To avoid leaving an insecure rule in place, either (a) create a uniquely named rule for this deployment and/or (b) detect and update/validate the existing rule’s source ranges before continuing.

Copilot uses AI. Check for mistakes.
Comment thread scripts/deploy_to_gcp.sh Outdated
echo "━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━"
echo "7. Verify:"
echo " curl -fs http://$EXTERNAL_IP/api/health"
echo " open http://$EXTERNAL_IP"

Copilot AI Apr 15, 2026

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The post-deploy instructions suggest running open http://$EXTERNAL_IP, but open is macOS-specific. Since this script is likely run from Linux as well, consider printing a more portable command (e.g., xdg-open) or just printing the URL to copy/paste.

Suggested change
echo " open http://$EXTERNAL_IP"
echo " Open in your browser: http://$EXTERNAL_IP"

Copilot uses AI. Check for mistakes.
Comment thread README.md Outdated
Comment on lines +99 to +100
# - gradeview.googleconfig.oauth.clientid in config.json
# - INSTANCE_CONNECTION_NAME + secrets/key.json (if using Cloud SQL)

Copilot AI Apr 15, 2026

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The setup checklist says gradeview.googleconfig.oauth.clientid in config.json is a minimum requirement before first boot, but the API actually requires the Google OAuth client ID to be present in the gradeview_config DB table (see api/lib/googleAuthHelper.mjs, which throws if it’s missing). The docs should add the required DB initialization step (e.g., run the config migration script or insert/update the google_oauth_client_id row) and clarify whether config.json is used only as an input to that migration.

Suggested change
# - gradeview.googleconfig.oauth.clientid in config.json
# - INSTANCE_CONNECTION_NAME + secrets/key.json (if using Cloud SQL)
# - Google OAuth client ID initialized in the `gradeview_config` DB table
# as `google_oauth_client_id`
# - If you keep the value in `config.json` as
# `gradeview.googleconfig.oauth.clientid`, treat that file as an input to
# the config migration/import step; it is not sufficient on its own unless
# that step populates `gradeview_config`
# - INSTANCE_CONNECTION_NAME + secrets/key.json (if using Cloud SQL)
#
# Before starting the stack, run the config migration/import script that copies
# config values into `gradeview_config`, or manually insert/update the
# `google_oauth_client_id` row.

Copilot uses AI. Check for mistakes.
@WeishuZ

WeishuZ commented Apr 15, 2026

Copy link
Copy Markdown
Collaborator Author

@copilot apply changes based on the comments in this thread

Copilot AI commented Apr 15, 2026

Copy link
Copy Markdown

@copilot apply changes based on the comments in this thread

Applied the thread updates in commit 52645a2.

This includes the remaining doc/script fixes: replaced stale demo command in docs/features/gradesync.md, corrected unifiedConfig caching wording, updated README clone/setup/auth-role guidance (including OAuth DB initialization and variable naming clarity), and improved deploy_to_gcp.sh with idempotent VM creation, actual repo URL, unique/enforced IAP SSH firewall rule handling, and portable verification output.

No UI files were changed in this update, so there is no UI screenshot to provide.

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 9 out of 9 changed files in this pull request and generated 14 comments.


💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment on lines +12 to +18
| `api/v2/Routes/login/index.js` | `POST /api/v2/login` handler |
| `api/v2/Routes/isadmin/index.js` | `GET /api/v2/isadmin` handler |

## Authentication Flow (step by step)

1. Browser calls Google OAuth and receives an **ID token**.
2. Frontend `POST /api/v2/login` with `{ token: "<google-id-token>" }`.

Copilot AI Apr 15, 2026

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The source-file table and login-flow steps describe POST /api/v2/login with a JSON body token, but the current implementation is GET /api/v2/login with the Google ID token in the Authorization: Bearer ... header (see api/v2/Routes/login/index.js and website/src/views/login.js). Please update the docs to match the actual request method and token transport.

Suggested change
| `api/v2/Routes/login/index.js` | `POST /api/v2/login` handler |
| `api/v2/Routes/isadmin/index.js` | `GET /api/v2/isadmin` handler |
## Authentication Flow (step by step)
1. Browser calls Google OAuth and receives an **ID token**.
2. Frontend `POST /api/v2/login` with `{ token: "<google-id-token>" }`.
| `api/v2/Routes/login/index.js` | `GET /api/v2/login` handler |
| `api/v2/Routes/isadmin/index.js` | `GET /api/v2/isadmin` handler |
## Authentication Flow (step by step)
1. Browser calls Google OAuth and receives an **ID token**.
2. Frontend calls `GET /api/v2/login` with `Authorization: Bearer <google-id-token>`.

Copilot uses AI. Check for mistakes.
Comment thread README.md
docker compose -f docker-compose.dev.yml up --build

# 6. Initialize runtime config rows in DB (required for Google OAuth login)
docker compose -f docker-compose.dev.yml exec api npm run migrateConfigToDb

Copilot AI Apr 15, 2026

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The quick-start step that runs npm run migrateConfigToDb inside the api container will be a no-op unless config.json is available at /api/config.json in that container. In the current docker-compose.dev.yml, config.json is not mounted into the API container, and the migration script (api/v2/Routes/config/migrate_config_to_db.js) explicitly looks for /api/config.json, so it will log that the root config is missing and skip configuring gradeview_config (blocking Google token verification). Update the instructions to either mount ./config.json:/api/config.json:ro for the API service or run the migration script from the host where it can read the repo-root config.json.

Suggested change
docker compose -f docker-compose.dev.yml exec api npm run migrateConfigToDb
# Run this from the repository root on the host so the script can read ./config.json.
npm run migrateConfigToDb
# If you prefer to run it inside the API container instead, first mount:
# ./config.json:/api/config.json:ro
# in docker-compose.dev.yml for the api service, then run:
# docker compose -f docker-compose.dev.yml exec api npm run migrateConfigToDb

Copilot uses AI. Check for mistakes.
Comment thread gradesync/START_HERE.md
# 在另一个终端
docker compose exec gradesync python3 create_demo_course.py --clean
# 4. Access the API docs
open http://localhost:8001/docs

Copilot AI Apr 15, 2026

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In the standalone GradeSync flow, docker compose up --build (from gradesync/) maps the API to host port 8000 (see gradesync/docker-compose.yml), not 8001. The docs link here should point to http://localhost:8000/docs for standalone mode, and reserve :8001 for the full-stack compose where gradesync is exposed on 8001.

Suggested change
open http://localhost:8001/docs
open http://localhost:8000/docs

Copilot uses AI. Check for mistakes.
```

## Rule of Thumb
All source directories are bind-mounted. Node.js services use `nodemon`; FastAPI uses `--reload`. Changes take effect without rebuilding.

Copilot AI Apr 15, 2026

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This line claims the dev stack runs FastAPI with --reload, but docker-compose.dev.yml currently starts GradeSync with uvicorn ... --workers 2 (no reload). Either update the docs to reflect the current command (changes require a restart) or change the dev compose command to use --reload for the intended hot-reload behavior.

Suggested change
All source directories are bind-mounted. Node.js services use `nodemon`; FastAPI uses `--reload`. Changes take effect without rebuilding.
All source directories are bind-mounted. Node.js services use `nodemon`; GradeSync/FastAPI is currently started without `--reload`, so Python code changes require a container restart rather than a rebuild.

Copilot uses AI. Check for mistakes.
Comment on lines +22 to +24
4. `userlib.mjs` queries the `users` table by `email`. If the user does not exist yet, a new row is inserted.
5. `userlib.mjs` queries `course_permissions` to build a list of `{ course_id, permission_level }` pairs.
6. A **JWT** is signed with `JWT_SECRET` (from `.env`) and returned to the browser.

Copilot AI Apr 15, 2026

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

api/lib/userlib.mjs does not currently resolve roles from users/course_permissions (it only delegates admin checks to unifiedConfig.mjs and student checks to dbHelper.mjs). Role resolution and permission snapshots are built in api/lib/iam.mjs (buildPermissionSnapshot) and used by the login route. Adjust the doc to point to the correct modules so readers don’t chase the wrong implementation.

Copilot uses AI. Check for mistakes.
Comment on lines +60 to +67
```bash
# Manual sync for one course (replace COURSE_ID with internal id like cs10_fa25)
curl -X POST http://localhost/gradesync/sync/COURSE_ID \
-H "Authorization: Bearer <admin-jwt>"

# Check sync status
curl http://localhost/gradesync/status/COURSE_ID \
-H "Authorization: Bearer <admin-jwt>"

Copilot AI Apr 15, 2026

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The sync trigger examples use endpoints that do not exist in the FastAPI app. GradeSync defines sync routes under /api/sync/{course_id} (see gradesync/api/app.py), so through the reverse proxy the URL would be /gradesync/api/sync/{course_id} (or direct http://localhost:8001/api/sync/{course_id} in the full stack). Also, the example mentions an admin JWT header, but GradeSync routes are not currently protected by JWT auth, and there is no /status/{course_id} route. Please update the examples to match the implemented routes/auth model.

Copilot uses AI. Check for mistakes.
Comment thread ARCHITECTURE.md
:5433 → Cloud SQL Proxy (host-side DB access)
:8001 → GradeSync FastAPI (debug)

Source code bind-mounts: api/, website/, gradesync/ → hot-reload via nodemon / uvicorn --reload

Copilot AI Apr 15, 2026

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The dev topology section states GradeSync hot-reloads via uvicorn --reload, but docker-compose.dev.yml currently runs uvicorn ... --workers 2 without --reload. Update this line to match the actual dev command, or update the compose command if hot-reload is expected.

Suggested change
Source code bind-mounts: api/, website/, gradesync/ → hot-reload via nodemon / uvicorn --reload
Source code bind-mounts: api/, website/, gradesync/ → hot-reload for api/ and website/ via nodemon / React dev server; GradeSync is bind-mounted but does not use `uvicorn --reload`

Copilot uses AI. Check for mistakes.

- Keep permissions in DB tables, not in config files.
- Keep config format stable and versioned for safer rollout.
Changes to `config.json` require a container restart — there is no live reload.

Copilot AI Apr 15, 2026

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This sentence says config changes require a container restart and that there is no live reload. That is true for GradeSync (ConfigManager loads /app/config.json at init), but the API loadUnifiedConfig() reads /api/config.json from disk on every call and will pick up changes immediately if the file is mounted. Consider clarifying the different reload behavior for API vs. GradeSync.

Suggested change
Changes to `config.json` require a container restart — there is no live reload.
Reload behavior differs by service: GradeSync loads `/app/config.json` at startup, so changes require a `gradesync` container restart; the API reads `/api/config.json` from disk on each `loadUnifiedConfig()` call, so mounted file changes are picked up on subsequent requests.

Copilot uses AI. Check for mistakes.
Comment on lines +47 to +53
| OAuth client ID | `config.json` |
| Global admin emails | `config.json` → migrate to `users` table over time |
| Course sync source IDs and credentials | `config.json` |
| Per-course staff permissions | `course_permissions` DB table |
| Student enrollment | `students` DB table |
| Grade data | `assignments` + `submissions` DB tables |

Copilot AI Apr 15, 2026

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The “Config vs. Database” table implies the OAuth client ID lives in config.json, but Google token verification reads it from the gradeview_config DB table (api/lib/googleAuthHelper.mjs). If config.json is only a seed/migration input, it would be clearer to mark the DB as the runtime source of truth and mention the migration step explicitly.

Suggested change
| OAuth client ID | `config.json` |
| Global admin emails | `config.json` → migrate to `users` table over time |
| Course sync source IDs and credentials | `config.json` |
| Per-course staff permissions | `course_permissions` DB table |
| Student enrollment | `students` DB table |
| Grade data | `assignments` + `submissions` DB tables |
| OAuth client ID | Seed/bootstrap in `config.json`; runtime source of truth is the `gradeview_config` DB table |
| Global admin emails | Seed/bootstrap in `config.json`; migrate/sync into the `users` table, which is the runtime authority |
| Course sync source IDs and credentials | `config.json` |
| Per-course staff permissions | `course_permissions` DB table |
| Student enrollment | `students` DB table |
| Grade data | `assignments` + `submissions` DB tables |
If a setting exists in both `config.json` and a DB table, treat the DB row as the runtime source of truth. `config.json` is only the seed/bootstrap input for values that are migrated or synced into the database.

Copilot uses AI. Check for mistakes.
3. Set `gradesync.sources.gradescope.course_id` to the Gradescope numeric course ID.
4. Set `gradeview.buckets` to match the course grading scale.
5. Restart the `api` and `gradesync` containers.
6. Trigger an initial sync via the GradeSync admin UI or `POST /gradesync/sync/{course_id}`.

Copilot AI Apr 15, 2026

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Step 6 references triggering an initial sync via POST /gradesync/sync/{course_id}, but GradeSync sync routes are implemented under /api/sync/{course_id} (so via reverse proxy: /gradesync/api/sync/{course_id}). Update the endpoint here to match the actual FastAPI routes.

Suggested change
6. Trigger an initial sync via the GradeSync admin UI or `POST /gradesync/sync/{course_id}`.
6. Trigger an initial sync via the GradeSync admin UI or `POST /gradesync/api/sync/{course_id}`.

Copilot uses AI. Check for mistakes.
@WeishuZ WeishuZ removed their assignment Apr 15, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants