REV-2575: add orders app with endpoint to retrieve order info from ecommerce #24

dianekaplan · 2022-04-14T20:09:35Z

Description: Add an orders app that'll be used for communication around order retrieval. The first endpoint will be to request order information for a user, but in the future we'll add another (or modify this one) to be able to call an external provider for a user's orders.

JIRA: Link to JIRA ticket

Installation instructions:

follow the setup steps in the commerce-coordinator README file here
run commerce-coordinator locally

Testing instructions:
Test the endpoint: with this branch, go to the new endpoint test page, specifying a user that has orders in your local environment: http://localhost:8140/orders/order_history/?username=edx&page=1&page_size=20

Test via order history page: With this branch in progress, the order history page can go through this new endpoint to get the orders info.
Order history via CC endpoint:

Order history via ecommerce:

commerce_coordinator/settings/base.py

commerce_coordinator/apps/orders/tests.py

inventhouse

Overall excellent work; a little feedback to consider, let me know if you have questions or want to chat about any of it.

commerce_coordinator/apps/orders/tests.py

commerce_coordinator/apps/orders/urls.py

commerce_coordinator/apps/orders/clients.py

commerce_coordinator/apps/orders/views.py

commerce_coordinator/settings/base.py

commerce_coordinator/settings/local.py

inventhouse · 2022-04-26T18:49:48Z

provision-commerce-coordinator.sh

-
 # Create superuser
 echo -e "${GREEN}Creating super-user for ${name}...${NC}"
-docker exec -t commerce-coordinator.app bash -c "echo 'from django.contrib.auth import get_user_model; User = get_user_model(); User.objects.create_superuser(\"edx\", \"[email protected]\", \"edx\") if not User.objects.filter(username=\"edx\").exists() else None' | python /edx/app/${name}/manage.py shell"


We may not want to delete these things, or, at least, we may want them back when we do move commerce-coordinator into a container

Good call- I've saved it out in our share drive for future reference when the time comes: https://docs.google.com/document/d/1mu2VP7wh0E4oZhwURqCaVrOGDaYjtnYg204hl-7ybs0/edit

Shout it out if you think it would be better to keep this in the file just commented out, but this way seems cleaner for now since it's not in use.

I think leaving them commented-out in the file with a brief comment saying to put them back when we move development into docker would be best, it's easy to lose track of an external doc somewhere.

If that's the case, can we make them 2 files? Maybe make this something like local-provision-commerce-coordinator.sh so we can reference it in the README.md to properly set up the devstack auth on local.

ah I like that idea- thanks Phil! Done now, and updated README accordingly

…nd refer to it from clients.py

inventhouse

Recommend keeping the docker stuff in the file, otherwise looks good!

inventhouse · 2022-05-04T16:07:57Z

provision-commerce-coordinator.sh

-
 # Create superuser
 echo -e "${GREEN}Creating super-user for ${name}...${NC}"
-docker exec -t commerce-coordinator.app bash -c "echo 'from django.contrib.auth import get_user_model; User = get_user_model(); User.objects.create_superuser(\"edx\", \"[email protected]\", \"edx\") if not User.objects.filter(username=\"edx\").exists() else None' | python /edx/app/${name}/manage.py shell"


I think leaving them commented-out in the file with a brief comment saying to put them back when we move development into docker would be best, it's easy to lose track of an external doc somewhere.

pshiu

Looks good to me, nice work Diane! Just an observation that I think right now, anyone can query the order status of any other user, so we may need to figure out authentication in a later PR.

julianajlk · 2022-05-04T16:52:11Z

commerce_coordinator/apps/orders/tests.py

+        self.mock_response = sample_response
+
+    @patch('commerce_coordinator.apps.orders.clients.EcommerceApiClient.get_orders')
+    def test_EcommerceApiClient(self, mock_response):


It can be confusing in terms of consistency throughout the whole project to use both underscore and camel case in a function name?

https://peps.python.org/pep-0008/#function-and-variable-names
^ Not sure if it applies to test functions

goooooood question..... I'd imagined I should use the name of the thing I'm testing (which has a camel case name), but you're right: I'm looking at examples in ecommerce it looks like we keep camel case for the test class name (if applicable) and underscores for test names- good catch! Updated now!

…rovision file

dianekaplan · 2022-05-04T19:53:06Z

Looks good to me, nice work Diane! Just an observation that I think right now, anyone can query the order status of any other user, so we may need to figure out authentication in a later PR.

Thanks Phil- you are correct! I just checked manually, and even though the ecommerce OrderViewset filter_queryset gives a PermissionDenied if a non-staff user tries to see another's orders, you're right: when I log in as a new user locally and look up orders for an admin directly on the commerce-coordinator endpoint test view (link below), I do see the results!
http://localhost:8140/orders/order_history/?username=edx&page=1&page_size=20

When the order history page makes the request, it's getting the authenticated user to pass along, so the potential vulnerability would be with the commerce-coordinator view url, and making sure only staff accounts can access this. I've logged this work here: https://openedx.atlassian.net/browse/REV-2708

Diane Kaplan and others added 2 commits April 14, 2022 16:07

feat: add orders app with prelim endpoint for testing

a75f9d6

feat: add EcommerceApiClient

7835b20

dianekaplan changed the title ~~Dianekaplan/rev 2575 order history clean~~ [WIP] REV-2575: add orders app, and endpoint to retrieve order info from ecommerce Apr 14, 2022

