-
Notifications
You must be signed in to change notification settings - Fork 1
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: reactive schedules nudge emails filter #56
base: nau/redwood.master
Are you sure you want to change the base?
Conversation
832050f
to
6513278
Compare
6513278
to
1aef6f0
Compare
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.
Thanks for those PRs, @BryanttV.
I tested it, and it worked as expected ✨
About this PR, I have a few comments:
- We probably don't need the
definition.py
file; we will have that code inopenedx-filters
. We can update theopenedx-filters
package here, and to add more context, we can add a reference to the filter we are using in the pipeline doc. Or are we adding that for another reason? - I am not sure which case we encountered the FieldError. If you have identified, I would like to know.
Hi @MaferMazu, thanks for the review!
That's right, the
This validation is only in case the plugin migrations have not been executed, and the model does not exist. Do you think we don't need it? |
805dcbb
to
d10bb12
Compare
I understand why you added it, but I think this part of the code shouldn't be responsible for the lack of migration; having the migrations is a pre-requisite. If you want, we can add a small note to the filter docs, but I think it is not necessary. |
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.
This looks good to me ✨
Thanks for these PRs
@BryanttV, as I said, this looks good, but I think we should add some tests. |
Hi @MaferMazu, I added the unit tests. |
Description
This PR adds a pipeline step to use the
ScheduleQuerySetRequested
filter. This step allows filtering the users in the Schedule QuerySet to keep only users with theallow_newsletter
field set toTrue
.Related Issues
Testing Instructions
Create a Tutor environment in the Redwood version.
Install this plugin with the changes in this PR.⚠️ Don't forget to run migrations.
Install
openedx-filters
with the changes in this PR.Create a mount of
edx-platform
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/
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.