feat: theme config draft api implementation #246
Conversation
tehreem-sadat
changed the title
tehreem-sadat
force-pushed
the
tehreem/theme_config_draft_api_implementation
branch
from
7672fa3 to
47ff729
Compare
There was a problem hiding this comment.
thank you @tehreem-sadat for handling this critical PR. Please check my comments
| """Get draft config""" | ||
|
|
||
| updated_fields = get_draft_tenant_config(int(tenant_id)) | ||
| dict_str = json.dumps(updated_fields, sort_keys=True, separators=(',', ':')) if updated_fields else '' |
There was a problem hiding this comment.
please move the dump and the hashing to a function. You're going to need it when you implement publish API
| ) | ||
|
|
||
| new_value = data.get('new_value') | ||
| reset = data.get('reset', False) |
There was a problem hiding this comment.
please change this to 0 and 1 as like other flags we have (ex. include_staff)
| if not reset and not new_value: | ||
| raise FXCodedException( | ||
| code=FXExceptionCodes.INVALID_INPUT, message='Provide either new_value or reset.' | ||
| ) | ||
| if new_value and not isinstance(new_value, type(data['current_value'])): | ||
| raise FXCodedException( | ||
| code=FXExceptionCodes.INVALID_INPUT, message='New value type must match current value.' | ||
| ) |
There was a problem hiding this comment.
these validations are great, but here we have a hidden bug. new_value might exist but have None as a value. Same for current_value; it can be None (which will cause an exception every time we call the API for that key)
I'll move the discussion to slack, then we post here our discussion: will we allow None for config values or not
| :return: A dictionary with the updated fields and their new values. | ||
| :rtype: dict | ||
| """ | ||
| config = get_all_tenants_info()['config'].get(tenant_id) |
There was a problem hiding this comment.
well, this is one of the cases that we must not use the cache 🥲 . We need to keep safe editing in lms_config, so we'll have to always fetch from database
| message='Invalid path for key: ({key_access_info.key}), path: ({key_access_info.path}) ' | ||
| 'as it does not exist in tenant config for tenant or maybe its value is empty: {tenant_id}.' | ||
| ) | ||
| if stored_value != current_value: |
There was a problem hiding this comment.
we can let MySQL handle the comparison while updating the key. This is good, but it'll not guarantee error detection for parallel write commands
| tenant = TenantConfig.objects.get(id=tenant_id) | ||
| tenant.lms_configs['config_draft'] = draft_config | ||
| tenant.save() | ||
| invalidate_cache() |
There was a problem hiding this comment.
related to not using cache comment; invalidating the cache might jeopardize the dashboard when the designer started to be frequently used. Imaging an empty new tenant with one admin user playing with the designer for a couple of hours. No cache for hundreds of other admins within that period
Description:
Draft config api implementation for get,update,delete
Related Issue:
#240
#241
#243