Skip to content

[All] Add missing pandata events#3629

Merged
hermannakos merged 2 commits intomasterfrom
pageviews
Apr 7, 2026
Merged

[All] Add missing pandata events#3629
hermannakos merged 2 commits intomasterfrom
pageviews

Conversation

@hermannakos
Copy link
Copy Markdown
Collaborator

No description provided.

@github-actions
Copy link
Copy Markdown
Contributor

github-actions Bot commented Apr 7, 2026

🧪 Unit Test Results


📊 Summary

  • Total Tests: 0
  • Failed: 0
  • Skipped: 0
  • Status: ⚠️ No test results found

Last updated: Tue, 07 Apr 2026 12:48:06 GMT

Copy link
Copy Markdown

@claude claude Bot left a comment

Choose a reason for hiding this comment

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

PageView Annotation Additions — Review

This PR adds @PageView tracking to a number of fragments across the student and teacher apps. The overall approach is consistent with how page view tracking works elsewhere in the codebase. A few issues need attention before this is correct at runtime.


Issues

  • StudentContextFragment — private getters won't be found by reflection
    apps/teacher/src/main/java/com/instructure/teacher/adapters/StudentContextFragment.kt lines 94–97

    @get:PageViewUrlParam on private var mStudentId and private var mCourseId won't work. PageViewAnnotationProcessor uses enclosingClass.methods to find annotated params, which only returns public methods. A private var generates a private getter that is invisible to this reflection call, so {userId} and {courseId} in the URL template will never be substituted — the tracked URL will contain literal {userId} / {courseId} placeholders.

    All existing usages in the codebase (AssignmentDetailsFragment, SubmissionDetailsFragment, SpeedGraderFragment, etc.) use default (public) visibility. Change to:

    @get:PageViewUrlParam("userId")
    var mStudentId by LongArg()
    @get:PageViewUrlParam("courseId")
    var mCourseId by LongArg()
  • ModuleProgressionFragment — same private getter reflection issue
    apps/teacher/src/main/java/com/instructure/teacher/features/modules/progression/ModuleProgressionFragment.kt line 60–61

    private val moduleItemId with @get:PageViewUrlParam("moduleItemId") has the same problem — the private getter won't be discoverable, so {moduleItemId} won't be replaced.

    Change to:

    @get:PageViewUrlParam("moduleItemId")
    val moduleItemId by LongArg(key = RouterParams.MODULE_ITEM_ID, default = -1L)
  • EditSyllabusFragment@PageViewUrl method returns a relative path, not a full URL
    apps/teacher/src/main/java/com/instructure/teacher/features/syllabus/edit/EditSyllabusFragment.kt line 49

    PageViewAnnotationProcessor.getPageViewUrl() returns the @PageViewUrl method's result directly as the event URL with no further processing. All other @PageViewUrl implementations return full URLs (e.g. assignment.htmlUrl, videoUri). The current implementation returns "/courses/${course.id}/syllabus/edit", so the tracked URL will be that literal relative path rather than a full URL.

    Change to:

    fun makePageViewUrl(): String = "${ApiPrefs.fullDomain}/courses/${course.id}/syllabus/edit"
  • FileDetailsFragment — empty string fallback may produce a bad analytics event
    apps/teacher/src/main/java/com/instructure/teacher/features/files/details/FileDetailsFragment.kt line 57

    If makePageViewUrl() is called before the ViewModel has emitted data (fragment becomes visible before the async load completes), viewModel.data.value?.fileData is null and the method returns "". That empty string is passed directly to pageViewUtils.startEvent(...) and recorded as the page view URL.

    Consider using the PageViewPrerequisites / completePageViewPrerequisite pattern (already in the codebase via PageViewFragmentDelegate) to defer event recording until data is ready.


Positive notes

  • SpeedGraderFragment: correctly promotes courseId/assignmentId to public val — this is the right approach for @get:PageViewUrlParam to work via reflection, and it matches the pattern used by AssignmentDetailsFragment.
  • SyllabusRepositoryFragment, ModuleListFragment, CalendarFragment, ElementaryDashboardFragment, NewDashboardFragment: simple @PageView additions with static URL strings look correct.
  • AssignmentBasicFragment: @PageViewUrl returning assignment.htmlUrl.orEmpty() correctly returns a full Canvas URL.

@github-actions
Copy link
Copy Markdown
Contributor

github-actions Bot commented Apr 7, 2026

📊 Code Coverage Report

✅ Student

  • PR Coverage: 42.65%
  • Master Coverage: 42.65%
  • Delta: +0.00%

✅ Teacher

  • PR Coverage: 25.37%
  • Master Coverage: 25.37%
  • Delta: +0.00%

✅ Pandautils

  • PR Coverage: 23.69%
  • Master Coverage: 23.69%
  • Delta: +0.00%

📈 Overall Average

  • PR Coverage: 30.57%
  • Master Coverage: 30.57%
  • Delta: +0.00%

@hermannakos
Copy link
Copy Markdown
Collaborator Author

@claude fix these

Issues

  • StudentContextFragment — private getters won't be found by reflection
    apps/teacher/src/main/java/com/instructure/teacher/adapters/StudentContextFragment.kt lines 94–97
    @get:PageViewUrlParam on private var mStudentId and private var mCourseId won't work. PageViewAnnotationProcessor uses enclosingClass.methods to find annotated params, which only returns public methods. A private var generates a private getter that is invisible to this reflection call, so {userId} and {courseId} in the URL template will never be substituted — the tracked URL will contain literal {userId} / {courseId} placeholders.
    All existing usages in the codebase (AssignmentDetailsFragment, SubmissionDetailsFragment, SpeedGraderFragment, etc.) use default (public) visibility. Change to:
    @get:PageViewUrlParam("userId")
    var mStudentId by LongArg()
    @get:PageViewUrlParam("courseId")
    var mCourseId by LongArg()
  • ModuleProgressionFragment — same private getter reflection issue
    apps/teacher/src/main/java/com/instructure/teacher/features/modules/progression/ModuleProgressionFragment.kt line 60–61
    private val moduleItemId with @get:PageViewUrlParam("moduleItemId") has the same problem — the private getter won't be discoverable, so {moduleItemId} won't be replaced.
    Change to:
    @get:PageViewUrlParam("moduleItemId")
    val moduleItemId by LongArg(key = RouterParams.MODULE_ITEM_ID, default = -1L)
  • EditSyllabusFragment@PageViewUrl method returns a relative path, not a full URL
    apps/teacher/src/main/java/com/instructure/teacher/features/syllabus/edit/EditSyllabusFragment.kt line 49
    PageViewAnnotationProcessor.getPageViewUrl() returns the @PageViewUrl method's result directly as the event URL with no further processing. All other @PageViewUrl implementations return full URLs (e.g. assignment.htmlUrl, videoUri). The current implementation returns "/courses/${course.id}/syllabus/edit", so the tracked URL will be that literal relative path rather than a full URL.
    Change to:
    fun makePageViewUrl(): String = "${ApiPrefs.fullDomain}/courses/${course.id}/syllabus/edit"

@claude
Copy link
Copy Markdown

claude Bot commented Apr 7, 2026

Claude finished @hermannakos's task in 1m 3s —— View job


Fixed PageView annotation issues

  • Fix StudentContextFragment — removed private from mStudentId and mCourseId so @get:PageViewUrlParam getters are public and discoverable via reflection
  • Fix ModuleProgressionFragment — removed private from moduleItemId for the same reason
  • Fix EditSyllabusFragment — added ApiPrefs.fullDomain prefix so makePageViewUrl() returns a full URL instead of a relative path
  • Committed and pushed (commit 3ca1c80a)
    |

- Remove private modifier from mStudentId/mCourseId in StudentContextFragment so @get:PageViewUrlParam is accessible via reflection
- Remove private modifier from moduleItemId in ModuleProgressionFragment for the same reason
- Use ApiPrefs.fullDomain in EditSyllabusFragment to return a full URL instead of a relative path

Co-authored-by: Akos Hermann <[email protected]>
@hermannakos hermannakos merged commit 10ba8ab into master Apr 7, 2026
25 of 26 checks passed
@hermannakos hermannakos deleted the pageviews branch April 7, 2026 13:10
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants