Skip to content

refactor: remove deprecated course duration/lessons fields and simplify mappings - #263

Merged
syed-tp merged 4 commits into
mainfrom
ref/dto-and-companion-value-wrapping
Aug 20, 2026
Merged

refactor: remove deprecated course duration/lessons fields and simplify mappings#263
syed-tp merged 4 commits into
mainfrom
ref/dto-and-companion-value-wrapping

Conversation

@syed-tp

@syed-tp syed-tp commented Aug 20, 2026

Copy link
Copy Markdown
Collaborator

Why

  • Deprecated totalDuration and totalLessons fields were redundant since totalContents maps the same data.
  • The app is unreleased, so we can clean up database columns directly without migrations.
  • Companion mapping checks for 30+ nullable fields were overly verbose.

What

  • Removed totalDuration and totalLessons from CourseDto, CoursesTable, and mapping helpers.
  • Regenerated drift app_database.g.dart file.
  • Simplified lesson companion mappings using Drift's Value.absentIfNull.
  • Updated UI references, mocks, and test assertions to use totalContents.

@claude

claude Bot commented Aug 20, 2026

Copy link
Copy Markdown

Claude finished @syed-tp's task in 1m 40s —— View job


PR Review Summary

Todo list

  • Read AGENTS.md and core docs (ai_context.md, architecture.md)
  • Check for relevant ADRs (none needed — pure refactor, no boundary/pattern changes)
  • Gather PR diff (git diff origin/main...HEAD)
  • Review OpenSpec compliance
  • Review SDK boundaries, design system, a11y, motion, navigation, providers, data layer, generated files, etc.
  • Post final structured review

✅ What's good

  • OpenSpec change (refactor-dto-and-companion-value-wrapping) is present with skip_specs: true correctly set and a clear proposal/design/tasks — good practice even though this refactor is technically exempt.
  • app_database.g.dart regeneration is fully consistent with the courses_table.dart source edit (all totalDuration/totalLessons column plumbing removed together) — no hand-edited generated code detected.
  • Value.absentIfNull(...) simplification in course_repository.dart's _lessonDtoToCompanion is behaviorally equivalent to the removed 30+ line ternary block, including the examMetadataJson case.
  • schemaVersion correctly left at 1, consistent with the "app unreleased, no migration needed" rationale.
  • UI/test call sites (info_page.dart, course_card.dart, custom_exam_course_selection_screen.dart, test fixtures) were all updated to use totalContents instead of the removed totalLessons, and no test assertions depend on the removed fields.

🔴 CRITICAL — Must fix before merge

  1. Unrelated/unexplained mock data bug in packages/core/lib/data/sources/mock_data_source.dart:88-89. The first course's id/title were changed from 'jee-main-2026' / 'JEE Main 2026' to 'physics-jee-main' / 'Physics JEE Main', but every other reference to this course in the same file still uses the old id:

    • getChapters() switch statement (line 191): case 'jee-main-2026': return _jeeMainChapters();
    • Lesson/chapter courseId: fields (lines 300, 310, 318, 326, 1046, 1053, 1060, 1067) still hardcode 'jee-main-2026'.

    Since the course list entry no longer has id jee-main-2026, getChapters('physics-jee-main') now falls through to the default: return [] branch — this course will show no chapters/lessons in mock mode. This change is out of scope for a "remove totalDuration/totalLessons" refactor and looks like an accidental edit. Please revert the id/title back to 'jee-main-2026' / 'JEE Main 2026' (or update all the other references consistently if the rename was intentional).
    Fix this →

🟡 WARNING — Should fix

None identified — SDK boundaries, design-system usage, generated-file hygiene, and localization were unaffected by this diff (no new widgets, providers, or imports introduced).

🔵 SUGGESTION — Nice to have

  1. packages/profile/lib/providers/profile_providers.dart:13 — the .map<CourseDto>(...) explicit type argument is a minor no-op addition (type was already inferable); harmless but not necessary for this refactor's stated scope.

Verdict

