Testing Django 5.2 #5
Conversation
Removed Django version '5.2' from the workflow.
This error was occurring because the way the redirect URL was constructed caused the entire base path to be removed. This commit updates the URL construction method to correctly preserve the MFE's path.
Commit generated by workflow `openedx/edx-platform/.github/workflows/upgrade-one-python-dependency.yml@refs/heads/master`
…rprise-integrated-channels-904b6c1 feat: Upgrade Python dependency enterprise-integrated-channels
- Move Video Block JS files from xmodule/js/src/video/ to xmodule/assets/video/public/js/ - Update JavaScript files from RequireJS to ES6 import/export - test: Enable and fix Karma Js tests for Video XBlock (openedx#37351) --------- Co-authored-by: salmannawaz <[email protected]>
fix: Profile MFE redirection issue (URL path override)
feat!: Upgrading to `django52`.
ktyagiapphelix2u
force-pushed
the
ktyagi/django
branch
from
9275659 to
d484910
Compare
There was a problem hiding this comment.
Pull Request Overview
This PR migrates the video player codebase from RequireJS to ES6 modules to support testing with Django 5.2. The changes involve converting all video-related JavaScript modules from AMD/RequireJS format to native ES6 imports/exports, updating test files to use ES6 imports, and modifying the Python backend to use fragment.initialize_js() instead of the deprecated shim_xmodule_js() function.
Key Changes
- Converted all video JavaScript modules from RequireJS/AMD to ES6 module format
- Updated video block Python code to use
fragment.initialize_js()for module initialization - Modified test files to import modules using ES6 syntax
- Updated webpack configuration paths for fixture files
Reviewed Changes
Copilot reviewed 105 out of 105 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| xmodule/video_block/video_block.py | Replaced shim_xmodule_js() with fragment.initialize_js() and updated webpack bundle name for public view |
| xmodule/js/src/video/*.js | Removed all original RequireJS video module files |
| xmodule/assets/video/public/js/*.js | Added new ES6 module versions of video components |
| xmodule/js/spec/video/*.js | Updated test files to use ES6 imports and removed RequireJS wrappers |
| xmodule/js/karma_*.js | Updated karma configuration and runner with ES6 imports and corrected fixture paths |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
| spyOnEvent(state.videoProgressSlider.handle, 'focus'); | ||
| spyOn(state.videoProgressSlider, 'notifyThroughHandleEnd') | ||
| .and.callThrough(); | ||
|
|
There was a problem hiding this comment.
Extra blank line added (line 257) that serves no purpose and reduces code consistency.
Copilot uses AI. Check for mistakes.
| state.videoProgressSlider.notifyThroughHandleEnd({end: true}); | ||
|
|
||
| expect(state.videoProgressSlider.handle.attr('title')) | ||
| .toBe('Video ended'); | ||
|
|
||
| state.videoProgressSlider.handle.trigger('focus'); |
There was a problem hiding this comment.
[nitpick] Removed blank lines between related test assertions reduce readability. The original spacing helped separate the setup, assertion, and trigger steps.
Copilot uses AI. Check for mistakes.
| it('not shown when captions are not ai generated', () => { | ||
| Caption.updateGoogleDisclaimer(BASE_CAPTIONS) | ||
| expect(state.shouldShowGoogleDisclaimer).toBe(false); | ||
| expect(Caption.shouldShowGoogleDisclaimer).toBe(false); |
There was a problem hiding this comment.
Changed from state.shouldShowGoogleDisclaimer to Caption.shouldShowGoogleDisclaimer. This appears to be accessing the wrong object - the property should exist on the Caption instance (state.videoCaption or similar), not on the Caption constructor/module itself.
Copilot uses AI. Check for mistakes.
8 participants
No description provided.