mitodl / mitxonline

BSD 3-Clause "New" or "Revised" License
4 stars 2 forks source link

OpenedX: configuration and installation of ol_openedx_checkout_external #567

Closed rhysyngsun closed 2 years ago

rhysyngsun commented 2 years ago

This work can be done by anyone on the team even though it's infrastructure. Steps to perform this bellow:

arslanashraf7 commented 2 years ago

As a next step of the ticket I've setup a course and configurations in the Django admin to test this ticket in QA/RC. The steps below might be referred for creating future courses for the external checkout.

While setting up a course in edX and enabling it to show the Upgrade button we need to make a couple of tweaks, mentioning them below:

Once the above steps are complete the user would see the upgrade button in edX and when the external-checkout plugin is set up the user should be redirected to MITxOnline's cart.

FYI: @briangrossman @pdpinch @rhysyngsun @blarghmatey

arslanashraf7 commented 2 years ago

@blarghmatey I think the ECOMMERCE_PUBLIC_URL_ROOT value is not picked up properly in our MITxOnline QA instance. The value was set in here in a recent PR. But when we open this test course and navigate to Upgrade button it tries to take the user to something like http://localhost:8002/checkout-external/?sku=TESTSKU whereas the base URL should be same as the LMS base.

Could you tell what could be going wrong here?

blarghmatey commented 2 years ago

The new instance hadn't been deployed yet so the configs weren't live yet. I just ran the deployment so this should be working now.

arslanashraf7 commented 2 years ago

The new instance hadn't been deployed yet so the configs weren't live yet. I just ran the deployment so this should be working now.

@blarghmatey Thanks for the new build.

Now the eCommerce URL is correct but somehow MARKETING_SITE_CHECKOUT_URL setting value is not getting picked up properly and the plugin throws this configuration error. Could you tell why this value might not be getting picked?

blarghmatey commented 2 years ago

I'm not sure what the disconnect is with that configuration value. I logged into a running instance and verified that the setting is present in /edx/etc/lms.yml and that the ol-openedx-checkout-external plugin is installed at version 0.1.0

Let me know if you would like me to do further investigation or if it's something that you would like to look into at the plugin level.

rhysyngsun commented 2 years ago

@arslanashraf7 I think maybe the plugin needs something like this to load the vars from the env like this other plugin does:

https://github.com/mitodl/open-edx-plugins/blob/76ef69de89669d1c8a534192c767e68b02e318a4/src/ol_openedx_sentry/settings/sentry.py#L102-L104

I'm not quite sure how this worked locally for me, but based on the current source code settings.MARKETING_SITE_CHECKOUT_URL is always None.

@blarghmatey does that seem right? I'm not super familiar with the ENV_TOKENS stuff used there.

blarghmatey commented 2 years ago

Yeah, that seems likely. I know that it doesn't automatically inherit from the edX django settings, so it will probably need to pull the values from that function interface and then use it from there.

arslanashraf7 commented 2 years ago

@rhysyngsun Yes, I figured the plugin_settings method does load the settings but the problem in our case was that we are not loading them from the settings passed in production.py which we should. I was also looking at Canvas plugin implementation which also adds some settings and they are working fine. So the problem is in the settings in production.py in checkout plugin which also sets defult to None. I'll create a PR to update them in checkout plugin.

rhysyngsun commented 2 years ago

@arslanashraf7 @blarghmatey are we all set here? RC looks ok at least (and I wouldn't expect to see it in prod yet)

arslanashraf7 commented 2 years ago

The infra-related change was done in this PR e.g. adding the plugin + configurations. Also, there were specific details in this slack thread about building the AMIs that transition from CI->QA->Prod.

I think keeping it just on RC even if we deploy a new AMI build for prod @blarghmatey might guide better.

rhysyngsun commented 2 years ago

Ok, I'm going to close this then.