feat: add use of new filter for instructor dash tabs#38499
feat: add use of new filter for instructor dash tabs#38499holaontiveros wants to merge 11 commits into
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.
A couple of items to address, mostly around matching established conventions for filter consumers in this codebase. Also consider moving the filter call before the sort — currently plugin-added tabs will always appear at the end regardless of their sort_order, since the filter fires after sorting. No tests for the filter integration — other filter consumers in edx-platform have tests (e.g., test_instructor_dashboard.py tests InstructorDashboardRenderStarted).
Review assisted by Kiro
dwong2708
left a comment
There was a problem hiding this comment.
I like how clever this PR is. Overall, it looks good — I just left a few comments.
|
I see Taylor and Daniel are already reviewing. Did you need another reviewer or are you all set? |
Taylor is on vacations and Daniel doesn't have CC rights (he is still taking the course) so I do need an approval and merge, the last round of changes from Daniel were addressed so I would expect to be ready |
| filtered_tabs = InstructorDashboardTabsRequested.run_filter( | ||
| tabs=tabs, | ||
| user=request.user, | ||
| course_key=course_key | ||
| ) | ||
| custom_tabs = filtered_tabs if filtered_tabs is not None else tabs |
There was a problem hiding this comment.
I'm pretty new to filters, so forgive me if I'm wrong, but my understanding is that the user and course_key params are also supposed to be passed through the filter as well. That way, other plugins can also add pipeline steps which all operate on the result of the previous pipeline steps.
| filtered_tabs = InstructorDashboardTabsRequested.run_filter( | |
| tabs=tabs, | |
| user=request.user, | |
| course_key=course_key | |
| ) | |
| custom_tabs = filtered_tabs if filtered_tabs is not None else tabs | |
| filtered_tabs, user, course_key = InstructorDashboardTabsRequested.run_filter( | |
| tabs=tabs, | |
| user=request.user, | |
| course_key=course_key | |
| ) | |
| custom_tabs = filtered_tabs if filtered_tabs is not None else tabs |
I believe this would require fixing the upstream filter definition. Unlike the other filters I see in openedx-filters, InstructorDashboardTabsRequested.run_filter seems to just return the tabs. I'd expect it to return tabs, user, course_key.
Does that make sense, or am I off base here? @magajh , if you have a chance to look, let me know if you agree with my suggestion.
There was a problem hiding this comment.
I leav a quick link were Felipe Montoya also commented on that:
There was a problem hiding this comment.
I believe that the current code will superficially work in specific scenarios because of this clause in run_pipeline, which stops the filter if a non-dict is returned: https://github.com/openedx/openedx-filters/blob/8db18547dd8feb5c30e15e66e75c902388b267d0/openedx_filters/tooling.py#L211-L216.
So that will work OK if and only if:
- the final InstructorDashboardTabsRequested pipeline step returns a just a list of tabs
- the previous InstructorDashboardTabsRequested steps return the full (tabs, user, course_key) so that they can be passed on to the next filter pipeline steps
But we can't count on that always being the case. So in order to support multiple plugins adding pipeline steps to InstructorDashboardTabsRequested, I believe that the type signature of each pipeline step should take and return the same thing. That's why the convention exists, even if a bit cumbersome: it lets the pipeline steps flow together, ignorant to what filter step comes before or after them.
There was a problem hiding this comment.
Now I'm in doubt myself. I'll need to dive deep into the pipeline definition to see how it behaves in this case.
Description
In order to achieve openedx/frontend-app-instructor-dashboard#86 this PR will add use the new filter for the instructor dash tabs
depens on: openedx/openedx-filters#355
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-filters changes need to be in place
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
Other information
Include anything else that will help reviewers and consumers understand the change.