Checkout on Otto #11503
Checkout on Otto #11503
Conversation
|
Thanks for the pull request, @vkaracic! It looks like you're a member of a company that does contract work for edX. If you're doing this work as part of a paid contract with edX, you should talk to edX about who will review this pull request. If this work is not part of a paid contract with edX, then you should ensure that there is an OSPR issue to track this work in JIRA, so that we don't lose track of your pull request. To automatically create an OSPR issue for this pull request, just visit this link: https://openedx-webhooks.herokuapp.com/github/process_pr?number=11503&repo=edx%2Fedx-platform |
| @@ -39,7 +38,7 @@ class ChooseModeView(View): | |||
| """ | |||
|
|
|||
| @method_decorator(transaction.non_atomic_requests) | |||
| def dispatch(self, *args, **kwargs): # pylint: disable=missing-docstring | |||
| def dispatch(self, *args, **kwargs): # pylint: disable=missing-docstring | |||
There was a problem hiding this comment.
Would it be possible to add a docstring for this method and remove the pylint override?
There was a problem hiding this comment.
I'm not completely sure about all the use-cases for this view so I'm going to place a general description for this code.
|
@vkaracic done with my first pass -- let me know when you're ready for another. |
|
Also it looks like you have a quality violation to address |
|
@mattdrayer thanks for doing the review! I've addressed most of the comments, the ones with theming methods are left (I have a question for it: https://github.com/edx/edx-platform/pull/11503#discussion_r52730366) and the |
|
@mattdrayer I believe I have addressed the 'microsite2theming' comments with this commit: edx/edx-platform@e0b2d3f Aside from the |
|
@bderusha this is the PR that I've mentioned. Please take a look. |
| super(EcommerceServiceTests, self).setUp() | ||
|
|
||
| def test_is_enabled(self): | ||
| """Verify that is_enabled() returns True.""" |
There was a problem hiding this comment.
What about when it is disabled?
There was a problem hiding this comment.
Seeing your other tests, this docstring needs to be more informative: Verify `is_enabled() returns True when not used for a microsite.
| checkout_page, sku, course, request = self.prepare_course_about_with_ecommerce() | ||
| request.user = AnonymousUser() | ||
| mako_middleware_process_request(request) | ||
| ecommerce_enrollment_link = ( |
There was a problem hiding this comment.
There is a lot of duplicated code across these tests. Combine it into an assertion:
def assert_enrollment_link_present(self, request, href):
...There was a problem hiding this comment.
Additionally I could see these 4 tests using DDT
@ddt.data(
(None, True, '<a href="/register?course_id={}&enrollment_action=add_to_ecomm_cart&checkout_url={}?sku={}" class="add-to-cart">'),
... etc
)
@dtt.unpack
def test_ecommerce_checkout_with_anonymous(self, min_price, is_anonymous, enrollment_link_template):
request.user = AnonymousUser() if is_anonymous else self.user
... etcThere was a problem hiding this comment.
I ended up splitting the tests into two with DDT, because two of them test when the shopping cart setting is enabled: 559b964
|
👍 pending:
|
| @@ -117,7 +123,8 @@ def get(self, request, course_id, error=None): | |||
| ) | |||
|
|
|||
| context = { | |||
| "course_modes_choose_url": reverse("course_modes_choose", kwargs={'course_id': course_key.to_deprecated_string()}), | |||
| "course_modes_choose_url": reverse("course_modes_choose", | |||
There was a problem hiding this comment.
NIT: put both arguments one singly indented lines.
reverse(
"course_modes_choose",
kwargs={'course_id': course_key.to_deprecated_string()}
),|
👍 pending:
|
01570ce to
5c44398
Compare
| @@ -232,6 +233,45 @@ def test_course_about_in_cart(self): | |||
| self.assertEqual(response.status_code, 200) | |||
| self.assertIn(in_cart_span, response.content) | |||
|
|
|||
| def assert_enrollment_link_present(self, is_anonymous, _id=False): | |||
| """ Prepare ecommerce checkout data and assert if the ecommerce link is contained in the response. """ | |||
|
@clintonb I believe I have addressed all your comment. You were right about the login, I've removed that code which rendered the JS additions unnecessary so those are gone as well. @bderusha as I removed the JS changes this aside https://github.com/edx/edx-platform/pull/11503#discussion_r52785405 might not be applicable anymore. Also I tend to agree with Clinton that this comment https://github.com/edx/edx-platform/pull/11503#discussion_r53178474 isn't necessary so I removed it, I hope that's OK. |
|
@vkaracic That's fine |
|
Cool. |
2699462 to
86a4710
Compare
Introducing a new CommerceConfiguration. When it's enabled all the checkout functionality will be forwarded to Otto.
@mattdrayer @clintonb can you review this please?
@mjfrey