feat: add schedule queryset requested filter #235

BryanttV · 2024-12-06T17:36:49Z

Description: This PR adds a filter to modify the default schedule queryset of the automatic email messages in edx-platform.

The filter receives a single schedules argument of type QuerySet. An example of use is in this NAU pipeline that allows filtering the Schedules to only send emails to users who have the option to receive emails enabled in their profile (allow_newsletter=True)

JIRA: N/A

Dependencies:

feat: add schedule queryset request filter integration edx-platform#35982

Merge deadline: NO

Installation instructions:

Create a Tutor environment.
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. Please use the changes in this branch (For testing purposes).

Testing instructions::

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:

name: filter-settings
version: 0.1.0
patches:
  openedx-common-settings: |
    OPEN_EDX_FILTERS_CONFIG = {
      "org.openedx.learning.schedule.queryset.requested.v1": {
        "fail_silently": False,
        "pipeline": [
          "nau_openedx_extensions.filters.pipeline.FilterUsersWithAllowedNewsletter",
        ],
      },
    }

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 of Schedules in the console. For example, here the Schedule was filtered because the user does not have the field set to True

Reviewers:

Merge checklist:

Post merge:

Create a tag
Check new version is pushed to PyPI after tag-triggered build is finished.
Delete working branch (if not needed anymore)

Author concerns: N/A

openedx-webhooks · 2024-12-06T17:36:54Z

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 approval

If you haven't already, check this list to see if your contribution needs to go through the product review process.

If it does, you'll need to submit a product proposal for your contribution, and have it reviewed by the Product Working Group.
- This process (including the steps you'll need to take) is documented here.
If it doesn't, simply proceed with the next step.

🔘 Provide context

To 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:

Dependencies

This PR must be merged before / after / at the same time as ...
Blockers

This PR is waiting for OEP-1234 to be accepted.
Timeline information

This PR must be merged by XX date because ...
Partner information

This is for a course on edx.org.
Supporting documentation
Relevant Open edX discussion forum threads

🔘 Get a green build

If 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 @openedx/hooks-extension-framework. Tag them in a comment and let them know that your changes are ready for review.

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:

The size and impact of the changes that it introduces
The need for product review
Maintenance status of the parent repository

💡 As a result it may take up to several weeks or months to complete a review and merge your PR.

mariajgrimaldi · 2024-12-10T14:25:39Z

Thanks for the contribution, @BryanttV! There's a PR against edx-platform@master, meaning this implementation should work with Tutor nightly. Is that correct? Can I use a nightly environment to test instead of Redwood? Let me know.

mariajgrimaldi

I haven't tested this proposal, I only did a static review. I'll be testing this once I get a reply to this comment. Thank you again!

mariajgrimaldi · 2024-12-10T14:33:39Z

openedx_filters/learning/filters.py

+    class PreventScheduleQuerySetRequest(OpenEdxFilterException):
+        """
+        Custom class used to stop the schedule queryset request process.
+        """
+
+        def __init__(self, message: str, schedules: QuerySet):
+            """
+            Override init that defines specific arguments used in the schedule queryset request process.
+
+            Arguments:
+                message (str): error message for the exception.
+                schedules (QuerySet): Queryset of schedules to be sent
+            """
+            super().__init__(message, schedules=schedules)


Can you elaborate on which situations we would need to halt the filtering the queryset?

You might need to halt the filter when you get an error in the QuerySet filtering process. For example, here, the PreventScheduleQuerySetRequest exception is raised if the nauuserextendedmodel is not found as an attribute of the user model.

I understand. But what would the management command's behavior be when the exception is raised? Would it change in any way?

No, it doesn't change. If the exception is raised, the same QuerySet is returned and the command execution continues.

I'm trying to figure out whether we need an exception in this case. For instance, see https://github.com/openedx/openedx-filters/blob/main/openedx_filters/learning/filters.py#L784-L800 which is used to replace the IDV URL with another URL; there's no different behavior when a condition is met. In contrast, when an enrollment is forbidden for X or Y, then an exception can be raised and the application handles the new behavior differently. Do you think we'd need an exception here?

We can use that exception if we want to prevent the schedules, but if not, I think it's better if we handle the default flow ("the same QuerySet is returned and the command execution continues") from the pipeline.

Comment related: openedx/edx-platform#35982 (comment).

Thanks for your comments! In that case, the exception is not necessary. If you agree, I will remove it.

mariajgrimaldi · 2024-12-10T14:35:25Z

openedx_filters/learning/filters.py

+    """
+    Custom class used to create schedule queryset filters filters and its custom methods.
+    """


I understand that these docstrings are kind of a template for these filters, but I admit that they don't say much, and I'd like that to change. Can we be more explicit about the filter's usage here and in the other docstrings? That would be very valuable.

I agree. I updated the docstring: d12254d

BryanttV · 2024-12-10T14:43:17Z

Thanks for the contribution, @BryanttV! There's a PR against edx-platform@master, meaning this implementation should work with Tutor nightly. Is that correct? Can I use a nightly environment to test instead of Redwood? Let me know.

Yes, You can also use a nightly environment.

mariajgrimaldi · 2024-12-10T14:50:49Z

I'll be reviewing openedx/edx-platform#35982 as well to verify the proposal. Thank you.

mariajgrimaldi · 2024-12-10T18:54:14Z

@BryanttV: Thanks for the thorough explanation in both this PR and edx-platform's. We really appreciate it! Can you add a bit more info in the cover letter about what needs to be configured in the NAU extended profile model? Also, what exactly should I look for in the command line to check that this works? I added a log in the pipeline, that way I knew it was being called when running the command.

BryanttV · 2024-12-11T15:35:47Z

Hi @mariajgrimaldi, thanks for the review! I updated the cover letter with more detailed instructions. Let me know if you need anything else.

openedx_filters/learning/filters.py

mariajgrimaldi · 2024-12-12T14:57:27Z

Can you also document this use case here? https://docs.openedx.org/projects/openedx-filters/en/latest/reference/real-life-use-cases.html. Thanks!

mariajgrimaldi

LGTM!

BryanttV · 2024-12-12T16:22:02Z

Hi @mariajgrimaldi, I added the use case: 362768f

openedx-webhooks added the open-source-contribution PR author is not from Axim or 2U label Dec 6, 2024

BryanttV mentioned this pull request Dec 7, 2024

feat: add schedule queryset request filter integration openedx/edx-platform#35982

Open

BryanttV force-pushed the bav/add-schedule-queryset-filter branch from 0f4efec to a476204 Compare December 9, 2024 14:37

BryanttV mentioned this pull request Dec 9, 2024

feat: reactive schedules nudge emails filter fccn/nau-openedx-extensions#56

Open

BryanttV marked this pull request as ready for review December 9, 2024 18:01

BryanttV requested a review from a team as a code owner December 9, 2024 18:01

mariajgrimaldi self-assigned this Dec 10, 2024

mariajgrimaldi requested changes Dec 10, 2024

View reviewed changes

BryanttV requested a review from mariajgrimaldi December 11, 2024 17:34

BryanttV force-pushed the bav/add-schedule-queryset-filter branch from 22bf9d0 to d12254d Compare December 11, 2024 17:35

BryanttV added 3 commits December 12, 2024 07:14

feat: add schedule queryset requested filter

9fc67a4

chore: bump version and add new entry in changelog

e103bc6

docs: improve filter docstring

1e3738f

BryanttV force-pushed the bav/add-schedule-queryset-filter branch from d12254d to 1e3738f Compare December 12, 2024 12:15

chore: remove PreventScheduleQuerySetRequest exception

fefab96

BryanttV requested a review from MaferMazu December 12, 2024 14:43

mariajgrimaldi reviewed Dec 12, 2024

View reviewed changes

openedx_filters/learning/filters.py Show resolved Hide resolved

mariajgrimaldi approved these changes Dec 12, 2024

View reviewed changes

BryanttV added 2 commits December 12, 2024 11:06

docs: add links about schedules feature documentation

f099864

docs: add use case in documentation

362768f

chore: update release date of new entry in changelog

ab13f63

mariajgrimaldi merged commit f234354 into openedx:main Dec 12, 2024
8 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: add schedule queryset requested filter #235

feat: add schedule queryset requested filter #235

BryanttV commented Dec 6, 2024 •

edited

Loading

openedx-webhooks commented Dec 6, 2024 •

edited

Loading

mariajgrimaldi commented Dec 10, 2024

mariajgrimaldi left a comment

mariajgrimaldi Dec 10, 2024

BryanttV Dec 11, 2024

mariajgrimaldi Dec 11, 2024

BryanttV Dec 11, 2024

mariajgrimaldi Dec 11, 2024

MaferMazu Dec 11, 2024 •

edited

Loading

BryanttV Dec 12, 2024

mariajgrimaldi Dec 10, 2024 •

edited

Loading

BryanttV Dec 11, 2024 •

edited

Loading

BryanttV commented Dec 10, 2024

mariajgrimaldi commented Dec 10, 2024

mariajgrimaldi commented Dec 10, 2024 •

edited

Loading

BryanttV commented Dec 11, 2024

mariajgrimaldi commented Dec 12, 2024

mariajgrimaldi left a comment

BryanttV commented Dec 12, 2024

feat: add schedule queryset requested filter #235

feat: add schedule queryset requested filter #235

Conversation

BryanttV commented Dec 6, 2024 • edited Loading

openedx-webhooks commented Dec 6, 2024 • edited Loading

What's next?

🔘 Get product approval

🔘 Provide context

🔘 Get a green build

🔘 Let us know that your PR is ready for review:

Who will review my changes?

Where can I find more information?

When can I expect my changes to be merged?

mariajgrimaldi commented Dec 10, 2024

mariajgrimaldi left a comment

Choose a reason for hiding this comment

mariajgrimaldi Dec 10, 2024

Choose a reason for hiding this comment

BryanttV Dec 11, 2024

Choose a reason for hiding this comment

mariajgrimaldi Dec 11, 2024

Choose a reason for hiding this comment

BryanttV Dec 11, 2024

Choose a reason for hiding this comment

mariajgrimaldi Dec 11, 2024

Choose a reason for hiding this comment

MaferMazu Dec 11, 2024 • edited Loading

Choose a reason for hiding this comment

BryanttV Dec 12, 2024

Choose a reason for hiding this comment

mariajgrimaldi Dec 10, 2024 • edited Loading

Choose a reason for hiding this comment

BryanttV Dec 11, 2024 • edited Loading

Choose a reason for hiding this comment

BryanttV commented Dec 10, 2024

mariajgrimaldi commented Dec 10, 2024

mariajgrimaldi commented Dec 10, 2024 • edited Loading

BryanttV commented Dec 11, 2024

mariajgrimaldi commented Dec 12, 2024

mariajgrimaldi left a comment

Choose a reason for hiding this comment

BryanttV commented Dec 12, 2024

BryanttV commented Dec 6, 2024 •

edited

Loading

openedx-webhooks commented Dec 6, 2024 •

edited

Loading

MaferMazu Dec 11, 2024 •

edited

Loading

mariajgrimaldi Dec 10, 2024 •

edited

Loading

BryanttV Dec 11, 2024 •

edited

Loading

mariajgrimaldi commented Dec 10, 2024 •

edited

Loading