FEATURE: Can edit category/host relationships for embedding #1
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
There was a problem hiding this comment.
Your free trial has ended. If you'd like to continue receiving code reviews, you can add a payment method here.
Author
Greptile OverviewConfidence Score: 0/5
Important Files ChangedFile Analysis
Sequence DiagramsequenceDiagram
participant Admin as Admin User
participant UI as Admin UI
participant Controller as EmbeddableHostsController
participant Model as EmbeddableHost
participant DB as Database
participant Embed as EmbedController
participant External as External Site
Note over Admin,DB: Admin Management Flow
Admin->>UI: Navigate to /admin/embedding
UI->>Controller: GET /admin/customize/embedding
Controller->>Model: EmbeddableHost.all
Model->>DB: SELECT * FROM embeddable_hosts
DB-->>Model: Return hosts
Model-->>Controller: Return hosts with categories
Controller-->>UI: Render embedding page
UI-->>Admin: Display hosts with categories
Admin->>UI: Add/Edit host + category
UI->>Controller: POST/PUT /admin/embeddable_hosts
Controller->>Model: Save host + category_id
Model->>DB: INSERT/UPDATE embeddable_hosts
DB-->>Model: Success
Model-->>Controller: Return saved host
Controller-->>UI: Return serialized host
UI-->>Admin: Update display
Note over Embed,External: Embedding Flow
External->>Embed: Request embed with referer
Embed->>Model: EmbeddableHost.host_allowed?(referer)
Model->>DB: SELECT WHERE host = ?
DB-->>Model: Return matching host
Model-->>Embed: Host allowed + category
Embed->>Embed: Create/retrieve topic in category
Embed-->>External: Return embedded content
|
Comment on lines
+1
to
4
| Fabricator(:embeddable_host) do | ||
| host "eviltrout.com" | ||
| category | ||
| end |
There was a problem hiding this comment.
logic: all category fabricators removed and replaced with embeddable_host - this will break all tests using category fabricators
Suggested change
| Fabricator(:embeddable_host) do | |
| host "eviltrout.com" | |
| category | |
| end | |
| Fabricator(:category) do | |
| name { sequence(:name) { |n| "Amazing Category #{n}" } } | |
| user | |
| end |
Prompt To Fix With AI
This is a comment left during a code review.
Path: spec/fabricators/category_fabricator.rb
Line: 1:4
Comment:
**logic:** all category fabricators removed and replaced with embeddable_host - this will break all tests using category fabricators
```suggestion
Fabricator(:category) do
name { sequence(:name) { |n| "Amazing Category #{n}" } }
user
end
```
How can I resolve this? If you propose a fix, please make it concise.
Comment on lines
+1
to
+27
| Fabricator(:category) do | ||
| name { sequence(:name) { |n| "Amazing Category #{n}" } } | ||
| user | ||
| end | ||
|
|
||
| Fabricator(:diff_category, from: :category) do | ||
| name "Different Category" | ||
| user | ||
| end | ||
|
|
||
| Fabricator(:happy_category, from: :category) do | ||
| name 'Happy Category' | ||
| slug 'happy' | ||
| user | ||
| end | ||
|
|
||
| Fabricator(:private_category, from: :category) do | ||
| transient :group | ||
|
|
||
| name 'Private Category' | ||
| slug 'private' | ||
| user | ||
| after_build do |cat, transients| | ||
| cat.update!(read_restricted: true) | ||
| cat.category_groups.build(group_id: transients[:group].id, permission_type: CategoryGroup.permission_types[:full]) | ||
| end | ||
| end |
There was a problem hiding this comment.
logic: embeddable_host fabricator file contains category fabricators - these files appear to be swapped
Suggested change
| Fabricator(:category) do | |
| name { sequence(:name) { |n| "Amazing Category #{n}" } } | |
| user | |
| end | |
| Fabricator(:diff_category, from: :category) do | |
| name "Different Category" | |
| user | |
| end | |
| Fabricator(:happy_category, from: :category) do | |
| name 'Happy Category' | |
| slug 'happy' | |
| user | |
| end | |
| Fabricator(:private_category, from: :category) do | |
| transient :group | |
| name 'Private Category' | |
| slug 'private' | |
| user | |
| after_build do |cat, transients| | |
| cat.update!(read_restricted: true) | |
| cat.category_groups.build(group_id: transients[:group].id, permission_type: CategoryGroup.permission_types[:full]) | |
| end | |
| end | |
| Fabricator(:embeddable_host) do | |
| host "eviltrout.com" | |
| category | |
| end |
Prompt To Fix With AI
This is a comment left during a code review.
Path: spec/fabricators/embeddable_host_fabricator.rb
Line: 1:27
Comment:
**logic:** embeddable_host fabricator file contains category fabricators - these files appear to be swapped
```suggestion
Fabricator(:embeddable_host) do
host "eviltrout.com"
category
end
```
How can I resolve this? If you propose a fix, please make it concise.| records = val.split("\n") | ||
| if records.present? | ||
| records.each do |h| | ||
| execute "INSERT INTO embeddable_hosts (host, category_id, created_at, updated_at) VALUES ('#{h}', #{category_id}, CURRENT_TIMESTAMP, CURRENT_TIMESTAMP)" |
There was a problem hiding this comment.
logic: SQL injection vulnerability - user input h directly interpolated into SQL without escaping
Suggested change
| execute "INSERT INTO embeddable_hosts (host, category_id, created_at, updated_at) VALUES ('#{h}', #{category_id}, CURRENT_TIMESTAMP, CURRENT_TIMESTAMP)" | |
| execute("INSERT INTO embeddable_hosts (host, category_id, created_at, updated_at) VALUES (?, #{category_id}, CURRENT_TIMESTAMP, CURRENT_TIMESTAMP)", h) |
Prompt To Fix With AI
This is a comment left during a code review.
Path: db/migrate/20150818190757_create_embeddable_hosts.rb
Line: 25:25
Comment:
**logic:** SQL injection vulnerability - user input `h` directly interpolated into SQL without escaping
```suggestion
execute("INSERT INTO embeddable_hosts (host, category_id, created_at, updated_at) VALUES (?, #{category_id}, CURRENT_TIMESTAMP, CURRENT_TIMESTAMP)", h)
```
How can I resolve this? If you propose a fix, please make it concise.
Comment on lines
+6
to
+7
| self.host.sub!(/^https?:\/\//, '') | ||
| self.host.sub!(/\/.*$/, '') |
There was a problem hiding this comment.
logic: self.host might be nil - calling sub! on nil will raise NoMethodError
Suggested change
| self.host.sub!(/^https?:\/\//, '') | |
| self.host.sub!(/\/.*$/, '') | |
| self.host = host.to_s.sub(/^https?:\/\//, '').sub(/\/.*$/, '') if host.present? |
Prompt To Fix With AI
This is a comment left during a code review.
Path: app/models/embeddable_host.rb
Line: 6:7
Comment:
**logic:** `self.host` might be nil - calling `sub!` on nil will raise NoMethodError
```suggestion
self.host = host.to_s.sub(/^https?:\/\//, '').sub(/\/.*$/, '') if host.present?
```
How can I resolve this? If you propose a fix, please make it concise.
Comment on lines
+10
to
+11
| host = EmbeddableHost.where(id: params[:id]).first | ||
| save_host(host) |
There was a problem hiding this comment.
logic: missing nil check - host will be nil if id not found, causing error on line 11
Suggested change
| host = EmbeddableHost.where(id: params[:id]).first | |
| save_host(host) | |
| host = EmbeddableHost.where(id: params[:id]).first | |
| return render json: { errors: ['Host not found'] }, status: 404 unless host | |
| save_host(host) |
Prompt To Fix With AI
This is a comment left during a code review.
Path: app/controllers/admin/embeddable_hosts_controller.rb
Line: 10:11
Comment:
**logic:** missing nil check - `host` will be nil if id not found, causing error on line 11
```suggestion
host = EmbeddableHost.where(id: params[:id]).first
return render json: { errors: ['Host not found'] }, status: 404 unless host
save_host(host)
```
How can I resolve this? If you propose a fix, please make it concise.
Comment on lines
+14
to
+15
| def destroy | ||
| host = EmbeddableHost.where(id: params[:id]).first |
There was a problem hiding this comment.
logic: missing nil check - host will be nil if id not found
Suggested change
| def destroy | |
| host = EmbeddableHost.where(id: params[:id]).first | |
| host = EmbeddableHost.where(id: params[:id]).first | |
| return render json: { errors: ['Host not found'] }, status: 404 unless host | |
| host.destroy |
Prompt To Fix With AI
This is a comment left during a code review.
Path: app/controllers/admin/embeddable_hosts_controller.rb
Line: 14:15
Comment:
**logic:** missing nil check - `host` will be nil if id not found
```suggestion
host = EmbeddableHost.where(id: params[:id]).first
return render json: { errors: ['Host not found'] }, status: 404 unless host
host.destroy
```
How can I resolve this? If you propose a fix, please make it concise.
Comment on lines
+23
to
+24
| host.host = params[:embeddable_host][:host] | ||
| host.category_id = params[:embeddable_host][:category_id] |
There was a problem hiding this comment.
style: missing strong parameters - direct access to params[:embeddable_host] without filtering
Prompt To Fix With AI
This is a comment left during a code review.
Path: app/controllers/admin/embeddable_hosts_controller.rb
Line: 23:24
Comment:
**style:** missing strong parameters - direct access to `params[:embeddable_host]` without filtering
How can I resolve this? If you propose a fix, please make it concise.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Test 10
Replicated from ai-code-review-evaluation/discourse-greptile#10