[MBL-19421][Parent] Filter out concluded courses in Inbox course picker

[hermannakos]

Fixed an issue where soft concluded courses (courses concluded by term dates) were showing up for observers when composing a message in the Parent Inbox.

Problem

The Canvas API returns enrollment_state: "active" for soft concluded courses, so the existing enrollment state filtering was insufficient. Soft concluded courses are those that have ended based on term/course dates but were not manually concluded.

Solution

1. Modified CourseAPI.getCoursesByEnrollmentType() to include include[]=term parameter to fetch term information
2. Added filtering in ParentInboxCoursePickerViewModel using Course.isPastEnrolment() to exclude:
   • Hard concluded courses (workflowState == COMPLETED)
   • Soft concluded courses with past term end dates
   • Soft concluded courses with past course end dates
3. Added comprehensive test coverage for all concluded course scenarios

Test plan:

• ✅ Manually tested with test account (inboxobs on awarenski.instructure.com) that has multiple concluded enrollments
• ✅ Verified only active courses appear in the course picker when composing a message
• ✅ All unit tests pass (6/6 tests in ParentInboxCoursePickerViewModelTest)
• ✅ Tested new filtering logic with courses concluded by term dates
• ✅ Tested new filtering logic with courses concluded by course dates
• ✅ Tested new filtering logic with hard concluded courses

refs: MBL-19421
affects: Parent
release note: Fixed issue where concluded courses appeared when composing messages in Parent Inbox

Checklist

• Follow-up e2e test ticket created or not needed
• Tested in dark mode
• Tested in light mode
• A11y checked
• Approve from product

🤖 Generated with Claude Code

[claude]

Code Review

I've reviewed PR #3338 and have the following feedback:

✅ Strengths

1. Well-Designed Solution

• The approach correctly addresses the root cause: Canvas API returns enrollment_state: "active" for soft concluded courses
• Adding include[]=term to the API call (libs/canvas-api-2/src/main/java/com/instructure/canvasapi2/apis/CourseAPI.kt:123) is the right way to fetch necessary term data
• Reuses existing Course.isPastEnrolment() method which already handles all edge cases (hard concluded, soft concluded by term, soft concluded by course dates, section overrides)

2. Comprehensive Test Coverage

• Excellent test suite with 6 test scenarios covering all enrollment states and conclusion types
• Tests verify filtering of: completed/inactive enrollments, hard concluded courses, soft concluded by term dates, soft concluded by course dates
• Tests also verify that active/invited/creation_pending enrollments are properly included
• All tests follow existing project patterns using Mockk and Kotlin coroutines test utilities

3. Clean Code Architecture

• Changes follow MVVM pattern and existing repository patterns
• Filtering logic is clear with two-stage approach: enrollment state filter + course date filter
• Maintains consistency with project's Kotlin idioms and functional programming style

🔍 Code Quality Observations

ParentInboxCoursePickerViewModel.kt:69-76

The two-stage filtering approach is clean and readable:

val studentContextItems = enrollments
    .filter { it.enrollmentState in listOf("active", "invited", "creation_pending") }
    .mapNotNull { enrollment -> ... }
    .filter { !it.course.isPastEnrolment() }

Minor Consideration: The enrollment state strings are hardcoded. While this is consistent with the existing codebase pattern, consider if these should be constants in the Enrollment model for maintainability. However, this is outside the scope of this PR.

🎯 Potential Issues

None identified. The implementation is solid.

⚡ Performance Considerations

Positive:

• The filtering operations are efficient O(n) operations
• The API change adding include[]=term is necessary and shouldn't cause significant performance impact
• Using distinct() at the end prevents duplicate entries efficiently

🔒 Security Concerns

None. The changes don't introduce security vulnerabilities:

• No user input validation needed (internal filtering only)
• Properly uses existing repository pattern
• No changes to authentication or authorization logic

📝 Suggestions

1. API Documentation: Consider adding a comment in CourseAPI.kt:123 explaining why include[]=term is needed (to support filtering soft concluded courses), similar to the existing TODO comment on line 126.

2. Test Data Clarity: In the test cases, the date strings like "2024-01-01T00:00:00Z" and "2026-12-31T23:59:59Z" are clear, but could benefit from named constants like PAST_DATE and FUTURE_DATE for better readability. However, this is a minor style preference.

3. Edge Case Testing: Consider adding a test for courses with null term data to ensure graceful handling, though Course.isPastEnrolment() likely handles this already.

✅ Final Assessment

This is a high-quality PR that:

• ✅ Solves the problem correctly with a well-reasoned approach
• ✅ Has excellent test coverage (6 comprehensive test cases)
• ✅ Follows project conventions and architectural patterns
• ✅ Includes clear documentation in the PR description
• ✅ Has no security or performance concerns
• ✅ Makes minimal, targeted changes (130 additions, 7 deletions across 3 files)

Recommendation: Approve pending the minor checklist items (A11y check, product approval) which are noted in the PR description.

Great work on the thorough testing and clear problem/solution documentation! 🎉

[inst-danger]

Parent Install Page

[inst-danger]

Teacher Install Page

[github-actions]

📊 Code Coverage Report

✅ Student

• PR Coverage: 44.08%
• Master Coverage: 44.08%
• Delta: +0.00%

✅ Teacher

• PR Coverage: 24.84%
• Master Coverage: 24.84%
• Delta: +0.00%

✅ Pandautils

• PR Coverage: 22.52%
• Master Coverage: 22.52%
• Delta: +0.00%

---

📈 Overall Average

• PR Coverage: 30.48%
• Master Coverage: 30.48%
• Delta: +0.00%

[inst-danger]

Student Install Page

[kdeakinstructure]

QA 👍