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

Refactor StudioEditableBlock's callbacks for editing & duplication #33715

Closed
Tracked by #33640
kdmccormick opened this issue Nov 15, 2023 · 8 comments · Fixed by #33756
Closed
Tracked by #33640

Refactor StudioEditableBlock's callbacks for editing & duplication #33715

kdmccormick opened this issue Nov 15, 2023 · 8 comments · Fixed by #33756
Assignees
Labels
code health Proactive technical investment via refactorings, removals, etc.

Comments

@kdmccormick
Copy link
Member

kdmccormick commented Nov 15, 2023

Make studio_post_duplicate, editor_saved, and post_editor_saved real inheritable methods on StudioEditableBlock, rather than some getattr hackery, as suggested here: https://github.com/openedx/edx-platform/pull/7482/files#r27728847

Also, consider renaming:

  • studio_post_duplicate -> studio_duplicate
  • editor_saved -> studio_saving
  • post_editor_saved -> studio_saved
@kdmccormick kdmccormick changed the title Make studio_post_duplicate, editor_saved, and post_editor_saved real methods Declare base methods to StudioEditableBlock rather than using getattr Nov 15, 2023
@kdmccormick kdmccormick changed the title Declare base methods to StudioEditableBlock rather than using getattr Declare base methods on StudioEditableBlock rather than using getattr Nov 15, 2023
@kdmccormick kdmccormick added the code health Proactive technical investment via refactorings, removals, etc. label Nov 15, 2023
@kdmccormick kdmccormick changed the title Declare base methods on StudioEditableBlock rather than using getattr Refactor StudioEditableBlock's callbacks for editing & duplication Nov 15, 2023
@connorhaugh
Copy link
Contributor

Re: the rename:
This will require time, because we have to assume xblocks we don't know about (other than Library,Video and SplitTest) might implement what is now known as editor_saved. How do we go about changing an unofficial-ish part of the xblock api safely?

@kdmccormick
Copy link
Member Author

kdmccormick commented Nov 16, 2023

I would do a text search for the string editor_saved in the openedx org, recommend that someone at 2U do the same in the edx org, and lastly just mention the change in the named release notes.

I don't think DEPR or any other involved communication process is necessary here, since this was never a supported part of the public XBlock API.

@feanil
Copy link
Contributor

feanil commented Nov 16, 2023

It's important to remember that our APIs have not been historically strong so while people aren't supposed to use features, they still might be because of a lack of other options. It may be worth making and announcing the DEPR anyway if it's not significant overhead.

@kdmccormick
Copy link
Member Author

Maybe. To get edx-platform to a good state, I expect that we'll need to make many dozens of changes to things that aren't public APIs but might be in use by somebody. One DEPR is not significant overhead, but dozens of DEPRs would be.

Perhaps batching the refactors together into big DEPRs could work, a la [DEPR] Several changes to how Studio renders XBlocks

@DanielVZ96
Copy link
Contributor

Given that you also suggest renaming them, we could create new supported functions with those names and add a deprecation warning to the old ones. Then people can use the new inheritable methods when fixing the deprecation warnings.

Once there's a bunch of other DEPRs, they can all be removed at once in a singe big DEPR as suggested above.

@DanielVZ96
Copy link
Contributor

Also I can start working on implementing the base functions while deprecation is still in discussion

@DanielVZ96
Copy link
Contributor

@kdmccormick drafted the refactor here, please let me know if this looks like the approach you were looking for.

@kdmccormick
Copy link
Member Author

@DanielVZ96 Cool, I'll take a look tomorrow and let you know.

DanielVZ96 added a commit to DanielVZ96/edx-platform that referenced this issue Nov 25, 2023
…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.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
code health Proactive technical investment via refactorings, removals, etc.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants