Skip to content

Conversation

@suhaibabsi-inst
Copy link
Contributor

refs: MBL-19555
affects: Student, Teacher
builds: Student, Teacher
release note: Fixed compose message issue of un-selectable active courses when term ends before course end date

Test Plan

See ticket's description learn about steps to reproduce.

Checklist

  • Follow-up e2e test ticket created
  • A11y checked
  • Tested on phone
  • Tested on tablet
  • Tested in dark mode
  • Tested in light mode
  • Approve from product

…nds before course end date

refs: MBL-19555
affects: Student, Teacher
builds: Student, Teacher
release note: Fixed compose message issue of un-selectable active courses when term ends before course end date
@suhaibabsi-inst suhaibabsi-inst self-assigned this Dec 14, 2025
@claude
Copy link

claude bot commented Dec 14, 2025

Claude Code Review

Updated: 2025-12-14

Approved

Summary: Logic change correctly fixes the course selection issue by prioritizing course end_at over term end_at. The new flow logic is clearer and maintains backward compatibility for courses without their own end date.

@inst-danger
Copy link
Contributor

inst-danger commented Dec 14, 2025

Warnings
⚠️ One or more files are below the minimum test coverage 50%

Release Note:

Fixed compose message issue of un-selectable active courses when term ends before course end date

Affected Apps: Student, Teacher

Builds: Student, Teacher

MBL-19555

Coverage New % Master % Delta
Canvas iOS 91.11% 81.06% 10.04%
Horizon/Horizon/Sources/Common/View/CourseDetailsAccessibility.swift 0% -- --

Generated by 🚫 dangerJS against 7fbbcd1

@inst-danger
Copy link
Contributor

inst-danger commented Dec 14, 2025

Builds

Commit: Unit tests (7fbbcd1)
Build Number: 1048
Built At: Dec 21 12:38 CET (12/21 04:38 AM MST)

Student
Teacher

@suhaibabsi-inst suhaibabsi-inst marked this pull request as ready for review December 14, 2025 17:33
Copy link
Contributor

@rh12 rh12 left a comment

Choose a reason for hiding this comment

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

Please add unit test for this

@rh12 rh12 requested a review from petkybenedek December 18, 2025 15:13
petkybenedek
petkybenedek previously approved these changes Dec 18, 2025
Copy link
Contributor

@petkybenedek petkybenedek left a comment

Choose a reason for hiding this comment

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

QA +1
Had to change the term end date in the provided account to reproduce the issue

@claude
Copy link

claude bot commented Dec 21, 2025

Claude Code Review

Updated: 2025-12-21

Approved

  • Correct logic: prioritizes course end_at over term end_at as intended
  • No bugs or crashes identified
  • Proper null handling with optional chaining and fallback to .distantFuture
  • Good test coverage: 4 comprehensive test functions covering all scenarios
  • Backward compatible: courses without end date fall back to term end date

Copy link
Contributor

@petkybenedek petkybenedek left a comment

Choose a reason for hiding this comment

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

QA +1

Comment on lines +131 to +135
if let endDate = item.end_at {
return endDate < Clock.now
} else {
return (item.term?.end_at ?? .distantFuture) < Clock.now
}
Copy link
Collaborator

Choose a reason for hiding this comment

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

Maybe the format below would be more straightforward then the if-else one.

Suggested change
if let endDate = item.end_at {
return endDate < Clock.now
} else {
return (item.term?.end_at ?? .distantFuture) < Clock.now
}
return (item.end_at ?? item.term?.end_at ?? .distantFuture) < Clock.now

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.

6 participants