Skip to content

Conversation

@hussam789
Copy link

@hussam789 hussam789 commented Oct 30, 2025

User description

PR #6


PR Type

Enhancement


Description

  • Move website name logic from frontend to backend serializer

  • Show complete URL path when website domain matches instance domain

  • Display parent domain path when website is subdomain of instance

  • Add conditional inclusion of website_name attribute in serialization


Diagram Walkthrough

flowchart LR
  A["User Profile Website"] --> B["Backend Serializer"]
  B --> C["Compare Domains"]
  C --> D["Same Domain"]
  C --> E["Parent Domain"]
  C --> F["Different Domain"]
  D --> G["Return Full Path"]
  E --> G
  F --> H["Return Host Only"]
  G --> I["website_name Attribute"]
  H --> I
  I --> J["Frontend Template"]
Loading

File Walkthrough

Relevant files
Enhancement
user_serializer.rb
Add backend website_name serialization with domain logic 

app/serializers/user_serializer.rb

  • Add website_name attribute to serialized user attributes
  • Implement website_name method with domain comparison logic
  • Parse website URL and compare against instance hostname
  • Return full path for same/parent domains, host only for different
    domains
  • Add include_website_name conditional to only include when website
    present
+21/-0   
user.js.es6
Remove frontend websiteName computed property                       

app/assets/javascripts/discourse/controllers/user.js.es6

  • Remove websiteName computed property from controller
  • Simplify controller by delegating to backend serialization
+0/-6     
user.hbs
Update template to use backend website_name attribute       

app/assets/javascripts/discourse/templates/user/user.hbs

  • Replace websiteName controller property with model.website_name
  • Update template to use backend-provided website_name attribute
  • Maintain conditional rendering and link behavior
+3/-3     
Tests
user_serializer_spec.rb
Add website_name serialization tests with domain scenarios

spec/serializers/user_serializer_spec.rb

  • Update website test fixture to include URL path
  • Add comprehensive test suite for website_name attribute
  • Test different domain scenarios: unrelated, same, and parent domains
  • Verify correct path handling for each domain relationship
+19/-2   
Documentation
user.js.es6
Fix JSDoc property documentation reference                             

app/assets/javascripts/discourse/models/user.js.es6

  • Fix JSDoc comment from websiteName to profileBackground
  • Correct documentation property reference
+1/-1     

@qodo-code-review
Copy link

PR Compliance Guide 🔍

Below is a summary of compliance checks for this PR:

Security Compliance
URL validation/normalization

Description: The website_name method constructs a display string by concatenating host and
URI(website).path without normalizing or validating the URL, which could enable reflected
XSS or unsafe display if the website field contains malformed or javascript: URLs and the
frontend later renders it unsafely.
user_serializer.rb [137-150]

Referred Code
def website_name
  website_host = URI(website.to_s).host rescue nil
  discourse_host = Discourse.current_hostname
  return if website_host.nil?
  if website_host == discourse_host
    # example.com == example.com
    website_host + URI(website.to_s).path
  elsif (website_host.split('.').length == discourse_host.split('.').length) && discourse_host.split('.').length > 2
    # www.example.com == forum.example.com
    website_host.split('.')[1..-1].join('.') == discourse_host.split('.')[1..-1].join('.') ? website_host + URI(website.to_s).path : website_host
  else
    # example.com == forum.example.com
    discourse_host.ends_with?("." << website_host) ? website_host + URI(website.to_s).path : website_host
  end
Ticket Compliance
🎫 No ticket provided
  • Create ticket/issue
Codebase Duplication Compliance
Codebase context is not defined

Follow the guide to enable codebase context checks.

Custom Compliance
🟢
Generic: Meaningful Naming and Self-Documenting Code

Objective: Ensure all identifiers clearly express their purpose and intent, making code
self-documenting

Status: Passed

Generic: Secure Error Handling

Objective: To prevent the leakage of sensitive system information through error messages while
providing sufficient detail for internal debugging.

Status: Passed

Generic: Secure Logging Practices

Objective: To ensure logs are useful for debugging and auditing without exposing sensitive
information like PII, PHI, or cardholder data.

Status: Passed

Generic: Comprehensive Audit Trails

Objective: To create a detailed and reliable record of critical system actions for security analysis
and compliance.

Status:
No audit logs: The changes add serialization logic for website display without introducing any logging
for critical actions, but no critical actions appear to be performed here.

Referred Code
def website_name
  website_host = URI(website.to_s).host rescue nil
  discourse_host = Discourse.current_hostname
  return if website_host.nil?
  if website_host == discourse_host
    # example.com == example.com
    website_host + URI(website.to_s).path
  elsif (website_host.split('.').length == discourse_host.split('.').length) && discourse_host.split('.').length > 2
    # www.example.com == forum.example.com
    website_host.split('.')[1..-1].join('.') == discourse_host.split('.')[1..-1].join('.') ? website_host + URI(website.to_s).path : website_host
  else
    # example.com == forum.example.com
    discourse_host.ends_with?("." << website_host) ? website_host + URI(website.to_s).path : website_host
  end
end

def include_website_name
  website.present?
end
Generic: Robust Error Handling and Edge Case Management

Objective: Ensure comprehensive error handling that provides meaningful context and graceful
degradation

Status:
Fragile URL parse: The website_name method rescues URI parsing broadly and may return nil or partial results
without logging or handling malformed URLs or empty paths explicitly.

Referred Code
def website_name
  website_host = URI(website.to_s).host rescue nil
  discourse_host = Discourse.current_hostname
  return if website_host.nil?
  if website_host == discourse_host
    # example.com == example.com
    website_host + URI(website.to_s).path
  elsif (website_host.split('.').length == discourse_host.split('.').length) && discourse_host.split('.').length > 2
    # www.example.com == forum.example.com
    website_host.split('.')[1..-1].join('.') == discourse_host.split('.')[1..-1].join('.') ? website_host + URI(website.to_s).path : website_host
  else
    # example.com == forum.example.com
    discourse_host.ends_with?("." << website_host) ? website_host + URI(website.to_s).path : website_host
  end
end
Generic: Security-First Input Validation and Data Handling

Objective: Ensure all data inputs are validated, sanitized, and handled securely to prevent
vulnerabilities

Status:
Input validation: The code uses user-provided website to construct display strings without explicit
validation/sanitization beyond URI parsing, which may allow unexpected or unsafe formats.

Referred Code
def website_name
  website_host = URI(website.to_s).host rescue nil
  discourse_host = Discourse.current_hostname
  return if website_host.nil?
  if website_host == discourse_host
    # example.com == example.com
    website_host + URI(website.to_s).path
  elsif (website_host.split('.').length == discourse_host.split('.').length) && discourse_host.split('.').length > 2
    # www.example.com == forum.example.com
    website_host.split('.')[1..-1].join('.') == discourse_host.split('.')[1..-1].join('.') ? website_host + URI(website.to_s).path : website_host
  else
    # example.com == forum.example.com
    discourse_host.ends_with?("." << website_host) ? website_host + URI(website.to_s).path : website_host
  end
end
Compliance status legend 🟢 - Fully Compliant
🟡 - Partial Compliant
🔴 - Not Compliant
⚪ - Requires Further Human Verification
🏷️ - Compliance label

@qodo-code-review
Copy link

PR Code Suggestions ✨

Explore these optional code suggestions:

CategorySuggestion                                                                                                                                    Impact
High-level
Simplify domain comparison using a library

Replace the complex, custom domain comparison logic in user_serializer.rb with a
dedicated library like public_suffix. This simplifies the code and makes domain
matching more robust and reliable.

Examples:

