-
Notifications
You must be signed in to change notification settings - Fork 4
Improve Album suggestions SOG page #565
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: staging
Are you sure you want to change the base?
Conversation
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## staging #565 +/- ##
===========================================
- Coverage 99.92% 99.84% -0.08%
===========================================
Files 197 197
Lines 2658 2660 +2
===========================================
Hits 2656 2656
- Misses 2 4 +2 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
WalkthroughThe Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant App as App (PhotoAlbum scope)
participant AR as ActiveRecord
participant DB as Database
App->>AR: build subquery (albums ← photos ← tags), group by album_id\nselect album_id where (COUNT(DISTINCT tagged_photos) / COUNT(DISTINCT photos)) < 0.85
AR->>DB: execute SQL (JOIN + GROUP BY + HAVING)
DB-->>AR: return album_ids
AR-->>App: return ActiveRecord::Relation filtered by album_ids
rect rgba(0,128,96,0.08)
Note right of DB: HAVING computes per-album ratio and filters\n(ensure division by zero handled)
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning, 1 inconclusive)
✨ Finishing touches
🧪 Generate unit tests (beta)
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
app/models/photo_album.rb(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). (2)
- GitHub Check: Lint
- GitHub Check: Test
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 0
🧹 Nitpick comments (1)
app/models/photo_album.rb (1)
24-24: Prefer parameterized queries over string interpolation.Direct interpolation of
tag_percentage_thresholdinto the SQL string is not parameterized. While the variable is currently a local constant, this pattern could introduce SQL injection vulnerabilities if later refactored to accept external input.Consider using Arel or a parameterized placeholder:
- .having( - <<~SQL.squish - ( - COALESCE(COUNT(DISTINCT photo_tags.photo_id), 0) * 1.0 / NULLIF(COUNT(DISTINCT photos.id), 0) - ) < #{tag_percentage_threshold} - SQL - ) + .having( + 'COALESCE(COUNT(DISTINCT photo_tags.photo_id), 0) * 1.0 / NULLIF(COUNT(DISTINCT photos.id), 0) < ?', + tag_percentage_threshold + )
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
app/models/photo_album.rb(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). (2)
- GitHub Check: Lint
- GitHub Check: Test
🔇 Additional comments (1)
app/models/photo_album.rb (1)
14-29: Implementation correctly addresses the PR objective.The new threshold-based logic properly calculates the ratio of tagged photos to total photos per album. The combination of
INNER JOIN(line 17) andLEFT JOIN(line 18) ensures that only albums with at least one photo are considered, and photos without tags are correctly included in the denominator. TheNULLIFon line 23 provides good defensive programming against division by zero.
This PR makes it so the threshold for a photoalbum to be considerd tagged is moved to 85% instead of more than 1
Solves #564
Summary by CodeRabbit