feat: Alert for outdated course calendars on the course home screen #416
Conversation
shafqat-muneer
requested review from
saeedbashir,
volodymyr-chekyrta and
forgotvas
| @@ -140,6 +143,9 @@ public class CourseContainerViewModel: BaseCourseViewModel { | |||
| do { | |||
| if isInternetAvaliable { | |||
| courseStructure = try await interactor.getCourseBlocks(courseID: courseID) | |||
| Task { | |||
There was a problem hiding this comment.
getCourseBlocks is async function, why are you using Task there? Don't we need to wait for dates before will set isShowProgress and isShowRefresh to false?
There was a problem hiding this comment.
On the course home screen, we are executing the getCourseDates API call solely to determine whether to display an outdated calendar alert or not. There is no UI population involving dates at this stage. Hence, we can optimize the loading time of this screen by running the getCourseDates API call on a separate thread. Once we receive the response regarding the dates, we can then decide whether to present the outdated alert or not.
Additionally, it's essential to populate getCourseBlocks before invoking getCourseDates because the getCourseDates method involves offline population of courseStructure.
|
@volodymyr-chekyrta @saeedbashir Kingly review the changes made in this pull request? Thank you. |
@shafqat-muneer I'm not sure how but all of the old simulator environments are deleted from my machine. I'm downloading the simulator environment and will review when it's done. |
Same with me |
| Task { | ||
| await courseDatesViewModel.getCourseDates(courseID: courseID) | ||
| } |
There was a problem hiding this comment.
@shafqat-muneer, I don't know if this can be tested. We can't check what's going on inside the Task.
And this dependency on CourseDatesViewViewModel 🤔.
But I can't offer a good solution right now. Only we could send notification to CourseDatesViewModel instead of calling it directly.
What do you think?
In any case, the calendar flow will be heavily modified in FC-0047, so we can probably make do with this approach for now.
OpenEdX/DI/ScreenAssembly.swift
Outdated
| courseDatesViewModel: CourseDatesViewModel( | ||
| interactor: r.resolve(CourseInteractorProtocol.self)!, | ||
| router: r.resolve(CourseRouter.self)!, | ||
| cssInjector: r.resolve(CSSInjector.self)!, | ||
| connectivity: r.resolve(ConnectivityProtocol.self)!, | ||
| config: r.resolve(ConfigProtocol.self)!, | ||
| courseID: courseID, | ||
| courseName: courseName, | ||
| analytics: r.resolve(CourseAnalytics.self)! | ||
| ) |
There was a problem hiding this comment.
Why do we create CourseDatesViewModel manually instead of injecting it via r.resole(...)?
And with this approach, we will have 2 different instances of CourseDatesViewModel for CourseHome. Is this ok?
There was a problem hiding this comment.
Maybe we can pass here the previously injected VM?
|
@volodymyr-chekyrta I've restructured it by using notifications approach 🎉 |
LEARNER-9955: iOS - Alert for outdated course calendars on the course home screen
We need to display an alert for outdated course calendars on the course home screen. Currently, on the iOS application, this alert is shown within the dates tabs only.
Demo:
Simulator.Screen.Recording.-.iPhone.15.-.2024-04-26.at.09.16.16.mp4