Skip to content
This repository was archived by the owner on Aug 5, 2026. It is now read-only.

feat: Initial copy-paste circuit - #1

Merged
Kelketek merged 16 commits into
masterfrom
fox/BB-7295-initial-circuit
May 3, 2023
Merged

feat: Initial copy-paste circuit#1
Kelketek merged 16 commits into
masterfrom
fox/BB-7295-initial-circuit

Conversation

@Kelketek

@Kelketek Kelketek commented Apr 3, 2023

Copy link
Copy Markdown
Member

This PR creates the initial scaffolding for the section_to_course application.

JIRA tickets: https://tasks.opencraft.com/browse/BB-7295

Dependencies: openedx/openedx-platform#32058

Testing instructions:

To set up the app:

  1. Check out the edx-platform branch as linked in the Dependencies section
  2. In the src directory of the devstack, clone this repository and checkout this branch.
  3. In the devstack repo root, run make dev.shell.studio
  4. cd /edx/src/section_to_course
  5. pip install -e .

To run the unit tests:

  1. From the devstack repo root, run make dev.shell.studio
  2. DJANGO_SETTINGS_MODULE=cms.envs.test pytest --pyargs section_to_course --rootdir cms

To test the functionality manually:

  1. Create a course (or find an existing one) and pick a section from it. Derive the usage key string (this can be done, for instance, by duplicating a block and checking the network request in the devtools to see what the source block string is)
  2. From the devstack repo root, run make dev.shell.studio
  3. Run the management command to perform the copy. The command is section_to_course and should be followed by the destination course and the source section's usage key. For example, ./manage.py cms section_to_course course-v1:Beeper+B33P+BOOP block-v1:edX+DemoX+Demo_Course+type@chapter+block@graded_interactions edx.

Author notes and concerns:

The cookiecutter we derived from had a lot of nice CI functionalities, not to mention linting. I had to discard all of it since we rely on the edx-platform environment and can't (readily) import that into the GitHub CI. So, please be attentive to code quality when reviewing.

@Kelketek
Kelketek force-pushed the fox/BB-7295-initial-circuit branch from 4ae769f to bc0c1a1 Compare April 13, 2023 20:55
@Kelketek Kelketek self-assigned this Apr 13, 2023
@Kelketek Kelketek changed the title [WIP] build: Initial scaffolding. feat: Initial copy-paste circuit Apr 13, 2023
@Kelketek
Kelketek requested a review from Agrendalath April 13, 2023 21:19
Comment thread README.rst Outdated
Comment thread README.rst Outdated
Comment thread README.rst Outdated
Comment thread requirements/ci.in Outdated
Comment thread requirements/test.in Outdated
Comment thread section_to_course/management/commands/section_to_course.py Outdated
Comment thread section_to_course/models.py
Comment thread section_to_course/tests/test_utils.py Outdated
Comment thread section_to_course/tests/test_utils.py Outdated
Comment thread section_to_course/tests/test_utils.py Outdated
@Kelketek

Copy link
Copy Markdown
Member Author

@Agrendalath This is ready for another look!

@Agrendalath Agrendalath left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

When I clone the section into a new course, the subsections are not displayed on the Course Outline page. Do we need to refresh learning sequences manually to get it working? When I go to the [/admin/learning_sequences/coursecontext/](http://localhost:18000/admin/learning_sequences/coursecontext/) page, I don't see any sequences in the "Debug Details".

image

Also, we will need to backport the upstream PR to our Nutmeg branch and test it there before we merge this, right?

Comment thread section_to_course/management/commands/tests/test_selection_to_course.py Outdated
Comment thread section_to_course/management/commands/section_to_course.py Outdated
Comment thread section_to_course/utils.py
Comment thread section_to_course/management/commands/section_to_course.py Outdated
@Kelketek

Copy link
Copy Markdown
Member Author

When I clone the section into a new course, the subsections are not displayed on the Course Outline page. Do we need to refresh learning sequences manually to get it working? When I go to the [/admin/learning_sequences/coursecontext/](http://localhost:18000/admin/learning_sequences/coursecontext/) page, I don't see any sequences in the "Debug Details".

image

Also, we will need to backport the upstream PR to our Nutmeg branch and test it there before we merge this, right?

@Agrendalath I'm not able to reproduce this issue-- when I copy the section over and then refresh the learning MFE page, I see the new section in the outline. Can you give me step-by-step instructions to reproduce?

As for backporting the upstream branch, probably! We can make sure to clear that out first. I'll start the next task based on this branch while we wait on that merge. If it takes long enough we'll just put the commit into our Nutmeg branch while we wait.

Or, if you think it would be better, I can create the PR against our nutmeg branch now.

I've addressed the remaining notes-- could you take one more look?

@Agrendalath

Agrendalath commented Apr 20, 2023

Copy link
Copy Markdown
Member

@Kelketek,

Can you give me step-by-step instructions to reproduce?

I just followed your testing instructions for the first section of the demo course. It might be something with my devstack.

I'll start the next task based on this branch while we wait on that merge.

What are we waiting for? I already left my +1 and asked you in Jira if you want me to merge it now or wait for the Nutmeg testing.

Or, if you think it would be better, I can create the PR against our nutmeg branch now.

Yeah, we will deploy this to a Nutmeg instance, so let's do it now. We also won't need to worry about the Learning Sequences while rendering the Course Outline page.

I've addressed the remaining notes-- could you take one more look?

I'm still seeing a couple of unresolved discussions (example). They are marked as "outdated", so you may need to open them manually. They are still valid, though.

@Kelketek

Copy link
Copy Markdown
Member Author

What are we waiting for? I already left my +1 and asked you in Jira if you want me to merge it now or wait for the Nutmeg testing.

Sorry, forgot you had merge rights on edx-platform. I'll create the MR against our Nutmeg branch.

I'm still seeing a couple of unresolved discussions (#1 (comment)). They are marked as "outdated", so you may need to open them manually. They are still valid, though.

I've made a push to fix up the README. I didn't see any other outstanding discussions than the one you mentioned, though. If there are more, can you ping me on them?

@bradenmacdonald

Copy link
Copy Markdown
Member

Damn, the sheer amount of boilerplate required in a new django app for Open edX is truly gross.

@Kelketek

Kelketek commented Apr 20, 2023

Copy link
Copy Markdown
Member Author

@Agrendalath I went to backport the changes to Nutmeg and found that the structure/location of items in the code changed much more than I had anticipated. The most recent changes should allow the app to be installed and used in both versions while also allowing the quality tests to work.

Also, looks like @bradenmacdonald has just added new review notes on the code in edx-platform, so this changes things-- looks like I still have a bit left to go there.

@Kelketek

Copy link
Copy Markdown
Member Author

@Agrendalath The changes requested by Braden upstream are pretty involved, but I'm expecting the end result will have the same data structures. So, I don't think we need to hold up this PR for it if we juggle things a bit. Here's how I propose we move forward so the project doesn't fall behind but we still get all adjustments made:

1.We'll install and run this based on the backported PR to nutmeg.
2. I'll get the admin views in the remaining time this sprint.
3. The sprint after this I have the discovery for programs. I can do that alongside addressing Braden's notes on the upstream PR. If I find time to work on them earlier, I will.

I've added a note into the README about the need to use our Nutmeg branch for now.

@bradenmacdonald

Copy link
Copy Markdown
Member

@Kelketek I didn't realize any of my feedback would be "pretty involved", so feel free to discuss it more on that upstream PR first if you think it can save you some work.

@Agrendalath

Copy link
Copy Markdown
Member

@Kelketek,

I didn't see any other outstanding discussions than the one you mentioned, though. If there are more, can you ping me on them?

I see the following discussions:

  1. feat: Initial copy-paste circuit #1 (comment)
  2. feat: Initial copy-paste circuit #1 (comment)
  3. feat: Initial copy-paste circuit #1 (comment)
  4. feat: Initial copy-paste circuit #1 (comment)

1.We'll install and run this based on the open-craft/openedx-platform#532.

Is this step ready for review?

@Kelketek
Kelketek force-pushed the fox/BB-7295-initial-circuit branch from 3b4ea77 to 54b1b32 Compare April 25, 2023 17:14
@Kelketek

Copy link
Copy Markdown
Member Author

Thanks, @Agrendalath . I'm not sure why they weren't appearing for me earlier. I've added the workflows back, and the docs, which I've updated. Of course as you know, most of these will fail at the moment until Sathis completes the CI changes needed.

Is this step ready for review?

Yes.

@Agrendalath

Agrendalath commented Apr 26, 2023

Copy link
Copy Markdown
Member

@Kelketek, by this comment, I meant restoring the CHANGELOG.rst, not the whole docs directory. Having "Check the docs directory <docs/>_" that points to a plain directory without a clear table of content is simply ugly. Having the docs directory for this repo is overkill at this point - a simple README with a clear description is enough.

(...) until Sathis completes the CI changes needed.

Did I miss something? What changes are you referring to?

@Kelketek

Copy link
Copy Markdown
Member Author

@Kelketek, by #1 (comment), I meant restoring the CHANGELOG.rst, not the whole docs directory. Having "Check the docs directory _" that points to a plain directory without a clear table of content is simply ugly. Having the docs directory for this repo is overkill at this point - a simple README with a clear description is enough.

I know you didn't mean this-- I had elected to pull it back in since a bunch of small snippets were referring to the docs so I put it back in, since it seemed very 'may as well'. But you're right, it's verbose. I've re-removed it.

Did I miss something? What changes are you referring to?

Possibly. Here's a recap: Back in Columbia I had spoken with Braden asking them if there was any current CI solution to apps like this, which rely on platform internals to run. His answer was that he knew of none and it's a continued issue. This is why I removed most of the workflows. Once I submitted this PR though, you mentioned you want us to find a way to get CI running anyway. But that's a sizable project, so I scheduled a follow-up ticket, which is currently assigned to Sathis.

@Agrendalath

Copy link
Copy Markdown
Member

@Kelketek,

But that's a sizable project, so I scheduled a follow-up ticket, which is currently assigned to Sathis.

Ok, I see that you didn't add this ticket to any epic, so that's why I couldn't find it. Also, you added me as a reviewer while creating a ticket, so I didn't get any notification. IIRC, Jira sends the message if you do the assignments as a separate step.

@Kelketek

Copy link
Copy Markdown
Member Author

Great. @Agrendalath was there anything else you needed addressed on this PR? Did we want to move forward with merging it in anticipation of running it with the Nutmeg branch?

@Kelketek

Kelketek commented May 2, 2023

Copy link
Copy Markdown
Member Author

@Agrendalath Ping! See above question. :)

@Agrendalath Agrendalath left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

See my review comment before merging.

👍

  • I tested this: followed the testing instructions
  • I read through the code
  • I checked for accessibility issues: n/a
  • Includes documentation: not yet ⚠️
  • I made sure any change in configuration variables is reflected in the corresponding client's configuration-secure repository: n/a

Comment thread README.rst Outdated

Allows course authors to factor sections from Open edX courses into their own new course.

Check the `docs directory <docs/>`_ for details on how to set up and configure this application.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I suppose you forgot to add these back.

@Kelketek
Kelketek force-pushed the fox/BB-7295-initial-circuit branch 2 times, most recently from 78478e7 to 3863732 Compare May 3, 2023 20:57
@Kelketek
Kelketek force-pushed the fox/BB-7295-initial-circuit branch from 3863732 to 3e3c847 Compare May 3, 2023 21:07
@Kelketek

Kelketek commented May 3, 2023

Copy link
Copy Markdown
Member Author

@Agrendalath Went ahead and added the instructions to the README. If you find any issue with them, I'll go ahead and fix it in #2 . Merging now.

@Kelketek
Kelketek merged commit 95a819b into master May 3, 2023
@Kelketek
Kelketek deleted the fox/BB-7295-initial-circuit branch May 3, 2023 21:13
@Kelketek

Kelketek commented May 3, 2023

Copy link
Copy Markdown
Member Author

@Agrendalath I realized after I merged this that I forgot to squash, so since we haven't deployed yet I went ahead and did so on master. Not something I anticipate being able to do again. cc @sathiscode

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants