Closed navinkarkera closed 7 months ago
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 Any updates on the review of this PR ?
@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. Thanks for the patience!
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 edit_microsite_values
because in the code of this PR is still appearing the legacy edit_tenant_values
; I guess it will be necessary that you update this PR with the changes added in the PR https://github.com/eduNEXT/eox-tenant/pull/182/files to continue with the commands review.
By now, I'll be sharing this with my teammates so they can give their appreciation too.
Again, thank you so much.
@bra-i-am Thanks! I have rebased with branch with latest changes from master. edit_tenant_values
has been updated to edit_microsite_values
. Let me know if you need any other changes.
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. Let's wait one or two weeks for review and proceed with this PR.
Thanks again for your patience.
@navinkarkera, can you sign your commits? The base branch requires all commits to be signed.
@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