app/serializers/user_serializer.rb [137-151]
  def website_name
    website_host = URI(website.to_s).host rescue nil
    discourse_host = Discourse.current_hostname
    return if website_host.nil?
    if website_host == discourse_host
      # example.com == example.com
      website_host + URI(website.to_s).path
    elsif (website_host.split('.').length == discourse_host.split('.').length) && discourse_host.split('.').length > 2
      # www.example.com == forum.example.com
      website_host.split('.')[1..-1].join('.') == discourse_host.split('.')[1..-1].join('.') ? website_host + URI(website.to_s).path : website_host

 ... (clipped 5 lines)

Solution Walkthrough:

Before:

# app/serializers/user_serializer.rb
def website_name
  website_host = URI(website.to_s).host
  discourse_host = Discourse.current_hostname
  return if website_host.nil?

  if website_host == discourse_host
    website_host + URI(website.to_s).path
  elsif (/* complex string splitting to check for sibling domains */)
    # ... logic to compare parts of the domain
    website_host.split('.')[1..-1].join('.') == discourse_host.split('.')[1..-1].join('.') ? website_host + URI(website.to_s).path : website_host
  else
    # ... check if one domain ends with the other
    discourse_host.ends_with?("." << website_host) ? website_host + URI(website.to_s).path : website_host
  end
end

After:

# app/serializers/user_serializer.rb
# require 'public_suffix'

def website_name
  website_uri = URI(website.to_s)
  website_host = website_uri.host
  return if website_host.nil?

  website_domain = PublicSuffix.domain(website_host)
  discourse_domain = PublicSuffix.domain(Discourse.current_hostname)

  if website_domain == discourse_domain
    website_host + website_uri.path
  else
    website_host
  end
end
Suggestion importance[1-10]: 9

__

Why: The suggestion correctly identifies that the custom domain comparison logic is complex and brittle, and proposes a significantly more robust and maintainable solution using a standard library, which is a best practice.

High
Possible issue
Fix conditional inclusion method name

Rename the include_website_name method to include_website_name? to ensure it is
correctly used by the serializer for conditional attribute inclusion.

app/serializers/user_serializer.rb [153-155]

-def include_website_name
+def include_website_name?
   website.present?
 end
  • Apply / Chat
Suggestion importance[1-10]: 8

__

Why: The suggestion correctly identifies a bug where the conditional inclusion method include_website_name is missing the required trailing ?, which would cause it to not be recognized by the serializer, leading to incorrect behavior.

Medium
Simplify and fix domain comparison

Refactor the website_name method to simplify the domain comparison logic, fix a
bug where a website on a subdomain of the instance domain is not correctly
handled, and improve readability.

app/serializers/user_serializer.rb [137-151]

 def website_name
-  website_host = URI(website.to_s).host rescue nil
+  begin
+    uri = URI.parse(website.to_s)
+    website_host = uri.host
+  rescue URI::InvalidURIError
+    return
+  end
+
+  return unless website_host
+
   discourse_host = Discourse.current_hostname
-  return if website_host.nil?
-  if website_host == discourse_host
-    # example.com == example.com
-    website_host + URI(website.to_s).path
-  elsif (website_host.split('.').length == discourse_host.split('.').length) && discourse_host.split('.').length > 2
-    # www.example.com == forum.example.com
-    website_host.split('.')[1..-1].join('.') == discourse_host.split('.')[1..-1].join('.') ? website_host + URI(website.to_s).path : website_host
+
+  if website_host == discourse_host ||
+     website_host.end_with?(".#{discourse_host}") ||
+     discourse_host.end_with?(".#{website_host}")
+
+    website_host + uri.path
   else
-    # example.com == forum.example.com
-    discourse_host.ends_with?("." << website_host) ? website_host + URI(website.to_s).path : website_host
+    website_host
   end
 end
  • Apply / Chat
Suggestion importance[1-10]: 8

__

Why: The suggestion correctly identifies a bug in the domain comparison logic and provides a clearer, more robust, and more maintainable implementation that covers all intended subdomain/parent domain scenarios.

Medium
  • More

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants