Skip to content

Release Teacher 2.4.0 (88) #3513

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.

Sign up for GitHub

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

Merged
kristofnemere merged 4 commits into from
Feb 6, 2026
Merged

Release Teacher 2.4.0 (88) #3513

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
17a3207
Release Teacher 2.4.0 (87)
kristofnemere Jan 26, 2026
ff6de02
Excluded breaking E2E tests
kristofnemere Jan 26, 2026
901bb91
Fixed LTI launch issue
kristofnemere Jan 29, 2026
5fb3d65
Release Teacher 2.4.0 (87)
kristofnemere Jan 30, 2026
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
4 changes: 2 additions & 2 deletions apps/teacher/build.gradle
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -55,8 +55,8 @@ android {
defaultConfig {
minSdkVersion Versions.MIN_SDK
targetSdkVersion Versions.TARGET_SDK
versionCode = 86
versionName = '2.3.1'
versionCode = 88
versionName = '2.4.0'
vectorDrawables.useSupportLibrary = true
testInstrumentationRunner 'com.instructure.teacher.espresso.TeacherHiltTestRunner'
testInstrumentationRunnerArguments disableAnalytics: 'true'
Expand Down
3 changes: 1 addition & 2 deletions apps/teacher/flank_e2e.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,7 +13,7 @@ gcloud:
timeout: 60m
test-targets:
- annotation com.instructure.canvas.espresso.annotations.E2E
- notAnnotation com.instructure.canvas.espresso.annotations.Stub, com.instructure.canvas.espresso.annotations.FlakyE2E, com.instructure.canvas.espresso.annotations.KnownBug
- notAnnotation com.instructure.canvas.espresso.annotations.Stub, com.instructure.canvas.espresso.annotations.FlakyE2E, com.instructure.canvas.espresso.annotations.KnownBug, com.instructure.canvas.espresso.annotations.ReleaseExclude
device:
- model: Pixel2.arm
version: 29
Expand All @@ -23,4 +23,3 @@ gcloud:
flank:
testShards: 10
testRuns: 1

3 changes: 3 additions & 0 deletions .../teacher/src/androidTest/java/com/instructure/teacher/ui/e2e/compose/AssignmentE2ETest.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -25,6 +25,7 @@ import com.instructure.canvas.espresso.SecondaryFeatureCategory
import com.instructure.canvas.espresso.TestCategory
import com.instructure.canvas.espresso.TestMetaData
import com.instructure.canvas.espresso.annotations.E2E
import com.instructure.canvas.espresso.annotations.ReleaseExclude
import com.instructure.canvas.espresso.refresh
import com.instructure.dataseeding.api.AssignmentsApi
import com.instructure.dataseeding.api.SectionsApi
Expand Down Expand Up @@ -61,6 +62,7 @@ class AssignmentE2ETest : TeacherComposeTest() {
android.Manifest.permission.CAMERA
)

@ReleaseExclude
@E2E
@Test
@TestMetaData(Priority.MANDATORY, FeatureCategory.ASSIGNMENTS, TestCategory.E2E)
Expand Down Expand Up @@ -503,6 +505,7 @@ class AssignmentE2ETest : TeacherComposeTest() {
speedGraderPage.assertCommentDisplayed(commentUploadInfo.fileName, student.name)
}

@ReleaseExclude
@E2E
@Test
@TestMetaData(Priority.COMMON, FeatureCategory.ASSIGNMENTS, TestCategory.E2E, SecondaryFeatureCategory.SECTIONS)
Expand Down
2 changes: 2 additions & 0 deletions ...teacher/src/androidTest/java/com/instructure/teacher/ui/e2e/compose/SpeedGraderE2ETest.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -24,6 +24,7 @@ import com.instructure.canvas.espresso.Priority
import com.instructure.canvas.espresso.TestCategory
import com.instructure.canvas.espresso.TestMetaData
import com.instructure.canvas.espresso.annotations.E2E
import com.instructure.canvas.espresso.annotations.ReleaseExclude
import com.instructure.canvas.espresso.pressBackButton
import com.instructure.canvas.espresso.refresh
import com.instructure.dataseeding.api.SubmissionsApi
Expand Down Expand Up @@ -51,6 +52,7 @@ class SpeedGraderE2ETest : TeacherComposeTest() {

override fun enableAndConfigureAccessibilityChecks() = Unit

@ReleaseExclude
@OptIn(ExperimentalTestApi::class)
@E2E
@Test
Expand Down
34 changes: 34 additions & 0 deletions apps/teacher/src/main/java/com/instructure/teacher/router/RouteMatcher.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -31,6 +31,7 @@ import com.instructure.canvasapi2.managers.FileFolderManager
import com.instructure.canvasapi2.models.CanvasContext
import com.instructure.canvasapi2.models.Course
import com.instructure.canvasapi2.models.FileFolder
import com.instructure.canvasapi2.models.Group
import com.instructure.canvasapi2.utils.ApiPrefs
import com.instructure.canvasapi2.utils.ApiType
import com.instructure.canvasapi2.utils.LinkHeaders
Expand Down Expand Up @@ -329,6 +330,7 @@ object RouteMatcher : BaseRouteMatcher() {

fun route(activity: FragmentActivity, route: Route?) {
val uri = route?.uri
Copy link

@claude claude bot Feb 6, 2026

Choose a reason for hiding this comment

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

Consider removing this debug logging statement before merging to production. Debug logs with detailed routing information can clutter logs and potentially expose sensitive information in production builds.

Logger.i("RouteMatcher:route() - uri: $uri, routeContext: ${route?.routeContext}, primaryClass: ${route?.primaryClass}")
if (route == null || route.routeContext === RouteContext.DO_NOT_ROUTE) {
if (route?.uri != null) {
//No route, no problem
Expand Down Expand Up @@ -364,6 +366,8 @@ object RouteMatcher : BaseRouteMatcher() {
?.contains("media_attachments") == true
) {
handleStudioImmersiveViewRoute(route, activity)
} else if (route.routeContext === RouteContext.LTI) {
handleLtiRoute(activity, route)
} else if (activity.resources.getBoolean(R.bool.isDeviceTablet)) {
handleTabletRoute(activity, route)
} else {
Expand Down Expand Up @@ -477,6 +481,36 @@ object RouteMatcher : BaseRouteMatcher() {
}
}

private fun handleLtiRoute(activity: FragmentActivity, route: Route) {
Logger.i("RouteMatcher:handleLtiRoute()")
val url = route.uri?.toString()
if (url.isNullOrEmpty()) {
return
}

// Don't intercept sessionless_launch URLs - they're already being handled by an existing LtiLaunchFragment
Copy link

@claude claude bot Feb 6, 2026

Choose a reason for hiding this comment

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

The comment says "Don't intercept sessionless_launch URLs" but this check might be too broad. Consider whether this should be more specific (e.g., checking for the full path pattern rather than just substring matching). Also, this logic appears duplicated in LtiLaunchFragment.kt:180 - consider extracting to a shared utility function to maintain consistency.

if (url.contains("sessionless_launch")) {
Logger.i("RouteMatcher:handleLtiRoute() - Skipping sessionless_launch URL")
return
}

val contextId = route.paramsHash[RouterParams.COURSE_ID]?.toLongOrNull()
?: route.paramsHash[RouterParams.GROUP_ID]?.toLongOrNull()

val canvasContext: CanvasContext? = when (route.getContextType()) {
CanvasContext.Type.COURSE -> contextId?.let { Course(id = it) }
CanvasContext.Type.GROUP -> contextId?.let { Group(id = it) }
else -> null
}

if (canvasContext != null) {
Comment on lines +496 to +506
Copy link

@claude claude bot Feb 6, 2026

Choose a reason for hiding this comment

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

There's a potential null safety issue here. If contextId is null and both conditions fail, the function continues to check canvasContext != null, which will always be null in that case. Consider early return or better error handling when the context cannot be determined. Also, consider logging when falling back to handleWebViewUrl so debugging is easier.

val ltiRoute = LtiLaunchFragment.makeRoute(canvasContext, url, sessionLessLaunch = true)
route(activity, ltiRoute)
} else {
handleWebViewUrl(activity, url)
}
}

private fun handleWebViewRoute(context: Context, route: Route) {
context.startActivity(InternalWebViewActivity.createIntent(context, route, "", false))
}
Expand Down
1 change: 1 addition & 0 deletions libs/interactions/src/main/java/com/instructure/interactions/router/RouterParams.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@ object RouterParams {
const val EVENT_ID = "event_id"
const val CONVERSATION_ID = "conversation_id"
const val COURSE_ID = "course_id"
const val GROUP_ID = "group_id"
const val FILE_ID = "file_id"
const val FOLDER_NAME = "file_name"
const val GRADE_ID = "grade_id"
Expand Down
4 changes: 3 additions & 1 deletion libs/pandautils/src/main/java/com/instructure/pandautils/features/lti/LtiLaunchFragment.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -177,7 +177,9 @@ class LtiLaunchFragment : BaseCanvasFragment(), NavigationCallbacks {
}

private fun canRouteInternally(url: String) =
webViewRouter.canRouteInternally(url) && ltiUrl?.substringBefore("?") != url.substringBefore("?")
webViewRouter.canRouteInternally(url)
&& ltiUrl?.substringBefore("?") != url.substringBefore("?")
Comment on lines 179 to +181
Copy link

@claude claude bot Feb 6, 2026

Choose a reason for hiding this comment

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

Same sessionless_launch check as in RouteMatcher.kt:491. This duplication suggests these classes are making routing decisions independently. Consider:

  1. Extracting the sessionless_launch detection to a shared utility
  2. Documenting why sessionless_launch URLs need special handling
  3. Ensuring both checks remain synchronized if the logic needs to change

&& !url.contains("sessionless_launch")

override fun routeInternallyCallback(url: String) {
// Handle return button in external tools. Links to course homepage should close the tool.
Expand Down
2 changes: 1 addition & 1 deletion libs/pandautils/src/main/java/com/instructure/pandautils/features/lti/LtiLaunchViewModel.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -122,7 +122,7 @@ class LtiLaunchViewModel @Inject constructor(

private fun launchLti(url: String) {
viewModelScope.launch {
val authenticatedUrl = repository.authenticateUrl(url)
val authenticatedUrl = if (url.contains("session_token")) url else repository.authenticateUrl(url)
Copy link

@claude claude bot Feb 6, 2026

Choose a reason for hiding this comment

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

This conditional authentication logic could be problematic. The check if (url.contains("session_token")) is a simple substring match which could produce false positives. Consider:

  1. Using a more robust URL parsing approach (checking query parameters specifically)
  2. Adding a comment explaining why URLs with session_token should skip authentication
  3. Adding test coverage for this behavior to prevent regressions

if (openInternally || Assignment.internalLtiTools.any { url.contains(it) }) {
_events.send(LtiLaunchAction.LoadLtiWebView(authenticatedUrl))
} else {
Expand Down
Loading