-
Notifications
You must be signed in to change notification settings - Fork 4
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
feat: separate studio config and handler #185
Conversation
9960f0f
to
3602ea6
Compare
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 tested this: Tested in devstack
- I read through the code
- I checked for accessibility issues
- Includes documentation
- I made sure any change in configuration variables is reflected in the corresponding client's
configuration-secure
repository.
3602ea6
to
3beb91e
Compare
Hi @navinkarkera, thanks for your contribution. @santiagosuarezedunext, Can you check this to review in one of ours sprints? |
@santiagosuarezedunext friendly reminder to check this new feature |
@santiagosuarezedunext @Asespinel |
@santiagosuarezedunext do we have this review on our future plans? |
@kaustavb12 @navinkarkera Sorry for the delay, our team is working on some priorities, I'm going to move this to the prioritized section and as soon as we finish what we're at we'll start by reviewing PRs. |
hello @kaustavb12 @navinkarkera, I hope you are doing great. First of all, thank you so much for your interest in contributing to the improvement of this project and we're so sorry for the delay, only until now we have the time to give it the love it deserves. I've been testing the proposed modifications. I think they are, alongside solving the use case you expose in the description, pretty convenient for improving the experience of handling the tenant configs for the STUDIO; we didn't need to solve the current implementation yet due to we manage the CMS as a tenant config and everything have gone okay according to our requirements. I tested the PR using Tutor and most of the things worked as expected not only for your test case but also for the use cases we have in our sight. Nevertheless, there was a command that I wasn´t able to test that is By now, I'll be sharing this with my teammates so they can give their appreciation too. Again, thank you so much. |
3beb91e
to
bed7762
Compare
@bra-i-am Thanks! I have rebased with branch with latest changes from master. |
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.
hey @navinkarkera, thanks for the rebase. I've been testing again and everything is working as expected. I'll announce this to my teammates to continue with the merge.
Again, thank you so much for contributing to this project.
Hello @navinkarkera and @bra-i-am. Thanks to both for your contribution to this PR. About the problem this PR intends to solve, "to keep studio configuration separate from LMS configs," we don't have it because in edunext we create a tenant for our Studio and add the configuration for our Studio in our lms_config. We also have a studio for our free clients in the SaaS; and we have some special cases, for that, we have a tenant for their studios. I know we still have things to improve around cms_config and eox-tenant, but I think this change is a super breaking change for us. If we merge this, our configs around Studio will break because they are in lms_config, and Studio won't take it. I checked this, but, I will talk with @bra-i-am if there is a configuration I'm missing that could generate this problem. On the other hand, we have another use case for having studio configs as tenants: We usually have different domains for clients that have custom Studios, for example, mafermazu.edunext.io as LMS and mafermazu-studio.edunext.io. In my understanding, in this PR, the difference between when to use the lms_config or cms_config is the port. I tested this using Tutor, and it doesn't work well. As I said, I will request @bra-i-am's help to make this work, but I really think this is difficult to merge. I will tag people from our SaaS support and operations team. @DeimerM @DonatoBD @BetoFandino, do you have thoughts on this? And please correct me if I made a mistake here. |
@navinkarkera, I could test it with @bra-i-am, but as I said, I think this PR is difficult to merge because of the problems this will add to our platform. Let's wait until the SaaS support and operation team bring an opinion. |
@navinkarkera I talked with the SaaS support and operation team, and they told me it is not too complicated to apply the change in our instances for this PR. Thanks again for your patience. |
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 PR looks good to me. It works as expected, and the code looks good. ✨
I'm sorry for the delay; I wanted to be sure about the implications of these changes in our instances.
@navinkarkera, can you sign your commits? The base branch requires all commits to be signed. |
refactor: store config column names in constants
bed7762
to
70bf61f
Compare
@MaferMazu Done. |
Description
Makes use of
studio_configs
field fromTenantConfig
table to keep studio configuration separate from LMS configs. It achieves the separation by having separate signal handlers for LMS and studio.This is required due to conflicts between lms and studio settings. For example, repopulating
third_party_auth
plugin overrides few settings, one of which isSOCIAL_AUTH_STRATEGY
. It needs to be set toauth_backends.strategies.EdxDjangoStrategy
for studio andcommon.djangoapps.third_party_auth.strategy.ConfigurationModelStrategy
for lms for third_party auth to work smoothly.Testing instructions
PLATFORM_NAME
in bothlms_configs
andstudio_configs
field for an external key and link it to lacolhost.com route viaRoutes
table.PLATFORM_NAME
is set.Checklist for Merge