edx-platform Code Structure: Hackathon XIV
What should the code structure be of the edx-platform?
Do we agree that the current state of tangled inter-dependencies is hard to maintain, comprehend, extend, and test?
Goals
Code Structure
- Clearly named, single-purpose, apps and folders, with clearly defined interfaces.
- So they are easier to maintain, test, and comprehend.
- So they can be substituted, if needed.
- Easier to transition to a separate repo, if we choose to do so.
- Easier to transition to a separately deployed process, if we choose to do so.
- Dependencies
- Layered:
- Low-level apps/modules do not depend on higher layers.
- Higher layers can call exposed APIs on lower layers.
- It's Ok for higher layers to call lower layers that are not immediately below them (i.e., skipping layers is Ok for now).
- Loosely coupled:
- Where possible, use django signals to notify peer layers.
- Use clear API abstractions defined in api.py or via REST APIs
- which include the exposed public interfaces
- and the expected integration points for those modules
- Separate CMS/LMS: No code dependencies between LMS and CMS. Their lower layer common folders should not call back up into LMS/CMS.
- Layered:
- Assets
- Assets that belong to an app should be co-located within the app, this includes:
- Mako Templates
- Underscore Templates
- App-specific Images
- For any assets shared across apps, they can go into
- UI toolkit - Shared Sass
- Pattern Library - Shared JS
- common folder - Shared Images
- Assets that belong to an app should be co-located within the app, this includes:
- Models
- Need to consider them on a case-by-case basis.
- By using foreign keys, it tightly couples the relationship - and requires the apps deploy within the same container.
- URLs
- URLs exposed by an app should be co-located within the app.
- The top-level urls.py should then include the urls from each app's urls (with specified namespaces).
- Interfaces
- api.py should include all public in-process python interfaces
- urls.py should include all public REST APIs
- Tests (exact details TBD - here's a proposal)
- Unit tests
- Definition
- Do not have dependencies on peer or higher layers.
- May either depend on or mock lower layers.
- White-box tests of all edge conditions and paths.
- Guidelines
- Can use/test internal methods of the module.
- Use override_settings instead of requiring settings in a central envs/tests.py.
- Definition
- Integration tests
- Definition (Note: this differs from testing.rst.)
- End-to-end python integration tests that test dependencies between apps.
- Inter-dependencies are not mocked.
- May test multiple paths, including non-happy paths if impacted by inter-dependencies.
- Guidelines
- Stored separately (maybe centrally).
- Only use exposed public interfaces (see Interfaces above)
- Definition (Note: this differs from testing.rst.)
- Acceptance tests
- Definition
- End-to-end UI-level black-box tests.
- Mostly only happy paths are tested. Regularly occurring exceptional paths that are integral to the user-facing feature may also be tested.
- All scenarios described in user-documentation are typically covered.
- Definition
- Unit tests
Enforcement
- Have edx-linter automatically find exceptions to prevent regressions to the agreed rules.
Non-Goals
- Don't over-package.
Long-term Goals
- edx.org project is separate from the open-edx platform.
- Use decorators on views to conditionally enable them instead of using django settings in urls.py.
- Use course-scoped xblock settings (once implemented) instead of adding app-specific fields in course_module.py.
- Have unit tests in openedx/core run on their own, outside of the context of LMS/CMS.
Short-term Goals
| Task | Who's working on this? | Status |
|---|---|---|
| Clean up and delete the edx-platform/common folder. | Improved already! | |
Decentralize templates and assets. They should be co-located with their owning apps. | In progress: Updating student_account to this new structure. | |
| Decentralize urls.py. Each app should have its own and the central one includes the app-specific ones. | In progress, but plenty more to do: | |
| Document what an ideal folder structure would be - with consistent and accurate names, considered division of labor, and scoped responsibilities. |
Open Questions
- How do we support composability using Require.js?
- For now, just keep as is - where we configure all js files in one location.
- Karma cannot discover decentralized tests.
- Can each app locally declare its own django configuration settings so they are part of its public interface?
- They can then be defined/set centrally for the entire project, as needed.
Initial Cleanup Tasks
Here's a list of directories to address in common:
Too many cross dependencies to move right nowTo see all the open PRs in GitHub for the Hackemon Hackathon project, look in the "clean_platform" GitHub milestone:
https://github.com/edx/edx-platform/milestone/1
Important Hackathon Points
- edx-platform/common should no longer be used, in favor of openedx/core.
- Don't move any code to
openedx/corethat depends on code inlmsorcms!- Some dependencies have snuck in to openedx/core and common.
- We want openedx/core to be a place to put lower-level code.
- Modules in openedx/core can depend on each other (and common for now) - but never lms/cms.
- In some cases, it's appropriate to move Django applications completely outside of edx-platform.
- The config_models Django application was moved out of edx-platform for the hackathon.
- edx-platform and django-splash (and maybe others) were dependent on the app.
- Avoid gigantic urls.py files!
- Keep the url patterns for particular Django applications with the actual Django application.
- Include those urls into the lms/cms urls.py files.
- Django signals can be used to de-couple code and remove dependencies.
- Instead of direct calls into higher-level code, lower-level code can emit a signal which is caught by higher-level code.
- GitHub milestones are great!
- Consider using milestones for informal PR grouping.
Current issues in common folder
CMS
| common/lib/xmodule/XModule.egg-info | cms.lib.xblock.tagging:StructuredTagsAside |
| common/djangoapps/terrain | ui_helpers.py - requireJS dependencies on cms |
| common/djangoapps/tests | some skipped for cms |
LMS
| Module in Common | Dependency on LMS Module | LMS functionality | Proposed Fix | Owner & Status |
|---|---|---|---|---|
| auth_exchange/views | lms.djangoapps.oauth_dispatch | adapters | oauth_dispatch should be moved out of LMS | |
| course_modes/admin | lms.djangoapps.verify_student.models | VerificationDeadline | ||
| course_modes/views | lms.djangoapps.commerce.utils | EcommerceService | ||
| enrollment/views | lms.djangoapps.commerce.utils | audit_log | move audit_log to openedx/core | |
| student/helpers | lms.djangoapps.verify_student.models | VerificationDeadlineSoftwareSecurePhotoVerification | ||
| student/models | lms.djangoapps.badges.utils | badges_enabled | decouple by indirectly notifying badges app via django signal | |
lms.lib.comment_client | User.from_django_user | |||
lms.djangoapps.badges.events.course_meta | award_enrollment_badge | decouple by indirectly notifying badges app via django signal | ||
| student/views | lms.djangoapps.commerce.utils | EcommerceService | ||
lms.djangoapps.verify_student.models | SoftwareSecurePhotoVerification | |||
student/management/commands/ | lms.djangoapps.coursewarelms.djangoapps.certificates.modelslms.djangoapps.grades | courses.get_course_by_idGeneratedCertificatecourse_grades.summary | Move this entire command to the new lms grades app. | |
student/management/commands/ | lms.lib.comment_client | User.from_django_user | move management command to LMS | |
| lib/xmodule/..modulestore/django | lms.djangoapps.ccx.modulestore | CCXModulestoreWrapper |
| |
| lib/xmodule/..sequence/display.coffee | logging name: edx.ui.lms.sequence.tab_selected |
Current issues in openedx/core folder
CMS
| Dependency on CMS | proposed fix | |
|---|---|---|
| course_overviews/signals.py | cms.djangoapps.contentstore.courseware_index | Have courseware_index have its own signal handler for course_deleted. |
LMS
| Module in openedx | Dependency on LMS Module | LMS functionality | Proposed Fix | Owner & Status |
|---|---|---|---|---|
| bookmarks/views.py | lms.djangoapps.lms_xblock.runtime | unquote_slashes | move unquote_slashes to openedx | |
| ccxcon/api.pyccxcon/api.py | lms.djangoapps.courseware.courses | get_course_by_id | ||
lms.djangoapps.instructor.access | list_with_level | |||
| course_overviews/models.py | lms.djangoapps.django_comment_client.utils | is_discussion_enabled | move course_overviews to lms; but make sure the course publish/delete signal works across process space. | |
lms.djangoapps.certificates.api | get_active_web_certificate | |||
lms.djangoapps.ccx.utils | get_ccx_from_ccx_locator | |||
| course_groups | lms.djangoapps.django_comment_client.utils | get_discussion_category_map, get_discussion_categories_ids | ||
| credit/partition_schemes | lms.djangoapps.verify_student.models | SkippedReverification, VerificationStatus | ||
| credit/api/provider | lms.djangoapps.django_comment_client.utils | JsonResponse | move JsonResponse to openedx | |
programs/utils | lms.djangoapps.certificates | api | ||
lms.djangoapps.commerce.utils | EcommerceService | |||
| user_api/accounts/serializers.py | lms.djangoapps.badges.utils | badges_enabled | ||
| lib/api/view_utils | lms.djangoapps.courseware.courses | get_course_with_accessCoursewareAccessException | move view_course_access to lms | |
| gating/api | lms.djangoapps.courseware.access | _has_access_to_course | create common function to check for course staff access |
lib
| TODO: Search for django references and move to djangolib |