Skip to content
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: adds the CourseEnrollmentSiteFilterRequested filter #47

Merged
merged 7 commits into from
Jan 18, 2023

Conversation

JuanDavidBuitrago
Copy link
Contributor

@JuanDavidBuitrago JuanDavidBuitrago commented Nov 16, 2022

Description:

This adds the CourseEnrollmentSiteFilterRequested filter which receive a Query Set context with enrollments information to allowing the filter pipeline to return the information that we need to filter.

An example of use is in eox-tenant custom filter step pipeline, where it takes the enrollments information and is filtered by tenant request, returning only the enrollments in the current site for the user.

JIRA: N/A

Dependencies:

Merge deadline: N/A

Installation instructions:

Be sure to use edx-platform branch with changes openedx/edx-platform#31331

Testing instructions:

The change can be tested with testing instructions in eox-tenant plugin.

Reviewers:

Merge checklist:

  • All reviewers approved
  • CI build is green
  • Version bumped
  • Changelog record added
  • Documentation updated (not only docstrings)
  • Commits are squashed

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
Copy link

openedx-webhooks commented Nov 16, 2022

Thanks for the pull request, @JuanDavidBuitrago! Please note that it may take us up to several weeks or months to complete a review and merge your PR.

Feel free to add as much of the following information to the ticket as you can:

  • supporting documentation
  • Open edX discussion forum threads
  • timeline information ("this must be merged by XX date", and why that is)
  • partner information ("this is a course on edx.org")
  • any other information that can help Product understand the context for the PR

All technical communication about the code itself will be done via the GitHub pull request interface. As a reminder, our process documentation is here.

Please let us know once your PR is ready for our review and all tests are green.

@openedx-webhooks openedx-webhooks added the open-source-contribution PR author is not from Axim or 2U label Nov 16, 2022
@JuanDavidBuitrago JuanDavidBuitrago changed the title (FFI-3) filter user's course enrollment by microsite request (FFI-3/DS-300) filter user's course enrollment by microsite request Nov 16, 2022
@JuanDavidBuitrago JuanDavidBuitrago force-pushed the JDB/add_filter_enrollment_site branch 4 times, most recently from af48a08 to 4640e69 Compare November 22, 2022 17:20
@JuanDavidBuitrago JuanDavidBuitrago changed the title (FFI-3/DS-300) filter user's course enrollment by microsite request feat: adds the CourseEnrollmentSiteFilterRequested filter Nov 22, 2022
@JuanDavidBuitrago JuanDavidBuitrago marked this pull request as ready for review November 23, 2022 21:18
@JuanDavidBuitrago
Copy link
Contributor Author

Hi @mariajgrimaldi and @felipemontoya, could you help me review this?

@mariajgrimaldi mariajgrimaldi self-assigned this Dec 5, 2022
@JuanDavidBuitrago JuanDavidBuitrago force-pushed the JDB/add_filter_enrollment_site branch 2 times, most recently from 0eb7d1e to 4879bd5 Compare December 16, 2022 22:11
@JuanDavidBuitrago
Copy link
Contributor Author

Hi @mariajgrimaldi!!!

Could you check again the last changes? Thanks!! 😃

openedx_filters/learning/filters.py Outdated Show resolved Hide resolved
openedx_filters/learning/filters.py Outdated Show resolved Hide resolved

class CourseEnrollmentQuerysetRequested(OpenEdxPublicFilter):
"""
Custom class used to filter user's course enrollments by site, when a request is made by the user.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great description! The issue is that not many people will use this filter as described here. That type of documentation goes to the pipeline steps. I suggest something like:

Suggested change
Custom class used to filter user's course enrollments by site, when a request is made by the user.
Custom class used to create course enrollments queryset filters and its custom methods.

This following the other filters docstrings.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @mariajgrimaldi, I modified this part.


filter_type = "org.openedx.learning.course_enrollment_queryset.requested.v1"

class PreventEnrollmentSiteFilter(OpenEdxFilterException):
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It would help if you changed the name here too

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, you're right. I forgot to change this.

Thanks for your comments. Could you check again?


class PreventCourseEnrollmentQueryset(OpenEdxFilterException):
"""
Custom class used to stop the course enrollment queryset filter process.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
Custom class used to stop the course enrollment queryset filter process.
Custom class used to stop the course enrollment queryset request process.


filter_type = "org.openedx.learning.course_enrollment_queryset.requested.v1"

class PreventCourseEnrollmentQueryset(OpenEdxFilterException):
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The wording here could be clearer. I like better: PreventEnrollmentQuerysetRequest, what do you folks think?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's ok, I agree with that. Let me know if you consider another change.

@mphilbrick211 mphilbrick211 added the needs test run Author's first PR to this repository, awaiting test authorization from Axim label Jan 10, 2023
@e0d e0d removed the needs test run Author's first PR to this repository, awaiting test authorization from Axim label Jan 12, 2023
Copy link
Member

@mariajgrimaldi mariajgrimaldi left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just one last comment :). Let me know how it goes!

@JuanDavidBuitrago
Copy link
Contributor Author

Just one last comment :). Let me know how it goes!

Thank you, @mariajgrimaldi I made some changes, could you check again?

Copy link
Member

@mariajgrimaldi mariajgrimaldi left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@mariajgrimaldi mariajgrimaldi merged commit 5fa2717 into openedx:main Jan 18, 2023
@openedx-webhooks
Copy link

@JuanDavidBuitrago 🎉 Your pull request was merged! Please take a moment to answer a two question survey so we can improve your experience in the future.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
open-source-contribution PR author is not from Axim or 2U
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

5 participants