-
Notifications
You must be signed in to change notification settings - Fork 3.9k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
feat: add schedule queryset request filter integration #35982
base: master
Are you sure you want to change the base?
feat: add schedule queryset request filter integration #35982
Conversation
Thanks for the pull request, @BryanttV! What's next?Please work through the following steps to get your changes 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. 🔘 Let us know that your PR is ready for review:Who will review my changes?This repository is currently maintained by Where 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. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hello @BryanttV, thanks for this.
I tested and the filter works as expected.
About this PR, I left a comment about the naming and flow.
I think your arguments about adding this filter are good, but I would like a second opinion about this.
Note: when we merge openedx/openedx-filters#235, we should update the openedx-filter version here.
aef53ab
to
224fcaf
Compare
Hi @MaferMazu, the openedx-filters PR was merged, so, I upgraded the version. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Everything worked as expected; the filter worked, and when the filter is not set, this follow the default flow.
This looks good to me. Thanks ✨
I don't know if @mariajgrimaldi has anything to add. If not, we could merge this.
@MaferMazu @BryanttV: I'll approve once unittests are included in the PR, you can see some examples here: https://github.com/openedx/edx-platform/pull/32448/files#diff-e74d6ca48afe9122896613ff0f5c27e885f505e95d2075e4893145623bc75583 |
1e5d268
to
dbf97ed
Compare
"""Test to verify the schedule queryset was modified by the pipeline step.""" | ||
schedules = self.resolver.get_schedules_with_target_date_by_bin_and_orgs() | ||
|
||
self.assertEqual(TestScheduleQuerySetRequestedPipelineStep.filtered_schedules, schedules) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
can we also test the default case when the filter is not configured?
Description
This PR introduces the integration of a new openedx-filter (
ScheduleQuerySetRequested
) designed to provide an intermediate point to perform more specific filtering on the Schedules QuerySet.Use Case Example
A practical example of its application is in NAU, where this filter will be used in a specific pipeline. Currently, when the Schedule Emails feature is enabled, learners receive emails by default without the option to disable it. With this new filter, it becomes possible to customize the behavior by applying additional filtering to include only learners who meet certain conditions, such as having the
allow_newsletter
field equal toTrue
.Benefits:
Supporting information
Testing instructions
Create a Tutor environment.
Create a mount of
edx-platform
with the changes in this branch (For testing purposes).Install⚠️ Don't forget to run migrations.
nau-openedx-extensions
plugin with the changes in this PR.Install
openedx-filters
with the changes in this PR.Create a new Schedule config for your site in
{lms_domain}/admin/schedules/scheduleconfig/
Create some users and enroll them in a course.
Create an NAU user extended model for each user in
{lms_domain}/admin/nau_openedx_extensions/nauuserextendedmodel/
. Update the following field, depending on whether you want to receive the newsletter.Create a Tutor inline plugin with the filter configuration:
Send a message of recurring nudge using the following command in the LMS container:
python manage.py lms send_recurring_nudge {site} --date {enroll-date + 3 days} # Example python manage.py lms send_recurring_nudge local.edly.io:8000 --date 2024-12-07
Depending on the value of each user's
allow_newsletter
field, you should see the filtered QuerySet ofSchedules
in the console. For example, here the Schedule was filtered because the user does not have the field set to TrueDeadline
None
Other information
Important
The PR in openedx-filters should be merged before this one.