-
Notifications
You must be signed in to change notification settings - Fork 50
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
feature for lms oauth2 #1
Conversation
Hi @sodaling! Same question as usual 😉 what is this for? Does it solve a bug? |
@@ -1,4 +1,4 @@ | |||
ECOMMERCE_PUBLIC_URL_ROOT = "http://ecommerce:8000" | |||
ECOMMERCE_PUBLIC_URL_ROOT = "http://{{ ECOMMERCE_HOST }}" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This should probably be: ECOMMERCE_PUBLIC_URL_ROOT = "{% if ACTIVATE_HTTPS %}https{% else %}http{% endif %}://{{ ECOMMERCE_HOST }}"
@@ -1,3 +1,5 @@ | |||
# -*- coding: utf-8 -*- |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍
@@ -61,6 +78,22 @@ | |||
COMPRESS_OFFLINE = True | |||
|
|||
PAYMENT_PROCESSOR_CONFIG = { | |||
"openedx": json.loads("""{{ ECOMMERCE_PAYMENT_PROCESSORS|tojson(indent=4) }}""") | |||
"edx": json.loads("""{{ ECOMMERCE_PAYMENT_PROCESSORS|tojson(indent=4) }}""") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it's better if we keep the "openedx" naming convention. "edx" usually refers to edx.org.
PAYMENT_PROCESSORS = list(PAYMENT_PROCESSORS) + {{ ECOMMERCE_EXTRA_PAYMENT_PROCESSOR_CLASSES }} | ||
|
||
SCAR_DEFAULT_CURRENCY = 'CNY' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
SCAR_DEFAULT_CURRENCY
-> OSCAR_DEFAULT_CURRENCY
This should be a configurable setting that would be different for each user. Same for CURRENCY_SYMBOL
.
SCAR_DEFAULT_CURRENCY = 'CNY' | ||
CURRENCY_SYMBOL = '¥' | ||
ENABLE_ALIPAY_WECHATPAY = True |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Unfortunately, we cannot add alipay-specific settings for each user. But I do not think that you require such settings. Let's talk about it by chat?
PAYMENT_PROCESSORS = tuple(PAYMENT_PROCESSORS) | ||
|
||
SITE_ID = 2 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is probably not acceptable for most users.
|
||
# Setup minimal yml config file, which is required by production settings | ||
RUN echo "{}" > /openedx/config.yml | ||
ENV ECOMMERCE_CFG /openedx/config.yml |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The config.yml
file should not be required by the assets settings, as discussed here.
(same comment below for the dependency on production.py
)
@@ -8,8 +8,8 @@ export DJANGO_SETTINGS_MODULE=lms.envs.tutor.production | |||
./manage.py lms manage_user ecommerce ecommerce@openedx --staff --superuser | |||
|
|||
./manage.py lms create_oauth2_client \ | |||
"http://ecommerce:8000" \ | |||
"http://ecommerce:8000/complete/edx-oidc/" \ | |||
"{% if ACTIVATE_HTTPS %}https{% else %}http{% endif %}://ecommerce.{{ LMS_HOST }}" \ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This should be ECOMMERCE_HOST
instead of ecommerce.{{ LMS_HOST }}
.
@sodaling I'll cherry-pick some of the changes you suggested and release a new version. Others need to be individually discussed. |
Closing this after f7ee362. Please update this PR if you need other features. |
feature for lms oauth2