feat: XBlock overrides

[nsprenkle]

Add ability to override another XBlock with the xblock.v1.overrides entrypoint.

Notes

• Fixes Xblock version 5.0.0 broke plugin #777 which removed our current hack for overriding existing XBlock behavior with a plugin.
• See conversation here for historical context & reasoning.

Usage

setup(
    # ...
    entry_points={
        "xblock.v1.overrides": [
            "html = my_new_repo.my_new_block:HtmlBlock",
            "problem = my_new_repo.my_new_block:ProblemBlock",
            "video = my_new_repo.my_new_block:VideoBlock",
        ],
    }
)

Tests

• Unit tests
• Smoke tests in LMS
  • Overridden block types work as expected
  • Non-overridden block types work as expected
• Tests in Studio
  • Overridden block types work as expected
  • Non-overridden block types work as expected
• Import tests
  • Overridden block types work as expected
  • Non-overridden block types work as expected
• Export tests
  • Overridden block types work as expected
  • Non-overridden block types work as expected
• Other????

[farhan]

@nsprenkle Thanks for creating the PR

• Didn't do the manual testing
• Done with the quick review, suggest some changes
• Was thinking we are merging then filtering the entrypoints here but to keep the signature of method seems alright to me

[nsprenkle]

@farhan, thanks for the review. Have tried to address all your comments and is ready for your re-review

[farhan]

@kdmccormick PR seems alright to me. Your review is mandatory.

[kdmccormick]

Very nice implementation. Thanks so much for jumping in and making this happen. Just a few requests.

[nsprenkle]

@kdmccormick and @farhan , have updated to your suggestions. Ready for re-review.

[kdmccormick]

A few more comments but nothing that needs re-review. I haven't manually tested, so I'm approving on the assumption that you have.

Thank a bunch @nsprenkle ! This will be awesome for site operators to have 🎸

changes addressed

[farhan]

Following approach seems clean to me.
@kdmccormick You can give a second thought

    # Prioritize and return a single unambiguous override; otherwise, return the single entry point.
    # Raise errors for missing, multiple entry points, or multiple overrides.
    if len(all_entry_points) == 0:
        raise PluginMissingError(identifier)
    elif len(all_entry_points) > 1:
        raise AmbiguousPluginError(all_entry_points)
    elif len(overrides) > 1:
        raise AmbiguousPluginOverrideError(overrides)
    elif len(overrides) == 1:
        return overrides[0]
    else:  # len(all_entry_points) == 1
        return all_entry_points[0]

1. Avoid introducing new method i-e- _default_select_no_override.
2. Put the the _default_select_no_override docs in default_select method docs.
3. Introduce separate Exception for multiple override. For example AmbiguousPluginOverrideError. We can also use AmbiguousPluginError but pass overrides and generate separate message for it.

[kdmccormick]

Regarding _default_select_no_override -- Yeah, I do not feel strongly since it is an internal implementation detail, but combining the functions does make it easier to follow IMO. If you are willing to make that change @nsprenkle, great; if not, I don't want to block merging on this concern.

Regarding the error message -- @farhan makes a very good point, we should make it clear whether there are multiple regular plugins vs. multiple overrides. As it is, the error cases would be indistinguishable to an operator.

@nsprenkle , I don't have a preference between a new exception class vs. the same exception with a different message. If you do go with separate exceptions, please just make AmbiguousPluginOverrideError a subclass of AmbiguousPluginError so that existing catch AmbiguousPluginError statements will catch AmbiguousPluginOverrideError.

[nsprenkle]

@kdmccormick , added as a new Exception

[farhan]

Swap both lines as per order

        If no classes are found for `{cls.entry_points}`, it will raise a `PluginMissingError`.


        If multiple classes are found for either `{cls.entry_points}.overrides` or
        `{cls.entry_points}`, it will raise an `AmbiguousPluginError`.

[nsprenkle]

Arguably, load_class doesn't even care about those details since those reside in the select function. I would prefer to delete them entirely than split hairs over the ordering.

[kdmccormick]

Arguably, load_class doesn't even care about those details since those reside in the select function.

+1, they are good as is

FWIW they are helpful lines to have in the docstring

addressed

[nsprenkle]

@kdmccormick / @farhan. Will merge / create a new release on Monday. Thanks, both!

[farhan]

@nsprenkle Nice work 🌟
Rebase the branch, Squash merge it and release the new version