feat: theme config tenant creation api implementation #234
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
tehreem-sadat
force-pushed
the
tehreem/theme_config_tenant_creation_api_implementation
branch
from
4345c74 to
7ffef17
Compare
shadinaif
requested changes
Feb 25, 2025
| """ | ||
| Generate a tenant configuration by copying the default config and replacing placeholder PARAM. | ||
|
|
||
| :param name: Name to replace 'PARAM' in the default configuration |
There was a problem hiding this comment.
please use platform_name instead of name. The more descriptive is the better
Suggested change
| :param name: Name to replace 'PARAM' in the default configuration | |
| :param platform_name: Name to replace `{{platform_name}}` in the default configuration |
we also need sub_domain and course_org_filter as parameters
There was a problem hiding this comment.
As discussed we don't need course_org_filter separately as param. In default config to copy we can have somethings as follows which can be handled by sub_domain replacement.
"course_org_filter": [
"{sub_domain}"
]
| :param sub_domain: The subdomain to be used for the tenant | ||
| :return: The created TenantConfig object | ||
| """ | ||
| config_data = generate_tenant_config(sub_domain) |
There was a problem hiding this comment.
Suggested change
| config_data = generate_tenant_config(sub_domain) | |
| # .... Verify that the site domain is not already in `Route` .... | |
| config_data = generate_tenant_config(sub_domain) |
tehreem-sadat
force-pushed
the
tehreem/theme_config_tenant_creation_api_implementation
branch
5 times, most recently
from
958e098 to
8263304
Compare
tehreem-sadat
force-pushed
the
tehreem/theme_config_tenant_creation_api_implementation
branch
from
8263304 to
3353162
Compare
shadinaif
approved these changes
Feb 26, 2025
There was a problem hiding this comment.
Thank you @tehreem-sadat . Please address the change requests and merge
Comment on lines
1788
to
1789
| 'description': 'Create new tenant along with default theme config. The caller must have staff or fx ' | ||
| 'api global access.\n', |
There was a problem hiding this comment.
No need for this to be mentioned since it's configurable through View Roles admin page
Suggested change
| 'description': 'Create new tenant along with default theme config. The caller must have staff or fx ' | |
| 'api global access.\n', | |
| 'description': 'Create new tenant along with default theme config.\n', |
| caller=self.fx_permission_info['user'], | ||
| tenant_ids=[tenant_config.id], | ||
| user_keys=[data['owner_user_id']], | ||
| role='staff', |
There was a problem hiding this comment.
Suggested change
| role='staff', | |
| role=cs.COURSE_ACCESS_ROLES_STAFF_EDITOR, |
Comment on lines
93
to
98
| if cs.CACHE_NAMES.get(name) is None: | ||
| raise FXCodedException( | ||
| code=FXExceptionCodes.INVALID_INPUT, | ||
| message=f'Cache with {name} does not exist.' | ||
| ) | ||
| cache.set(name, None) |
There was a problem hiding this comment.
no need to raise an error
Suggested change
| if cs.CACHE_NAMES.get(name) is None: | |
| raise FXCodedException( | |
| code=FXExceptionCodes.INVALID_INPUT, | |
| message=f'Cache with {name} does not exist.' | |
| ) | |
| cache.set(name, None) | |
| cache.delete(name) |
Comment on lines
357
to
361
| if TenantConfig.objects.filter(external_key=sub_domain).exists(): | ||
| raise FXCodedException( | ||
| code=FXExceptionCodes.TENANT_ALREADY_EXIST, | ||
| message=f'Tenant already exists for given sub_domain: ({sub_domain}).', | ||
| ) |
There was a problem hiding this comment.
No need. The external key is a random number sometimes. We'll trust Route and that's it
Suggested change
| if TenantConfig.objects.filter(external_key=sub_domain).exists(): | |
| raise FXCodedException( | |
| code=FXExceptionCodes.TENANT_ALREADY_EXIST, | |
| message=f'Tenant already exists for given sub_domain: ({sub_domain}).', | |
| ) |
| @@ -71,3 +74,36 @@ def wrapped(*args: Any, **kwargs: Any) -> Dict[str, Any]: | |||
|
|
|||
| return wrapped | |||
| return decorator | |||
|
|
|||
|
|
|||
| def invalidate_cache(cache_name: str) -> None: | |||
There was a problem hiding this comment.
I suggest that we use None instead of __all__. Because it's less error prune. So, invalidate_cache() will invalidate everything, looks very clear to me. What do you think?
Suggested change
| def invalidate_cache(cache_name: str) -> None: | |
| def invalidate_cache(cache_name: str = None) -> None: |
tehreem-sadat
force-pushed
the
tehreem/theme_config_tenant_creation_api_implementation
branch
from
3353162 to
7e7ea0e
Compare
tehreem-sadat
deleted the
tehreem/theme_config_tenant_creation_api_implementation
branch
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Description:
New tenant creation API
Related Issue:
#225