feat: add new advanced modules to the default list #74
Conversation
papphelix
requested review from
djoseph-apphelix,
jristau1984,
pganesh-apphelix and
viv-helix
There was a problem hiding this comment.
Pull request overview
This PR adds 12 new XBlock modules to the DEFAULT_ADVANCED_MODULES list, making them available by default in the Studio component menu. The new modules include: annotatable, done, freetextresponse, imagemodal, h5pxblock, invideoquiz, oppia, ubcpi-xblock, qualtricssurvey, scorm, edx_sga, submit-and-compare, and recommender. Additionally, 'split_test' is repositioned within the list.
Key changes:
- Expanded DEFAULT_ADVANCED_MODULES from 6 to 20 modules
- Added 12 new advanced XBlock modules to enable richer course authoring capabilities
- Repositioned 'split_test' in the list
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
papphelix
force-pushed
the
fix/TNL2-472
branch
from
f2114a1 to
6226c28
Compare
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 2 out of 2 changed files in this pull request and generated 3 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| self.course.advanced_modules.append("survey") | ||
|
|
||
| self.templates = get_component_templates(self.course) | ||
| advanced_templates = self.get_templates_of_type("advanced") |
There was a problem hiding this comment.
After removing the previous test logic, the test now accesses advanced_templates[0] at line 3038 without verifying that advanced_templates is not None or empty. While the DEFAULT_ADVANCED_MODULES list now includes many modules making it unlikely to be empty, defensive programming suggests adding an assertion like self.assertIsNotNone(advanced_templates) and self.assertGreater(len(advanced_templates), 0) before accessing index 0 to prevent potential IndexError or TypeError.
| advanced_templates = self.get_templates_of_type("advanced") | |
| advanced_templates = self.get_templates_of_type("advanced") | |
| self.assertIsNotNone(advanced_templates) | |
| self.assertGreater(len(advanced_templates), 0) |
| # Verify that components are not added twice | ||
| self.course.advanced_modules.append("video") | ||
| self.course.advanced_modules.append("drag-and-drop-v2") |
There was a problem hiding this comment.
The DEFAULT_ADVANCED_MODULES list has been expanded to include 12 new modules (annotatable, done, freetextresponse, imagemodal, h5pxblock, invideoquiz, oppia, ubcpi-xblock, qualtricssurvey, scorm, edx_sga, submit-and-compare, recommender), but the test test_advanced_components was simplified by removing test coverage for the 'done' module. Consider adding test cases that verify at least some of the newly added default modules are properly loaded and available as advanced components, similar to the removed test for 'done'.
| only_template = advanced_templates[0] | ||
| self.assertNotEqual(only_template.get("category"), "video") | ||
| self.assertNotEqual(only_template.get("category"), "drag-and-drop-v2") |
There was a problem hiding this comment.
The removal of the length assertion weakens the test for verifying that duplicate modules are not added to the advanced templates list. While the current test checks that specific modules don't appear in position 0, it doesn't comprehensively verify that duplicates are prevented. Consider adding an assertion like self.assertEqual(len(advanced_templates), len(set(t['category'] for t in advanced_templates))) to ensure all templates have unique categories, or verify the expected count matches the size of DEFAULT_ADVANCED_MODULES (since video, drag-and-drop-v2, poll, google-document, and survey should deduplicate).
Ticket: https://2u-internal.atlassian.net/browse/TNL2-472
Before:
After:
