feat: add filter for instructor mfe tabs#355
Conversation
|
Thanks for the pull request, @holaontiveros! This repository is currently maintained by Once you've gone through the following steps feel free to tag them in a comment and let them know that your changes are ready for engineering review. 🔘 Get product approvalIf you haven't already, check this list to see if your contribution needs to go through the product review process.
🔘 Provide contextTo help your reviewers and other members of the community understand the purpose and larger context of your changes, feel free to add as much of the following information to the PR description as you can:
🔘 Get a green buildIf one or more checks are failing, continue working on your changes until this is no longer the case and your build turns green. DetailsWhere can I find more information?If you'd like to get more details on all aspects of the review process for open source pull requests (OSPRs), check out the following resources: When can I expect my changes to be merged?Our goal is to get community contributions seen and reviewed as efficiently as possible. However, the amount of time that it takes to review and merge a PR can vary significantly based on factors such as:
💡 As a result it may take up to several weeks or months to complete a review and merge your PR. |
wgu-taylor-payne
left a comment
There was a problem hiding this comment.
The filter concept and placement are solid — this is the right extension point for the MFE instructor dashboard. Main feedback: the run_filter method adds custom defensive logic that no other filter in the codebase uses. Simplifying it to match the standard pattern resolves most of the inline comments below. Also, the PR checklist marks the changelog entry as done but I don't see a scriv fragment in changelog.d/ in the diff.
Review assisted by Kiro
There was a problem hiding this comment.
Overall, it looks good, thanks! I just have a few comments:
- Could we link the
openedx-platformPR that executes the filter? - Although
scrivis configured in the repository, we still need to resolve a few things to get it 100% ready. Please manually add a new entry to theCHANGELOGand bump the version to3.4.0in both__init__andpyproject.toml. Thanks a lot!
Co-authored-by: Copilot <copilot@github.com>
|
@BryanttV I added the WIP of the platform PR and addressed your requests, thanks for the review |
felipemontoya
left a comment
There was a problem hiding this comment.
I think the usage is solid. I left some comments about some conventions worth following.
Often times filters are added at the botom of the file, but I think it would be better to keep this next to InstructorDashboardRenderStarted so people looking for instructor dashboards filters get the MFE version next to the non-mfe.
I think it would be a nice touch to add the comment in the docstring about this being the mfe version of the same pattern from the old filter. You could even add that in both docstrings so this flows into the auto generated documentation that reads such strings (e.g: https://arunmozhi.in/openedx-plugin-slots-browser/filters/InstructorDashboardRenderStarted/)
999b4c4 to
7e5fe6a
Compare
BryanttV
left a comment
There was a problem hiding this comment.
LGTM! Thanks
Since we add the entry manually, could we remove the entry generated in the changelog.d folder?
Co-authored-by: Felipe Montoya <felipe.montoya@edunext.co>
Co-authored-by: Felipe Montoya <felipe.montoya@edunext.co>
|
All done again, thanks for those suggestions @felipemontoya I did change the things but my autocomplete reverted some of them back, great catch Whenever someone with enough power has time, please merge it |
|
I'll merge later. I just need the ci to run |
|
The only outstanding issue comes from the commit messages that were suggested via the github UI and those will disappear during the squash and merge process. I'm moving on to merging now. |
Description
In order to achieve openedx/frontend-app-instructor-dashboard#86 this PR will add a filter so the new APIs for the instructor-dashboard app can get the tabs modifed by other plugins like aspects
PR for openedx-platform integration: openedx/openedx-platform#38499
The PR on the platform side will be on-hold while this get's merged
Once this is in place any operator / plugin creator can do something like:
in whichever shape fits their need to be able to conditionally add or modify data of the tabs for the instructor dashboard.
Testing instructions
Note: to test this through the API or the UI edx-platform changes that leverage this filter need to be available (TBD)
Take in account that the frontend should have a slot to manage the URL that it's being returned for example a slot like this
is added to the frontend-base build then the
custom_analyticsfrom the path from the plugin example will match the tabIdcustom_analyticsfrom here and the tab will be displayedThe API request that uses this is: `http://local.openedx.io:8000/api/instructor/v2/courses/{:courseId}
Deadline
None
Checklists
Check off if complete or not applicable:
Merge Checklist:
Post Merge: