eduNEXT / eox-tenant

Plugin for managing multiple tenants (organizations) within a single Open edX instance.
GNU Affero General Public License v3.0
7 stars 4 forks source link

fix: prioritize current site setting for org value #189

Closed navinkarkera closed 8 months ago

navinkarkera commented 1 year ago

Description

If multiple tenant configs are enabled for a given org, fetching value from site config for the organization should return current site config value if the org is included in the org_filter.

Private-ref: BB-7927

Testing instructions

See https://github.com/open-craft/eox-tenant/pull/3

Checklist for Merge

kaustavb12 commented 1 year ago

@navinkarkera Could you please add our internal ticket number here too as private ref, for easy reference.

bra-i-am commented 9 months ago

Hello @navinkarkera and @kaustavb12, I hope you are doing well. We thank you for contributing to this project and apologize for the delay in reviewing it due to some urgent matters.

The solution seems logical. However, before moving forward, I would like to test it to gain a more comprehensive understanding of the expected behavior and the match with all the use cases.

I followed the steps in the referenced PR https://github.com/open-craft/eox-tenant/pull/3, but it didn't work for me. In order to be more specific, here are the steps I followed:

  1. Ran make {lms,cms}-up
  2. Cloned your repo fork (to be allowed to use the branch navin/upstream/current-site-priority-for-org-value), went into the shells in the LMS & CMS, and ran the pip install -e command to the repo
  3. Ran the two migrations despite only the LMS executed the changes (expected behavior)
  4. Created the two Routes with their corresponding tenant configs the same as you did
  5. Updated the redirect URLs in thestudio-sso application
  6. Added the setting in the private.py files
  7. Reseted

This is what I receive when I try to see the link when View live is hovered: PR SCREEN

These are my tenant configs with the settings you told in open-craft#3 and there are shown the domains I use too image

To test this PR I mounted a new environment in devstack, please let me know if maybe you have any other setting/configuration I'm missing or maybe you have an idea about what can I do to run this PR properly.

Perhaps, if you have tested this PR using Tutor, could you please guide us a little through the process you followed?

I will be closely monitoring your response to continue with my review or provide any more information if needed.

Thank you so much ✨

navinkarkera commented 8 months ago

Hello @bra-i-am, I just rebased the branch with master and followed the steps mentioned in https://github.com/open-craft/eox-tenant/pull/3 to test in master devstack. It worked for me and I did not do anything special.

lacolhost-screenshot localhost-screenshot

Please make sure that you are using open-craft:navin/upstream/current-site-priority-for-org-value branch

MaferMazu commented 8 months ago

@navinkarkera, can you sign your commit, please?

Ref: https://docs.github.com/en/authentication/managing-commit-signature-verification/about-commit-signature-verification

navinkarkera commented 8 months ago

@MaferMazu Done.