Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Improve documentation use of common and openedx directories #33139

Open
robrap opened this issue Aug 30, 2023 · 4 comments
Open

Improve documentation use of common and openedx directories #33139

robrap opened this issue Aug 30, 2023 · 4 comments

Comments

@robrap
Copy link
Contributor

robrap commented Aug 30, 2023

When someone new to the platform (or not new to the platform) is trying to determine where a new app should be created, it is not very clear. This is made more complex due to the lack of clarity around usage of the common and openedx directories.

Documentation Notes/Questions:

  • Update READMEs to clarify when to use and when not to use each directory.
  • Ensure docs can answer the question: Where should I create my new app?
  • Potentially create an ADR for any decisions that need to be made.
  • Ensure docs are cross-referenced appropriately so the appropriate location can be found.

Usage Questions:

  • Are there differences between common and openedx that are important?
  • One reason openedx was created was to enable a "feature" to contain all of its templates, etc., in the same directory with its supporting code.
    • Is this something that is not available under "lms" and "cms"?
    • Is this something that no longer matters as we are shifting to MFEs?
  • Do we need/want both, or are we just stuck with both?
  • Was it simply a mistake to have lms and cms specific apps in openedx? Should it only include features common to both?

Other notes:

  • This is related to guidance around when and if someone even needs a new app, or whether or not to use an existing one.
@bradenmacdonald
Copy link
Contributor

Thanks for raising this @robrap!

Some further thoughts/questions for consideration:

  • Why is xmodule a top-level directory but still part of the platform?
  • Is there any way to tell if an app in openedx is part of LMS, CMS, or both?
  • What is the difference between openedx/core/djangoapps and openedx/features ? (it seems code in features cannot be tested as CMS code, so those are likely all LMS)
  • Do we still need to distinguish between lib and djangolib ? Can we merge those?
  • Some code moves have happened, but generally the status quo is still quite a mess. What are the considerations around re-organizing and cleaning this up? e.g. Can we consider merging common into openedx and moving all lms-specific or cms-specific code into lms and cms?
  • lms and cms django settings files are also confusing and the CMS ones import the LMS ones - can we extract some settings to a common settings file that LMS and CMS both derive from?

@robrap
Copy link
Contributor Author

robrap commented Sep 21, 2023

Do we still need to distinguish between lib and djangolib ? Can we merge those?

I think that at the time there was a substantial difference in testing time between the two, which made it much nicer to work in lib when this was split out. So much has changed since then, that it is likely that this no longer an issue, and merging would simplify.

FYI: @feanil @kdmccormick: Regarding this ticket in general, not just this comment.

@kdmccormick
Copy link
Member

kdmccormick commented Oct 2, 2023

+1 to this ticket 👍🏻

@robrap @bradenmacdonald Some context:

Are there differences between common and openedx that are important?

The reason the openedx dir exists was a desire to have a top-level import for all openedx code. The idea is that eventually we'd nest everything under it, even across repositories, eg from openedx.learning import blah, from openedx.discovery import blah, etc. Some other Python projects do this; notably, Django allows third-party extension authors to define modules under django.contrib..

This clearly didn't pan out, so now we just have two folders for common code. I support picking one and making it clear that no new code should be added to the other one.

One reason openedx was created was to enable a "feature" to contain all of its templates, etc., in the same directory with its supporting code.

The openedx/features folder actually came three years later without much explanation. Based on what I recall @nasthagiri telling me, I believe you are right that the impetus for it was to better co-locate Python modules with their supporting assets. Based on that, I think we can abandon this structure as we migrate those features to MFEs.

Was it simply a mistake to have lms and cms specific apps in openedx? Should it only include features common to both?

Great question. I think that apps which are clearly only used by lms or cms should be moved to the corresponding directory.

(That said, as @ormsbee has pointed out, most content-related apps don't fall in this category, as they have both author-facing features and learning-facing features. For example, consider a "scheduling" app: authors need to set schedules in Studio, and then instructors and learners need to be see the effects of those schedules in Courseware. What's the right architecture there? Does the entire app belong in the common dir? Or should the code be split into three parts (authoring, learning, shared)? I think the ideal pattern would be something like XBlock: there's be a single "scheduling" app which would coherently hook itself in to cms and lms, but without creating coupling between cms and lms.)

Why is xmodule a top-level directory but still part of the platform?

A couple years ago I worked to make sure that edx-platform's folder structure matched its import paths. That is, if you see an import like from x.y.z import w, you can now be sure that it's located at x/y/z/w.py.

The folder at common/lib/xmodule/xmodule was configured to be imported as from xmodule import .... Given the time constraints I was under, changing all xmodule import paths wasn't an option, so I just moved the xmodule folder to the root in order to match the import path. More details are in the ADR.

Looking to the ~5-year future, I think we could eventually dissolve this directory by extracting all the XBlocks, and then moving Modulestore to the cms directory. This would require severing the lms->Modulstore dependency, which is a long-term goal of the Learning Core.

Do we still need to distinguish between lib and djangolib ? Can we merge those?

I agree with Robert, we can merge these. These are a relic from the time when we imagined that XBlocks wouldn't depend on Django. That ship has sailed. There's a wiki page or ADR for this somewhere.

Some code moves have happened, but generally the status quo is still quite a mess. What are the considerations around re-organizing and cleaning this up? e.g. Can we consider merging common into openedx and moving all lms-specific or cms-specific code into lms and cms?

Based on my experience with the import work, reorganization is totally doable and carries huge benefit. That said, it's careful work and folks are generally too busy or too nervous. The toughest parts I found are:

  • Dealing with any non-trivial mock paths in unit tests.
  • Ensuring that linting or unit tests aren't accidentally skipped after moving (observing the unit test counts is helpful here).
  • Dealing with our byzantine legacy JS configuration whenever legacy frontend code needs to be moved.
  • Supporting both old and new import paths for a time so that platform plugins which depend on import paths can smoothly be fixed (this is I why I hate it so much when plugins import from edx-platform).

I believe @feanil is considering this sort of work for the new Aximprovements team.

@robrap
Copy link
Contributor Author

robrap commented Oct 2, 2023

Thanks @kdmccormick. I think the quicker we can update the docs (followed by linting) for what we know, the better. It will make things more clear, and will keep any future migration from growing.

@ormsbee ormsbee closed this as completed Oct 2, 2023
@ormsbee ormsbee reopened this Oct 2, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

4 participants