[DEPR]: Complete removal of EdxRestApiClient

[robrap]

Proposal Date

11 April 2023

Target Ticket Acceptance Date

11 April 2023

Earliest Open edX Named Release Without This Functionality

Q - 2023-10

Rationale

Note that this is not a new proposal, but is a ticket to track completion of the original DEPR documented here: #37.

This ticket is to clean-up some small remaining usages, and ultimately to remove the deprecated EdxRestApiClient so the code no longer lives in the platform.

Removal

Here is a general search for uses of EdxRestApiClient: https://github.com/search?q=org%3Aopenedx%20EdxRestApiClient&type=code

• Here is usage in edx-platform: https://github.com/openedx/edx-platform/blob/d622db59b7bfe2e4c11fc11b7cb2d475cfda551b/openedx/core/djangoapps/commerce/utils.py#L56
• Here is a comment that simply should no longer reference EdxRestApiClient: https://github.com/openedx/edx-analytics-data-api/blob/6916f071301c57457cad668ac1c33ae702b061fa/analyticsdataserver/settings/base.py#L325
• It's unclear if this is still an active use, but it definitely needs to be addressed: https://github.com/openedx/credentials/blob/b6090f97097aee61e927f375995a10582adc0731/credentials/apps/catalog/utils.py#L37
• Tests that should be removed. Do they need to be replaced? See https://github.com/openedx/edx-platform/blob/d622db59b7bfe2e4c11fc11b7cb2d475cfda551b/lms/djangoapps/commerce/tests/__init__.py#L28.
• More tests using the client. Are these credentials e2e tests still in use? See https://github.com/openedx/credentials/blob/b6090f97097aee61e927f375995a10582adc0731/acceptance_tests/e2e/mixins.py#L38.
• The final change should be removal of the actual class in a breaking change. We should also remove the slumber library, since we will no longer need this outdated library. See https://github.com/openedx/edx-rest-api-client/blob/8eb8837c2d5e94ef2cc809124ed371ef12509f50/edx_rest_api_client/client.py#L288
  • Includes tests for this class: https://github.com/openedx/edx-rest-api-client/blob/8eb8837c2d5e94ef2cc809124ed371ef12509f50/edx_rest_api_client/tests/test_client.py#L37
• There is a separate ticket for follow-up on an ADR detailed in Write an ADR for use of a user's JWT now that EdxApiRestClient has been removed edx-rest-api-client#277. Marking this closed to not block closing this DEPR, although the issue has not yet been addressed.
• The newly introduced DeprecatedEdxRestApiClient in edx-platform will need to be removed, in addition to slumber. This requires fixing the past bug found during the first attempt. See [DEPR]: Complete removal of EdxRestApiClient #189 (comment) for details. (This may die with ecommerce code in edx-platform. This should probably just get its own edx-platform ticket rather than trying to complete as part of this DEPR.)

Replacement

See original DEPR for details: #37

Deprecation

No response

Migration

No response

Additional Info

No response

[kdmccormick]

I support this deprecation! 😉

[idegtiarov]

me too )))

[dianakhuang]

This snuck back into edx-platform: openedx/edx-platform@a35176e

[robrap]

Note that I have questions about some of the replacement work from the original DEPR ticket, but I have created the following ticket to address that: openedx/edx-rest-api-client#277

[Yagnesh1998]

assign me

[robrap]

@dianakhuang @feanil: At some point we had discussed removing EdxRestApiClient from the library, and updating edx-platform's lone usage (that snuck back in) by making a local copy, renamed to DeprecatedEdxRestApiClient, so hopefully no ohter usages would crop up.

@Yagnesh1998 was working on that, and it struck me that:

1. This DEPR won't be closed until we actually got rid of any new DeprecatedEdxRestApiClient, especially because this requires slumber library that we wish to retire, and
2. Since we need to get rid of it anyway, is it possible to simply introduce a fix to use the new client (or requests) without ever introducing DeprecatedEdxRestApiClient?

I've asked @Yagnesh1998 to look into that based on PRs from the original DEPR work.

[Yagnesh1998]

@dianakhuang, @robrap, Yes, I will start working on it. And let me know if there is any update or any change, thanks.

[robrap]

I have since learned more about the final fix that is needed. We will continue with the plan to merge a refactor to DeprecatedEdxRestApiClient, so that we can remove EdxRestApiClient from the library.

The new DeprecatedEdxRestApiClient will need to be fixed/removed to close this DEPR.

The initial attempt to remove the client failed with a bug in Production. Here is the PR with the (partial) revert: openedx/edx-platform#30792. Note that there were many changes in the original PR, so this PR just reverts the broken change.

Also, the revert PR refers to a private ticket in a code comment, and here are its contents:

https://2u-internal.atlassian.net/browse/ENT-6112
fix: get_ecommerce_api_client returns “405 Method Not Allowed” when calling "create_refunded_voucher" API view
get_ecommerce_api_client returns “405 Method Not Allowed” when called from within codebase https://github.com/openedx/edx-platform/blob/6d80bddb954dfa6f53601a391eec62c55447211e/openedx/features/enterprise_support/signals.py#L109. The same method works when we call it through EdxRestApiClient. Discovered when working on https://2u-internal.atlassian.net/browse/ENT-5938

How to reproduce the bug?

1. Try URL localhost:18000
2. Enroll in a verified track course
3. Unenroll from the course

[dianakhuang]

I believe this only needs to be reviewed and merged: openedx/edx-rest-api-client#303

[robrap]

@dianakhuang:

1. Are the checkboxes on this ticket out-of-date?
2. Can we get [DEPR]: Remove EdxRestApiClient from edx-rest-api-client library #218 (comment) over the line?
3. Should removal of DeprecatedEdxRestApiClient from edx-platform get its own edx-platform issue, whether it is a DEPR or not?

Basically, I'm wondering how to get these related DEPR tickets closed.

[dianakhuang]

1. I think they probably are, but I haven't had time to really review them.
2. Yup, I would like to do that asap.
3. I think maybe we should create one when we get a chance. Maybe we can discuss during the DEPR meeting. I think it will depend on when ecommerce actually gets killed. It feels like it's been on the verge for the last few years.

[dianakhuang]

The ecommerce code will be removed within the next year anyway from edx-platform and the DEPR work for cleaning up DeprecatedRestApiClient will go with it, so we aren't making a separate ticket.