Skip to content
Merged
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion apps/chat/serializers/chat_authentication.py
Original file line number Diff line number Diff line change
Expand Up @@ -88,7 +88,7 @@ def profile(self):
"custom_theme": application_setting.custom_theme
if application_setting
else {"theme_color": "", "header_font_color": ""},
"logo": application_setting.avatar if application_setting else "./favicon.ico",
"logo": application_access_token.application.icon,
"meta": {
"application_name": application_access_token.application.name,
"application_id": str(application_access_token.application_id),

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Here is an analysis of the code change, highlighting potential issues and suggestions:

1. Logic Inconsistency (Potential Bug)

  • Issue: The custom_theme line previously relied on an application_setting variable (which might be nullable), while the new logo line relies directly on application_access_token.application.icon. If application_setting was intended to override or provide defaults for logo/theme, ignoring it for the logo breaks symmetry.
  • Question: Should the logo also respect application_setting? For example, if there is a user-uploaded avatar/logo in application_setting, it should probably take precedence over the default application.icon.

2. Lack of Null Safety / Fallbacks

  • Issue: The new code assumes application_access_token and application_access_token.application are never None.
    • If application_access_token is None, accessing .application raises an AttributeError.
    • If application_access_token.application is None, accessing .icon raises an AttributeError.
  • Suggestion: Add safety checks similar to the previous pattern (if ... else ...) or ensure upstream guarantees these objects exist.

3. Inconsistent Handling of Missing Values

  • Previous Code: Provided a string fallback ("./favicon.ico") if application_setting was missing.
  • New Code: No fallback. If icon is null/empty, it may break frontend rendering expecting a valid URL/path.

✅ Recommended Optimized Version

# Assuming you want to prioritize application_setting.avatar if available, 
# falling back to application icon, then favicon.

def profile(self):
    app = application_access_token.application if application_access_token else None
    
    # Determine logo with priority: setting.avatar > app.icon > default
    logo_url = None
    if application_setting and application_setting.avatar:
        logo_url = application_setting.avatar
    elif app and app.icon:
        logo_url = app.icon
    else:
        logo_url = "./favicon.ico"

    return {
        "custom_theme": application_setting.custom_theme
        if application_setting
        else {"theme_color": "", "header_font_color": ""},
        "logo": logo_url,
        "meta": {
            "application_name": app.name if app else "Unknown App",
            "application_id": str(app.id if app else application_access_token.application_id) if application_access_token else None,
        }
    }

Summary of Changes Needed:

  1. Restore fallback logic for the logo.
  2. Consider using application_setting.avatar first if that’s the user-customized logo.
  3. Add null checks for application_access_token and its nested properties to prevent runtime crashes.

Expand Down
Loading