feat: support multiple syllabuses per course offering#337
Conversation
PR Preview BuildsBuild Number: 1148 |
3c99c67 to
315392e
Compare
📝 WalkthroughWalkthroughSyllabus content fields are extracted from ChangesSyllabus Extraction and Multi-ID Refactor
Sequence Diagram(s)sequenceDiagram
participant UI
participant syllabusProvider
participant CourseRepository
participant AppDatabase
participant CourseService
UI->>syllabusProvider: watch(offeringId, teacherId)
syllabusProvider->>CourseRepository: watchSyllabus(offeringId, teacherId)
CourseRepository->>AppDatabase: stream Syllabuses row
alt fetchedAt is null (stub row)
CourseRepository->>CourseService: getSyllabus(courseNumber, teacherId)
CourseService-->>CourseRepository: SyllabusDto?
alt SyllabusDto is null
CourseRepository->>AppDatabase: delete Syllabuses row
else SyllabusDto provided
CourseRepository->>AppDatabase: upsert Syllabuses content
CourseRepository->>AppDatabase: update CourseOfferings headers
CourseRepository->>AppDatabase: update TeacherSemesters email
end
else fetchedAt present
CourseRepository->>CourseRepository: refreshSyllabus in background
end
AppDatabase-->>CourseRepository: Syllabus?
CourseRepository-->>syllabusProvider: Syllabus?
syllabusProvider-->>UI: AsyncValue<Syllabus?>
Estimated code review effort🎯 4 (Complex) | ⏱️ ~75 minutes 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
test/services/course_service_test.dart (1)
395-398: ⚡ Quick winExercise non-first syllabus IDs in integration tests.
These assertions always use the first ID, so regressions affecting secondary IDs in multi-syllabus offerings can pass undetected. Randomizing (or iterating) the selected ID improves coverage for this PR’s core behavior.
♻️ Proposed update
- final syllabus = await courseService.getSyllabus( - courseNumber: course.number!, - syllabusId: course.syllabusIds!.first, - ); + final syllabus = await courseService.getSyllabus( + courseNumber: course.number!, + syllabusId: course.syllabusIds!.pickRandom(), + );Also applies to: 453-456, 503-506
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@test/services/course_service_test.dart` around lines 395 - 398, The test always uses the first syllabus ID from course.syllabusIds!.first when calling getSyllabus, which means secondary syllabus IDs are never tested and regressions affecting them can pass undetected. Instead of always selecting the first ID, randomize or iterate through different indices of the course.syllabusIds list to exercise multiple syllabus IDs in the assertions. Apply this change to all three occurrences of the getSyllabus call in the test file (around lines 395-398, 453-456, and 503-506).
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@lib/repositories/course_repository.dart`:
- Around line 757-767: After the refreshSyllabus(id) call succeeds, the code
currently yields the old syllabus stub instead of the refreshed data. To fix
this, fetch the updated syllabus from the repository after the successful
refreshSyllabus(id) call within the try block, and then yield that refreshed
syllabus. Only yield the original syllabus stub in the catch block to maintain
the current error-handling behavior. This ensures the UI receives the latest
data after a successful refresh rather than the stale pre-refresh version.
---
Nitpick comments:
In `@test/services/course_service_test.dart`:
- Around line 395-398: The test always uses the first syllabus ID from
course.syllabusIds!.first when calling getSyllabus, which means secondary
syllabus IDs are never tested and regressions affecting them can pass
undetected. Instead of always selecting the first ID, randomize or iterate
through different indices of the course.syllabusIds list to exercise multiple
syllabus IDs in the assertions. Apply this change to all three occurrences of
the getSyllabus call in the test file (around lines 395-398, 453-456, and
503-506).
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 1327bad3-f0f1-413a-9464-6887e881b801
📒 Files selected for processing (11)
lib/database/actions.dartlib/database/database.dartlib/database/database.g.dartlib/database/schema.dartlib/database/views.dartlib/repositories/course_repository.dartlib/screens/main/course_table/course_table_providers.dartlib/services/course/course_service.dartlib/services/course/mock_course_service.dartlib/services/course/ntut_course_service.darttest/services/course_service_test.dart
💤 Files with no reviewable changes (2)
- lib/database/views.dart
- lib/database/actions.dart
A course offering can have multiple syllabi — one per teacher who submits one. Each 查詢 link in the course list carries the authoring teacher's code, so a syllabus maps to a teacher (built on multiple-teacher support). Add a Syllabuses table (one row per offering+teacher) holding the per-teacher syllabus content, and move the shared header fields off the flat CourseOfferings columns. refreshCourseTable records which syllabi are available as stub rows; their content is fetched lazily and per-teacher via a new watchSyllabus/refreshSyllabus pair (which also writes the offering-level header fields and the instructor email). The course offering detail exposes the available syllabi; the UI shows the first for now.
Pick a random syllabus id from syllabusIds instead of always the first, so secondary syllabus ids are exercised and regressions affecting them are caught.
The stub branch of watchSyllabus awaited refreshSyllabus then yielded the empty stub row, flashing empty content until the DB write's re-emit delivered fresh data — defeating the branch's stated purpose. Re-read the row after a successful refresh and yield that; yield the stub only on failure so the UI still exits its loading state.
315392e to
57ee454
Compare
There was a problem hiding this comment.
🧹 Nitpick comments (1)
lib/database/schema.dart (1)
499-545: 💤 Low valueConsider adding an index on
courseOfferingfor query consistency and performance.Other tables with a
courseOfferingforeign key (Schedules,Materials) declare a@TableIndexon that column. Adding one here would maintain consistency and improve lookup performance when fetching syllabi for an offering.♻️ Suggested change
/// A teacher-authored syllabus for a course offering (教學大綱與進度). ... // Without `@DataClassName`, Drift names the row class 'Syllabuse'. `@DataClassName`('Syllabus') +@TableIndex(name: 'syllabus_course_offering', columns: {`#courseOffering`}) class Syllabuses extends Table with AutoIncrementId, Fetchable {🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@lib/database/schema.dart` around lines 499 - 545, The Syllabuses table is missing a `@TableIndex` annotation on its courseOffering column, whereas other tables with courseOffering foreign keys like Schedules and Materials declare this index. Add a `@TableIndex` annotation on the courseOffering field in the Syllabuses class to maintain consistency with other tables and improve query performance when fetching syllabi for a specific course offering.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@lib/database/schema.dart`:
- Around line 499-545: The Syllabuses table is missing a `@TableIndex` annotation
on its courseOffering column, whereas other tables with courseOffering foreign
keys like Schedules and Materials declare this index. Add a `@TableIndex`
annotation on the courseOffering field in the Syllabuses class to maintain
consistency with other tables and improve query performance when fetching
syllabi for a specific course offering.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 66c33263-1a70-4f29-a845-3411afaa33ce
📒 Files selected for processing (11)
lib/database/actions.dartlib/database/database.dartlib/database/database.g.dartlib/database/schema.dartlib/database/views.dartlib/repositories/course_repository.dartlib/screens/main/course_table/course_table_providers.dartlib/services/course/course_service.dartlib/services/course/mock_course_service.dartlib/services/course/ntut_course_service.darttest/services/course_service_test.dart
💤 Files with no reviewable changes (2)
- lib/database/views.dart
- lib/database/actions.dart
🚧 Files skipped from review as they are similar to previous changes (7)
- lib/database/database.dart
- lib/services/course/course_service.dart
- lib/screens/main/course_table/course_table_providers.dart
- test/services/course_service_test.dart
- lib/repositories/course_repository.dart
- lib/services/course/mock_course_service.dart
- lib/services/course/ntut_course_service.dart
There was a problem hiding this comment.
Pull request overview
Adds support for multiple syllabi per course offering (one per teacher) by normalizing syllabus content into a new Syllabuses table and fetching syllabus content lazily, while keeping shared “header” fields on CourseOfferings. This fits the app’s existing pattern of caching parsed NTUT data in Drift and exposing reactive watchX() streams to the UI.
Changes:
- Replace single
ScheduleDto.syllabusIdwithsyllabusIdsand parse all 查詢 links from the course list. - Add
Syllabusestable and repository flows to reconcile per-offering syllabus stubs and lazilywatchSyllabus/refreshSyllabus. - Update course service integration tests and mock data to reflect multiple available syllabi.
Reviewed changes
Copilot reviewed 10 out of 11 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| test/services/course_service_test.dart | Adjusts integration tests to use syllabusIds and pick an available syllabus ID. |
| lib/services/course/course_service.dart | Updates DTO contract and getSyllabus docs to describe teacher-code-based syllabus IDs. |
| lib/services/course/ntut_course_service.dart | Parses multiple syllabus IDs from the 查詢 column and plumbs them into DTOs. |
| lib/services/course/mock_course_service.dart | Updates mock course table data to provide syllabusIds lists. |
| lib/database/schema.dart | Removes offering-level flat syllabus content columns; adds new Syllabuses table keyed by offering+teacher. |
| lib/database/database.dart | Registers the new Syllabuses table with the Drift database. |
| lib/database/views.dart | Removes syllabus content fields from CourseOfferingOverviews view selection. |
| lib/database/actions.dart | Updates course offering upsert helper to no longer accept/store a single syllabusId. |
| lib/repositories/course_repository.dart | Adds offering-level syllabi composition, stub reconciliation, and lazy watchSyllabus/refreshSyllabus. |
| lib/screens/main/course_table/course_table_providers.dart | Adds syllabusProvider and updates offering provider docs to reflect DB-composed detail + lazy syllabus fetch. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
A syllabus's code is its author's teacher code, so any teacher may have one. Stop enumerating syllabusIds; key a syllabus by (offering, teacher) and fetch it lazily per teacher. - getSyllabus returns null on 尚未登錄 (red marker, distinct from the session 尚未登錄入口網站). - watch/refreshSyllabus take (offeringId, teacherId); rows are upserted on a successful fetch, deleted on 尚未登錄, no stubs pre-created. - Drop OfferingSyllabus/CourseOfferingDetail.syllabi and unused Syllabuses.fetchedAt; the UI drives syllabi off the offering's teachers.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@lib/repositories/course_repository.dart`:
- Around line 696-705: After the `refreshSyllabus` call succeeds (within the try
block), re-fetch the syllabus value from the database before yielding it instead
of yielding the stale `null` value. This ensures that after a successful
refresh, the code queries the updated data from the database and emits that
fresh value, preventing the UI from seeing a stale null state before the
database stream re-emits.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 8d959898-ec95-49bf-9a3a-4ef1f7b70f9f
📒 Files selected for processing (8)
lib/database/database.g.dartlib/database/schema.dartlib/repositories/course_repository.dartlib/screens/main/course_table/course_table_providers.dartlib/services/course/course_service.dartlib/services/course/mock_course_service.dartlib/services/course/ntut_course_service.darttest/services/course_service_test.dart
🚧 Files skipped from review as they are similar to previous changes (1)
- lib/screens/main/course_table/course_table_providers.dart
watchSyllabus re-queries after a successful first fetch so the UI gets the inserted row instead of the stale null snapshot before the stream re-emits. Also reword the doc — it shows the cached row first, so drop the contradictory "rather than caching".
What
A course offering can have multiple syllabi — one per teacher who submits one. A syllabus's code is the authoring teacher's code, so each syllabus maps to a teacher.
Changes
Syllabusestable holding per-teacher syllabus content (one row per offering + teacher); it replaces the offering's flat syllabus columns, while the shared header fields (course type, enrolled, withdrawn) stay onCourseOfferings.watchSyllabus/refreshSyllabuspair keyed by(offeringId, teacherId): a row is created on the first successful fetch and deleted when the teacher hasn't submitted one; the refresh also writes the offering header fields and the instructor email.getSyllabusreturnsnullwhen the teacher hasn't submitted a syllabus (the page shows 尚未登錄, distinct from the session-expiry 尚未登錄入口網站).Testing
flutter analyzeclean; generated Drift code regenerated.Temporary syllabus UI (debug)
Temporary debug UI in
course_table_detail_sheet.dartlisting the offering's teachers, each expandable to lazily fetch and dump its syllabus fields (or 查無大綱 when the teacher hasn't submitted one). Marked// TODO: temporary. Patch:course_table_detail_sheet.dart.patch