Skip to content
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

Merged
merged 1 commit into from
Apr 15, 2024

Conversation

navinkarkera
Copy link
Contributor

@navinkarkera navinkarkera commented Jul 3, 2023

Description

Makes use of studio_configs field from TenantConfig 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 is SOCIAL_AUTH_STRATEGY. It needs to be set to auth_backends.strategies.EdxDjangoStrategy for studio and common.djangoapps.third_party_auth.strategy.ConfigurationModelStrategy for lms for third_party auth to work smoothly.

Testing instructions

  • Setup nutmeg or master devstack.
  • Install this plugin in both studio and lms.
  • Add a new tenant, for example, lacolhost.com (points to localhost), so you can visit lacolhost.com:18000 to access LMS and lacolhost.com:18010 to access studio.
  • Set different PLATFORM_NAME in both lms_configs and studio_configs field for an external key and link it to lacolhost.com route via Routes table.
  • Visit lacolhost.com:18000 and lacolhost.com:18010 and verify that different PLATFORM_NAME is set.
Screenshots:

image

image

Checklist for Merge

  • Tested in a remote environment
  • Updated documentation
  • Rebased master/main
  • Squashed commits

eox_tenant/receivers_helpers.py Show resolved Hide resolved
eox_tenant/models.py Show resolved Hide resolved
eox_tenant/signals.py Outdated Show resolved Hide resolved
Copy link

@kaustavb12 kaustavb12 left a 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.

@JuanDavidBuitrago
Copy link
Member

Hi @navinkarkera, thanks for your contribution.

@santiagosuarezedunext, Can you check this to review in one of ours sprints?

@Asespinel
Copy link
Contributor

@santiagosuarezedunext friendly reminder to check this new feature

@kaustavb12
Copy link

@santiagosuarezedunext @Asespinel
Any updates on the review of this PR ?

@Asespinel
Copy link
Contributor

@santiagosuarezedunext do we have this review on our future plans?

@santiagosuarezedunext
Copy link

@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!

@bra-i-am
Copy link
Contributor

bra-i-am commented Feb 26, 2024

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.

@navinkarkera
Copy link
Contributor Author

@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.

Copy link
Contributor

@bra-i-am bra-i-am left a 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.

@MaferMazu MaferMazu added the needs maintainer attention Issue or PR specifically needs the attention of the maintainer. label Mar 18, 2024
@MaferMazu
Copy link
Contributor

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.

@MaferMazu MaferMazu removed the needs maintainer attention Issue or PR specifically needs the attention of the maintainer. label Mar 20, 2024
@bra-i-am bra-i-am self-requested a review March 20, 2024 02:21
@MaferMazu
Copy link
Contributor

@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.

@MaferMazu
Copy link
Contributor

@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.

Copy link
Contributor

@MaferMazu MaferMazu left a 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.

@MaferMazu
Copy link
Contributor

@navinkarkera, can you sign your commits? The base branch requires all commits to be signed.

@MaferMazu MaferMazu added the waiting on author PR author needs to resolve review requests, answer questions, fix tests, etc. label Apr 2, 2024
refactor: store config column names in constants
@navinkarkera
Copy link
Contributor Author

@MaferMazu Done.

@bra-i-am bra-i-am merged commit 17cf513 into eduNEXT:master Apr 15, 2024
4 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
waiting on author PR author needs to resolve review requests, answer questions, fix tests, etc.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants