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: CourseEnrollmentAllowed API #33059

Merged
merged 1 commit into from Sep 15, 2023

Conversation

MaferMazu
Copy link
Contributor

Description

This PR adds the methods GET and POST to CourseEnrollmentAllowed to have and usable API only for Staff or Owners.

Supporting information

We want to add this for the WooCommerce Plugin we are building to integrate third-party e-commerce with Open edX.

Testing instructions

GET /api/enrollment/v1/enrollment_allowed?email=user@example.com

Parameters

  • email (optional, string, query_params)

POST /api/enrollment/v1/enrollment_allowed

Example request data:

{
        "email": "user@example.com",
        "course_id": "course-v1:edX+DemoX+Demo_Course",
        "auto_enroll": true
}

Deadline

We would like to have this merged by 08/31/2023

Other information

ADR when we discuss these additions: eduNEXT/platform-plugin-ecommerce-api#1 (comment)

How to have a JWT Token

POST /oauth2/access_token
Body in Form:

client_id="CLIENT_ID"
client_secret="CLIENT_SECRET"
grant_type="client_credentials"
token_type="jwt"

@openedx-webhooks openedx-webhooks added the open-source-contribution PR author is not from Axim or 2U label Aug 18, 2023
@openedx-webhooks
Copy link

Thanks for the pull request, @MaferMazu! 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.

Copy link
Contributor

@feanil feanil left a comment

Choose a reason for hiding this comment

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

Mostly looks good, just a couple of small change requests!

openedx/core/djangoapps/enrollments/tests/test_views.py Outdated Show resolved Hide resolved
openedx/core/djangoapps/enrollments/tests/test_views.py Outdated Show resolved Hide resolved
openedx/core/djangoapps/enrollments/tests/test_views.py Outdated Show resolved Hide resolved
openedx/core/djangoapps/enrollments/views.py Outdated Show resolved Hide resolved
@MaferMazu
Copy link
Contributor Author

Thank @feanil; tomorrow, I will improve the PR and request again for your feedback.

@mphilbrick211 mphilbrick211 added the waiting on author PR author needs to resolve review requests, answer questions, fix tests, etc. label Aug 23, 2023
@MaferMazu MaferMazu force-pushed the mfmz/course-enrollment-allowed-api branch 4 times, most recently from fe5083a to d109239 Compare August 24, 2023 04:20
@MaferMazu
Copy link
Contributor Author

@feanil I applied your feedback; thanks again.

I still have a test failed, and the output is:

django.urls.exceptions.NoReverseMatch: Reverse for 'courseenrollmentallowed' not found. 'courseenrollmentallowed' is not a valid view function or pattern name.

I have that name in my URLs, but I wonder if I'm missing something. Do you know how to fix this error?

cc @felipemontoya

@MaferMazu MaferMazu requested a review from feanil August 25, 2023 16:27
@MaferMazu MaferMazu force-pushed the mfmz/course-enrollment-allowed-api branch 2 times, most recently from 39c2a71 to d46fe16 Compare August 25, 2023 22:21
@MaferMazu
Copy link
Contributor Author

Tests fixed!
cc @mphilbrick211 @feanil @felipemontoya

@mphilbrick211 mphilbrick211 removed the waiting on author PR author needs to resolve review requests, answer questions, fix tests, etc. label Aug 28, 2023
Copy link
Contributor

@pshiu pshiu left a comment

Choose a reason for hiding this comment

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

Great add of a serializer for the existing CourseEnrollmentAllowed model! Apologies, but still one security question:

openedx/core/djangoapps/enrollments/views.py Show resolved Hide resolved
openedx/core/djangoapps/enrollments/views.py Outdated Show resolved Hide resolved
openedx/core/djangoapps/enrollments/views.py Outdated Show resolved Hide resolved
openedx/core/djangoapps/enrollments/views.py Show resolved Hide resolved
openedx/core/djangoapps/enrollments/views.py Outdated Show resolved Hide resolved
@MaferMazu
Copy link
Contributor Author

Thanks for the patience, @pshiu and @felipemontoya. By the middle of this week, I can retake this effort.

@pshiu
Copy link
Contributor

pshiu commented Sep 5, 2023

(@MaferMazu No problem, thank you!)

@MaferMazu MaferMazu force-pushed the mfmz/course-enrollment-allowed-api branch 5 times, most recently from dcd8200 to 0df3229 Compare September 8, 2023 20:10
@MaferMazu
Copy link
Contributor Author

Changes in my latest commit:

  • Add the delete method
  • Add the delete tests
  • Apply the feedback: enrollment_allowed and isAdminUser
  • Refactor of the required args checks
  • Improve the test documentation

What do you think, @felipemontoya and @pshiu?

@MaferMazu MaferMazu force-pushed the mfmz/course-enrollment-allowed-api branch from 5042638 to cc45fa9 Compare September 14, 2023 18:38
@MaferMazu
Copy link
Contributor Author

@felipemontoya @pshiu, do you have more feedback about this API?

Copy link
Contributor

@pshiu pshiu left a comment

Choose a reason for hiding this comment

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

Thank you for incorporating the feedback, @MaferMazu. Great work.

@pshiu pshiu self-assigned this Sep 14, 2023
@pshiu
Copy link
Contributor

pshiu commented Sep 14, 2023

@MaferMazu, when you've finished collecting feedback from @felipemontoya & @feanil, give me a ping anytime and happy to be the merger & monitor deployment.

@mphilbrick211 mphilbrick211 added the waiting on author PR author needs to resolve review requests, answer questions, fix tests, etc. label Sep 14, 2023
Copy link
Contributor

@feanil feanil left a comment

Choose a reason for hiding this comment

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

One small nit, but otherwise looks good to me as well!

openedx/core/djangoapps/enrollments/urls.py Outdated Show resolved Hide resolved
Copy link
Member

@felipemontoya felipemontoya left a comment

Choose a reason for hiding this comment

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

Thanks for all the work @MaferMazu. With the latest round of changes addressing all the feedback I think this is good to go.

fix: correct test, add no staff test for post and apply feedback.

feat: add delete method and refactor the code

fix: correct quality tests

fix: add trailing slash to the url

Co-authored-by: Feanil Patel <feanil@axim.org>
@MaferMazu MaferMazu force-pushed the mfmz/course-enrollment-allowed-api branch from a8de7dd to 5939dcf Compare September 15, 2023 17:44
@MaferMazu
Copy link
Contributor Author

@pshiu, please give it a last look, and you can merge it when the tests are finished 🙌

@pshiu pshiu merged commit 76dbcde into openedx:master Sep 15, 2023
43 checks passed
@openedx-webhooks
Copy link

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

@edx-pipeline-bot
Copy link
Contributor

2U Release Notice: This PR has been deployed to the edX staging environment in preparation for a release to production.

@edx-pipeline-bot
Copy link
Contributor

2U Release Notice: This PR has been deployed to the edX production environment.

1 similar comment
@edx-pipeline-bot
Copy link
Contributor

2U Release Notice: This PR has been deployed to the edX production environment.

MaferMazu added a commit to eduNEXT/edx-platform that referenced this pull request Oct 2, 2023
0x29a pushed a commit to open-craft/edx-platform that referenced this pull request Apr 22, 2024
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 waiting on author PR author needs to resolve review requests, answer questions, fix tests, etc.
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

None yet

7 participants