Skip to content

Further theme support for ECOMMERCE_* settings - #11966

Closed
e-kolpakov wants to merge 2 commits into
openedx:mattdrayer/theme-ecommerce-urlfrom
open-craft:ekolpakov/theme-ecommerce-support
Closed

Further theme support for ECOMMERCE_* settings#11966
e-kolpakov wants to merge 2 commits into
openedx:mattdrayer/theme-ecommerce-urlfrom
open-craft:ekolpakov/theme-ecommerce-support

Conversation

@e-kolpakov

Copy link
Copy Markdown
Contributor

Description: Expands https://github.com/edx/edx-platform/pull/11954 to support checkout on Otto for themed sites/microsites and allows themed site/microsite config to override ECOMMERCE_API_URL and ECOMMERCE_API_SIGNING_KEY

@openedx-webhooks

Copy link
Copy Markdown

Thanks for the pull request, @e-kolpakov! 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?repo=edx%2Fedx-platform&number=11966

@e-kolpakov

Copy link
Copy Markdown
Contributor Author

jenkins run js, please

@e-kolpakov
e-kolpakov force-pushed the ekolpakov/theme-ecommerce-support branch from 9ad15ec to 150ced3 Compare March 28, 2016 13:25
@e-kolpakov

Copy link
Copy Markdown
Contributor Author

jenkins run python

""" Check if the user is activated, if the service is enabled and that the site is not a microsite. """
return (user.is_active and self.config.checkout_on_ecommerce_service and not
helpers.is_request_in_themed_site())
return user.is_active and self.config.checkout_on_ecommerce_service

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

@e-kolpakov -- I think if we were to merge this line in, all microsites would begin using the Otto checkout process, correct? This seems like something that we should avoid at the present time because we are not ready to enable Otto for the microsites running on edx.org. I discussed with @mjfrey and @douglashall, and we are thinking that this should be a separate PR to master. Alternatively, we could add a site_id to the commerce configuration model and that would allow for multiple microsites to control their own setups. Thoughts?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

@mattdrayer Right, I missed that commerce config is per-instance as well, not per-microsite. I believe the best course of actions would be to push for model-backed site configuration, but if it's not something we could achieve within the time bounds we have (i.e. before the WL-analytics launch), adding site_id would be an appropriate solution.

Alternatively, this setting can live in microsite config as a separate flag ("USE_OTTO") or as an implicit switch (i.e. if ECOMMERCE_PUBLIC_URL_ROOT is set).

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

We do have model-backed config for Sites waiting in the wings (WL-327 / #11806) -- we will be picking it up as soon as WL-325 / #11885 clears its gates.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

@mattdrayer ok, than I believe it is safe to close this PR (I'll do that) and just wait for the SiteConfiguration PR to land and implement it in a proper way.

@e-kolpakov e-kolpakov closed this Mar 28, 2016
@e-kolpakov
e-kolpakov deleted the ekolpakov/theme-ecommerce-support branch July 19, 2016 11:02
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants