feat: add s3 storage support and waffle flag#7
Conversation
In this commit, - add s3 storage support. - add waffle flag to enable it on stage and keep it disable on prod. - correct entry points. - format files using black.
There was a problem hiding this comment.
Pull Request Overview
This PR adds S3 storage support to the Games XBlock with a waffle flag for controlled rollout. The changes include infrastructure for configurable storage backends, new toggle functionality, enhanced image upload validation, and Black code formatting applied throughout.
- Adds S3 storage configuration via
GAMESXBLOCK_STORAGEsetting with fallback to default Django storage - Introduces waffle flag
legacy_studio.enable_games_xblockfor staged rollout - Enhances image upload with file extension validation and returns file_path for tracking
Reviewed Changes
Copilot reviewed 9 out of 9 changed files in this pull request and generated 6 comments.
Show a summary per file
| File | Description |
|---|---|
| setup.py | Adds django-waffle and edx-toggles dependencies; corrects entry point path |
| games/utils.py | New utility module providing storage configuration and image deletion functions |
| games/toggles.py | New module defining waffle flag for enabling games xblock |
| games/handlers/matching.py | Black formatting applied (double quotes, line breaks) |
| games/handlers/flashcards.py | Black formatting applied (double quotes, line breaks) |
| games/handlers/common.py | S3 storage integration, file validation, and new delete handler |
| games/handlers/init.py | Black formatting applied |
| games/games.py | Black formatting and new delete_image_handler method |
| games/constants.py | Black formatting and removes unused DEFAULT_EXTENSION constant |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| return Response(json_body={"success": False, "error": str(e)}, status=400) | ||
|
|
||
| @staticmethod | ||
| def delete_image_handler(self, data, suffix=""): |
There was a problem hiding this comment.
The method signature is inconsistent with other static methods in this class. It has self as the first parameter while being marked as @staticmethod. This will cause a runtime error when called. Change self to xblock to match the pattern used by other static methods like upload_image and save_settings.
| if not key: | ||
| return {"success": False, "error": "Missing key"} | ||
| try: | ||
| is_deleted = delete_image(self.asset_storage, key) |
There was a problem hiding this comment.
References self.asset_storage which doesn't exist. The storage should be obtained via get_gamesxblock_storage() as done in the upload_image method. Change to asset_storage = get_gamesxblock_storage() and then use delete_image(asset_storage, key).
| is_deleted = delete_image(self.asset_storage, key) | |
| asset_storage = get_gamesxblock_storage() | |
| is_deleted = delete_image(asset_storage, key) |
| Expected: { "key": "gamesxblock/<block_id>/<hash>.ext" } | ||
| """ | ||
| # TODO: Delete API is not integrated yet, will handle this one after API is integrated if any change needed. | ||
| return CommonHandlers.delete_image(self, data, suffix) |
There was a problem hiding this comment.
Calls the wrong method name. The handler is defined as delete_image_handler in common.py but called as delete_image here. Change to CommonHandlers.delete_image_handler(self, data, suffix) to match the actual method name.
| return CommonHandlers.delete_image(self, data, suffix) | |
| return CommonHandlers.delete_image_handler(self, data, suffix) |
| status=400, | ||
| ) | ||
| blob = upload_file.read() | ||
| file_hash = hashlib.md5(blob).hexdigest() |
There was a problem hiding this comment.
MD5 is cryptographically weak and should not be used for security purposes. While this appears to be used for file deduplication rather than security, consider using a stronger hash like SHA256 to avoid potential collision attacks. Use hashlib.sha256(blob).hexdigest() instead.
| file_hash = hashlib.md5(blob).hexdigest() | |
| file_hash = hashlib.sha256(blob).hexdigest() |
| def delete_image(storage, key: str): | ||
| if storage.exists(key): | ||
| storage.delete(key) | ||
| return True |
There was a problem hiding this comment.
[nitpick] The function silently returns False if the file doesn't exist, making it unclear whether deletion failed or the file was already absent. Consider adding logging to distinguish between these cases, e.g., log.info(f'File {key} does not exist, skipping deletion') before returning False.
| return True | |
| return True | |
| log.info(f"File {key} does not exist, skipping deletion") |
| ) | ||
| _, ext = file_name.rsplit(".", 1) | ||
| ext = ext.lower() | ||
| allowed_exts = ["jpg", "jpeg", "png", "gif", "webp", "svg"] |
There was a problem hiding this comment.
[nitpick] The allowed file extensions list is hardcoded in the handler. Consider moving this to a constant in constants.py (e.g., UPLOAD.ALLOWED_EXTENSIONS) to make it easier to maintain and modify without changing handler code.
| allowed_exts = ["jpg", "jpeg", "png", "gif", "webp", "svg"] | |
| allowed_exts = UPLOAD.ALLOWED_EXTENSIONS |
In this PR,
add s3 storage support.
add waffle flag to enable it on stage and keep it disable on prod.
correct entry points.
format files using black.
related terraform PR:
related edx-internal PRs to install gamesXblock and its s3 storage settings:
related edx-platform PR: