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 X-EdX-Api-Key usage #104

Merged
merged 4 commits into from
May 22, 2024

Conversation

mudassir-hafeez
Copy link
Contributor

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

What are the relevant tickets?

#103

Description (What does it do?)

This PR removes the usage of X-EdX-Api-Key as Open edX is planning to deprecate the EDX_API_KEY. More details can be found at openedx/edx-platform#34039. The X-EdX-Api-Key permission scheme is introduced to the EdX API to allow both server-to-server access and authenticated user access. It is specifically introduced for the enrollment API (and potentially other APIs) to grant requests that use the API key access to view and modify other users' enrollments. Therefore, we need to ensure that any functionality using the enrollment and other APIs functions properly without the X-EdX-Api-Key.

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, and make sure edx is configured.
  • Test all the endpoints that currently use X-EdX-Api-Key for client requests. These should be tested thoroughly as this header will now be removed.
    • course_structure
    • course_detail
    • course_mode
    • enrollments
    • ccx
    • email_settings
    • certificates
    • user_info
    • bulk_user_retirement
  • Most of the endpoints can be tested manually using xpro, such as registering a user or enrolling in a course. However, testers can also use management commands to validate APIs. Some of these commands are listed below:
    • create_enrollment
    • sync_grades_and_certificates
    • sync_courseruns
      or by manually calling some tasks if the following tasks call the corresponding APIs on edX :
    • generate_course_certificates
    • sync_courseruns_data
    • change_edx_user_email_async
    • change_edx_user_name_async
    • create_edx_user_from_id
    • create_user_from_id

Important to be tested:

  • Test user enrollment with different course "privileged" modes, i.e., no-id-professional, audit, and verified.
    • For xpro, we use no-id-professional as the default mode. If there is a failover, we try to enroll the user in audit mode. However, the process may vary for mitxonline.
    • 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 Xpro features/management commands I found that directly or indirectly use this API.
    • defer_enrollment
    • refund_enrollment
    • transfer_enrollment
    • sheets.tasks.handle_unprocessed_deferral_requests
    • sheets.tasks.handle_unprocessed_refund_requests

Additional Context

@mudassir-hafeez mudassir-hafeez marked this pull request as ready for review May 14, 2024 13:03
@@ -11,6 +11,8 @@ jobs:
uses: actions/setup-python@v2
with:
python-version: "3.5"
env:
PIP_TRUSTED_HOST: "pypi.python.org pypi.org files.pythonhosted.org"

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This change fixes Python 3.5 setup fails with an invalid certificate when running pip. It's reported in actions/setup-python: actions/setup-python#866

@@ -32,4 +34,3 @@ jobs:
uses: codecov/codecov-action@v1
with:
file: ./coverage.xml
fail_ci_if_error: true
Copy link
Contributor Author

@mudassir-hafeez mudassir-hafeez May 17, 2024

Choose a reason for hiding this comment

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

Removed this check for now as it is hitting a rate limit on Codecov and causing a CI build failure. To bypass this limit, we need to use a Codecov repository upload token.

This issue also exists in other repositories. You can see the build step Upload coverage to CodeCov failing on the master branches, e.g., mitxpro and mitxonline.

Could someone from the DevOps team take a look into this?

@blarghmatey FYI

Copy link
Contributor

Choose a reason for hiding this comment

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

@mudassir-hafeez Upgrading codecov/codecov-action@v1 to codecov/codecov-action@v3 might fix this. Could you check that?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@asadali145 It seems upgrading to codecov/codecov-action@v3 did not resolve the issue. I've noticed this issue is being faced by multiple users with v3 as well. And Codecov team is suggesting moving on v4 with Codecov token.

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! We should test this PR with MITx Online before merging. Once we have merged this PR, we will need to create a new release for edx-api-client and update the versions in MIT xPro and MITx Online.

@mudassir-hafeez
Copy link
Contributor Author

As discussed, we can merge this PR as it has been tested with MITxPro. It can be tested with MITx Online separately after merging. So, proceeding with merging this PR without any further delay.
CC: @asadali145 @arslanashraf7

@mudassir-hafeez mudassir-hafeez merged commit 3fa1b40 into master May 22, 2024
4 checks passed
@mudassir-hafeez mudassir-hafeez deleted the mudassir/remove-x-edx-api-key-usage branch May 22, 2024 06:57
@odlbot odlbot mentioned this pull request May 22, 2024
1 task
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.

2 participants