Return family tracking data to OwnTrack app #1964
Conversation
WalkthroughPointsController#create now returns formatted OwnTracks locations for family members (via a new OwnTracks::FamilyLocationsFormatter) instead of an empty JSON object; three new OwnTracks::Params helpers were added for battery, connection, and velocity conversions. Changes
Sequence DiagramsequenceDiagram
participant Client
participant PointsCtrl as PointsController#create
participant JobQueue as Job Queue
participant Formatter as OwnTracks::FamilyLocationsFormatter
participant DB as Database
participant Response
Client->>PointsCtrl: POST /api/v1/owntracks/points
PointsCtrl->>JobQueue: Enqueue point creation job
PointsCtrl->>Formatter: new(current_user).call()
rect rgb(230, 245, 255)
Formatter->>DB: Check family feature enabled & load family
Formatter->>DB: Filter members with sharing enabled (exclude current user)
Formatter->>DB: Fetch latest point per sharing member
Formatter->>Formatter: Convert points -> OwnTracks payloads (uses Params helpers)
end
Formatter-->>PointsCtrl: Array of formatted locations
PointsCtrl->>Response: Render JSON with family locations
Response-->>Client: 200 OK with locations
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Tip 📝 Customizable high-level summaries are now available in beta!You can now customize how CodeRabbit generates the high-level summary in your pull requests — including its content, structure, tone, and formatting.
Example instruction:
Note: This feature is currently in beta for Pro-tier users, and pricing will be announced later. 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 |
![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
🧹 Nitpick comments (1)
app/services/own_tracks/family_locations_formatter.rb (1)
33-49: Minor style improvements suggested.RuboCop flags a few style issues:
- Line 45: Trailing comma after the last hash item
- Lines 34-48: Redundant assignment before returning
Apply this diff to address the style issues:
def build_owntracks_location(point) - location = { + { _type: 'location', lat: point.lat.to_f, lon: point.lon.to_f, tst: point.timestamp.to_i, tid: point.user.email, acc: point.accuracy, alt: point.altitude, batt: point.battery, bs: OwnTracks::Params.battery_status_to_numeric(point.battery_status), vel: OwnTracks::Params.velocity_to_kmh(point.velocity), - conn: OwnTracks::Params.connection_to_string(point.connection), + conn: OwnTracks::Params.connection_to_string(point.connection) } - - location end
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
app/controllers/api/v1/owntracks/points_controller.rb(1 hunks)app/services/own_tracks/family_locations_formatter.rb(1 hunks)app/services/own_tracks/params.rb(1 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
app/controllers/api/**/*.rb
📄 CodeRabbit inference engine (CLAUDE.md)
Protect API endpoints with Bearer API keys, except explicitly public-share endpoints which must verify UUID access
Files:
app/controllers/api/v1/owntracks/points_controller.rb
🧬 Code graph analysis (3)
app/services/own_tracks/params.rb (1)
app/serializers/export_serializer.rb (2)
battery_status(47-54)connection(69-76)
app/services/own_tracks/family_locations_formatter.rb (2)
app/services/own_tracks/params.rb (7)
initialize(6-8)call(12-35)battery_status_to_numeric(96-104)battery_status(41-50)velocity_to_kmh(115-124)connection_to_string(106-113)connection(67-76)app/models/concerns/user_family.rb (1)
in_family?(16-18)
app/controllers/api/v1/owntracks/points_controller.rb (2)
app/controllers/api_controller.rb (1)
current_api_user(29-31)app/services/own_tracks/family_locations_formatter.rb (1)
call(10-19)
🪛 RuboCop (1.81.7)
app/services/own_tracks/params.rb
[convention] 111-111: Redundant else-clause.
(Style/EmptyElse)
app/services/own_tracks/family_locations_formatter.rb
[convention] 33-49: Assignment Branch Condition size for build_owntracks_location is too high. [<1, 17, 0> 17.03/17]
(Metrics/AbcSize)
[convention] 33-49: Method has too many lines. [14/10]
(Metrics/MethodLength)
[convention] 34-46: Redundant assignment before returning detected.
(Style/RedundantAssignment)
[convention] 45-45: Avoid comma after the last item of a hash.
(Style/TrailingCommaInHashLiteral)
🔇 Additional comments (5)
app/services/own_tracks/params.rb (2)
96-104: LGTM! Battery status conversion is correct.The reverse mapping from string status to numeric codes matches the forward mapping in the
battery_statusmethod (lines 41-50).
115-124: LGTM! Velocity conversion is mathematically correct.The conversion from m/s to km/h using the 3.6 multiplier is correct and properly handles nil cases. This correctly reverses the conversion in the
speedmethod (lines 78-84).app/controllers/api/v1/owntracks/points_controller.rb (1)
7-13: LGTM! API endpoint is properly protected and implementation is correct.The endpoint is protected with
authenticate_active_api_user!(line 4), which aligns with the coding guidelines. The async job enqueueing followed by returning family locations is correct—the response appropriately shows other family members' locations, while the submitted point is processed asynchronously.app/services/own_tracks/family_locations_formatter.rb (2)
27-31: Remove the suggested refactor—family_sharing_enabled?is not a database column.The method is a computed property that accesses a nested JSONB structure (
settings['family']['location_sharing']), validates conditions, and parses expiration timestamps. This logic cannot be moved to a database query using.where(family_sharing_enabled: true)as suggested. The current implementation with.select(&:family_sharing_enabled?)is the appropriate approach given the complexity of the conditional logic.Likely an incorrect or invalid review comment.
39-39: User email exposure via tid field is architectural and unavoidable; no alternative identifier exists.Verification confirms that the
tidfield is set topoint.user.email, but investigation reveals this is not a fixable issue as suggested:
No alternative identifier available: The User model schema contains only email, encrypted_password, and reset_password_token—there is no username, display_name, or numeric ID-based display name field that could serve as an alternative tracker ID.
Email exposure is system-wide, not isolated: Family members' emails are already exposed throughout the feature in family member lists, map markers (via email initials), and invitation management. Using email in
tidis consistent with the existing family feature design, which is fundamentally email-based.Intentional architectural choice: The system appears designed to use email as the family member identifier. Changing this would require significant refactoring of the family membership and identification system.
The concern about user awareness is valid—consider ensuring users understand that family members will see each other's email addresses, either through UI documentation or consent flows. However, the code itself correctly reflects the system's architecture.
| sharing_members = family_members_with_sharing_enabled | ||
| return [] unless sharing_members.any? | ||
|
|
||
| latest_points = sharing_members.map { |member| member.points.order(timestamp: :desc).first }.compact |
There was a problem hiding this comment.
🧩 Analysis chain
N+1 query issue when fetching latest points.
This line executes one query per family member to fetch their latest point. For families with many members, this will generate excessive database queries.
Apply this diff to fetch all latest points in a single query:
- latest_points = sharing_members.map { |member| member.points.order(timestamp: :desc).first }.compact
- latest_points.map { |point| build_owntracks_location(point) }.compact
+ member_ids = sharing_members.map(&:id)
+ latest_points = Point.where(user_id: member_ids)
+ .select('DISTINCT ON (user_id) *')
+ .order('user_id, timestamp DESC')
+ latest_points.map { |point| build_owntracks_location(point) }.compactNote: The exact SQL may vary depending on your database. For PostgreSQL, DISTINCT ON works well. For MySQL, consider a subquery approach.
Run this script to verify the current query behavior:
🏁 Script executed:
#!/bin/bash
# Description: Check if there are test cases that might reveal the expected query pattern
# Search for existing tests of this formatter
rg -n "FamilyLocationsFormatter" --type=rb -C 3
# Search for similar patterns of fetching latest points per user
rg -n "points\.order.*timestamp.*desc.*first" --type=rb -C 2Length of output: 53
I'll correct the shell scripts to use proper syntax and verify the issue:
Now let me search the actual codebase with correct syntax:
🏁 Script executed:
#!/bin/bash
# Search for the formatter file and examine it
find . -name "family_locations_formatter.rb" -type f 2>/dev/null | head -20Length of output: 55
🏁 Script executed:
#!/bin/bash
# Check database configuration
find . -name "database.yml" -o -name "database.yml.example" 2>/dev/null | head -5Length of output: 21
🏁 Script executed:
#!/bin/bash
# Search for test files related to FamilyLocationsFormatter
find . -name "*family_locations*" -o -name "*own_tracks*" 2>/dev/null | grep -E "(spec|test)" | head -20Length of output: 26
🏁 Script executed:
#!/bin/bash
# Look for similar N+1 patterns in codebase
rg "\.map.*\.first.*compact" --max-count=10 2>/dev/null || echo "Pattern search completed"Length of output: 158
🏁 Script executed:
#!/bin/bash
# Read the formatter file
cat ./app/services/own_tracks/family_locations_formatter.rbLength of output: 1279
🏁 Script executed:
#!/bin/bash
# Check database configuration
cat ./config/database.yml | head -30Length of output: 694
🏁 Script executed:
#!/bin/bash
# List test files in the own_tracks spec directory
ls -la ./spec/services/own_tracks/Length of output: 245
🏁 Script executed:
#!/bin/bash
# Search for Point model to understand the relationship
find . -name "point.rb" -path "*/models/*" 2>/dev/null | head -5Length of output: 21
🏁 Script executed:
#!/bin/bash
# Examine Point model to understand relationships
cat ./app/models/point.rbLength of output: 3352
🏁 Script executed:
#!/bin/bash
# Examine User model relationships
rg -n "class User" --type=ruby -A 20 | head -40Length of output: 2535
🏁 Script executed:
#!/bin/bash
# Check for family_sharing_enabled? implementation
rg -n "family_sharing_enabled" --type=ruby -B 2 -A 5Length of output: 3192
🏁 Script executed:
#!/bin/bash
# Check Point-User relationship and indexes
rg -n "has_many.*point|belongs_to.*user" --type=ruby -C 2 | head -40Length of output: 1804
N+1 query issue when fetching latest points.
This line executes one query per family member to fetch their latest point. For families with many members, this will generate excessive database queries.
Apply this diff to fetch all latest points in a single query:
- latest_points = sharing_members.map { |member| member.points.order(timestamp: :desc).first }.compact
- latest_points.map { |point| build_owntracks_location(point) }.compact
+ member_ids = sharing_members.map(&:id)
+ latest_points = Point.where(user_id: member_ids)
+ .select('DISTINCT ON (user_id) *')
+ .order('user_id, timestamp DESC')
+ latest_points.map { |point| build_owntracks_location(point) }.compactThe codebase uses PostgreSQL with PostGIS, so the DISTINCT ON approach is supported.
Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In app/services/own_tracks/family_locations_formatter.rb around line 17, the
current map that calls member.points.order(...).first issues one query per
member; replace it with a single query using PostgreSQL DISTINCT ON to select
the latest point per member_id (filter by the set of sharing member IDs and use
DISTINCT ON(member_id) with ORDER BY member_id, timestamp DESC) and then build a
hash keyed by member_id to map those points back to the members so you return
the latest points without N+1 queries.
| def connection_to_string(connection) | ||
| case connection | ||
| when 'mobile' then 'm' | ||
| when 'wifi' then 'w' | ||
| when 'offline' then 'o' | ||
| else nil | ||
| end | ||
| end |
There was a problem hiding this comment.
Inconsistent return value for unknown connection type.
This method returns nil for unknown connection types (line 111), but the export_serializer.rb (lines 68-75) returns 'unknown'. While this may be intentional for the OwnTracks format, consider whether consistency across the codebase is preferred.
Also, the else nil clause on line 111 is redundant since Ruby returns nil by default.
Apply this diff to remove the redundant else clause:
def connection_to_string(connection)
case connection
when 'mobile' then 'm'
when 'wifi' then 'w'
when 'offline' then 'o'
- else nil
end
end📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| def connection_to_string(connection) | |
| case connection | |
| when 'mobile' then 'm' | |
| when 'wifi' then 'w' | |
| when 'offline' then 'o' | |
| else nil | |
| end | |
| end | |
| def connection_to_string(connection) | |
| case connection | |
| when 'mobile' then 'm' | |
| when 'wifi' then 'w' | |
| when 'offline' then 'o' | |
| end | |
| end |
🧰 Tools
🪛 RuboCop (1.81.7)
[convention] 111-111: Redundant else-clause.
(Style/EmptyElse)
🤖 Prompt for AI Agents
app/services/own_tracks/params.rb around lines 106 to 113: the method
connection_to_string contains an explicit `else nil` which is redundant (Ruby
returns nil implicitly) and creates an inconsistency with other serializers;
remove the `else nil` clause so the method ends with the case branches only,
returning nil implicitly for unknown connection types (or, if you prefer
consistency with export_serializer, replace the fallback with the string
'unknown' in both places — but at minimum remove the redundant `else nil`).
andresokol
force-pushed
the
owntracks-family
branch
from
d16e354 to
c181a2f
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (2)
app/services/own_tracks/params.rb (1)
94-125: Reverse-conversion helpers look correct; drop redundantelse nilinconnection_to_string.The mappings in
battery_status_to_numericandvelocity_to_kmhlook consistent with the existing forward conversions and OwnTracks expectations. One small improvement inconnection_to_string:
- The explicit
else nilis redundant in Ruby and flagged by RuboCop, and it also differs fromExportSerializer#connection, which uses'unknown'as a fallback. Ifnilis intentional for the OwnTracks format, you can keep that semantic but still simplify the code by removing theelsebranch.Suggested change:
def connection_to_string(connection) case connection when 'mobile' then 'm' when 'wifi' then 'w' when 'offline' then 'o' - else nil end endThis preserves the current behavior (unknown →
nil) while satisfying the style cop and prior review feedback.app/services/own_tracks/family_locations_formatter.rb (1)
14-18: Avoid N+1 queries when loading latest points for sharing members.
sharing_members.map { |member| member.points.order(timestamp: :desc).first }issues one query per member. For larger families this will not scale well. Since you’re on PostgreSQL with PostGIS, you can fetch all latest points in a single query usingDISTINCT ON:- latest_points = sharing_members.map { |member| member.points.order(timestamp: :desc).first }.compact - latest_points.map { |point| build_owntracks_location(point) }.compact + member_ids = sharing_members.map(&:id) + latest_points = Point.where(user_id: member_ids) + .select('DISTINCT ON (user_id) *') + .order('user_id, timestamp DESC') + latest_points.map { |point| build_owntracks_location(point) }.compactThis preserves behavior while eliminating the N+1.
🧹 Nitpick comments (1)
app/services/own_tracks/family_locations_formatter.rb (1)
33-49: Simplifybuild_owntracks_locationand double‑check identifier/PII choice fortid.Functionally the payload looks fine and uses the new helpers correctly. Two follow‑ups:
- Style/cleanup (matches RuboCop hints): you can drop the temporary
locationvariable and trailing comma:- def build_owntracks_location(point) - location = { - _type: 'location', - lat: point.lat.to_f, - lon: point.lon.to_f, - tst: point.timestamp.to_i, - tid: point.user.email, - acc: point.accuracy, - alt: point.altitude, - batt: point.battery, - bs: OwnTracks::Params.battery_status_to_numeric(point.battery_status), - vel: OwnTracks::Params.velocity_to_kmh(point.velocity), - conn: OwnTracks::Params.connection_to_string(point.connection), - } - - location - end + def build_owntracks_location(point) + { + _type: 'location', + lat: point.lat.to_f, + lon: point.lon.to_f, + tst: point.timestamp.to_i, + tid: point.user.email, + acc: point.accuracy, + alt: point.altitude, + batt: point.battery, + bs: OwnTracks::Params.battery_status_to_numeric(point.battery_status), + vel: OwnTracks::Params.velocity_to_kmh(point.velocity), + conn: OwnTracks::Params.connection_to_string(point.connection) + } + end
- Identifier privacy: using
point.user.emailastidexposes family members’ email addresses to the requesting user. If other OwnTracks payloads already do this it may be acceptable, but if a non‑PII identifier (e.g., initials, username, or a dedicated OwnTracks ID) exists, it might be safer to use that instead. Please confirm that
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
app/controllers/api/v1/owntracks/points_controller.rb(1 hunks)app/services/own_tracks/family_locations_formatter.rb(1 hunks)app/services/own_tracks/params.rb(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- app/controllers/api/v1/owntracks/points_controller.rb
🧰 Additional context used
🧬 Code graph analysis (2)
app/services/own_tracks/params.rb (1)
app/serializers/export_serializer.rb (2)
battery_status(47-54)connection(69-76)
app/services/own_tracks/family_locations_formatter.rb (2)
app/services/own_tracks/params.rb (5)
battery_status_to_numeric(96-104)battery_status(41-50)velocity_to_kmh(115-124)connection_to_string(106-113)connection(67-76)app/models/concerns/user_family.rb (1)
in_family?(16-18)
🪛 RuboCop (1.81.7)
app/services/own_tracks/params.rb
[convention] 111-111: Redundant else-clause.
(Style/EmptyElse)
app/services/own_tracks/family_locations_formatter.rb
[convention] 33-49: Assignment Branch Condition size for build_owntracks_location is too high. [<1, 17, 0> 17.03/17]
(Metrics/AbcSize)
[convention] 33-49: Method has too many lines. [14/10]
(Metrics/MethodLength)
[convention] 34-46: Redundant assignment before returning detected.
(Style/RedundantAssignment)
[convention] 45-45: Avoid comma after the last item of a hash.
(Style/TrailingCommaInHashLiteral)
1 participant
OwnTracks supports returning other shared locations in the HTTP API. This PR extends OwnTracks integration by responding with locations of other users in the family.
API Response Enhancement:
createaction inapp/controllers/api/v1/owntracks/points_controller.rbnow returns a JSON array of formatted family member locations instead of an empty object.Family Locations Formatting:
OwnTracks::FamilyLocationsFormatterservice (app/services/own_tracks/family_locations_formatter.rb) to gather and format the latest location points from family members who have sharing enabled, outputting them in Owntracks-compatible format.Owntracks Format Conversion Utilities:
OwnTracks::Paramsfor converting battery status, connection type, and velocity to Owntracks-specific representations (battery_status_to_numeric,connection_to_string,velocity_to_kmh).Summary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings.