UX: show complete URL path if website domain is same as instance domain#6
UX: show complete URL path if website domain is same as instance domain#6ShashankFC wants to merge 1 commit into
Conversation
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
📝 WalkthroughWalkthroughThe changes migrate the Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In @app/serializers/user_serializer.rb:
- Around line 153-155: Rename the conditional serializer method
include_website_name to include_website_name? so ActiveModelSerializers
recognizes it as a conditional include; keep the implementation returning
website.present? (i.e., have include_website_name? return website.present?) and
update any references to the old method name if present.
🧹 Nitpick comments (2)
app/serializers/user_serializer.rb (1)
137-151: Consider simplifying this method to reduce complexity and avoid repeated URI parsing.The method parses
URI(website.to_s)up to 4 times and uses a rescue modifier that silently swallows all exceptions. This also addresses the static analysis warnings about high cyclomatic complexity.♻️ Proposed refactor
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 + uri = URI(website.to_s) + website_host = uri.host + return if website_host.nil? + + discourse_host = Discourse.current_hostname + website_path = uri.path + + if same_domain?(website_host, discourse_host) + website_host + website_path + else + website_host + end + rescue URI::InvalidURIError + nil end + + private + + def same_domain?(website_host, discourse_host) + return true if website_host == discourse_host + + website_parts = website_host.split('.') + discourse_parts = discourse_host.split('.') + + # Check if same parent domain with different subdomains (e.g., www.example.com vs forum.example.com) + if website_parts.length == discourse_parts.length && discourse_parts.length > 2 + return website_parts[1..-1] == discourse_parts[1..-1] + end + + # Check if website is parent domain of discourse host (e.g., example.com vs forums.example.com) + discourse_host.end_with?(".#{website_host}") + endspec/serializers/user_serializer_spec.rb (1)
76-91: Consider adding a test for sibling subdomains.The serializer has specific logic for handling sibling subdomains (e.g.,
www.example.comvsforum.example.comat lines 144-146 inuser_serializer.rb), but this case isn't covered by the tests.📝 Additional test case
it "returns complete website path when website and instance are sibling subdomains" do user.user_profile.website = 'http://www.example.com/user' Discourse.stubs(:current_hostname).returns('forum.example.com') serialized = UserSerializer.new(user, scope: Guardian.new, root: false).as_json expect(serialized[:website_name]).to eq 'www.example.com/user' end
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
app/assets/javascripts/discourse/controllers/user.js.es6app/assets/javascripts/discourse/models/user.js.es6app/assets/javascripts/discourse/templates/user/user.hbsapp/serializers/user_serializer.rbspec/serializers/user_serializer_spec.rb
💤 Files with no reviewable changes (1)
- app/assets/javascripts/discourse/controllers/user.js.es6
🧰 Additional context used
🧬 Code graph analysis (2)
spec/serializers/user_serializer_spec.rb (1)
app/serializers/user_serializer.rb (1)
website(133-135)
app/serializers/user_serializer.rb (2)
lib/discourse.rb (1)
current_hostname(178-184)lib/global_path.rb (1)
path(2-4)
🪛 RuboCop (1.82.1)
app/serializers/user_serializer.rb
[convention] 137-151: Assignment Branch Condition size for website_name is too high. [<2, 35, 12> 37.05/23]
(Metrics/AbcSize)
[convention] 137-151: Cyclomatic complexity for website_name is too high. [8/7]
(Metrics/CyclomaticComplexity)
[convention] 137-151: Perceived complexity for website_name is too high. [9/8]
(Metrics/PerceivedComplexity)
[convention] 138-138: Avoid using rescue in its modifier form.
(Style/RescueModifier)
🔇 Additional comments (2)
app/assets/javascripts/discourse/models/user.js.es6 (1)
56-66: LGTM!The JSDoc correction properly aligns the
@propertytag with the actualprofileBackgroundproperty it documents.app/assets/javascripts/discourse/templates/user/user.hbs (1)
66-73: LGTM!The template correctly references the new
model.website_nameattribute from the serializer while preservingmodel.websitefor the full URL in thehref. The conditional logic and display behavior are maintained.
| def include_website_name | ||
| website.present? | ||
| end |
There was a problem hiding this comment.
Missing ? suffix on include method.
ActiveModelSerializers expects the method to be named include_website_name? with a trailing ? for the conditional inclusion to work. Without this, the attribute will always be serialized regardless of whether website is present.
🐛 Proposed fix
- def include_website_name
+ def include_website_name?
website.present?
end🤖 Prompt for AI Agents
In @app/serializers/user_serializer.rb around lines 153 - 155, Rename the
conditional serializer method include_website_name to include_website_name? so
ActiveModelSerializers recognizes it as a conditional include; keep the
implementation returning website.present? (i.e., have include_website_name?
return website.present?) and update any references to the old method name if
present.
| context "has a website name" do | ||
| it "returns website host name when instance domain is not same as website domain" do | ||
| Discourse.stubs(:current_hostname).returns('discourse.org') | ||
| expect(json[:website_name]).to eq 'example.com' | ||
| end | ||
|
|
||
| it "returns complete website path when instance domain is same as website domain" do | ||
| Discourse.stubs(:current_hostname).returns('example.com') | ||
| expect(json[:website_name]).to eq 'example.com/user' | ||
| end | ||
|
|
||
| it "returns complete website path when website domain is parent of instance domain" do | ||
| Discourse.stubs(:current_hostname).returns('forums.example.com') | ||
| expect(json[:website_name]).to eq 'example.com/user' | ||
| end | ||
| end |
There was a problem hiding this comment.
Tests may not work as expected due to let memoization order.
The json variable is memoized via let at line 21, which calls serializer.as_json. The stubs for Discourse.current_hostname are set up after json would be evaluated on first access. Depending on when json is first accessed, the stub might not be in effect.
Consider using let! to force eager evaluation after stubbing, or restructure to ensure stubs are in place before serialization:
🐛 Proposed fix
context "has a website name" do
it "returns website host name when instance domain is not same as website domain" do
Discourse.stubs(:current_hostname).returns('discourse.org')
- expect(json[:website_name]).to eq 'example.com'
+ serialized = UserSerializer.new(user, scope: Guardian.new, root: false).as_json
+ expect(serialized[:website_name]).to eq 'example.com'
end
it "returns complete website path when instance domain is same as website domain" do
Discourse.stubs(:current_hostname).returns('example.com')
- expect(json[:website_name]).to eq 'example.com/user'
+ serialized = UserSerializer.new(user, scope: Guardian.new, root: false).as_json
+ expect(serialized[:website_name]).to eq 'example.com/user'
end
it "returns complete website path when website domain is parent of instance domain" do
Discourse.stubs(:current_hostname).returns('forums.example.com')
- expect(json[:website_name]).to eq 'example.com/user'
+ serialized = UserSerializer.new(user, scope: Guardian.new, root: false).as_json
+ expect(serialized[:website_name]).to eq 'example.com/user'
end
end2 participants
Test 6
Summary by CodeRabbit
Refactor
Tests
✏️ Tip: You can customize this high-level summary in your review settings.
Replicated from ai-code-review-evaluation/discourse-coderabbit#6