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

refactor: remove EdX-Api-Key usage #2217

Merged
merged 4 commits into from
May 24, 2024

Conversation

mudassir-hafeez
Copy link
Contributor

@mudassir-hafeez mudassir-hafeez commented May 20, 2024

What are the relevant tickets?

#2102

Description (What does it do?)

This PR removes EDX_API_KEY usage as Open edX is going to remove the EDX_API_KEY. Details can be found at openedx/edx-platform#34039. It also fixes a potential impact on the deactivate_enrollment API for non-staff users if the EdX-Api-Key is removed.

Screenshots (if appropriate):

  • Desktop screenshots
  • Mobile width screenshots

How can this be tested?

  • Fetch, check out to this branch, install edx-api-client with editable mode having this branch, and make sure edx is configured.
  • Test all the endpoints that currently utilize EdX-Api-Key for client requests. For example.
    • You can check the following tasks if they are working with open edX without issue.
    • openedx.tasks.change_edx_user_email_async
    • openedx.tasks.change_edx_user_name_async
    • openedx.tasks.create_edx_user_from_id
    • openedx.tasks.create_user_from_id
    • openedx.tasks.regenerate_openedx_auth_tokens
    • openedx.tasks.repair_faulty_openedx_users
    • openedx.tasks.retry_failed_edx_enrollments
    • openedx.tasks.update_edx_user_profile
    • courses.tasks.subscribe_edx_course_emails
    • courses.tasks.sync_courseruns_data
      You can also use management commands to test where applicable. i.e:
    • regenerate_edx_auth_tokens
    • repair_missing_courseware_records
    • retry_edx_enrollment
    • sync_enrollments
    • update_edx_profiles

Important to be tested:

  • Test user enrollment with different course "privileged" modes, i.e., audit, and verified.
    • This is how I tested, But It would be good if the tester tests the different enrollment through front-end.
from edx_api.client import EdxApi
api = EdxApi({"access_token": "<ACCESS_TOKEN>"}, base_url="http://host.docker.internal:18000")
api.enrollments.create_student_enrollment(course_id="course-v1:edX+DemoX+Demo_Course", username="verified", mode="audit")
  • Test the deactivate_enrollment API, here are the few mitxonline features/management commands I found that directly or indirectly use this API.
    • unenroll_enrollment
    • refund_fulfilled_order
    • transfer_enrollment
      or can test the course unenroll from user dashboard.

Additional Context

@mudassir-hafeez mudassir-hafeez marked this pull request as ready for review May 21, 2024 13:09
@mudassir-hafeez mudassir-hafeez force-pushed the mudassir/remove_edx_api_key_usage branch from c93f5ac to 8f7a319 Compare May 22, 2024 12:17
@asadali145 asadali145 self-assigned this May 22, 2024
@mudassir-hafeez mudassir-hafeez force-pushed the mudassir/remove_edx_api_key_usage branch from 9b73ef9 to 45f6e92 Compare May 24, 2024 08:59
@mudassir-hafeez
Copy link
Contributor Author

mudassir-hafeez commented May 24, 2024

@asadali145 can you please review as discussed to bump edx-api-client version to 1.9.0? The edx-api-client version 1.9.0 was published at mitodl/edx-api-client#105.

Copy link
Contributor

@asadali145 asadali145 left a comment

Choose a reason for hiding this comment

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

LGTM!

There is an issue with the enrollments which is not part of this PR. I will try to finalize the steps and create another issue.

CC: @arslanashraf7

@mudassir-hafeez mudassir-hafeez merged commit 5b0c3d4 into main May 24, 2024
7 checks passed
@arslanashraf7
Copy link
Contributor

arslanashraf7 commented May 24, 2024

@asadali145 / @mudassir-hafeez Thanks. It would be good to keep MITx Online dev team in loop about this change.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants