Skip to content
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

Merged
merged 11 commits into from
Jan 5, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
10 changes: 2 additions & 8 deletions app/src/main/java/org/openedx/app/AppRouter.kt
Original file line number Diff line number Diff line change
Expand Up @@ -9,7 +9,6 @@ import org.openedx.auth.presentation.restore.RestorePasswordFragment
import org.openedx.auth.presentation.signin.SignInFragment
import org.openedx.auth.presentation.signup.SignUpFragment
import org.openedx.core.FragmentViewType
import org.openedx.core.domain.model.CoursewareAccess
import org.openedx.core.presentation.course.CourseViewMode
import org.openedx.core.presentation.global.app_upgrade.AppUpgradeRouter
import org.openedx.core.presentation.global.app_upgrade.UpgradeRequiredFragment
Expand Down Expand Up @@ -134,14 +133,9 @@ class AppRouter : AuthRouter, DiscoveryRouter, DashboardRouter, CourseRouter, Di

override fun navigateToNoAccess(
fm: FragmentManager,
title: String,
coursewareAccess: CoursewareAccess,
auditAccessExpires: Date?
title: String
) {
replaceFragment(
fm,
NoAccessCourseContainerFragment.newInstance(title, coursewareAccess, auditAccessExpires)
)
replaceFragment(fm, NoAccessCourseContainerFragment.newInstance(title))
}
//endregion

Expand Down
14 changes: 7 additions & 7 deletions core/src/main/java/org/openedx/core/data/api/CourseApi.kt
Original file line number Diff line number Diff line change
Expand Up @@ -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/")
Copy link
Contributor

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

Copy link
Contributor Author

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?

Copy link
Contributor

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 ?

Copy link
Contributor

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

suspend fun getEnrolledCourses(
@Header("Cache-Control") cacheControlHeaderParam: String? = null,
@Path("username") username: String,
@Query("org") org: String? = null,
@Query("page") page: Int
): DashboardCourseList
): CourseEnrollments

@GET("/mobile_api_extensions/courses/v1/courses/")
@GET("/api/courses/v1/courses/")
suspend fun getCourseList(
@Query("search_term") searchQuery: String? = null,
@Query("page") page: Int,
@Query("mobile") mobile: Boolean,
@Query("mobile_search") mobileSearch: Boolean,
@Query("username") username: String? = null,
@Query("org") org: String? = null,
@Query("permissions") permission: List<String> = listOf(
Expand All @@ -28,15 +29,14 @@ interface CourseApi {
)
): CourseList

@GET("/mobile_api_extensions/v1/courses/{course_id}")
@GET("/api/courses/v1/courses/{course_id}")
suspend fun getCourseDetail(
@Path("course_id") courseId: String?,
@Query("username") username: String? = null,
@Query("is_enrolled") isEnrolled: Boolean = true,
@Query("username") username: String? = null
): CourseDetails

@GET(
"/mobile_api_extensions/{api_version}/blocks/?" +
"/api/mobile/{api_version}/course_info/blocks/?" +
omerhabib26 marked this conversation as resolved.
Show resolved Hide resolved
"depth=all&" +
"requested_fields=contains_gated_content,show_gated_sections,special_exam_info,graded,format,student_view_multi_device,due,completion&" +
"student_view_data=video,discussion&" +
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,8 @@
package org.openedx.core.data.model

import com.google.gson.annotations.SerializedName

data class CourseEnrollments(
@SerializedName("enrollments")
val enrollments: DashboardCourseList
omerhabib26 marked this conversation as resolved.
Show resolved Hide resolved
)
Original file line number Diff line number Diff line change
Expand Up @@ -51,7 +51,7 @@ data class CourseStructureModel(
startDisplay = startDisplay ?: "",
startType = startType ?: "",
end = TimeUtils.iso8601ToDate(end ?: ""),
coursewareAccess = coursewareAccess?.mapToDomain()!!,
coursewareAccess = coursewareAccess?.mapToDomain(),
media = media?.mapToDomain(),
certificate = certificate?.mapToDomain(),
isSelfPaced = isSelfPaced ?: false
Expand All @@ -70,7 +70,7 @@ data class CourseStructureModel(
startDisplay = startDisplay ?: "",
startType = startType ?: "",
end = end ?: "",
coursewareAccess = coursewareAccess?.mapToRoomEntity()!!,
coursewareAccess = coursewareAccess?.mapToRoomEntity(),
media = MediaDb.createFrom(media),
certificate = certificate?.mapToRoomEntity(),
isSelfPaced = isSelfPaced ?: false
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -61,7 +61,7 @@ data class EnrolledCourseData(
end = TimeUtils.iso8601ToDate(end ?: ""),
dynamicUpgradeDeadline = dynamicUpgradeDeadline ?: "",
subscriptionId = subscriptionId ?: "",
coursewareAccess = coursewareAccess?.mapToDomain()!!,
coursewareAccess = coursewareAccess?.mapToDomain(),
media = media?.mapToDomain(),
courseImage = courseImage ?: "",
courseAbout = courseAbout ?: "",
Expand All @@ -86,7 +86,7 @@ data class EnrolledCourseData(
end = end ?: "",
dynamicUpgradeDeadline = dynamicUpgradeDeadline ?: "",
subscriptionId = subscriptionId ?: "",
coursewareAccess = coursewareAccess?.mapToRoomEntity()!!,
coursewareAccess = coursewareAccess?.mapToRoomEntity(),
media = MediaDb.createFrom(media),
courseImage = courseImage ?: "",
courseAbout = courseAbout ?: "",
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -33,7 +33,7 @@ data class CourseStructureEntity(
@ColumnInfo("end")
val end: String?,
@Embedded
val coursewareAccess: CoursewareAccessDb,
val coursewareAccess: CoursewareAccessDb?,
@Embedded
val media: MediaDb?,
@Embedded
Expand All @@ -54,7 +54,7 @@ data class CourseStructureEntity(
startDisplay,
startType,
TimeUtils.iso8601ToDate(end ?: ""),
coursewareAccess.mapToDomain(),
coursewareAccess?.mapToDomain(),
media?.mapToDomain(),
certificate?.mapToDomain(),
isSelfPaced
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -61,7 +61,7 @@ data class EnrolledCourseDataDb(
@ColumnInfo("subscriptionId")
val subscriptionId: String,
@Embedded
val coursewareAccess: CoursewareAccessDb,
val coursewareAccess: CoursewareAccessDb?,
@Embedded
val media: MediaDb?,
@ColumnInfo(name = "course_image_link")
Expand Down Expand Up @@ -93,7 +93,7 @@ data class EnrolledCourseDataDb(
TimeUtils.iso8601ToDate(end),
dynamicUpgradeDeadline,
subscriptionId,
coursewareAccess.mapToDomain(),
coursewareAccess?.mapToDomain(),
media?.mapToDomain(),
courseImage,
courseAbout,
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -13,7 +13,7 @@ data class CourseStructure(
val startDisplay: String,
val startType: String,
val end: Date?,
val coursewareAccess: CoursewareAccess,
val coursewareAccess: CoursewareAccess?,
val media: Media?,
val certificate: Certificate?,
val isSelfPaced: Boolean
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -16,7 +16,7 @@ data class EnrolledCourseData(
val end: Date?,
val dynamicUpgradeDeadline: String,
val subscriptionId: String,
val coursewareAccess: CoursewareAccess,
val coursewareAccess: CoursewareAccess?,
val media: Media?,
val courseImage: String,
val courseAbout: String,
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -21,7 +21,7 @@ class CourseRepository(
private var courseStructure: CourseStructure? = null

suspend fun getCourseDetail(id: String): Course {
val course = api.getCourseDetail(id)
val course = api.getCourseDetail(id, preferencesManager.user?.username)
courseDao.updateCourseEntity(CourseEntity.createFrom(course))
return course.mapToDomain()
}
Expand Down Expand Up @@ -51,7 +51,7 @@ class CourseRepository(
suspend fun preloadCourseStructure(courseId: String) {
val response = api.getCourseStructure(
"stale-if-error=0",
"v1",
"v3",
omerhabib26 marked this conversation as resolved.
Show resolved Hide resolved
preferencesManager.user?.username,
courseId
)
Expand Down
Original file line number Diff line number Diff line change
@@ -1,7 +1,6 @@
package org.openedx.course.presentation

import androidx.fragment.app.FragmentManager
import org.openedx.core.domain.model.CoursewareAccess
import org.openedx.core.presentation.course.CourseViewMode
import org.openedx.course.presentation.handouts.HandoutsType
import java.util.Date
Expand All @@ -14,9 +13,7 @@ interface CourseRouter {

fun navigateToNoAccess(
fm: FragmentManager,
title: String,
coursewareAccess: CoursewareAccess,
auditAccessExpires: Date?
title: String
)

fun navigateToCourseSubsections(
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -66,18 +66,14 @@ class CourseContainerFragment : Fragment(R.layout.fragment_course_container) {
}

private fun observe() {
viewModel.dataReady.observe(viewLifecycleOwner) { coursewareAccess ->
if (coursewareAccess != null) {
if (coursewareAccess.hasAccess) {
initViewPager()
} else {
router.navigateToNoAccess(
requireActivity().supportFragmentManager,
courseTitle,
coursewareAccess,
null
)
}
viewModel.dataReady.observe(viewLifecycleOwner) { isReady ->
if (isReady == true) {
Copy link
Contributor

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
                )
            }
        }

initViewPager()
} else {
router.navigateToNoAccess(
requireActivity().supportFragmentManager,
courseTitle
)
}
}
viewModel.errorMessage.observe(viewLifecycleOwner) {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -3,10 +3,10 @@ package org.openedx.course.presentation.container
import androidx.lifecycle.LiveData
import androidx.lifecycle.MutableLiveData
import androidx.lifecycle.viewModelScope
import kotlinx.coroutines.launch
import org.openedx.core.BaseViewModel
import org.openedx.core.R
import org.openedx.core.SingleEventLiveData
import org.openedx.core.domain.model.CoursewareAccess
import org.openedx.core.exception.NoCachedDataException
import org.openedx.core.extension.isInternetError
import org.openedx.core.system.ResourceManager
Expand All @@ -15,6 +15,7 @@ import org.openedx.core.system.notifier.CourseNotifier
import org.openedx.core.system.notifier.CourseStructureUpdated
import org.openedx.course.domain.interactor.CourseInteractor
import org.openedx.course.presentation.CourseAnalytics
import java.util.Date
import kotlinx.coroutines.launch
import org.openedx.core.config.Config
import org.openedx.core.system.notifier.CourseCompletionSet
Expand All @@ -31,8 +32,8 @@ class CourseContainerViewModel(

val isCourseTopTabBarEnabled get() = config.isCourseTopTabBarEnabled()

private val _dataReady = MutableLiveData<CoursewareAccess>()
val dataReady: LiveData<CoursewareAccess>
private val _dataReady = MutableLiveData<Boolean?>()
val dataReady: LiveData<Boolean?>
get() = _dataReady

private val _errorMessage = SingleEventLiveData<String>()
Expand Down Expand Up @@ -70,7 +71,9 @@ class CourseContainerViewModel(
}
val courseStructure = interactor.getCourseStructureFromCache()
courseName = courseStructure.name
_dataReady.value = courseStructure.coursewareAccess
_dataReady.value = courseStructure.start?.let { start ->
start < Date()
}
} catch (e: Exception) {
if (e.isInternetError() || e is NoCachedDataException) {
_errorMessage.value =
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -25,7 +25,6 @@ import androidx.compose.ui.unit.dp
import androidx.compose.ui.zIndex
import androidx.core.os.bundleOf
import androidx.fragment.app.Fragment
import org.openedx.core.domain.model.CoursewareAccess
import org.openedx.core.extension.parcelable
import org.openedx.core.ui.*
import org.openedx.core.ui.theme.OpenEdXTheme
Expand All @@ -36,19 +35,6 @@ import org.openedx.course.R as courseR

class NoAccessCourseContainerFragment : Fragment() {

private var courseTitle = ""
Copy link
Contributor

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

private var coursewareAccess: CoursewareAccess? = null
private var auditAccessExpires: Date? = null

override fun onCreate(savedInstanceState: Bundle?) {
super.onCreate(savedInstanceState)
with(requireArguments()) {
courseTitle = getString(ARG_TITLE, "")
coursewareAccess = parcelable(ARG_COURSEWARE_ACCESS)
auditAccessExpires = parcelable(ARG_AUDIT_ACCESS_EXPIRES)
}
}

override fun onCreateView(
inflater: LayoutInflater,
container: ViewGroup?,
Expand All @@ -58,12 +44,9 @@ class NoAccessCourseContainerFragment : Fragment() {
setContent {
OpenEdXTheme {
val windowSize = rememberWindowSize()
val auditAccessExpired =
auditAccessExpires != null && Date().after(auditAccessExpires)
NoAccessCourseContainerScreen(
windowSize = windowSize,
title = courseTitle,
auditAccessExpired = auditAccessExpired,
title = requireArguments().getString(ARG_TITLE, ""),
onBackClick = {
requireActivity().supportFragmentManager.popBackStack()
}
Expand All @@ -74,19 +57,13 @@ class NoAccessCourseContainerFragment : Fragment() {

companion object {
private const val ARG_TITLE = "title"
private const val ARG_COURSEWARE_ACCESS = "coursewareAccess"
private const val ARG_AUDIT_ACCESS_EXPIRES = "auditAccessExpires"

fun newInstance(
title: String,
coursewareAccess: CoursewareAccess,
auditAccessExpires: Date?
): NoAccessCourseContainerFragment {
val fragment = NoAccessCourseContainerFragment()
fragment.arguments = bundleOf(
ARG_TITLE to title,
ARG_COURSEWARE_ACCESS to coursewareAccess,
ARG_AUDIT_ACCESS_EXPIRES to auditAccessExpires
ARG_TITLE to title
)
return fragment
}
Expand All @@ -99,7 +76,6 @@ class NoAccessCourseContainerFragment : Fragment() {
private fun NoAccessCourseContainerScreen(
windowSize: WindowSize,
title: String,
auditAccessExpired: Boolean,
onBackClick: () -> Unit
) {
val scaffoldState = rememberScaffoldState()
Expand Down Expand Up @@ -165,11 +141,7 @@ private fun NoAccessCourseContainerScreen(
)
Spacer(modifier = Modifier.height(10.dp))
Text(
text = if (auditAccessExpired) {
stringResource(id = courseR.string.course_access_expired)
} else {
stringResource(id = courseR.string.course_not_started)
},
text = stringResource(id = courseR.string.course_not_started),
color = MaterialTheme.appColors.textPrimary,
style = MaterialTheme.appTypography.bodyLarge
)
Expand All @@ -189,8 +161,7 @@ fun NoAccessCourseContainerScreenPreview() {
OpenEdXTheme {
NoAccessCourseContainerScreen(
windowSize = WindowSize(WindowType.Compact, WindowType.Compact),
title = "Example title",
auditAccessExpired = false,
title = "Course title",
onBackClick = {}
)
}
Expand Down
2 changes: 1 addition & 1 deletion course/src/main/res/values-uk/strings.xml
Original file line number Diff line number Diff line change
Expand Up @@ -33,7 +33,7 @@
<string name="course_back_to_outline">Повернутись до модуля</string>
<string name="course_you_cant_enroll">Ви не можете записатися на цей курс, оскільки термін запису вже минув.</string>
<string name="course_not_started">Цей курс ще не розпочався.</string>
<string name="course_access_expired">Ваш доступ до цього курсу закінчився.</string>
<string name="course_not_connected_to_internet">Ви не підключені до Інтернету. Будь ласка, перевірте ваше підключення до Інтернету.</string>
<string name="course_navigation_course">Курс</string>
<string name="course_navigation_video">Відео</string>
<string name="course_navigation_discussion">Обговорення</string>
Expand Down
2 changes: 1 addition & 1 deletion course/src/main/res/values/strings.xml
Original file line number Diff line number Diff line change
Expand Up @@ -33,7 +33,7 @@
<string name="course_next_section">Next section</string>
<string name="course_you_cant_enroll">You cannot enroll in this course because the enrollment date is over.</string>
<string name="course_not_started">This course hasn’t started yet.</string>
<string name="course_access_expired">Your access to this course has expired.</string>
<string name="course_not_connected_to_internet">You are not connected to the Internet. Please check your Internet connection.</string>
<string name="course_navigation_course">Course</string>
<string name="course_navigation_video">Video</string>
<string name="course_navigation_discussion">Discussion</string>
Expand Down
Loading
Loading