Skip to content

Conversation

@suhaibabsi-inst
Copy link
Contributor

@suhaibabsi-inst suhaibabsi-inst commented Dec 14, 2025

refs: MBL-19512
affects: Student
builds: Student
release note: Fixed course sync failure in certain cases.

This an alternative approach on fixing the issue of course sync for accounts with limited rate, failures of rate limit exceeded was addressed in previous PR, but this one is to continue the process of sync in case of accidental failure while downloading an image or file referenced in pages.

Test Plan

Use account mentioned in ticket to test course sync and offline mode. On offline mode, the course modules should load correctly and studio video should also be playing.

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

refs: MBL-19512
affects: Student
builds: Student
release note: Fixed course sync failure in certain cases.
@claude
Copy link

claude bot commented Dec 14, 2025

Claude Code Review

Updated: 2025-12-14

Critical Issue

  • Silent data loss on download failures (lines 98-100): .replaceError(with: nil) + .compactMap() silently drops any failed file/image downloads from the replacement map. Offline content will be incomplete without any logging, error handling, or fallback to original URLs. Users won't know which assets failed to sync.

✅ Approved - PR achieves the stated goal of bypassing sync failures to allow sync to complete. However, consider adding logging for observability when assets fail to download for offline mode.

@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 course sync failure in certain cases.

Affected Apps: Student

Builds: Student

MBL-19512

Coverage New % Master % Delta
Canvas iOS 91% 81.06% 9.94%
Core/Core/Common/Extensions/UIKit/UIScreenExtensions.swift 0% -- --
Horizon/Horizon/Sources/Common/View/CourseDetailsAccessibility.swift 0% -- --
Student/Student/WidgetRouting/WidgetRouter+GradeListWidget.swift 0% 0% 0%
Student/Student/Assignments/AssignmentDetails/SubAssignmentsCard/ViewModel/StudentSubAssignmentsCardItem.swift 0% 100% -100%

Generated by 🚫 dangerJS against 459fc2b

@inst-danger
Copy link
Contributor

inst-danger commented Dec 14, 2025

Builds

Commit: Merge branch 'chore/MBL-19602-API-RateLimitExceeded-Retrial-Logic-Revised' into bugfix/MBL-19512-Course-Sync-Issue-CanineBehaviorCollege (459fc2b)
Build Number: 1003
Built At: Dec 14 20:13 CET (12/14 12:13 PM MST)

Student

…ised' into bugfix/MBL-19512-Course-Sync-Issue-CanineBehaviorCollege
@claude
Copy link

claude bot commented Dec 14, 2025

Claude Code Review

Critical Issues

  • Silent failure on file/image downloads: Changed to .replaceError(with: nil) + .compactMap, so failed downloads are silently dropped. Offline content will be incomplete if downloads fail, with no logging or visibility.

  • Inconsistent error handling: downloadAttachment() still returns AnyPublisher<String, Error>, but parse() now silently succeeds. This behavioral inconsistency could cause issues.

  • Data loss without observability: Failed downloads filtered out with no fallback or logging. Users get incomplete offline content with no indication of what failed.

✅ Approved - achieves goal of bypassing sync failures, but consider adding logging for observability of lost assets.

@suhaibabsi-inst suhaibabsi-inst marked this pull request as ready for review December 14, 2025 17:35
…ised' into bugfix/MBL-19512-Course-Sync-Issue-CanineBehaviorCollege
}
.replaceError(with: nil)
}
.compactMap({ $0 })
Copy link
Contributor

@petkybenedek petkybenedek Dec 18, 2025

Choose a reason for hiding this comment

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

.compactMap { $0 }
For consistency, if there's no specific reason for the parenthesis

}
.replaceError(with: nil)
}
.compactMap({ $0 })
Copy link
Contributor

Choose a reason for hiding this comment

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

.compactMap { $0 }

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

@vargaat vargaat closed this in #3809 Jan 5, 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.

4 participants