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: enhance importdemocourse ; add importdemolibraries #998

Conversation

kdmccormick
Copy link
Collaborator

@kdmccormick kdmccormick commented Feb 5, 2024

Description

This is a redo of #976, with the following differences:

  • Per @regisb's suggestion, in importdemocourse, we now auto-find course.xml if --repo-dir is not provided. This simplifies the CLI and makes the command backwards-compatible 🎉
  • Since the change is now compatible with any demo course version, I'm merging it to master instead of nightly. This means Tutor users can use it right away instead of waiting until Redwood.
  • I changed importdemolibrary to importdemolibraries. Furthermore, instead of taking a --repo-library-tar argument, the command now searches for directories with library.xml files, run tar on them, and imports them. I've done this to:
    1. Make its behavior more similar to importdemocourse.
    2. Leave room for the introduction of more demo libraries, which I think is likely.
  • There's excitement to have the new demo course available ASAP, so in my second commit, I take the liberty of bumping the version to v17.0.2 and running scriv collect. If you'd rather do that yourself @regisb , please feel free to chop off that last commit. No need to bump the version now since Quince.2 is coming out tomorrow.

Testing

Happy paths

Import the old demo course

tutor dev do importdemocourse  # after Redwood, will require '--version open-release/quince.1'

Import the new demo course and library

tutor dev do importdemocourse --version master
tutor dev do importdemolibraries admin --version master   # Replace 'admin' with any username

See #976 for more details.

Sad paths

Fail to import a demo course (no course.xml)

tutor dev do importdemocourse --version kdmccormick/zero-course-xmls
INFO: Found course.xml files(s):  
ERROR: Could not find course.xml. Are you sure this is the right repository?

Fail to import a demo course (multiple course.xmls)

tutor dev do importdemocourse --version kdmccormick/multiple-course-xmls
INFO: Found course.xml files(s): /tmp/course/demo-course/another-course/course.xml /tmp/course/demo-course/course/course.xml
ERROR: Found multiple course.xml files--course root is ambiguous!
       Please specify a course root dir (relative to repo root) using --repo-dir.

Fail to import demo libraries (no library.xmls)

tutor dev do importdemolibraries admin --version open-release/quince.1
ERROR: No library.xml files found in repository. Are you sure this is the right repository and version?

Fail to import demo libraries (library.xml outside of library/ dir)

tutor dev do importdemolibraries admin --version kdmccormick/misplaced-library-xml
INFO: Will import library at /tmp/library/demo-course/course
ERROR: can only import library.xml files that are within a directory named 'library'

Copy link
Contributor

@regisb regisb left a comment

Choose a reason for hiding this comment

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

Awesome! This is ready to merge, except that I'd rather not force the version bump just now. Quince.2 is coming tomorrow, so maybe we can bundle this feature with the new release? I don't know how to remove a commit from a PR, can you please do it yourself?

course_root=/tmp/course/{repo_dir}
else
course_xml_first="$(find /tmp/course -name course.xml | head -n 1)"
course_xml_extra="$(find /tmp/course -name course.xml | tail -n +2)"
Copy link
Contributor

Choose a reason for hiding this comment

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

the force is strong with this bash developer.

echo "INFO: Will import course data at: $course_root" && echo

# Import into CMS
python ./manage.py cms import ../data "$course_root"
Copy link
Contributor

Choose a reason for hiding this comment

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

Do we want to automatically import demo libraries if we detect a library.xml file? There would remain the difficulty of defining the importing user name.

We don't have to decide this now, this would be a new feature that would land in Redwood.

Copy link
Collaborator Author

@kdmccormick kdmccormick Feb 8, 2024

Choose a reason for hiding this comment

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

There would remain the difficulty of defining the importing user name.

That is indeed the difficulty :) In my previous attempt I had made it so the library is imported iff a library owner is provided. It think it was slightly counterintuitive that --library-owner controls whether or not libraries are imported.

I do think the separate importdemolibraries command makes it very clear what is going on, and it is not much more typing for someone to do, so personally I am OK leaving it as-is.

@kdmccormick kdmccormick force-pushed the kdmccormick/new-demo-course-master branch from 1d824eb to 974134e Compare February 8, 2024 15:32
@kdmccormick kdmccormick changed the title feat: enhance importdemocourse ; add importdemolibraries (v17.0.2) feat: enhance importdemocourse ; add importdemolibraries Feb 8, 2024
@kdmccormick
Copy link
Collaborator Author

Quince.2 is tomorrow--perfect! Version bump removed.

@kdmccormick kdmccormick merged commit 4a30c4a into overhangio:master Feb 8, 2024
1 check passed
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.

3 participants