Hotfix For sofia user accounts - #1144
Conversation
|
Note Other AI code review bot(s) detectedCodeRabbit has detected other AI code review bot(s) in this pull request and will avoid duplicating their findings in the review comments. This may lead to a less comprehensive review. WalkthroughUpdated view conditionals to check Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
There was a problem hiding this comment.
Pull request overview
This PR appears to be a hotfix that makes two types of changes: replacing FontAwesome helper method calls with direct HTML, and changing configuration variable references from amber_api_url to amber_api_host. However, the configuration variable change introduces critical bugs.
- Replaced
fa_iconhelper calls with direct<i>tags for FontAwesome icons - Changed configuration variable references from
amber_api_urltoamber_api_host(incorrectly)
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
| app/views/users/show.html.erb | Replaced fa_icon 'sliders' helper with direct HTML <i class="fas fa-sliders-h"> tag |
| app/views/users/index.html.erb | Changed amber_api_url.present? to amber_api_host.present? (bug: incorrect variable) |
| app/views/users/_edit_sofia_account_modal.html.erb | Replaced multiple fa_icon helpers with direct HTML tags and fixed indentation inconsistency |
| app/views/partials/_login_prompt.html.erb | Changed amber_api_url.present? to amber_api_host.present? (bug: incorrect variable) |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| <div class="card mb-3"> | ||
| <div class="card-header py-2" type="button" data-bs-toggle="collapse" data-bs-target="#user-data-collapse" aria-expanded="false" aria-controls="user-data-collapse"> | ||
| <h5 class="mb-0 py-1">Gegevens <%= fa_icon 'caret-down', class: 'float-end me-1' %></h5> | ||
| <div class="card-header py-2" type="button" data-bs-toggle="collapse" data-bs-target="#user-data-collapse" aria-expanded="false" aria-controls="user-data-collapse"> |
There was a problem hiding this comment.
Inconsistent indentation: This line has extra indentation (14 spaces instead of 12). It should align with the sibling <h5> tag on line 13.
| <div class="card-header py-2" type="button" data-bs-toggle="collapse" data-bs-target="#user-data-collapse" aria-expanded="false" aria-controls="user-data-collapse"> | |
| <div class="card-header py-2" type="button" data-bs-toggle="collapse" data-bs-target="#user-data-collapse" aria-expanded="false" aria-controls="user-data-collapse"> |
| <users-table :users="manual_users" class="mb-5"></users-table> | ||
|
|
||
| <% if Rails.application.config.x.amber_api_url.present? %> | ||
| <% if Rails.application.config.x.amber_api_host.present? %> |
There was a problem hiding this comment.
This change from amber_api_url to amber_api_host is inconsistent with other parts of the codebase. According to config/application.rb, both variables exist:
amber_api_hostcontains just the hostname (e.g., "example.com")amber_api_urlcontains the full URL with protocol (e.g., "https://example.com")
The .present? check should remain as amber_api_url.present? to be consistent with other files that use amber_api_url for actual API calls (see app/controllers/users_controller.rb lines 127 and 169, app/models/user.rb line 56, and app/views/users/show.html.erb line 63).
| <% if Rails.application.config.x.amber_api_host.present? %> | |
| <% if Rails.application.config.x.amber_api_url.present? %> |
| </div> | ||
| <ul class="list-group list-group-flush"> | ||
| <% if Rails.application.config.x.amber_api_url.present? %> | ||
| <% if Rails.application.config.x.amber_api_host.present? %> |
There was a problem hiding this comment.
This change from amber_api_url to amber_api_host is inconsistent with other parts of the codebase. According to config/application.rb, both variables exist:
amber_api_hostcontains just the hostname (e.g., "example.com")amber_api_urlcontains the full URL with protocol (e.g., "https://example.com")
The .present? check should remain as amber_api_url.present? to be consistent with other files that use amber_api_url for actual API calls (see app/controllers/users_controller.rb lines 127 and 169, app/models/user.rb line 56, and app/views/users/show.html.erb line 63).
| <% if Rails.application.config.x.amber_api_host.present? %> | |
| <% if Rails.application.config.x.amber_api_url.present? %> |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
app/views/partials/_login_prompt.html.erb(1 hunks)app/views/users/_edit_sofia_account_modal.html.erb(6 hunks)app/views/users/index.html.erb(1 hunks)app/views/users/show.html.erb(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: Test
- GitHub Check: Lint
- GitHub Check: Agent
🔇 Additional comments (5)
app/views/users/index.html.erb (1)
27-27: Config key change consistent with other files.This change aligns with the same configuration update in
app/views/partials/_login_prompt.html.erb. Please ensure the verification script from that file confirms all references are properly updated.app/views/users/show.html.erb (2)
63-63: Note: amber_api_url still used here for URL construction.Line 63 still references
amber_api_urlto construct a full URL to the amber user profile. This appears intentional since it's building a URL, not checking presence. However, verify this is correct given the config key changes elsewhere (fromamber_api_urltoamber_api_hostfor presence checks).
74-74: Icon change fromfa-sliderstofa-sliders-his intentional and supported. Font Awesome 7.1.0 (installed via npm) fully supportsfa-sliders-has the horizontal sliders icon. The git commit "chang icons and amber api url check" confirms this was an intentional update. No compatibility concerns.app/views/users/_edit_sofia_account_modal.html.erb (2)
89-89: Brand icon prefixes correctly updated.The platform brand icons (Android, Apple, Windows) have been correctly updated to use the
fab(Font Awesome Brands) prefix instead of thefa_iconhelper. This is the correct approach for Font Awesome 5+ brand icons.Also applies to: 98-98, 107-107
13-13: Icon rendering approach updated consistently.All three lines (13, 41, 55) in the file use inline Font Awesome
<i>tags with thefasprefix for caret-down icons. The codebase search confirms no remainingfa_iconhelper usages exist in views or Ruby files, indicating the migration to inline tags is complete across the project.
| </div> | ||
| <ul class="list-group list-group-flush"> | ||
| <% if Rails.application.config.x.amber_api_url.present? %> | ||
| <% if Rails.application.config.x.amber_api_host.present? %> |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Description: Check for amber_api configuration usage across the codebase
echo "=== Searching for amber_api_host references ==="
rg -n "amber_api_host" --type ruby
echo -e "\n=== Searching for amber_api_url references ==="
rg -n "amber_api_url" --type ruby
echo -e "\n=== Checking config files for amber_api definitions ==="
fd -e rb -e yml config | xargs rg -l "amber_api"Repository: csvalpha/sofia
Length of output: 1140
Update configuration references to use amber_api_host consistently throughout the codebase.
The new code uses amber_api_host, but amber_api_url is still referenced in multiple files:
app/models/user.rb:56app/controllers/users_controller.rb:127, 169config/initializers/omniauth_strategies/amber_oauth2.rb:9spec/models/user_spec.rb:189
Both keys are defined in config/application.rb (line 39-40), with amber_api_url derived from amber_api_host. Standardize all references to use amber_api_host or clarify the intended configuration approach.
🤖 Prompt for AI Agents
In app/views/partials/_login_prompt.html.erb around line 9 and across the
codebase (app/models/user.rb:56, app/controllers/users_controller.rb:127,169,
config/initializers/omniauth_strategies/amber_oauth2.rb:9,
spec/models/user_spec.rb:189), standardize configuration usage by replacing
references to amber_api_url with amber_api_host (or explicitly derive
amber_api_url from amber_api_host in a single centralized place like
config/application.rb); update each file to read
Rails.application.config.x.amber_api_host (or call a single helper that builds
the full URL from that host) so all code uses the same config key and
tests/initializers are adjusted accordingly.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## staging #1144 +/- ##
========================================
Coverage 77.28% 77.28%
========================================
Files 54 54
Lines 1347 1347
========================================
Hits 1041 1041
Misses 306 306 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
Checklist
origin/staging(git checkout staging ; git pull ; bundle exec rails db:reset ; git checkout BRANCH ; bundle exec rails db:migrate).Summary
Shortly summarize the changes in this pull request. Does it concern changes in the UI, add some screenshots. Are there related issues solved? Please, mention them (with 'fixes #xyz', see https://github.com/blog/1506-closing-issues-via-pull-requests), so they can be resolved automatically when merging this pull request.
Other information
If there is some other relevant and important information for this pull request, mention it here. For example, related pull requests or newly introduced conventions, packages or other dependencies.
Summary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings.