Add a tutor plugin to easily run this on a devstack #50
Conversation
openedx-webhooks
added
the
open-source-contribution
|
Thanks for the pull request, @bradenmacdonald! 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. |
bradenmacdonald
force-pushed
the
tutor-devstack-plugin
branch
from
ea660fa
to
6e234e0
Compare
bradenmacdonald
force-pushed
the
tutor-devstack-plugin
branch
from
6e234e0
to
bb9ee65
Compare
|
@bradenmacdonald Thank you for your contribution. Please let me know once this is ready for our review. |
|
@natabene It's ready now. There is an error in the CI build but it's not related to this. I'm not sure if this repo has a maintainer at the moment? Because the build is failing because nobody is updating the dependencies of this repo and/or several related repos like @edx/frontend-component-footer |
|
Braden, I gave this a test (for local dev development) on my local tutor and it seems to work. I'll give a thumb here when we resolve the unrelated CI issues (which we are working on presently) and see this PR merged. |
|
@bradenmacdonald a rebase off master should fix the CI issues. |
bradenmacdonald
force-pushed
the
tutor-devstack-plugin
branch
from
76c1756
to
d719044
Compare
Codecov ReportBase: 42.20% // Head: 48.69% // Increases project coverage by
Additional details and impacted files@@ Coverage Diff @@
## master #50 +/- ##
==========================================
+ Coverage 42.20% 48.69% +6.49%
==========================================
Files 72 76 +4
Lines 1834 1957 +123
Branches 317 354 +37
==========================================
+ Hits 774 953 +179
+ Misses 1025 970 -55
+ Partials 35 34 -1
Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here. ☔ View full report at Codecov. |
|
@connorhaugh Thanks!! Yep, that did it. |
|
I tested this locally. tl;drI needed to run Questions
Play by playFirst issue I encountered was related to the line which, for testing purposes, i updated to be when running it, I encountered an error which I resolved by escaping the when running that, I encountered another error I'm not sure what the conventions around using I pushed the branch to my fork, and made a commit (brian-smith-tcril@4982704) updating installing this using worked as expected, at which point I uninstalled and decided to try which worked as expected |
|
After getting it running, I tried creating a content library and adding some components. Creating the library worked as expected, but every component is failing to load with the error: I'm thinking this may be related to the change from |
There was a problem hiding this comment.
Overall this is really great! I'm super excited to see MFEs working as tutor plugins!
The big one here is the change in plugin.py, without that this just doesn't work.
I'm not sure about escaping the & in the readme, but running the command didn't work for me without it. I'm also not sure what to do about the tutor_contrib_library_authoring_mfe vs tutor_library_authoring_mfe thing, so it'd be helpful to get some outside input on that.
Looking forward to seeing what we can do to get this merged in!
tutor-contrib-library-authoring-mfe/tutor_library_authoring_mfe/plugin.py
Outdated
Show resolved
Hide resolved
|
Thanks for the in-depth review, @brian-smith-tcril! From where I stand those are all great catches, though I'll let @bradenmacdonald weigh in and/or pull in your changes before we merge. Also,
There's some prior art by our colleague @kdmccormick: https://github.com/openedx/tutor-contrib-coursegraph Which is to say, I'd try to fix it in the other direction, if possible. (In other words, getting rid of the error when the package is prefixed with |
|
I'm excited about this plugin @bradenmacdonald, and thanks for working through that @brian-smith-tcril. I agree that the Python distribution name (the thing defined in The Python package name is good to remain as (the docs on all of this ^ aren't great yet, but they're deductible by reverse engineering https://github.com/overhangio/cookiecutter-tutor-plugin) Some other notes:
|
|
Thanks for the review @brian-smith-tcril ! I've pushed bd5f231 to make those fixes. Would you mind checking if that did it?
Using |
There was a problem hiding this comment.
Perfect!
I can verify this worked locally for me when I followed the instructions in tutor-contrib-library-authoring-mfe/README.rst when changing the install step to
pip install -e 'git+https://github.com/open-craft/frontend-app-library-authoring.git@tutor-devstack-plugin#egg=tutor-contrib-library-authoring-mfe&subdirectory=tutor-contrib-library-authoring-mfe'
I can verify again using the actual line from the README after this is merged.
|
@brian-smith-tcril @arbrandes Can we merge this now? |
|
@bradenmacdonald 🎉 Your pull request was merged! Please take a moment to answer a two question survey so we can improve your experience in the future. |
|
Let's merge this! Apologies for the delay, @bradenmacdonald, and thanks for the contrib! |
It's a bit tricky to get this working on a Tutor-based devstack, but this plugin should make it easy!
This updates the README to explain how to use this MFE on a Tutor-based devstack, using the included plugin. See https://github.com/openedx/frontend-app-library-authoring/blob/bb9ee6590cc715daaf74fe84947ccc0baf3a07b8/tutor-contrib-library-authoring-mfe/README.rst for the details on how to use it.
Tested end-to-end using the drag and drop v2 XBlock. Other XBlocks may not work correctly for unrelated reasons.
CI failure is unrelated.