Skip to content

Commit be6994a

Browse files
stakachclaude
andcommitted
fix(signage): stable index ordering for pagination
Unordered indexes made OFFSET pagination unstable. Media and template indexes now return newest first (created_at DESC with an id tiebreak); the playlists index sorts by name with natural ordering a->z (first embedded number compared numerically, so "Screen 2" precedes "Screen 10", case-insensitive). paginate_sql runs its COUNT with ordering unscoped as Postgres rejects non-aggregated ORDER BY columns in aggregate queries. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
1 parent c3ddf1d commit be6994a

7 files changed

Lines changed: 64 additions & 1 deletion

File tree

spec/controllers/signage/playlist_media_spec.cr

Lines changed: 12 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -84,6 +84,18 @@ module PlaceOS::Api
8484
end
8585

8686
describe "index filtering" do
87+
it "returns newest items first" do
88+
authority = Model::Authority.find_by_domain("localhost").not_nil!
89+
oldest = Model::Generator.item(authority: authority).save!
90+
middle = Model::Generator.item(authority: authority).save!
91+
newest = Model::Generator.item(authority: authority).save!
92+
93+
result = client.get(base, headers: Spec::Authentication.headers)
94+
result.status_code.should eq 200
95+
ids = Array(Hash(String, JSON::Any)).from_json(result.body).map(&.["id"].as_s)
96+
ids.should eq [newest.id.to_s, middle.id.to_s, oldest.id.to_s]
97+
end
98+
8799
it "scopes non-admin callers to items linked to their groups" do
88100
authority = Model::Authority.find_by_domain("localhost").not_nil!
89101
user, headers = Spec::Authentication.authentication(sys_admin: false, support: false)

spec/controllers/signage/playlists_spec.cr

Lines changed: 22 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -125,6 +125,28 @@ module PlaceOS::Api
125125
end
126126

127127
describe "index filtering" do
128+
it "sorts playlists by name with natural ordering" do
129+
authority = Model::Authority.find_by_domain("localhost").not_nil!
130+
131+
names = ["Screen 10", "atrium", "Screen 2", "Foyer"]
132+
playlists = names.map do |name|
133+
playlist = Model::Generator.playlist(authority: authority)
134+
playlist.name = name
135+
playlist.save!
136+
end
137+
by_name = playlists.to_h { |p| {p.name, p.id.to_s} }
138+
139+
result = client.get(base, headers: Spec::Authentication.headers)
140+
result.status_code.should eq 200
141+
ids = Array(Hash(String, JSON::Any)).from_json(result.body).map(&.["id"].as_s)
142+
ids.should eq [
143+
by_name["atrium"],
144+
by_name["Foyer"],
145+
by_name["Screen 2"],
146+
by_name["Screen 10"],
147+
]
148+
end
149+
128150
it "scopes non-admin callers to playlists linked to their groups" do
129151
authority = Model::Authority.find_by_domain("localhost").not_nil!
130152
user, headers = Spec::Authentication.authentication(sys_admin: false, support: false)

spec/controllers/signage/templates_spec.cr

Lines changed: 12 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -166,6 +166,18 @@ module PlaceOS::Api
166166
end
167167

168168
describe "index filtering" do
169+
it "returns newest templates first" do
170+
authority = Model::Authority.find_by_domain("localhost").not_nil!
171+
oldest = Model::Generator.signage_template(authority: authority).save!
172+
middle = Model::Generator.signage_template(authority: authority).save!
173+
newest = Model::Generator.signage_template(authority: authority).save!
174+
175+
result = client.get(base, headers: Spec::Authentication.headers)
176+
result.status_code.should eq 200
177+
ids = Array(Hash(String, JSON::Any)).from_json(result.body).map(&.["id"].as_s)
178+
ids.should eq [newest.id.to_s, middle.id.to_s, oldest.id.to_s]
179+
end
180+
169181
it "scopes non-admin callers to templates linked to their groups" do
170182
authority = Model::Authority.find_by_domain("localhost").not_nil!
171183
user, headers = Spec::Authentication.authentication(sys_admin: false, support: false)

src/placeos-rest-api/controllers/application.cr

Lines changed: 3 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -98,7 +98,9 @@ module PlaceOS::Api
9898
offset : Int32 = 0,
9999
route : String = base_route,
100100
)
101-
total = query.count.to_i32
101+
# ORDER BY must not reach the aggregate — Postgres rejects
102+
# non-aggregated order columns in a COUNT query
103+
total = query.unscope(:order).count.to_i32
102104
results = query.offset(offset).limit(limit).to_a
103105

104106
range_end = offset + results.size

src/placeos-rest-api/controllers/signage/playlist_media.cr

Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -156,6 +156,9 @@ module PlaceOS::Api
156156
query = query.where("tags && ARRAY[#{placeholders}]::text[]", args: filter_tags)
157157
end
158158

159+
# newest items first; id tiebreak keeps pagination stable
160+
query = query.order("created_at DESC, id")
161+
159162
paginate_sql(query, type: "playlist_items", limit: limit, offset: offset)
160163
end
161164

src/placeos-rest-api/controllers/signage/playlists.cr

Lines changed: 9 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -148,6 +148,15 @@ module PlaceOS::Api
148148
query = query.where("(name ILIKE ? OR description ILIKE ?)", pattern, pattern)
149149
end
150150

151+
# natural name ordering a->z: leading text, then the first embedded
152+
# number numerically ("Screen 2" before "Screen 10"), then the full
153+
# name case-insensitively; id tiebreak keeps pagination stable
154+
query = query.order(
155+
"substring(lower(name) from '^\\D*'), " \
156+
"NULLIF(substring(name from '\\d+'), '')::numeric NULLS FIRST, " \
157+
"lower(name), id"
158+
)
159+
151160
paginate_sql(query, type: "playlists", limit: limit, offset: offset)
152161
end
153162

src/placeos-rest-api/controllers/signage/templates.cr

Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -203,6 +203,9 @@ module PlaceOS::Api
203203
query = query.where("(name ILIKE ? OR description ILIKE ?)", pattern, pattern)
204204
end
205205

206+
# newest templates first; id tiebreak keeps pagination stable
207+
query = query.order("created_at DESC, id")
208+
206209
paginate_sql(query, type: "templates", limit: limit, offset: offset)
207210
end
208211

0 commit comments

Comments
 (0)