refactor: inheritable authoring mixin callbacks for editing & duplication #33756
Conversation
openedx-webhooks
added
the
open-source-contribution
|
Thanks for the pull request, @DanielVZ96! 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:
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. |
|
Thanks @DanielVZ96 ! When you're ready for review, let me know. |
DanielVZ96
force-pushed
the
dvz/studio-editable-block-callback-refactor
branch
2 times, most recently
from
6a598c5 to
3b82bbe
Compare
…uplication Solves openedx#33715 Instead of applying duplicated logic after getattr, that logic is incorporated into inheritable methods of the StudioEditable block. Risks: - Assumes all cms blocks that are duplicated inherit from StudioEditableBlock.
DanielVZ96
force-pushed
the
dvz/studio-editable-block-callback-refactor
branch
from
49da980 to
5901e25
Compare
| @@ -296,46 +294,6 @@ def modify_xblock(usage_key, request): | |||
| ) | |||
|
|
|||
|
|
|||
| class StudioPermissionsService: | |||
There was a problem hiding this comment.
moved into services for reusability and reduce dependencies too
cms/djangoapps/contentstore/xblock_storage_handlers/view_handlers.py
Outdated
Show resolved
Hide resolved
cms/envs/common.py
Outdated
| @@ -969,6 +970,7 @@ | |||
| XModuleMixin, | |||
| EditInfoMixin, | |||
| AuthoringMixin, | |||
| StudioEditableBlock, | |||
There was a problem hiding this comment.
Is this allowed? this was the only way i could make sure all blocks would have the required callbacks
There was a problem hiding this comment.
@DanielVZ96 Great question.
I would rather not add StudioEditableBlock to this list, as that would make render_children and get_preview_view_name available on every XBlock. I'd like to avoid adding methods to the base XBlock API except in the cases where we really need to (like editor_saved, post_editor_saved, studio_post_duplicate).
That said, you are right that we need some way of defining editor_saved, post_editor_saved, and studio_post_duplicate on every block. I was mistaken that we could just add them to StudioEditableBlock. Could I suggest adding them to AuthoringMixin instead, which is already mixed into all CMS blocks?
There was a problem hiding this comment.
Done! thanks for the suggestion
|
@kdmccormick @Agrendalath ready for review |
DanielVZ96
force-pushed
the
dvz/studio-editable-block-callback-refactor
branch
from
33778c5 to
b4a4c96
Compare
There was a problem hiding this comment.
Thanks for taking this on @DanielVZ96 .
As I understand it, this is how duplicate_block would now work:
- duplicate_block calls dest.studio_post_duplicate....
- by default, that calls dest.handle_children_duplication, which calls util.handle_children_duplication, and returns True.
- but either dest.studio_post_duplicate or dest.handle_children_duplication could be overridden...
- and if dest.studio_post_duplicate returns False, then we call util.handle_children_duplication directly.
- Through all of the above, duplicate_block passes itself in as an argument, so that it can be recursively called on children.
I found that a bit hard to follow. I'd like to find a way to do this that'd yield a smaller stack of calls, and would avoid passing around recursive functions as args.
As an alternative, I think we could have AuthoringMixin define both:
- studio_duplicate, containing the logic currently in duplicate_block; and
- studio_post_duplicate, containing the logic currently in util.handle_children_duplication.
(The duplicate_block function itself would just become a thin wrapper around xblock.studio_duplicate.)
In other words, studio_duplicate would handle duplicating just the top-level block itself, and then it would delegate to studio_post_duplicate to handle children. The latter would call child.studio_duplicate(...) in order to recursively handle children, so we wouldn't need the duplication_function arg any more. Classes like LibraryContentBlock could still override studio_post_duplicate for special children handling.
This would eliminate the need for returning a children_handled boolean, and it would eliminate the handle_children_duplication layer.
What do you think?
|
Thanks @DanielVZ96 . Your code looks good to me. Through no fault of your own, we're currently wading through a couple different bugs around Library Content duplication and copying (one that we believe we've fixed, and another the we haven't yet). In light of that, I'd like to hold off on merging this refactoring until both bugs are solidly resolved. From your end, feel free to consider this "Done", and I can take care of rebasing and merging it when we're ready. Thanks for your patience! |
|
@mphilbrick211 This PR is all set from the author side, but is blocked by some related work. I'll rebase & merge when it's unblocked. |
|
FYI @bradenmacdonald -- this PR refactors the code that we are thinking of mirroring for copy-paste. In particular, it puts the callbacks into the class hierarchy rather than using getattr. I was going to wait until we land the copy-paste fix before merging this, but feel free to merge this first if you'd like. |
cms/lib/xblock/authoring_mixin.py
Outdated
| By default, is a no-op. Can be overriden in subclasses. | ||
| """ | ||
|
|
||
| def studio_duplicate( |
There was a problem hiding this comment.
This function feels very much like runtime code to me. What's the rationale for putting it into an XBlock mixin rather than the runtime? It seems like the goal here is to make studio_duplicate "inheritable" but I'm not sure that's a good thing, nor does it seem to be used?
And what about duplicating in v2 content libraries - are we going to have a NewRuntimeAuthoringMixin that implements studio_duplicate and studio_post_duplicate for the new/blockstore/learning-core runtime?
I guess my assumption has been that methods on XBlocks or their mixins generally only interact with the core XBlock API, the runtime, and/or runtime services. Any code that's specific to modulestore should be abstracted behind a runtime method or a service. I know we haven't always followed this pattern consistently, but I thought we had been moving in that direction.
There was a problem hiding this comment.
^ Note: This is more a question than a request for changes. And probably @kdmccormick and @Agrendalath are the ones best positioned to answer :)
There was a problem hiding this comment.
I guess my assumption has been that methods on XBlocks or their mixins generally only interact with the core XBlock API, the runtime, and/or runtime services. Any code that's specific to modulestore should be abstracted behind a runtime method or a service. I know we haven't always followed this pattern consistently, but I thought we had been moving in that direction.
Yeah, that makes sense to me. This PR is part of a bigger GitHub issue (#33640), which I haven't been tracking closely enough to answer this, though. @kdmccormick definitely has a better idea about the planned follow-ups.
There was a problem hiding this comment.
Good point @bradenmacdonald . This does seem like runtime capability rather than a block capability. Until recently I didn't really grasp the distinction between those two :P
@DanielVZ96 and @Agrendalath , when I wrote up the issue for this refactoring, I thought it'd be pretty straightfoward and self-contained. In reality, it seems like it's wrapped up in the complexities of Content Libraries V2 and copy-paste. I should have warned you folks of that when you originally opened your draft PR; that's my bad. I need to spend the next couple of work days sorting out our plan for duplication, copy-paste, and import for library_content blocks, and then I'll come back with a recommendation of what to do here. Thanks for your responsiveness and patience so far.
cms/lib/xblock/authoring_mixin.py
Outdated
| def studio_post_duplicate( | ||
| self, | ||
| source_block, | ||
| store, |
There was a problem hiding this comment.
If we're changing the API of studio_post_duplicate, I would love to find a way to remove store, since it's specific to modulestore. The store parameter wasn't actually used prior to this PR.
Passing user also seems a bit redundant as the user is available via the user service, as an ID via self.scope_ids.user_id, and via self.runtime.user (non-standard but works in all Open edX runtimes).
There was a problem hiding this comment.
i agree, i really wanted to not inject the store dependency but this was the only way i figured i could keep the exact same behavior/code as before. The API is very recent and I doubt anyone uses it considering it's also internal.
I'll wait until @kdmccormick comes back to see what we can do to improve this API.
|
@kdmccormick hey, do you have any updates so we can move this forward? |
|
I'll come back and re-review this once #34066 and openedx/XBlock#718 merge, which I think should take about a week. |
|
@kdmccormick hey, just to be sure, one of the PRs is merged, but the other one is stale since like 3 weeks ago. do you have an updated eta? |
|
@DanielVZ96 I just managed to merge the second one today--phew. @bradenmacdonald Once we store the upstreams and defaults of LibraryContentBlock children, we should be able to copy-paste and duplicate LibraryContentBlocks just like any other XBlock. So, it seems like we would no longer need a studio_duplicate hook--we could just delete it after implementing the linked ADR, right? If that sounds right to you, do you agree that this PR should leave studio_duplicate alone, but still add declarations for the editor_saved and post_editor_saved hooks? |
|
@kdmccormick We [will] still need the I'm in favor of:
|
|
@kdmccormick What's left before we can get this PR merged? |
Agreed, please revert the studio_duplicate changes from this PR. Sorry for sending you that direction @DanielVZ96 , that was poor architectural advice on my part.
I believe there will be no remaining use cases once this ADR is implemented, so it would be good to then remove the hook. I'll add that to the running list of content libraries cleanup items.
Agreed. These are a nice improvement. |
DanielVZ96
force-pushed
the
dvz/studio-editable-block-callback-refactor
branch
from
136e945 to
e94fa52
Compare
DanielVZ96
force-pushed
the
dvz/studio-editable-block-callback-refactor
branch
from
46b952f to
5312b5e
Compare
| @@ -47,7 +48,6 @@ | |||
| from cms.djangoapps.contentstore.xblock_storage_handlers.view_handlers import ( | |||
| handle_xblock, | |||
| create_xblock_info, | |||
| load_services_for_studio, | |||
There was a problem hiding this comment.
it's an indirect import, should be going directly to contentstore.utils
| @@ -45,7 +45,7 @@ | |||
| wrap_xblock_aside | |||
| ) | |||
|
|
|||
| from ..utils import get_visibility_partition_info, StudioPermissionsService | |||
| from ..utils import StudioPermissionsService, get_visibility_partition_info | |||
|
@kdmccormick @bradenmacdonald I think this is finally ready for one last review |
|
@DanielVZ96 Those changes look good to me! Do we have an XBlock that's ready to use these new APIs? I just don't want to merge without a concrete use case. |
|
@bradenmacdonald For what it's worth, I don't see this as a public API, just a formalization of an internal edx-platform API. |
|
@DanielVZ96 🎉 Your pull request was merged! Please take a moment to answer a two question survey so we can improve your experience in the future. |
|
2U Release Notice: This PR has been deployed to the edX staging environment in preparation for a release to production. |
|
2U Release Notice: This PR has been deployed to the edX production environment. |
1 similar comment
|
2U Release Notice: This PR has been deployed to the edX production environment. |
Description
Instead of applying duplicated logic after getattr, that logic is incorporated into inheritable methods of the StudioEditable block. Should not affect other roles besides developers.
Supporting information
Resolves #33715
Testing instructions
Courses:
Libraries:
Deadline
None