Open
Conversation
|
Can one of the admins verify this patch? |
1 similar comment
|
Can one of the admins verify this patch? |
jdamerow
requested changes
Nov 24, 2025
Member
There was a problem hiding this comment.
This should be the generic class, that get parametrized with the type of object it handles and the repo doing the CRUD functionality, e.g.
abstract class ContentBlockManager<T, R> {
...
public void deleteBlockById(String id, String slideId) {
// here you get the repository via an abstract getRepository method
// that is implemented in the subclasses
getRepository().delete(...
}
...
and then the subclasses implement the getRepository method and all methods that are specific to a certain block type.
Does that make sense?
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Guidelines for Pull Requests
If you haven't yet read our code review guidelines, please do so, You can find them here.
Please confirm the following by adding an x for each item (turn
[ ]into[x]).Please provide a brief description of your ticket
ContentBlockManager class needs to be separated into multiple manager classes
Description
The contentblockmanager class is getting too big. Let's separate the logic out into multiple manager classes, one for each type of content block.
There is a lot of duplicate code in there. Let’s extract a superclass with a shared delete and and get method, and an abstract getRepository method. The subclasses will then implement getRepository to return the repository for the class they manage, and create and update methods.
Additionally, contentOrder should not be a parameter for the create methods. I’m fairly certain the controllers just use the findMaxContentOrder method to get the last index and then pass that into the create methods (please check). Instead, the create methods should call the findMaxContentOrder .
VSPC-209
Anything else the reviewer needs to know?
... describe here ...