FC-0001: Replace usages enterprise_catalogs Enterprise API V1 #3742

ruzniaievdm · 2022-06-13T16:14:21Z

(1 of 5) for closes openedx/public-engineering#61

Latest: openedx/edx-enterprise#1582

Description:

Replace these usages1 usages2 with corresponding to the enterprise-catalog service

openedx-webhooks · 2022-06-13T16:14:26Z

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

ecommerce/coupons/tests/mixins.py

feanil · 2022-06-14T15:35:27Z

ecommerce/enterprise/tests/mixins.py

@@ -564,24 +563,29 @@ def mock_enterprise_catalog_api(self, enterprise_customer_uuid, raise_exception=
        Helper function to register the enterprise catalog API endpoint.
        """
        enterprise_catalog_api_response = {
-            'count': 10,


This should be the total number of results which should not change because we're mocking a single page in the response I think.

This change does not make sense, becouse we're mocking, returned as it was

feanil · 2022-06-14T15:36:43Z

ecommerce/enterprise/tests/mixins.py

-            'num_pages': 3,
-            'current_page': 2,
+            'count': 2,
+            'next': urljoin(self.ENTERPRISE_CATALOG_URL, f'?enterprise_customer={enterprise_customer_uuid}/'),


If this is meant to imitate page 2 of the response, I think the the next url would be different.

Suggested change

'next': urljoin(self.ENTERPRISE_CATALOG_URL, f'?enterprise_customer={enterprise_customer_uuid}/'),

'next': urljoin(self.ENTERPRISE_CATALOG_URL, f'?page=3&page_size=2'),

enterprise_customer query shouldn't be included?
I did not simulate 2 page of the answer but changed response

I think in the old API, it would using the customer_uuid as a way of figuring out what the first customer to display is. But the new API doesn't have that feature, it just does paging. If you haven't already done so, you should make sure any non test calls to this endpoint don't pass the enterprise_customer parameter.

Hm, do you mean that the new enterprise-catalog endpoint should not accept a query parameter enterprise_customer? Because this implementation I moved to the new endpoint

pr to enterprise-catalog: https://github.com/openedx/enterprise-catalog/pull/468/files

Ah, gotcha, hadn't gotten to that PR yet, makes sense. But you still need paging information for the next URL right? If the endpoint is meant to provide more results on a second page, the URL for that should be consistent. It sounds like it would include the enterprise_customer but also the next page number and page size.

you are right, have already been added those changes

ecommerce/enterprise/utils.py

feanil

This looks good to me, I think it's good to merge once the setting has been added to site-configuration and we've updated the wiki page for olive to mention the settings change.

feanil · 2022-06-14T19:22:09Z

ecommerce/enterprise/utils.py

@@ -560,14 +615,14 @@ def get_enterprise_catalog(site, enterprise_catalog, limit, page, endpoint_reque

    api_client = site.siteconfiguration.oauth_api_client
    enterprise_api_url = urljoin(
-        f"{site.siteconfiguration.enterprise_api_url}/",
-        f"{resource}/{str(enterprise_catalog)}/"
+        f"{site.siteconfiguration.enterprise_catalog_api_url}/",


This change probably deserves some sort of change log entry, at the very least we should update the olive page to tell people the old setting will not work and they'll need to add this new setting to properly connect ecommerce to the enterprise catalog.

This still needs to be done on the olive page and linked back to here before this PR is ready to merge.

@dianakhuang can you help confirm that this setting is set in stage and prod for edx.org? I think if it is, then this is safe to merge.

In particular I just want to confirm that settings.ENTERPRISE_CATALOG_API_URL is set correctly since this is just a passthrough for that.

ruzniaievdm · 2022-06-14T19:26:15Z

@feanil Thanks! Before merging I want to rebase to one commit

ruzniaievdm · 2022-06-15T17:26:49Z

@feanil I leave clarifications and comments in discussions, please take a look

feanil

This is pretty close to landing at this point I think. Just a few small things to hash out.

feanil · 2022-06-22T12:30:40Z

ecommerce/enterprise/tests/mixins.py

-            'num_pages': 3,
-            'current_page': 2,
+            'count': 2,
+            'next': urljoin(self.ENTERPRISE_CATALOG_URL, f'?enterprise_customer={enterprise_customer_uuid}/'),


Ah, gotcha, hadn't gotten to that PR yet, makes sense. But you still need paging information for the next URL right? If the endpoint is meant to provide more results on a second page, the URL for that should be consistent. It sounds like it would include the enterprise_customer but also the next page number and page size.

feanil · 2022-06-22T12:34:00Z

ecommerce/enterprise/utils.py

@@ -560,14 +615,14 @@ def get_enterprise_catalog(site, enterprise_catalog, limit, page, endpoint_reque

    api_client = site.siteconfiguration.oauth_api_client
    enterprise_api_url = urljoin(
-        f"{site.siteconfiguration.enterprise_api_url}/",
-        f"{resource}/{str(enterprise_catalog)}/"
+        f"{site.siteconfiguration.enterprise_catalog_api_url}/",


This still needs to be done on the olive page and linked back to here before this PR is ready to merge.

ecommerce/enterprise/utils.py

ruzniaievdm · 2022-06-23T12:17:55Z

rebased to one commit

feanil

I've updated the olive page with the relevant changes, @dianakhuang if you confirm the one setting exists then I think this is good to merge.

feanil · 2022-07-28T20:09:42Z

@natabene this is ready for 2U review.

colinbrash · 2022-10-20T18:55:03Z

This PR is scheduled for review the week of Nov 7.

mphilbrick211 · 2022-11-21T22:10:56Z

@colinbrash Hi there! Just seeing if this is ready to be merged?

e0d · 2023-01-20T21:25:51Z

@colinbrash can you let us known when this can be schedule for review?

openedx-webhooks added needs triage open-source-contribution PR author is not from Axim or 2U labels Jun 13, 2022

ruzniaievdm mentioned this pull request Jun 13, 2022

FC-0001: Remove enterprise_catalogs API endpoint openedx/edx-enterprise#1582

Closed

4 tasks

openedx-webhooks removed the needs triage label Jun 14, 2022

feanil reviewed Jun 14, 2022

View reviewed changes

robrap assigned dianakhuang Jun 14, 2022

feanil approved these changes Jun 14, 2022

View reviewed changes

feanil reviewed Jun 22, 2022

View reviewed changes

refactor: Replace usages enterprise_catalogs Enterprise API V1

cba2cc2

ruzniaievdm force-pushed the ruzniaievdm/remove-enterprise-api branch from ebaaaa9 to cba2cc2 Compare June 23, 2022 12:16

feanil approved these changes Jul 12, 2022

View reviewed changes

mphilbrick211 added the engineering review label Nov 21, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

FC-0001: Replace usages enterprise_catalogs Enterprise API V1 #3742

FC-0001: Replace usages enterprise_catalogs Enterprise API V1 #3742

ruzniaievdm commented Jun 13, 2022 •

edited

Loading

openedx-webhooks commented Jun 13, 2022 •

edited

Loading

feanil Jun 14, 2022

ruzniaievdm Jun 14, 2022

feanil Jun 14, 2022

ruzniaievdm Jun 14, 2022

feanil Jun 14, 2022

ruzniaievdm Jun 15, 2022

feanil Jun 22, 2022

ruzniaievdm Jun 23, 2022 •

edited

Loading

feanil left a comment

feanil Jun 14, 2022

feanil Jun 22, 2022

feanil Jul 12, 2022

feanil Jul 12, 2022 •

edited

Loading

ruzniaievdm commented Jun 14, 2022

ruzniaievdm commented Jun 15, 2022

feanil left a comment

feanil Jun 22, 2022

feanil Jun 22, 2022

ruzniaievdm commented Jun 23, 2022

feanil left a comment

feanil commented Jul 28, 2022

colinbrash commented Oct 20, 2022

mphilbrick211 commented Nov 21, 2022

e0d commented Jan 20, 2023

	'next': urljoin(self.ENTERPRISE_CATALOG_URL, f'?enterprise_customer={enterprise_customer_uuid}/'),
	'next': urljoin(self.ENTERPRISE_CATALOG_URL, f'?page=3&page_size=2'),

FC-0001: Replace usages enterprise_catalogs Enterprise API V1 #3742

Are you sure you want to change the base?

FC-0001: Replace usages enterprise_catalogs Enterprise API V1 #3742

Conversation

ruzniaievdm commented Jun 13, 2022 • edited Loading

Latest: openedx/edx-enterprise#1582

Description:

openedx-webhooks commented Jun 13, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ruzniaievdm Jun 23, 2022 • edited Loading

Choose a reason for hiding this comment

feanil left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

feanil Jul 12, 2022 • edited Loading

Choose a reason for hiding this comment

ruzniaievdm commented Jun 14, 2022

ruzniaievdm commented Jun 15, 2022

feanil left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ruzniaievdm commented Jun 23, 2022

feanil left a comment

Choose a reason for hiding this comment

feanil commented Jul 28, 2022

colinbrash commented Oct 20, 2022

mphilbrick211 commented Nov 21, 2022

e0d commented Jan 20, 2023

ruzniaievdm commented Jun 13, 2022 •

edited

Loading

openedx-webhooks commented Jun 13, 2022 •

edited

Loading

ruzniaievdm Jun 23, 2022 •

edited

Loading

feanil Jul 12, 2022 •

edited

Loading