CHANGES REQUESTED — the mock data id/title mismatch (#1 above) needs to be fixed or reverted before merge; everything else in this refactor is clean.

Note: flutter/dart tooling was not available in this sandbox, so dart analyze / flutter test could not be run directly — review was done via manual diff inspection and static tracing.

@syed-tp

syed-tp commented Aug 20, 2026

Copy link
Copy Markdown
Collaborator Author

@claude review

@claude

claude Bot commented Aug 20, 2026

Copy link
Copy Markdown

Claude finished @syed-tp's task in 2m 23s —— View job


PR Review Summary

Todo list

  • Read AGENTS.md and core docs (ai_context.md, architecture.md)
  • Check for relevant ADRs (none needed — pure refactor, no boundary/pattern changes)
  • Gather PR diff (git diff origin/main HEAD)
  • Re-check the previously flagged mock_data_source.dart id/title mismatch
  • Review OpenSpec compliance, SDK boundaries, design system, a11y, generated files, etc.
  • Post final structured review

✅ What's good

  • The previously flagged CRITICAL mock-data bug (course id/title mismatch causing getChapters('physics-jee-main') to return []) is now resolvedid/title at mock_data_source.dart:88-89 are back to 'jee-main-2026' / 'JEE Main 2026' and consistent with the case 'jee-main-2026': switch and all courseId: references throughout the file.
  • app_database.g.dart regeneration is fully consistent with courses_table.dart — all totalDuration/totalLessons column plumbing (verification metas, column list, mapping, insertable) removed together, no hand-edited generated code detected.
  • Value.absentIfNull(...) in course_repository.dart's _lessonDtoToCompanion is a valid Drift 2.x API (compatible with the pinned drift: ^2.21.0) and is behaviorally equivalent to the removed 30+ line ternary block, including the examMetadataJson case (short-circuits before calling jsonEncode, so no null-deref risk).
  • schemaVersion correctly left at 1, consistent with the "app unreleased, no migration needed" rationale.
  • No import changes anywhere in the diff — SDK package boundaries are unaffected.
  • All UI/test call sites (info_page.dart, course_card.dart, custom_exam_course_selection_screen.dart, certificates_provider.dart, profile_providers.dart, mocks, test fixtures) consistently migrated to totalContents; a repo-wide grep confirms no dangling references to CourseDto.totalLessons/totalDuration remain (the remaining totalLessons/totalDuration hits belong to unrelated LessonDto/DashboardDto fields, correctly left untouched).
  • OpenSpec change (refactor-dto-and-companion-value-wrapping) is present with skip_specs: true and a clear proposal/design/tasks doc — good practice even though this refactor is technically exempt.

🔴 CRITICAL — Must fix before merge

None.

🟡 WARNING — Should fix

None — SDK boundaries, design-system usage, generated-file hygiene, and localization were unaffected by this diff (no new widgets, providers, or imports introduced).

🔵 SUGGESTION — Nice to have

  1. Commit messages for 291deb2 and 1541b80 say "add lmsdemo configuration" / "add lmsdemo configuration file", but their actual diffs only touch profile_providers.dart/profile_providers.g.dart (course-mapping simplification and hash regen) — no lmsdemo config file is added in either commit. Not blocking, but worth tidying up commit messages so git log/git blame stay trustworthy for future readers.
  2. The branch's openspec/changes/ diff currently shows the fix-course-detail-sync-and-search-state change "un-archiving" (moving out of archive/) relative to origin/main — this is just because the branch was cut before main's later archival commit (07bc093) and isn't something this PR's commits actually did. It should resolve itself on merge/rebase; flagging only so it isn't mistaken for an intentional revert during review.

Verdict

APPROVED — the refactor is clean, consistent, and the previously-blocking mock-data bug has been fixed. Only minor commit-message hygiene notes remain, which are non-blocking.

Note: flutter/dart tooling was not available in this sandbox, so dart analyze / flutter test could not be run directly — review was done via manual diff inspection and static tracing.

@syed-tp
syed-tp merged commit d47b649 into main Aug 20, 2026
6 checks passed
@syed-tp
syed-tp deleted the ref/dto-and-companion-value-wrapping branch August 20, 2026 07:26
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.

1 participant