feat: new event to track LTI Launches via LTI Provider#320
Conversation
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hello @xitij2000, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed!
Summary of Changes
Hello team,
Gemini (gemini-code-assist) here to provide a summary of this pull request based on the provided title, empty description, and patch content.
Based on the changes, the primary intent of this PR appears to be the implementation of an event signal for successful LTI launches within the LMS. This signal, LTI_PROVIDER_LAUNCH_SUCCESS, is emitted after a valid LTI launch request has been processed and the user has been authenticated and parameters stored. The event carries detailed data about the launch, including user information (with PII), course and usage keys, and sanitized launch parameters.
To support this, the PR also updates the openedx-events dependency to point to a specific branch (kshitij/lti-launch-event) on a GitHub repository, suggesting that the necessary event definition and data structures are included in that particular version of the library.
Highlights
- New Event Signal: Introduces the
LTI_PROVIDER_LAUNCH_SUCCESSevent signal, emitted upon successful LTI launches. - Event Data: The new event includes structured data about the launch, such as user details (including PII like username, email, and name), the course and usage keys, and the LTI launch parameters (excluding sensitive OAuth fields).
- Dependency Update: Updates the
openedx-eventslibrary dependency across various requirements files to point to a specific development branch (kshitij/lti-launch-event) where the new event definition likely resides.
Changelog
Click here to see the changelog
- lms/djangoapps/lti_provider/tests/test_views.py
- Added imports for new event data classes from
openedx_events.learning.data(line 13). - Patched the
LTI_PROVIDER_LAUNCH_SUCCESS.send_eventsignal intest_valid_launch(line 100). - Added an assertion to verify that the
send_eventmethod is called with the correctLtiProviderLaunchDatastructure containing user, course, usage, and launch parameter details (lines 110-129).
- Added imports for new event data classes from
- lms/djangoapps/lti_provider/views.py
- Added imports for event data classes and the
LTI_PROVIDER_LAUNCH_SUCCESSsignal fromopenedx_events.learning(lines 15-16). - Added code within the
lti_launchview to copy and sanitize launch parameters by removingoauth_prefixed keys (lines 112-116). - Added code to construct
LtiProviderLaunchDatausing user details, course/usage keys, and sanitized launch parameters (lines 118-137). - Added a call to
LTI_PROVIDER_LAUNCH_SUCCESS.send_eventwith the constructed launch data (line 118).
- Added imports for event data classes and the
- requirements/constraints.txt
- Updated the
openedx-eventsdependency to reference a specific git branch (kshitij/lti-launch-event) instead of a fixed version (line 172).
- Updated the
- requirements/edx/base.txt
- Updated the
openedx-eventsdependency to reference a specific git branch (kshitij/lti-launch-event) instead of a fixed version (line 804).
- Updated the
- requirements/edx/development.txt
- Updated the
openedx-eventsdependency to reference a specific git branch (kshitij/lti-launch-event) instead of a fixed version (line 1361).
- Updated the
- requirements/edx/doc.txt
- Updated the
openedx-eventsdependency to reference a specific git branch (kshitij/lti-launch-event) instead of a fixed version (line 975).
- Updated the
- requirements/edx/testing.txt
- Updated the
openedx-eventsdependency to reference a specific git branch (kshitij/lti-launch-event) instead of a fixed version (line 1033).
- Updated the
Using Gemini Code Assist
The full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips.
Invoking Gemini
You can request assistance from Gemini at any point in your pull request via creating an issue comment (i.e. comment on the pull request page) using either /gemini <command> or @gemini-code-assist <command>. Below is a summary of the supported commands.
| Feature | Command | Description |
|---|---|---|
| Code Review | /gemini review |
Performs a code review for the current pull request in its current state. |
| Pull Request Summary | /gemini summary |
Provides a summary of the current pull request in its current state. |
| Comment | @gemini-code-assist | Responds in comments when explicitly tagged, both in issue comments and review comments. |
| Help | /gemini help |
Displays a list of available commands. |
Customization
To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a .gemini/ folder in the base of the repository. Detailed instructions can be found here.
Limitations & Feedback
Gemini Code Assist is currently in preview and may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments to provide feedback.
You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension.
A signal sent,
Launch data flows out,
Event now known.
Footnotes
-
Review the Privacy Notices, Generative AI Prohibited Use Policy, Terms of Service, and learn how to configure Gemini Code Assist in GitHub here. Gemini can make mistakes, so double check it and use code with caution. ↩
xitij2000
changed the title
There was a problem hiding this comment.
Code Review
This pull request introduces an event signal for successful LTI provider launches. The implementation is straightforward and includes necessary tests and updates to dependencies. The core logic for event data preparation and emission is sound. Key areas for attention are the accuracy of test assertions and ensuring the PR description is comprehensive for future reference and review. The dependency on a development branch of openedx-events should also be managed before merging to a stable environment.
Summary of Findings
- Test Accuracy for
extra_params: The testtest_valid_launchassertsextra_params={}. However, thebuild_launch_requestfunction and the view logic imply thatextra_paramsshould contain other LTI parameters likelis_result_sourcedid,lis_outcome_service_url, andtool_consumer_instance_guid. The test assertion needs to be updated to reflect this for correctness. - PR Description: The pull request description is currently a template. It should be filled out to provide context, testing instructions, and other relevant information about the change.
- Dependency Management: The PR changes
openedx-eventsto a git branch. This is fine for development but should be updated to a released version before merging to a stable branch or for a release.
Merge Readiness
This PR introduces a valuable event for LTI launches. Before merging, I recommend addressing the high-severity issue in the test to ensure its accuracy. Additionally, filling out the PR description and having a clear plan for managing the openedx-events dependency (updating it to a released version eventually) would be beneficial. I am not authorized to approve pull requests, so please ensure further review and approval from authorized maintainers.
| lti_launch_success_send_event.assert_called_with( | ||
| launch_data=LtiProviderLaunchData( | ||
| user=UserData( | ||
| id=request.user.id, | ||
| is_active=request.user.is_active, | ||
| pii=UserPersonalData( | ||
| username=request.user.username, | ||
| email=request.user.email, | ||
| name=request.user.profile.name, | ||
| ) | ||
| ), | ||
| course_key=COURSE_KEY, | ||
| usage_key=USAGE_KEY, | ||
| launch_params=LtiProviderLaunchParamsData( | ||
| roles='Instructor,urn:lti:instrole:ims/lis/Administrator', | ||
| context_id='lti_launch_context_id', | ||
| user_id='LTI_User', extra_params={} | ||
| ) | ||
| ) | ||
| ) |
There was a problem hiding this comment.
The assertion for extra_params is currently extra_params={}. However, based on the build_launch_request function (which populates the initial LTI parameters for the test) and the logic in lti_provider.views.lti_launch (which processes these parameters), several non-OAuth, non-explicitly-popped parameters should remain and be collected into extra_params.
Specifically, build_launch_request includes:
lis_result_sourcedid: 'test_sourcedid'lis_outcome_service_url: 'test_url'tool_consumer_instance_guid: 'test_guid'
These parameters are not OAuth parameters and are not among those explicitly popped (course_key, usage_key, roles, context_id, user_id) before constructing extra_params in the view. Therefore, they should be present in the extra_params dictionary.
Could you update the assertion to reflect the expected extra_params? This would make the test more accurately verify the behavior of the extra_params collection.
lti_launch_success_send_event.assert_called_with(
launch_data=LtiProviderLaunchData(
user=UserData(
id=request.user.id,
is_active=request.user.is_active,
pii=UserPersonalData(
username=request.user.username,
email=request.user.email,
name=request.user.profile.name,
)
),
course_key=COURSE_KEY,
usage_key=USAGE_KEY,
launch_params=LtiProviderLaunchParamsData(
roles='Instructor,urn:lti:instrole:ims/lis/Administrator',
context_id='lti_launch_context_id',
user_id='LTI_User', extra_params={
'lis_result_sourcedid': 'test_sourcedid',
'lis_outcome_service_url': 'test_url',
'tool_consumer_instance_guid': 'test_guid'
}
)
)
)There was a problem hiding this comment.
build_launch_request takes an extra_post_data if that is blank then no extra params will be added and extra_params will be empty as it is in the reviewed code.
| openedx-django-wiki==2.1.0 | ||
| # via -r requirements/edx/kernel.in | ||
| openedx-events==10.2.0 | ||
| openedx-events @ git+https://github.com/open-craft/openedx-events.git@kshitij/lti-launch-event |
There was a problem hiding this comment.
This change pins openedx-events to a specific branch. While this is fine for development, it's important to ensure this is updated to a released version before merging to a stable branch or for a release. This ensures that the platform is using stable, tested code and not code that is still under development.
openedx-events==10.2.0
| xmlsec==1.3.14 | ||
| social-auth-core==4.5.4 | ||
|
|
||
| openedx-events @ git+https://github.com/open-craft/openedx-events.git@kshitij/lti-launch-event |
There was a problem hiding this comment.
This change pins openedx-events to a specific branch. While this is fine for development, it's important to ensure this is updated to a released version before merging to a stable branch or for a release. This ensures that the platform is using stable, tested code and not code that is still under development.
openedx-events==10.2.0
| # -r requirements/edx/doc.txt | ||
| # -r requirements/edx/testing.txt | ||
| openedx-events==10.2.0 | ||
| openedx-events @ git+https://github.com/open-craft/openedx-events.git@kshitij/lti-launch-event |
There was a problem hiding this comment.
This change pins openedx-events to a specific branch. While this is fine for development, it's important to ensure this is updated to a released version before merging to a stable branch or for a release. This ensures that the platform is using stable, tested code and not code that is still under development.
openedx-events==10.2.0
| openedx-django-wiki==2.1.0 | ||
| # via -r requirements/edx/base.txt | ||
| openedx-events==10.2.0 | ||
| openedx-events @ git+https://github.com/open-craft/openedx-events.git@kshitij/lti-launch-event |
There was a problem hiding this comment.
This change pins openedx-events to a specific branch. While this is fine for development, it's important to ensure this is updated to a released version before merging to a stable branch or for a release. This ensures that the platform is using stable, tested code and not code that is still under development.
openedx-events==10.2.0
| openedx-django-wiki==2.1.0 | ||
| # via -r requirements/edx/base.txt | ||
| openedx-events==10.2.0 | ||
| openedx-events @ git+https://github.com/open-craft/openedx-events.git@kshitij/lti-launch-event |
There was a problem hiding this comment.
This change pins openedx-events to a specific branch. While this is fine for development, it's important to ensure this is updated to a released version before merging to a stable branch or for a release. This ensures that the platform is using stable, tested code and not code that is still under development.
openedx-events==10.2.0
|
@xitij2000 we'd prefer to get this code from upstream. Have you opened a backport to Teak with this? |
|
@pdpinch I'm not sure I understand. Are you asking if there is a PR to backport it to the upstream teak branch? If so, there isn't one, and I'm not sure if there should be once since this is a new feature. The upstream PR against the master branch is here: openedx#36756 |
pdpinch
left a comment
pdpinch
left a comment
There was a problem hiding this comment.
Sorry to leave you hanging @xitij2000. It makes sense that this wouldn't be back-ported upstream.
I'll approve so we can deploy to Residential MITx from the future.
3 participants
Description
Cherry pick of openedx#36756 to teak