Skip to content
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
Original file line number Diff line number Diff line change
Expand Up @@ -36,6 +36,7 @@ import com.instructure.canvasapi2.utils.PendoInitCallbackHandler
import com.instructure.canvasapi2.utils.weave.apiAsync
import com.instructure.canvasapi2.utils.weave.catch
import com.instructure.canvasapi2.utils.weave.tryWeave
import com.instructure.horizon.HorizonActivity
import com.instructure.loginapi.login.tasks.LogoutTask
import com.instructure.loginapi.login.util.QRLogin.performSSOLogin
import com.instructure.loginapi.login.util.QRLogin.verifySSOLoginUri
Expand All @@ -46,6 +47,7 @@ import com.instructure.pandautils.typeface.TypefaceBehavior
import com.instructure.pandautils.utils.Const
import com.instructure.pandautils.utils.FeatureFlagProvider
import com.instructure.pandautils.utils.Utils.generateUserAgent
import com.instructure.pandautils.utils.orDefault
import com.instructure.student.R
import com.instructure.student.databinding.InterwebsToApplicationBinding
import com.instructure.student.databinding.LoadingCanvasViewBinding
Expand Down Expand Up @@ -209,9 +211,15 @@ class InterwebsToApplication : BaseCanvasActivity() {
finish()
return@tryWeave
} else {
// Allow the UI to show
delay(700)
RouteMatcher.routeUrl(this@InterwebsToApplication, url, domain)
if (ApiPrefs.canvasCareerView.orDefault()) {
val intent = Intent(this@InterwebsToApplication, HorizonActivity::class.java)
intent.data = Uri.parse(url)
intent.addFlags(Intent.FLAG_ACTIVITY_CLEAR_TOP or Intent.FLAG_ACTIVITY_NEW_TASK)
startActivity(intent)
} else {
RouteMatcher.routeUrl(this@InterwebsToApplication, url, domain)
Copy link

Choose a reason for hiding this comment

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

Question: Route handling for Horizon feature

This adds routing logic to direct users to HorizonActivity when the career view flag is enabled.

Please ensure:

  1. This behavior is thoroughly tested with the flag both enabled and disabled
  2. The route params (ROUTE_HORIZON_CAREER_COURSE_ID, etc.) are properly validated
  3. Consider what happens if ApiPrefs.canvasCareerView changes state while the app is running

}
finish()
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -33,6 +33,19 @@ class FakeGetHorizonCourseManager(): HorizonGetCoursesManager {
return DataResult.Success(getCourses())
}

override suspend fun getCourseWithProgressById(
courseId: Long,
userId: Long,
forceNetwork: Boolean
): DataResult<CourseWithProgress> {
val course = getCourses().find { it.courseId == courseId }
return if (course != null) {
DataResult.Success(course)
} else {
DataResult.Fail()
}
}

override suspend fun getEnrollments(
userId: Long,
forceNetwork: Boolean
Expand Down Expand Up @@ -84,6 +97,9 @@ class FakeGetHorizonCourseManager(): HorizonGetCoursesManager {
CourseWithModuleItemDurations(
courseId = courseId,
courseName = "Program Course",
moduleItemsDuration = emptyList(),
startDate = null,
endDate = null
)
)
}
Expand All @@ -94,6 +110,7 @@ class FakeGetHorizonCourseManager(): HorizonGetCoursesManager {
CourseWithProgress(
courseId = courses[0].id,
courseName = courses[0].name,
courseImageUrl = null,
courseSyllabus = "Syllabus for Course 1",
progress = 0.25
)
Expand All @@ -102,6 +119,7 @@ class FakeGetHorizonCourseManager(): HorizonGetCoursesManager {
CourseWithProgress(
courseId = courses[1].id,
courseName = courses[1].name,
courseImageUrl = null,
courseSyllabus = "Syllabus for Course 2",
progress = 1.0
)
Expand All @@ -110,6 +128,7 @@ class FakeGetHorizonCourseManager(): HorizonGetCoursesManager {
CourseWithProgress(
courseId = courses[2].id,
courseName = courses[2].name,
courseImageUrl = null,
courseSyllabus = null,
progress = 0.0
)
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,19 @@
query HorizonGetCourseById($courseId: ID!, $userId: ID!) {
legacyNode(_id: $courseId, type: Course) {
... on Course {
id: _id
name
image_download_url: imageUrl
syllabus_body: syllabusBody
usersConnection(filter: {userIds: [$userId]}) {
nodes {
courseProgression {
requirements {
completionPercentage
}
}
}
}
}
}
}
Copy link

Choose a reason for hiding this comment

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

Style: Missing newline at end of file

This file should end with a newline character per standard conventions.

Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,7 @@ package com.instructure.canvasapi2.managers.graphql.horizon
import com.apollographql.apollo.ApolloClient
import com.apollographql.apollo.api.Optional
import com.instructure.canvasapi2.GetCoursesQuery
import com.instructure.canvasapi2.HorizonGetCourseByIdQuery
import com.instructure.canvasapi2.HorizonGetProgramCourseByIdQuery
import com.instructure.canvasapi2.QLClientConfig
import com.instructure.canvasapi2.enqueueQuery
Expand All @@ -29,6 +30,8 @@ import java.util.Date
interface HorizonGetCoursesManager {
suspend fun getCoursesWithProgress(userId: Long, forceNetwork: Boolean = false): DataResult<List<CourseWithProgress>>

suspend fun getCourseWithProgressById(courseId: Long, userId: Long, forceNetwork: Boolean = false): DataResult<CourseWithProgress>

suspend fun getEnrollments(userId: Long, forceNetwork: Boolean = false): DataResult<List<GetCoursesQuery.Enrollment>>

suspend fun getProgramCourses(courseId: Long, forceNetwork: Boolean = false): DataResult<CourseWithModuleItemDurations>
Expand All @@ -50,14 +53,38 @@ class HorizonGetCoursesManagerImpl(private val apolloClient: ApolloClient): Hori
}
}

override suspend fun getCourseWithProgressById(courseId: Long, userId: Long, forceNetwork: Boolean): DataResult<CourseWithProgress> {
return try {
val query = HorizonGetCourseByIdQuery(courseId.toString(), userId.toString())
val result = apolloClient.enqueueQuery(query, forceNetwork).dataAssertNoErrors

val progress = result.legacyNode?.onCourse?.usersConnection?.nodes?.firstOrNull()?.courseProgression?.requirements?.completionPercentage ?: 0.0
val courseId = result.legacyNode?.onCourse?.id?.toLongOrNull() ?: -1L
val courseName = result.legacyNode?.onCourse?.name ?: ""
val courseSyllabus = result.legacyNode?.onCourse?.syllabus_body ?: ""
val imageUrl = result.legacyNode?.onCourse?.image_download_url ?: ""
val course = CourseWithProgress(courseId, courseName, imageUrl, courseSyllabus, progress)

return DataResult.Success(course)
} catch (e: Exception) {
DataResult.Fail(Failure.Exception(e))
}
Copy link

Choose a reason for hiding this comment

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

Potential bug: Returning success with invalid data when course not found

The current implementation uses -1L as a fallback for courseId and empty strings for other fields, then returns DataResult.Success even when the course doesn't exist. This could lead to confusing behavior for callers.

Consider checking if the course data is valid and returning a failure result when not found:

val course = result.legacyNode?.onCourse
if (course?.id == null) {
    return DataResult.Fail()
}

val courseId = course.id.toLongOrNull() ?: return DataResult.Fail()
val courseName = course.name ?: ""
val courseImageUrl = course.courseImage?.url ?: ""

CourseWithProgress(
    courseId = courseId,
    courseName = courseName,
    courseCode = course.courseCode ?: "",
    courseImageUrl = courseImageUrl,
    courseProgress = result.progress?.let {
        CourseProgress(
            requirementCount = it.requirementCount ?: 0,
            requirementCompletedCount = it.requirementCompletedCount ?: 0,
            nextRequirementUrl = it.nextRequirement?.htmlUrl ?: "",
            completedAt = it.completedAt
        )
    }
)

}

private fun mapCourse(course: GetCoursesQuery.Course?): CourseWithProgress? {
val progress = course?.usersConnection?.nodes?.firstOrNull()?.courseProgression?.requirements?.completionPercentage ?: 0.0
val courseId = course?.id?.toLong()
val courseName = course?.name
val courseSyllabus = course?.syllabus_body
val imageUrl = course?.image_download_url

return if (courseId != null && courseName != null) {
CourseWithProgress(courseId, courseName, courseSyllabus, progress)
CourseWithProgress(
courseId,
courseName,
imageUrl,
courseSyllabus,
progress)
} else {
null
}
Expand Down Expand Up @@ -113,7 +140,8 @@ class HorizonGetCoursesManagerImpl(private val apolloClient: ApolloClient): Hori
data class CourseWithProgress(
val courseId: Long,
val courseName: String,
val courseSyllabus: String? = null,
val courseImageUrl: String?,
val courseSyllabus: String?,
val progress: Double,
)

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -24,7 +24,6 @@ import androidx.navigation.compose.rememberNavController
import androidx.test.ext.junit.runners.AndroidJUnit4
import com.instructure.horizon.R
import com.instructure.horizon.features.dashboard.DashboardItemState
import com.instructure.horizon.features.dashboard.widget.DashboardPaginatedWidgetCardButtonRoute
import com.instructure.horizon.features.dashboard.widget.DashboardPaginatedWidgetCardHeaderState
import com.instructure.horizon.features.dashboard.widget.DashboardPaginatedWidgetCardItemState
import com.instructure.horizon.features.dashboard.widget.DashboardPaginatedWidgetCardState
Expand Down Expand Up @@ -59,7 +58,7 @@ class HorizonDashboardCourseSectionUiTest {
iconRes = R.drawable.book_2
),
title = "Program 1",
route = DashboardPaginatedWidgetCardButtonRoute.HomeRoute("")
route = ""
)
)
),
Expand Down Expand Up @@ -99,9 +98,8 @@ class HorizonDashboardCourseSectionUiTest {
)
)
composeTestRule.setContent {
val mainNavController = rememberNavController()
val homeNavController = rememberNavController()
DashboardCourseSection(state, mainNavController,homeNavController)
val navController = rememberNavController()
DashboardCourseSection(state, navController)
}

composeTestRule.onNodeWithText("Program 1").assertExists()
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -27,7 +27,6 @@ import androidx.test.ext.junit.runners.AndroidJUnit4
import androidx.test.platform.app.InstrumentationRegistry
import com.instructure.horizon.R
import com.instructure.horizon.features.dashboard.DashboardItemState
import com.instructure.horizon.features.dashboard.widget.DashboardPaginatedWidgetCardButtonRoute
import com.instructure.horizon.features.dashboard.widget.DashboardPaginatedWidgetCardHeaderState
import com.instructure.horizon.features.dashboard.widget.DashboardPaginatedWidgetCardItemState
import com.instructure.horizon.features.dashboard.widget.DashboardPaginatedWidgetCardState
Expand Down Expand Up @@ -55,7 +54,7 @@ class DashboardAnnouncementBannerWidgetUiTest {
)

composeTestRule.setContent {
DashboardAnnouncementBannerSection(uiState, rememberNavController(), rememberNavController())
DashboardAnnouncementBannerSection(uiState, rememberNavController())
}

composeTestRule.waitForIdle()
Expand All @@ -73,7 +72,7 @@ class DashboardAnnouncementBannerWidgetUiTest {
)

composeTestRule.setContent {
DashboardAnnouncementBannerSection(uiState, rememberNavController(), rememberNavController())
DashboardAnnouncementBannerSection(uiState, rememberNavController())
}

val errorMessage = context.getString(R.string.dashboardAnnouncementBannerErrorMessage)
Expand All @@ -97,7 +96,7 @@ class DashboardAnnouncementBannerWidgetUiTest {
title = "Important Announcement",
source = "Course Name",
date = Date(),
route = DashboardPaginatedWidgetCardButtonRoute.MainRoute("")
route = ""
)

val uiState = DashboardAnnouncementBannerUiState(
Expand All @@ -109,7 +108,7 @@ class DashboardAnnouncementBannerWidgetUiTest {
)

composeTestRule.setContent {
DashboardAnnouncementBannerSection(uiState, rememberNavController(), rememberNavController())
DashboardAnnouncementBannerSection(uiState, rememberNavController())
}

val announcementLabel = context.getString(R.string.notificationsAnnouncementCategoryLabel)
Expand All @@ -132,7 +131,7 @@ class DashboardAnnouncementBannerWidgetUiTest {
title = "First Announcement",
source = "Course 1",
date = Date(),
route = DashboardPaginatedWidgetCardButtonRoute.MainRoute("")
route = ""
),
DashboardPaginatedWidgetCardItemState(
headerState = DashboardPaginatedWidgetCardHeaderState(
Expand All @@ -143,7 +142,7 @@ class DashboardAnnouncementBannerWidgetUiTest {
title = "Second Announcement",
source = "Course 2",
date = Date(),
route = DashboardPaginatedWidgetCardButtonRoute.MainRoute("")
route = ""
)
)

Expand All @@ -154,7 +153,7 @@ class DashboardAnnouncementBannerWidgetUiTest {
)

composeTestRule.setContent {
DashboardAnnouncementBannerSection(uiState, rememberNavController(), rememberNavController())
DashboardAnnouncementBannerSection(uiState, rememberNavController())
}

composeTestRule.onNodeWithText("First Announcement").assertIsDisplayed()
Expand All @@ -174,7 +173,7 @@ class DashboardAnnouncementBannerWidgetUiTest {
title = "Global Announcement",
source = null,
date = Date(),
route = DashboardPaginatedWidgetCardButtonRoute.MainRoute("")
route = ""
)

val uiState = DashboardAnnouncementBannerUiState(
Expand All @@ -184,7 +183,7 @@ class DashboardAnnouncementBannerWidgetUiTest {
)

composeTestRule.setContent {
DashboardAnnouncementBannerSection(uiState, rememberNavController(), rememberNavController())
DashboardAnnouncementBannerSection(uiState, rememberNavController())
}

val announcementLabel = context.getString(R.string.notificationsAnnouncementCategoryLabel)
Expand All @@ -204,7 +203,7 @@ class DashboardAnnouncementBannerWidgetUiTest {
title = "Test Announcement",
source = "Test Course",
date = Date(),
route = DashboardPaginatedWidgetCardButtonRoute.MainRoute("")
route = ""
)

val uiState = DashboardAnnouncementBannerUiState(
Expand All @@ -214,7 +213,7 @@ class DashboardAnnouncementBannerWidgetUiTest {
)

composeTestRule.setContent {
DashboardAnnouncementBannerSection(uiState, rememberNavController(), rememberNavController())
DashboardAnnouncementBannerSection(uiState, rememberNavController())
}


Expand All @@ -235,7 +234,7 @@ class DashboardAnnouncementBannerWidgetUiTest {
title = "Dated Announcement",
source = "Test Course",
date = testDate,
route = DashboardPaginatedWidgetCardButtonRoute.MainRoute("")
route = ""
)

val uiState = DashboardAnnouncementBannerUiState(
Expand All @@ -245,7 +244,7 @@ class DashboardAnnouncementBannerWidgetUiTest {
)

composeTestRule.setContent {
DashboardAnnouncementBannerSection(uiState, rememberNavController(), rememberNavController())
DashboardAnnouncementBannerSection(uiState, rememberNavController())
}

composeTestRule.onNodeWithText("Jan 01, 2024").assertIsDisplayed()
Expand All @@ -262,7 +261,7 @@ class DashboardAnnouncementBannerWidgetUiTest {
title = "No Date Announcement",
source = "Test Course",
date = null,
route = DashboardPaginatedWidgetCardButtonRoute.MainRoute("")
route = ""
)

val uiState = DashboardAnnouncementBannerUiState(
Expand All @@ -272,7 +271,7 @@ class DashboardAnnouncementBannerWidgetUiTest {
)

composeTestRule.setContent {
DashboardAnnouncementBannerSection(uiState, rememberNavController(), rememberNavController())
DashboardAnnouncementBannerSection(uiState, rememberNavController())
}

composeTestRule.onNodeWithText("No Date Announcement").assertIsDisplayed()
Expand All @@ -290,7 +289,7 @@ class DashboardAnnouncementBannerWidgetUiTest {
title = longTitle,
source = "Test Course",
date = Date(),
route = DashboardPaginatedWidgetCardButtonRoute.MainRoute("")
route = ""
)

val uiState = DashboardAnnouncementBannerUiState(
Expand All @@ -300,7 +299,7 @@ class DashboardAnnouncementBannerWidgetUiTest {
)

composeTestRule.setContent {
DashboardAnnouncementBannerSection(uiState, rememberNavController(), rememberNavController())
DashboardAnnouncementBannerSection(uiState, rememberNavController())
}

composeTestRule.onNodeWithText(longTitle).assertIsDisplayed()
Expand Down
Loading
Loading