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

feat!: default importdemocourse to the new course & lib #976

Closed

Conversation

kdmccormick
Copy link
Collaborator

@kdmccormick kdmccormick commented Jan 10, 2024

Description (will be copied into commit)

We're overhauling the Demo Course repo!. That PR will restructure the repo, rebuild the Demo Course from scratch, and a Demo Content Library to accompany the course.

This PR, which should merge right after that one, modifies the existing tutor (local|dev|k8s) do importdemocourse command accordingly:

  • The --repo-dir option now defaults to a new subdirectory: demo-course/course.
  • The --library-tar option is added, defaulting to the library archive: dist/demo-content-library.tar.gz
  • Content libraries require an owner user in order to be imported, so --library-owner is added as an option. Omitting the option means that the course will import fine, but the library import will be skipped and a warning will be printed. This is OK, as the course works fine without the library.

💥 BREAKING CHANGE: The new course is significantly different than the old one. Importing the old demo course remains possible, but with a couple extra arguments: tutor local do importdemocourse --repo-dir . --version open-release/palm.4.

Testing

Run:

tutor local do importdemocourse --version kdmccormick/new-demo-course --library-owner <any admin user>

Fire up Tutor and go to Studio. On your course listing, you should see the Demo Course, and under your library listing, you should see "Respiratory System Question Bank 1" (that's the demo library). Go to LMS, "Discover New", and choose Axim/DemoX (the one on the right).

image

Enroll. The outline should look like this:

image

tests/commands/test_jobs.py Outdated Show resolved Hide resolved
tutor/commands/jobs.py Outdated Show resolved Hide resolved
./manage.py cms reindex_course --all --setup"""
template = f"git clone {repo} --branch {version} --depth 1 /tmp/course"
if library_owner:
template += f"\nyes | ./manage.py cms import_content_library /tmp/course/{library_tar} {library_owner}"
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Note to self: if we go with this approach, we should add a -y/--yes option to the import_content_library management command. We can take care of it after this PR is merged.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

agreed

tutor/commands/jobs.py Outdated Show resolved Hide resolved
@regisb
Copy link
Contributor

regisb commented Jan 12, 2024

Very much looking forward to this change and seeing the old demo course go away!

@kdmccormick kdmccormick force-pushed the kdmccormick/new-demo-course branch from be6fbb6 to 472697e Compare January 19, 2024 01:20
@kdmccormick kdmccormick force-pushed the kdmccormick/new-demo-course branch from 472697e to 10c712e Compare January 19, 2024 01:21
@kdmccormick kdmccormick requested a review from regisb January 19, 2024 01:24
@@ -0,0 +1,2 @@
- 💥 [Feature] `tutor local do importdemocourse` will now import the new [Open edX Demo Course](https://github.com/openedx/openedx-demo-course), which has been rebuilt from scratch. If you wish to import the old demo course instead (which no longer receives updates), you can run: `tutor local do importdemocourse --repo-dir . --version open-release/palm.4`.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We should replace "palm.4" by "quince.1".

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does this mean that you need to be on quince.2 to get the new version of the course, or will it be possible to be on an older version of the platform but still import the new course?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We should replace "palm.4" by "quince.1".

Right, or even quince.master.

Does this mean that you need to be on quince.2 to get the new version of the course, or will it be possible to be on an older version of the platform but still import the new course?

Older versions of Open edX and Tutor will default to the old demo course, but it is possible to import the new demo course into them with the right arguments:

tutor local do importdemocourse --repo-dir demo-course/course --version master

I'll update this message to be clearer about this.

show_default=True,
help="Git repository that contains the course to be imported",
)
@click.option(
"-d",
"--repo-dir",
default="",
default="demo-course/course",
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This change will cause some existing commands to fail, when users try to import a different custom course that is not stored in a subdirectory:

tutor local do importdemocourse --repo https://github.com/me/mycourse

I wonder if we could simplify the CLI by auto-detecting the location of the "course.xml" file? This would make the import script much more complex. But I see no other option if we want to preserve ease-of-use and compatibility with the new demo course structure.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Or maybe, for now, if the repo path ends with openedx-demo-course, treat the default as demo-course/course, otherwise ""?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@regisb I am OK with that complexity. I'll implement this logic:

  • If the --repo-dir argument is provided, use it.
  • Else:
    • find the course.xml file
    • If there is exactly one course.xml, then use that directory.
    • Else if there are 0 or 2+ course.xml files, raise an exception.

@DawoudSheraz The old and new demo course are both stored in the same repo, "openedx-demo-course". "edx-demo-course" has redirected to "openedx-demo-course" a couple years.

@kdmccormick
Copy link
Collaborator Author

Closed in favor of #998

@kdmccormick kdmccormick closed this Feb 5, 2024
@kdmccormick kdmccormick deleted the kdmccormick/new-demo-course branch February 5, 2024 21:23
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Development

Successfully merging this pull request may close these issues.

4 participants