refactor: Duplicate and update primitives made available.

[Kelketek]

Description

This pull request makes a couple of changes to the xblock handler in the CMS. These changes add a handful of utility functions and modify the existing ones to make reuse of existing blocks easier. With these changes, it is possible to copy an entire section from one course to another, and then later refresh that section, and all of its children, without destroying the blocks next to it.

The existing _duplicate_block function was modified to have a shallow keyword to avoid copying children, and the update_from_source function was added to make it easy to copy attributes over from one block to another. These functions can be used alongside copy_from_template in the modulestore to copy over blocks and their children without requiring them to be within any particular container (other than a library or course root)-- thus allowing library-like inclusion without the library content block. This is especially useful for cases like copying sections rather than unit content.

The actual code for copying sections into new courses is not included, as the UX for such a feature is not yet decided, but prototypical Django admin functionality is being built in a separate Django app repository.

From the perspective of the user, this PR should change nothing. From the perspective of the developer, it should make things much easier.

Testing instructions

The unit tests should prove this is working correctly and doesn't impact existing functionality. However a companion Django app is being developed which leverages these features and can be made available to test as well.

Deadline

None

Misc

Internal/private-ref: BB-7295

[openedx-webhooks]

Thanks for the pull request, @Kelketek! Please note that it may take us up to several weeks or months to complete a review and merge your PR.

Feel free to add as much of the following information to the ticket as you can:

• supporting documentation
• Open edX discussion forum threads
• timeline information ("this must be merged by XX date", and why that is)
• partner information ("this is a course on edx.org")
• any other information that can help Product understand the context for the PR

All technical communication about the code itself will be done via the GitHub pull request interface. As a reminder, our process documentation is here.

Please let us know once your PR is ready for our review and all tests are green.

[Kelketek]

@Agrendalath This is ready for another look.

[Agrendalath]

👍

• I tested this: checked that duplicating the blocks is still working; checked that this plugin can use new functions
• I read through the code
• I checked for accessibility issues: n/a
• Includes documentation: n/a
• I made sure any change in configuration variables is reflected in the corresponding client's configuration-secure repository: n/a

[Kelketek]

@bradenmacdonald This is ready for you.

[Kelketek]

@bradenmacdonald Forgot Piotr was a CC and can move this forward-- but heads up on the changes in any case.

[bradenmacdonald]

From what I can see, a lot of the diff is just moving some things to separate functions.

So is this what's actually changed?

• Added shallow to duplicate_block
• Turned duplicate_block from internal method to public API (should perhaps be moved out of views then?)
• Added a new update_from_source python API, but it's not used by the platform

These functions can be used alongside copy_from_template

So there are three functions, duplicate_block, update_from_source, and copy_from_template. Can you give an example of how you're planning to use them? I feel like it may be a little unclear the differences between them and when/how to decide which to use, so I'd like to see that made clearer within the code/docstrings. I'm also wondering if we need three separate high level APIs when it seems like it's all aiming to do one thing: copy blocks into a destination and update them if they already exist there.

without requiring them to be within any particular container (other than a library or course root)

I was a little confused at first because I thought you meant "without requiring the source block to be in any particular container like a library". But I see you mean "without requiring them to be placed into a library content block or any other "special" parent container".

So my question is: how does the currently available API require the use of a particular container for the destination, and how does this PR remove that requirement?

[bradenmacdonald]

Can you please document all these arguments? It doesn't say what shallow or is_child do, nor the purpose of dest_usage_key.

[bradenmacdonald]

If this is a python API that you intend to use from other python code and which doesn't itself provide a REST API, it doesn't belong in views/. It should be in api.py or similar. (And maybe it should be in the modulestore code somewhere instead of contentstore? Not really sure on that.) I know the existing code in this file doesn't follow such conventions very clearly, but we can at least not make it worse. OEP-49 for reference.

[Kelketek]

@bradenmacdonald I've addressed a few of your notes and responded to some comments. I've added more to the docstrings to make it clearer when they're to be used.

The remaining issue is a much larger one and I'm not sure how to approach it. That issue being 'moving these functions into their own API module', which sounds simple, but there is already an api module in contentstore, so either I would need to rename that module, or put this new api module within the existing api package, or call it something else. The simplest solution would be to stuff them in some other module that, while it wouldn't follow OEP-45, would at least not be in the middle of the views. This might be the best option.

Another alternative would be to place it in the modulestore package. There is not currently an api package in the modulestore, and this seems like an odd place to start-- especially since we didn't make one as part of BD-13. However, it is an option. Another potential option might be to turn these functions into function that live on the modulestore itself, but, oddly enough, there's already a copy function within the modulestore. So we'll have copy, copy_from_template, and duplicate_block. Not ideal.

I don't know that any approach is going to be perfect, but I wanted to see if you had any other ideas. Otherwise I'll probably try just moving the functions out of the views to another module, even if it's not named api.py.

[bradenmacdonald]

@Kelketek You're right, this is an older part of the code that's definitely not following the conventions I'm referring to, so it's difficult to integrate with it in that way. I'm actually fine with what you're doing here, though I think that putting it into any file outside of views is better (maybe cms/djangoapps/contentstore/utils.py ?). I don't want to put a larger refactor onto your shoulders just because you're adding one little python method.

What's important is that it's clear who will be using this method. As it is, if it looks like a helper method in views/block.py but none of the views in that file are using it, I'd be inclined to think it's a "forgotten" helper method that's no longer used, and delete it. So that's something a good docstring can help with. As it stands I would see this method as "ready for deletion".

[Kelketek]

@bradenmacdonald OK-- I think this is ready now. Assuming we don't want to remove the tests for asides, that is-- and it sounds like maybe we don't, since now more than one group is using them and there appears to be no interest in deprecating them.

[Kelketek]

@bradenmacdonald Thanks-- I've rebased this and updated the docstrings you mentioned.

[Kelketek]

@bradenmacdonald @Agrendalath Now that this is approved, is there anything left to do on my end or can one of you merge this in?

[openedx-webhooks]

@Kelketek 🎉 Your pull request was merged! Please take a moment to answer a two question survey so we can improve your experience in the future.

[edx-pipeline-bot]

EdX Release Notice: This PR has been deployed to the staging environment in preparation for a release to production.

[edx-pipeline-bot]

EdX Release Notice: This PR has been deployed to the production environment.