-
Notifications
You must be signed in to change notification settings - Fork 2
feat: add client_id support for AI unit summary tracking #184
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
base: main
Are you sure you want to change the base?
Conversation
ff456af to
73b66df
Compare
73b66df to
965a750
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull request overview
This pull request adds client_id support for AI unit summary tracking, allowing the frontend to identify the client making requests to the AI service. The version has been bumped to 3.8.3.
Key changes:
- Added
data-client-idattribute to the summary hook HTML template with value "edx-unit-summaries" - Introduced
SUMMARY_CLIENT_IDsetting with a default fallback value - Updated tests to verify the client_id is properly rendered in the HTML output
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
ai_aside/__init__.py |
Version bumped from 3.8.2 to 3.8.3 |
ai_aside/block.py |
Added data-client-id template variable and logic to read from settings with fallback |
ai_aside/settings/devstack.py |
Added SUMMARY_CLIENT_ID setting configured as 'edx-unit-summaries' |
tests/test_block.py |
Updated test to assert the presence of data-client-id attribute in rendered HTML |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| """ | ||
|
|
||
| __version__ = '3.8.2' | ||
| __version__ = '3.8.3' |
Copilot
AI
Jan 9, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The version has been bumped to 3.8.3, but the CHANGELOG.rst file is missing a corresponding entry for this version. According to the merge checklist in the PR description, a changelog record should be added. Please add an entry under the "Unreleased" section or create a new "3.8.3" section documenting the addition of client_id support for AI unit summary tracking.
| __version__ = '3.8.3' | |
| __version__ = '3.8.2' |
| settings.SUMMARY_HOOK_MIN_SIZE = 500 | ||
| settings.SUMMARY_ENABLED_BY_DEFAULT = False | ||
| settings.AISPOT_LMS_NAME = 'lms' # in docker ai-spot sees the LMS as 'lms' not 'localhost' | ||
| settings.SUMMARY_CLIENT_ID = 'edx-unit-summaries' |
Copilot
AI
Jan 9, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The SUMMARY_CLIENT_ID setting is configured in devstack.py but is missing from common.py where other similar settings like SUMMARY_HOOK_HOST, SUMMARY_HOOK_JS_PATH, and AISPOT_LMS_NAME are configured. Consider adding SUMMARY_CLIENT_ID to common.py to allow it to be configured via ENV_TOKENS, following the same pattern as the other settings. This would ensure consistent configuration management across different environments.
Description
Add client_id support for AI unit summary tracking
Merge checklist:
Check off if complete or not applicable:
Related Ticket
COSMO2-779
Related PR
https://github.com/2uinc/ai-spot/pull/51