-
Notifications
You must be signed in to change notification settings - Fork 25
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
feat: api migration #171
feat: api migration #171
Conversation
…logic in CourseContainerFragment
# Conflicts: # app/src/main/java/org/openedx/app/AppRouter.kt # config.yaml
Thanks for the pull request, @volodymyr-chekyrta! Please note that it may take us up to several weeks or months to complete a review and merge your PR. Feel free to add as much of the following information to the ticket as you can:
All technical communication about the code itself will be done via the GitHub pull request interface. As a reminder, our process documentation is here. Please let us know once your PR is ready for our review and all tests are green. |
I have started its review |
# Conflicts: # course/src/main/java/org/openedx/course/presentation/container/CourseContainerFragment.kt # course/src/main/java/org/openedx/course/presentation/container/CourseContainerViewModel.kt
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
CourseDetailInfo isn't working else only some minor nits.
Suggestion: IMO, For the APIs we can define their versions in Config.yaml
.
@@ -6,19 +6,20 @@ import retrofit2.http.* | |||
|
|||
interface CourseApi { | |||
|
|||
@GET("/mobile_api_extensions/v1/users/{username}/course_enrollments") | |||
@GET("/api/mobile/v3/users/{username}/course_enrollments/") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we can also pass the api_version
in params
i.e,
@GET("/api/mobile/{api_version}/users/{username}/course_enrollments/")
suspend fun getEnrolledCourses(
@Header("Cache-Control") cacheControlHeaderParam: String? = null,
@Path("api_version") apiVersion: String,
@Path("username") username: String,
@Query("org") org: String? = null,
@Query("page") page: Int
): CourseEnrollments
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I believe it's a great suggestion 👍👍👍
It might be beneficial to handle this improvement in a separate pull request to include a user API version to the config as well.
What do you think?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
sure,
@moiz994 can you please create a ticket for these improvements ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No need to create the ticket for now, we will handle it later
core/src/main/java/org/openedx/core/data/model/CourseEnrollments.kt
Outdated
Show resolved
Hide resolved
course/src/main/java/org/openedx/course/data/repository/CourseRepository.kt
Show resolved
Hide resolved
@omerhabib26, thank you for review! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Only a minor suggestion else LGTM 🚀
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
/api/mobile/v3/course_info/
API not working and giving 400
# Conflicts: # course/src/main/res/values-uk/strings.xml # course/src/main/res/values/strings.xml
The API has been changed and PR is now ready for review |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Some APIs are failing in case of course expiry we need to handle them properly
) | ||
} | ||
viewModel.dataReady.observe(viewLifecycleOwner) { isReady -> | ||
if (isReady == true) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This condition doesn't work properly with the expired course and shows an API loading error.
CourseStatusInfo, CourseDates, and Topis API are failing due to course expiry. I guess we can update this change with the following for now. We also need to update the message on NoAccessScreen accordingly. Right now it showing The Course hasn't started yet
for every case.
viewModel.dataReady.observe(viewLifecycleOwner) { coursewareAccess ->
if (coursewareAccess != null && coursewareAccess.hasAccess) {
initViewPager()
} else {
router.navigateToNoAccess(
requireActivity().supportFragmentManager,
courseTitle
)
}
}
@@ -36,19 +35,6 @@ import org.openedx.course.R as courseR | |||
|
|||
class NoAccessCourseContainerFragment : Fragment() { | |||
|
|||
private var courseTitle = "" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I guess we can revert this change and handle it properly
I know about this case. We encountered a similar problem in the iOS PR openedx/openedx-app-ios#213 and decided to address it in a separate PR. |
@volodymyr-chekyrta 🎉 Your pull request was merged! Please take a moment to answer a two question survey so we can improve your experience in the future. |
Migration API endpoints from the RG API plugin to the edx-platform upstream.