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
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 2 additions & 0 deletions changelog.d/20240110_101228_kyle_importnewdemocourse.md
Original file line number Diff line number Diff line change
@@ -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.

- [Feature] The new command `tutor local do importdemolibrary` will import a Demo Content Library, which is an optional but helpful extension to the Open edX Demo Course.
29 changes: 28 additions & 1 deletion tests/commands/test_jobs.py
Original file line number Diff line number Diff line change
Expand Up @@ -38,7 +38,34 @@ def test_import_demo_course(self) -> None:
self.assertEqual(0, result.exit_code)
self.assertIn("cms-job", dc_args)
self.assertIn(
"git clone https://github.com/openedx/edx-demo-course", dc_args[-1]
"git clone https://github.com/openedx/openedx-demo-course", dc_args[-1]
)

def test_import_demo_library(self) -> None:
with temporary_root() as root:
self.invoke_in_root(root, ["config", "save"])
with patch("tutor.utils.docker_compose") as mock_docker_compose:
result = self.invoke_in_root(
root,
[
"local",
"do",
"importdemolibrary",
"-t",
"fake/path.tar.gz",
"admin",
],
)
dc_args, _dc_kwargs = mock_docker_compose.call_args
self.assertIsNone(result.exception)
self.assertEqual(0, result.exit_code)
self.assertIn("cms-job", dc_args)
self.assertIn(
"git clone https://github.com/openedx/openedx-demo-course", dc_args[-1]
)
self.assertIn(
"./manage.py cms import_content_library /tmp/course/fake/path.tar.gz admin",
dc_args[-1],
)

def test_set_theme(self) -> None:
Expand Down
36 changes: 34 additions & 2 deletions tutor/commands/jobs.py
Original file line number Diff line number Diff line change
Expand Up @@ -124,14 +124,14 @@ def create_user_template(
@click.option(
"-r",
"--repo",
default="https://github.com/openedx/edx-demo-course",
default="https://github.com/openedx/openedx-demo-course",
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.

show_default=True,
help="Git relative subdirectory to import data from",
)
Expand All @@ -154,6 +154,37 @@ def importdemocourse(
yield ("cms", template)


@click.command(help="Import the demo content library")
@click.argument("owner_username")
@click.option(
"-r",
"--repo",
default="https://github.com/openedx/openedx-demo-course",
show_default=True,
help="Git repository that contains the library to be imported",
)
@click.option(
"-t",
"--repo-tar-path",
default="dist/demo-content-library.tar.gz",
show_default=True,
help="Git relative .tar.gz path to import content library from",
)
@click.option(
"-v",
"--version",
help="Git branch, tag or sha1 identifier. If unspecified, will default to the value of the OPENEDX_COMMON_VERSION setting.",
)
def importdemolibrary(
owner_username: str, repo: str, repo_tar_path: str, version: t.Optional[str]
) -> t.Iterable[tuple[str, str]]:
version = version or "{{ OPENEDX_COMMON_VERSION }}"
template = f"""
git clone {repo} --branch {version} --depth 1 /tmp/course
yes | ./manage.py cms import_content_library /tmp/course/{repo_tar_path} {owner_username}"""
yield ("cms", template)


@click.command(
name="print-edx-platform-setting",
help="Print the value of an edx-platform Django setting.",
Expand Down Expand Up @@ -324,6 +355,7 @@ def do_callback(service_commands: t.Iterable[tuple[str, str]]) -> None:
[
createuser,
importdemocourse,
importdemolibrary,
initialise,
print_edx_platform_setting,
settheme,
Expand Down