/
edx-platform Code Structure: Hackathon XIV
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 |
Related content
edX/2U Architecture Manifesto
edX/2U Architecture Manifesto
More like this
Decoupled Frontend Architecture
Decoupled Frontend Architecture
More like this
Open edX and Microfrontends
Open edX and Microfrontends
More like this
Architecture Challenges (2017-2018)
Architecture Challenges (2017-2018)
More like this
Libraries we KNOW we want to move out of the monolith
Libraries we KNOW we want to move out of the monolith
More like this
Architecture and Engineering: Home
Architecture and Engineering: Home
More like this