This was referenced Apr 14, 2022

[WIP] REV-2575: add orders app with prelim endpoint for testing #22

Closed

REV-2575: update order status page to go through commerce-coordinator openedx-unsupported/frontend-app-ecommerce#181

Merged

dianekaplan commented Apr 19, 2022

View reviewed changes

commerce_coordinator/settings/base.py Show resolved Hide resolved

dianekaplan force-pushed the dianekaplan/REV-2575_order_history_clean branch from d904a1a to 5f8de03 Compare April 19, 2022 20:49

Diane Kaplan added 2 commits April 19, 2022 16:50

fix: cleanup

dd968a1

fix: add frontend-app-ecommerce into CORS_ORIGIN_WHITELIST

9a07564

dianekaplan force-pushed the dianekaplan/REV-2575_order_history_clean branch 7 times, most recently from ab19b4e to 0d330b4 Compare April 26, 2022 13:50

fix: add endpoint unit test, do cleanup

37212d9

dianekaplan force-pushed the dianekaplan/REV-2575_order_history_clean branch from 19c0a5d to 37212d9 Compare April 26, 2022 14:09

dianekaplan commented Apr 26, 2022

View reviewed changes

commerce_coordinator/apps/orders/tests.py Outdated Show resolved Hide resolved

dianekaplan force-pushed the dianekaplan/REV-2575_order_history_clean branch from f16f5f6 to a097604 Compare April 26, 2022 14:47

fix: rename test, update comments

7ef427b

dianekaplan force-pushed the dianekaplan/REV-2575_order_history_clean branch from a097604 to 7ef427b Compare April 26, 2022 14:52

inventhouse reviewed Apr 26, 2022

View reviewed changes

Diane Kaplan added 2 commits April 26, 2022 15:09

fix: add test for views.py function

576839c

fix: review feedback- move CORS WHITELIST to local.py

d7da8f9

dianekaplan force-pushed the dianekaplan/REV-2575_order_history_clean branch 5 times, most recently from 9b02b40 to 16a5fe8 Compare April 27, 2022 19:51

dianekaplan force-pushed the dianekaplan/REV-2575_order_history_clean branch 3 times, most recently from dd7e237 to a94398c Compare April 28, 2022 14:16

Diane Kaplan added 2 commits April 28, 2022 14:08

fix: feedback- update path name to be generic

ddb9139

fix: review feedback- move sample response out into readable block, a…

dedeba1

…nd refer to it from clients.py

dianekaplan force-pushed the dianekaplan/REV-2575_order_history_clean branch from a94398c to dedeba1 Compare April 28, 2022 18:08

fix: cleanup pp commentwq

60e5db1

dianekaplan force-pushed the dianekaplan/REV-2575_order_history_clean branch from 6b0b6fe to 812bbf2 Compare May 2, 2022 19:44

dianekaplan changed the title ~~[WIP] REV-2575: add orders app, and endpoint to retrieve order info from ecommerce~~ REV-2575: add orders app, and endpoint to retrieve order info from ecommerce May 4, 2022

fix: add unit test for view function, simplify mock

8fc0333

dianekaplan force-pushed the dianekaplan/REV-2575_order_history_clean branch from 50fbc33 to 8fc0333 Compare May 4, 2022 14:07

dianekaplan changed the title ~~REV-2575: add orders app, and endpoint to retrieve order info from ecommerce~~ REV-2575: add orders app with endpoint to retrieve order info from ecommerce May 4, 2022

inventhouse approved these changes May 4, 2022

View reviewed changes

pshiu approved these changes May 4, 2022

View reviewed changes

julianajlk reviewed May 4, 2022

View reviewed changes

fix: rename local provision file, reference from README, add docker p…

e61a400

…rovision file

dianekaplan force-pushed the dianekaplan/REV-2575_order_history_clean branch from ce19558 to e61a400 Compare May 4, 2022 17:09

fix: rename test

99fc25a

dianekaplan merged commit 0c6561f into main May 5, 2022

dianekaplan deleted the dianekaplan/REV-2575_order_history_clean branch May 5, 2022 14:38

REV-2575: add orders app with endpoint to retrieve order info from ecommerce #24

REV-2575: add orders app with endpoint to retrieve order info from ecommerce #24

Uh oh!

Conversation

dianekaplan commented Apr 14, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Uh oh!

inventhouse left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

inventhouse Apr 26, 2022

Choose a reason for hiding this comment

Uh oh!

dianekaplan May 4, 2022

Choose a reason for hiding this comment

Uh oh!

inventhouse May 4, 2022

Choose a reason for hiding this comment

Uh oh!

pshiu May 4, 2022

Choose a reason for hiding this comment

Uh oh!

dianekaplan May 4, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

inventhouse left a comment

Choose a reason for hiding this comment

Uh oh!

inventhouse May 4, 2022

Choose a reason for hiding this comment

Uh oh!

pshiu left a comment

Choose a reason for hiding this comment

Uh oh!

julianajlk May 4, 2022

Choose a reason for hiding this comment

Uh oh!

julianajlk May 4, 2022

Choose a reason for hiding this comment

Uh oh!

dianekaplan May 4, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dianekaplan commented May 4, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

dianekaplan commented Apr 14, 2022 •

edited

Loading

dianekaplan May 4, 2022 •

edited

Loading

dianekaplan May 4, 2022 •

edited

Loading

dianekaplan commented May 4, 2022 •

edited

Loading