diff --git a/CLAUDE.md b/CLAUDE.md new file mode 100644 index 00000000..483a37f8 --- /dev/null +++ b/CLAUDE.md @@ -0,0 +1,66 @@ +PlaceOS is a building automation platform. + +## Working on this project + +This crystal lang library is the API layer, made up of RESP API controllers. Functioning similar to ruby on rails controllers. + +Use `crystal tool format` and `./bin/ameba` to format and lint code. +Run specs using `./test` make sure to run specs using a subagent. You can also run individual spec files or use `focus: true` to isolate the spec you're working on. Tests can take some time to run. + +Make sure to write thorough tests. Reference existing models in the lib folder if you want to see ORM internals. There are some extentions to the ORM in this project in ./lib/placeos-models/base/* if you need to understand why something isn't working. + +Make sure to create and maintain a PLAN.md file to keep track of progress. + +## 1. Plan Node Default +- Enter plan mode for ANY non-trivial task (3+ steps or architectural decisions) +- If something goes sideways, STOP and re-plan immediately, don’t keep pushing +- Use plan mode for verification steps, not just building +- Write detailed specs upfront to reduce ambiguity + +## 2. Subagent Strategy +- Use subagents liberally to keep main context window clean +- Offload research, exploration, and parallel analysis to subagents +- For complex problems, throw more compute at it via subagents +- One task per subagent for focused execution + +## 3. Self-Improvement Loop +- After ANY correction from the user, update `tasks/lessons.md` with the pattern +- Write rules for yourself that prevent the same mistake +- Ruthlessly iterate on these lessons until mistake rate drops +- Review lessons at session start for relevant project + +## 4. Verification Before Done +- Never mark a task complete without proving it works +- Diff behavior between main and your changes when relevant +- Ask yourself: "Would a staff engineer approve this?" +- Run tests, check logs, demonstrate correctness + +## 5. Demand Elegance (Balanced) +- For non-trivial changes, pause and ask: "Is there a more elegant way?" +- If a fix feels hacky: "Knowing everything I know now, implement the elegant solution" +- Skip this for simple, obvious fixes, don’t over-engineer +- Challenge your own work before presenting it + +## 6. Autonomous Bug Fixing +- When given a bug report, don’t ask for hand-holding +- Don’t start by trying to fix it. Instead, start by writing a test that reproduces the bug. Then, have subagents try to fix the bug and prove it by passing that test. +- Point at logs, errors, failing tests, then resolve them +- Zero context switching required from the user + +--- + +## Task Management + +1. **Plan First**: Write plan to `tasks/todo.md` with checkable items +2. **Verify Plan**: Check in before starting implementation +3. **Track Progress**: Mark items complete as you go +4. **Explain Changes**: High-level summary at each step +5. **Document Results**: Add review section to `tasks/todo.md` +6. **Capture Lessons**: Update `tasks/lessons.md` after corrections + +--- + +## Core Principles + +- **Simplicity First**: Make every change as simple as possible. Impact minimal code. +- **No Laziness**: Find root causes. No temporary fixes. Senior developer standards. diff --git a/OPENAPI_DOC.yml b/OPENAPI_DOC.yml index bb7a7f9d..ffe647e4 100644 --- a/OPENAPI_DOC.yml +++ b/OPENAPI_DOC.yml @@ -10271,6 +10271,7 @@ paths: callers must have Read on the group schema: type: string + format: uuid nullable: true - name: subsystem in: query @@ -13872,6 +13873,7 @@ paths: required: true schema: type: string + format: uuid responses: 200: description: OK @@ -13952,6 +13954,7 @@ paths: required: true schema: type: string + format: uuid responses: 200: description: OK @@ -14026,6 +14029,7 @@ paths: required: true schema: type: string + format: uuid responses: 202: description: Accepted @@ -14102,6 +14106,7 @@ paths: required: true schema: type: string + format: uuid responses: 200: description: OK @@ -14257,6 +14262,7 @@ paths: in: query schema: type: string + format: uuid nullable: true - name: limit in: query @@ -14365,6 +14371,7 @@ paths: required: true schema: type: string + format: uuid responses: 200: description: OK @@ -14442,6 +14449,7 @@ paths: in: query schema: type: string + format: uuid nullable: true - name: limit in: query @@ -14630,6 +14638,7 @@ paths: required: true schema: type: string + format: uuid responses: 200: description: OK @@ -14703,6 +14712,7 @@ paths: required: true schema: type: string + format: uuid responses: 202: description: Accepted @@ -14777,6 +14787,7 @@ paths: required: true schema: type: string + format: uuid responses: 200: description: OK @@ -14856,6 +14867,7 @@ paths: in: query schema: type: string + format: uuid nullable: true - name: user_id in: query @@ -15048,6 +15060,7 @@ paths: required: true schema: type: string + format: uuid responses: 200: description: OK @@ -15132,6 +15145,7 @@ paths: required: true schema: type: string + format: uuid responses: 200: description: OK @@ -15210,6 +15224,7 @@ paths: required: true schema: type: string + format: uuid responses: 202: description: Accepted @@ -15290,6 +15305,7 @@ paths: required: true schema: type: string + format: uuid responses: 200: description: OK @@ -15364,6 +15380,7 @@ paths: in: query schema: type: string + format: uuid nullable: true - name: zone_id in: query @@ -15551,6 +15568,7 @@ paths: required: true schema: type: string + format: uuid - name: zone_id in: path required: true @@ -15635,6 +15653,7 @@ paths: required: true schema: type: string + format: uuid - name: zone_id in: path required: true @@ -15713,6 +15732,7 @@ paths: required: true schema: type: string + format: uuid - name: zone_id in: path required: true @@ -15793,6 +15813,7 @@ paths: required: true schema: type: string + format: uuid - name: zone_id in: path required: true @@ -21127,6 +21148,7 @@ paths: the group) schema: type: string + format: uuid nullable: true - name: q in: query @@ -21251,6 +21273,7 @@ paths: callers) schema: type: string + format: uuid nullable: true responses: 201: @@ -21719,6 +21742,7 @@ paths: required: true schema: type: string + format: uuid responses: 200: description: OK @@ -21807,6 +21831,7 @@ paths: on the group) schema: type: string + format: uuid nullable: true - name: q in: query @@ -21935,6 +21960,7 @@ paths: callers) schema: type: string + format: uuid nullable: true responses: 201: @@ -22332,6 +22358,7 @@ paths: required: true schema: type: string + format: uuid responses: 200: description: OK @@ -28039,6 +28066,7 @@ paths: so callers can drill down. schema: type: string + format: uuid nullable: true - name: include_children_count in: query diff --git a/shard.lock b/shard.lock index b8b6a32a..7a209162 100644 --- a/shard.lock +++ b/shard.lock @@ -211,7 +211,7 @@ shards: placeos-frontend-loader: git: https://github.com/placeos/frontend-loader.git - version: 2.7.1+git.commit.7a6d8c313b0d4f58fbb9ba71948fe889c6d51ab5 + version: 2.7.1+git.commit.a2e3892abaa33f33dbd0918d307e82d1099cea65 placeos-log-backend: git: https://github.com/place-labs/log-backend.git @@ -219,7 +219,7 @@ shards: placeos-models: git: https://github.com/placeos/models.git - version: 9.90.0 + version: 9.90.1 placeos-resource: git: https://github.com/place-labs/resource.git @@ -267,7 +267,7 @@ shards: search-ingest: git: https://github.com/placeos/search-ingest.git - version: 2.11.3+git.commit.fc516c008c9f2116586db762924e79c6fff587a7 + version: 2.11.3+git.commit.1a187036e24d2c16fe44e8976c54cb8a417d3139 secrets-env: # Overridden git: https://github.com/spider-gazelle/secrets-env.git diff --git a/spec/controllers/signage/playlists_spec.cr b/spec/controllers/signage/playlists_spec.cr index ff43e810..cb9441ff 100644 --- a/spec/controllers/signage/playlists_spec.cr +++ b/spec/controllers/signage/playlists_spec.cr @@ -303,5 +303,93 @@ module PlaceOS::Api result.status_code.should eq 403 end end + + describe "GET /:id/media and /:id/media/revisions" do + it "returns the latest revision with media hydrated for a playlist Reader, even when items aren't shared with their group" do + authority = Model::Authority.find_by_domain("localhost").not_nil! + user, headers = Spec::Authentication.authentication(sys_admin: false, support: false) + + # Reader has access to the playlist via this group only — items are NOT linked here. + reader_group = Model::Generator.group(authority: authority).save! + Model::Generator.group_user(user: user, group: reader_group, permissions: Model::Permissions::Read).save! + + playlist = Model::Generator.playlist(authority: authority).save! + Model::Generator.group_playlist(group: reader_group, playlist: playlist).save! + + # Items are admin-only (no GroupPlaylistItem rows) — they were created by an + # admin / another team and should still be visible to the reader because they + # belong to a playlist the reader can access. + item_a = Model::Generator.item(authority: authority).save! + item_b = Model::Generator.item(authority: authority).save! + + author = Model::Generator.user(authority: authority).save! + revision = Model::Generator.revision(playlist: playlist, user: author) + revision.items = [item_a.id.as(String), item_b.id.as(String)] + revision.save! + + result = client.get(File.join(base, playlist.id.to_s, "media"), headers: headers) + result.status_code.should eq 200 + + body = JSON.parse(result.body).as_h + body["items"].as_a.map(&.as_s).sort!.should eq [item_a.id.to_s, item_b.id.to_s].sort + body["media"].should_not be_nil + media_ids = body["media"].as_a.map(&.as_h.["id"].as_s).sort! + media_ids.should eq [item_a.id.to_s, item_b.id.to_s].sort + end + + it "returns all revisions with media hydrated for a playlist Reader" do + authority = Model::Authority.find_by_domain("localhost").not_nil! + user, headers = Spec::Authentication.authentication(sys_admin: false, support: false) + + reader_group = Model::Generator.group(authority: authority).save! + Model::Generator.group_user(user: user, group: reader_group, permissions: Model::Permissions::Read).save! + + playlist = Model::Generator.playlist(authority: authority).save! + Model::Generator.group_playlist(group: reader_group, playlist: playlist).save! + + item_a = Model::Generator.item(authority: authority).save! + item_b = Model::Generator.item(authority: authority).save! + + author = Model::Generator.user(authority: authority).save! + first = Model::Generator.revision(playlist: playlist, user: author) + first.items = [item_a.id.as(String)] + first.approved = true + first.save! + + second = Model::Generator.revision(playlist: playlist, user: author) + second.items = [item_a.id.as(String), item_b.id.as(String)] + second.save! + + result = client.get(File.join(base, playlist.id.to_s, "media", "revisions"), headers: headers) + result.status_code.should eq 200 + + revisions = JSON.parse(result.body).as_a.map(&.as_h) + revisions.size.should be >= 1 + revisions.each do |rev| + rev["media"].should_not be_nil + item_ids = rev["items"].as_a.map(&.as_s).sort! + media_ids = rev["media"].as_a.map(&.as_h.["id"].as_s).sort! + media_ids.should eq item_ids + end + end + + it "returns an empty media array when the playlist has no revisions" do + authority = Model::Authority.find_by_domain("localhost").not_nil! + user, headers = Spec::Authentication.authentication(sys_admin: false, support: false) + + reader_group = Model::Generator.group(authority: authority).save! + Model::Generator.group_user(user: user, group: reader_group, permissions: Model::Permissions::Read).save! + + playlist = Model::Generator.playlist(authority: authority).save! + Model::Generator.group_playlist(group: reader_group, playlist: playlist).save! + + result = client.get(File.join(base, playlist.id.to_s, "media"), headers: headers) + result.status_code.should eq 200 + + body = JSON.parse(result.body).as_h + body["items"].as_a.should be_empty + body["media"].as_a.should be_empty + end + end end end diff --git a/src/placeos-rest-api/controllers/groups.cr b/src/placeos-rest-api/controllers/groups.cr index be646de2..154ce50b 100644 --- a/src/placeos-rest-api/controllers/groups.cr +++ b/src/placeos-rest-api/controllers/groups.cr @@ -26,9 +26,9 @@ module PlaceOS::Api ############################################################################################### @[AC::Route::Filter(:before_action, except: [:index, :create, :current])] - def find_current_group(id : String) - Log.context.set(group_id: id) - @current_group = ::PlaceOS::Model::Group.find!(UUID.new(id)) + def find_current_group(id : UUID) + Log.context.set(group_id: id.to_s) + @current_group = ::PlaceOS::Model::Group.find!(id) end getter! current_group : ::PlaceOS::Model::Group diff --git a/src/placeos-rest-api/controllers/groups/history.cr b/src/placeos-rest-api/controllers/groups/history.cr index a88def44..25920c16 100644 --- a/src/placeos-rest-api/controllers/groups/history.cr +++ b/src/placeos-rest-api/controllers/groups/history.cr @@ -17,9 +17,9 @@ module PlaceOS::Api ############################################################################################### @[AC::Route::Filter(:before_action, only: [:show])] - def find_current_history(id : String) - Log.context.set(group_history_id: id) - @current_history = ::PlaceOS::Model::GroupHistory.find!(UUID.new(id)) + def find_current_history(id : UUID) + Log.context.set(group_history_id: id.to_s) + @current_history = ::PlaceOS::Model::GroupHistory.find!(id) end getter! current_history : ::PlaceOS::Model::GroupHistory @@ -42,20 +42,19 @@ module PlaceOS::Api # pass `group_id=` and have Manage on it. @[AC::Route::GET("/")] def index( - group_id : String? = nil, + group_id : UUID? = nil, limit : Int32 = 100, offset : Int32 = 0, ) : Array(::PlaceOS::Model::GroupHistory) if user_admin? query = ::PlaceOS::Model::GroupHistory.all - query = query.where(group_id: UUID.new(group_id)) if group_id + query = query.where(group_id: group_id) if group_id else # Non-admin must scope to a specific group they manage. raise Error::Forbidden.new("group_id is required") unless group_id - target = UUID.new(group_id) - group = ::PlaceOS::Model::Group.find!(target) + group = ::PlaceOS::Model::Group.find!(group_id) ensure_manage!(current_user, group) - query = ::PlaceOS::Model::GroupHistory.where(group_id: target) + query = ::PlaceOS::Model::GroupHistory.where(group_id: group_id) end paginate_sql(query, type: "group_history", limit: limit, offset: offset) diff --git a/src/placeos-rest-api/controllers/groups/invitation.cr b/src/placeos-rest-api/controllers/groups/invitation.cr index 0681a0ec..a88b603d 100644 --- a/src/placeos-rest-api/controllers/groups/invitation.cr +++ b/src/placeos-rest-api/controllers/groups/invitation.cr @@ -21,9 +21,9 @@ module PlaceOS::Api ############################################################################################### @[AC::Route::Filter(:before_action, except: [:index, :create])] - def find_current_invitation(id : String) - Log.context.set(invitation_id: id) - @current_invitation = ::PlaceOS::Model::GroupInvitation.find!(UUID.new(id)) + def find_current_invitation(id : UUID) + Log.context.set(invitation_id: id.to_s) + @current_invitation = ::PlaceOS::Model::GroupInvitation.find!(id) end getter! current_invitation : ::PlaceOS::Model::GroupInvitation @@ -72,7 +72,7 @@ module PlaceOS::Api # callers must have Manage on the target group. @[AC::Route::GET("/")] def index( - group_id : String? = nil, + group_id : UUID? = nil, limit : Int32 = 100, offset : Int32 = 0, ) : Array(::PlaceOS::Model::GroupInvitation) @@ -86,12 +86,11 @@ module PlaceOS::Api query = ::PlaceOS::Model::GroupInvitation.where(group_id: authority_group_ids) if group_id - target = UUID.new(group_id) unless user_admin? - group = ::PlaceOS::Model::Group.find!(target) + group = ::PlaceOS::Model::Group.find!(group_id) ensure_manage!(current_user, group) end - query = query.where(group_id: target) + query = query.where(group_id: group_id) else unless user_admin? managed = manageable_group_ids(current_user) diff --git a/src/placeos-rest-api/controllers/groups/user.cr b/src/placeos-rest-api/controllers/groups/user.cr index afb276ca..16acb343 100644 --- a/src/placeos-rest-api/controllers/groups/user.cr +++ b/src/placeos-rest-api/controllers/groups/user.cr @@ -29,9 +29,9 @@ module PlaceOS::Api ############################################################################################### @[AC::Route::Filter(:before_action, except: [:index, :create])] - def find_current_group_user(user_id : String, group_id : String) - Log.context.set(user_id: user_id, group_id: group_id) - @current_group_user = ::PlaceOS::Model::GroupUser.find!({user_id, UUID.new(group_id)}) + def find_current_group_user(user_id : String, group_id : UUID) + Log.context.set(user_id: user_id, group_id: group_id.to_s) + @current_group_user = ::PlaceOS::Model::GroupUser.find!({user_id, group_id}) end getter! current_group_user : ::PlaceOS::Model::GroupUser @@ -76,7 +76,7 @@ module PlaceOS::Api # callers without Manage on the specified group see only their own. @[AC::Route::GET("/")] def index( - group_id : String? = nil, + group_id : UUID? = nil, user_id : String? = nil, limit : Int32 = 100, offset : Int32 = 0, @@ -100,15 +100,14 @@ module PlaceOS::Api end if group_id - target = UUID.new(group_id) unless user_admin? - group = ::PlaceOS::Model::Group.find!(target) + group = ::PlaceOS::Model::Group.find!(group_id) # Non-Manager: allow only if asking about their own entries. unless user_has_manage?(current_user, group) raise Error::Forbidden.new unless user_id == "me" || user_id == current_user.id end end - query = query.where(group_id: target) + query = query.where(group_id: group_id) elsif !user_admin? # No group filter + non-admin: restrict to groups they manage OR their own rows. managed = manageable_group_ids(current_user) diff --git a/src/placeos-rest-api/controllers/groups/zone.cr b/src/placeos-rest-api/controllers/groups/zone.cr index c74340b4..40769fa7 100644 --- a/src/placeos-rest-api/controllers/groups/zone.cr +++ b/src/placeos-rest-api/controllers/groups/zone.cr @@ -28,9 +28,9 @@ module PlaceOS::Api ############################################################################################### @[AC::Route::Filter(:before_action, except: [:index, :create])] - def find_current_group_zone(group_id : String, zone_id : String) - Log.context.set(group_id: group_id, zone_id: zone_id) - @current_group_zone = ::PlaceOS::Model::GroupZone.find!({UUID.new(group_id), zone_id}) + def find_current_group_zone(group_id : UUID, zone_id : String) + Log.context.set(group_id: group_id.to_s, zone_id: zone_id) + @current_group_zone = ::PlaceOS::Model::GroupZone.find!({group_id, zone_id}) end getter! current_group_zone : ::PlaceOS::Model::GroupZone @@ -73,7 +73,7 @@ module PlaceOS::Api # List GroupZone rows. Filterable by group_id / zone_id. @[AC::Route::GET("/")] def index( - group_id : String? = nil, + group_id : UUID? = nil, zone_id : String? = nil, limit : Int32 = 100, offset : Int32 = 0, @@ -88,12 +88,11 @@ module PlaceOS::Api query = ::PlaceOS::Model::GroupZone.where(group_id: authority_group_ids) if group_id - target = UUID.new(group_id) unless user_admin? - group = ::PlaceOS::Model::Group.find!(target) + group = ::PlaceOS::Model::Group.find!(group_id) ensure_member!(current_user, group) end - query = query.where(group_id: target) + query = query.where(group_id: group_id) else unless user_admin? viewable = viewable_group_ids(current_user) diff --git a/src/placeos-rest-api/controllers/signage/playlist_media.cr b/src/placeos-rest-api/controllers/signage/playlist_media.cr index 39579aa2..876187c4 100644 --- a/src/placeos-rest-api/controllers/signage/playlist_media.cr +++ b/src/placeos-rest-api/controllers/signage/playlist_media.cr @@ -87,7 +87,7 @@ module PlaceOS::Api @[AC::Route::GET("/")] def index( @[AC::Param::Info(description: "filter to items linked to this group (caller must have Read on the group)")] - group_id : String? = nil, + group_id : UUID? = nil, @[AC::Param::Info(description: "case-insensitive substring search on name and description (SQL ILIKE)")] q : String? = nil, limit : Int32 = 100, @@ -96,13 +96,12 @@ module PlaceOS::Api query = ::PlaceOS::Model::Playlist::Item.where(authority_id: authority.id.as(String)) if group_id - gid = UUID.new(group_id) unless user_support? - perms = group_memberships(current_user)[gid]? || ::PlaceOS::Model::Permissions::None + perms = group_memberships(current_user)[group_id]? || ::PlaceOS::Model::Permissions::None raise Error::Forbidden.new unless perms.read? end linked_ids = ::PlaceOS::Model::GroupPlaylistItem - .where(group_id: gid) + .where(group_id: group_id) .to_a .map(&.playlist_item_id) if linked_ids.empty? @@ -180,24 +179,22 @@ module PlaceOS::Api @[AC::Route::POST("/", status_code: HTTP::Status::CREATED)] def create( @[AC::Param::Info(description: "group id to auto-link the new item to (required for non-admin callers)")] - group_id : String? = nil, + group_id : UUID? = nil, ) : ::PlaceOS::Model::Playlist::Item item = item_update item.authority_id = authority.id - target_gid = group_id.try { |g| UUID.new(g) } - unless user_support? - raise Error::Forbidden.new("group_id required") if target_gid.nil? - perms = group_memberships(current_user)[target_gid]? || ::PlaceOS::Model::Permissions::None + raise Error::Forbidden.new("group_id required") if group_id.nil? + perms = group_memberships(current_user)[group_id]? || ::PlaceOS::Model::Permissions::None raise Error::Forbidden.new("missing Create permission on the target group") unless perms.create? end ::PgORM::Database.transaction do |_tx| raise Error::ModelValidation.new(item.errors) unless item.save - if target_gid + if group_id link = ::PlaceOS::Model::GroupPlaylistItem.new( - group_id: target_gid, + group_id: group_id, playlist_item_id: item.id.as(String), ) raise Error::ModelValidation.new(link.errors) unless link.save @@ -223,7 +220,7 @@ module PlaceOS::Api @[AC::Param::Info(description: "comma-separated item ids to share into the target group")] items : Array(String), @[AC::Param::Info(description: "target group id (must participate in the 'signage' subsystem)", name: "to")] - to : String, + to : UUID, ) : NamedTuple(linked: Array(String), already_present: Array(String)) return {linked: [] of String, already_present: [] of String} if items.empty? @@ -252,9 +249,8 @@ module PlaceOS::Api {linked: to_link, already_present: existing} end - private def resolve_share_target_group(to : String) : ::PlaceOS::Model::Group - target_gid = UUID.new(to) - group = ::PlaceOS::Model::Group.find!(target_gid) + private def resolve_share_target_group(to : UUID) : ::PlaceOS::Model::Group + group = ::PlaceOS::Model::Group.find!(to) raise Error::Forbidden.new("target group must be in the same authority") unless group.authority_id == authority.id raise Error::Forbidden.new("target group must participate in the 'signage' subsystem") unless group.subsystems.includes?("signage") group diff --git a/src/placeos-rest-api/controllers/signage/playlists.cr b/src/placeos-rest-api/controllers/signage/playlists.cr index e3dfbe93..f2e702d9 100644 --- a/src/placeos-rest-api/controllers/signage/playlists.cr +++ b/src/placeos-rest-api/controllers/signage/playlists.cr @@ -99,7 +99,7 @@ module PlaceOS::Api @[AC::Route::GET("/")] def index( @[AC::Param::Info(description: "filter to playlists linked to this group (caller must have Read on the group)")] - group_id : String? = nil, + group_id : UUID? = nil, @[AC::Param::Info(description: "case-insensitive substring search on name and description (SQL ILIKE)")] q : String? = nil, limit : Int32 = 100, @@ -108,13 +108,12 @@ module PlaceOS::Api query = ::PlaceOS::Model::Playlist.where(authority_id: authority.id.as(String)) if group_id - gid = UUID.new(group_id) unless user_support? - perms = group_memberships(current_user)[gid]? || ::PlaceOS::Model::Permissions::None + perms = group_memberships(current_user)[group_id]? || ::PlaceOS::Model::Permissions::None raise Error::Forbidden.new unless perms.read? end linked_ids = ::PlaceOS::Model::GroupPlaylist - .where(group_id: gid) + .where(group_id: group_id) .to_a .map(&.playlist_id) if linked_ids.empty? @@ -179,24 +178,22 @@ module PlaceOS::Api @[AC::Route::POST("/", status_code: HTTP::Status::CREATED)] def create( @[AC::Param::Info(description: "group id to auto-link the new playlist to (required for non-admin callers)")] - group_id : String? = nil, + group_id : UUID? = nil, ) : ::PlaceOS::Model::Playlist playlist = playlist_update playlist.authority_id = authority.id - target_gid = group_id.try { |g| UUID.new(g) } - unless user_support? - raise Error::Forbidden.new("group_id required") if target_gid.nil? - perms = group_memberships(current_user)[target_gid]? || ::PlaceOS::Model::Permissions::None + raise Error::Forbidden.new("group_id required") if group_id.nil? + perms = group_memberships(current_user)[group_id]? || ::PlaceOS::Model::Permissions::None raise Error::Forbidden.new("missing Create permission on the target group") unless perms.create? end ::PgORM::Database.transaction do |_tx| raise Error::ModelValidation.new(playlist.errors) unless playlist.save - if target_gid + if group_id link = ::PlaceOS::Model::GroupPlaylist.new( - group_id: target_gid, + group_id: group_id, playlist_id: playlist.id.as(String), ) raise Error::ModelValidation.new(link.errors) unless link.save @@ -229,7 +226,7 @@ module PlaceOS::Api @[AC::Param::Info(description: "comma-separated playlist ids to share into the target group")] items : Array(String), @[AC::Param::Info(description: "target group id (must participate in the 'signage' subsystem)", name: "to")] - to : String, + to : UUID, ) : NamedTuple(linked: Array(String), already_present: Array(String)) return {linked: [] of String, already_present: [] of String} if items.empty? @@ -258,9 +255,8 @@ module PlaceOS::Api {linked: to_link, already_present: existing} end - private def resolve_share_target_group(to : String) : ::PlaceOS::Model::Group - target_gid = UUID.new(to) - group = ::PlaceOS::Model::Group.find!(target_gid) + private def resolve_share_target_group(to : UUID) : ::PlaceOS::Model::Group + group = ::PlaceOS::Model::Group.find!(to) raise Error::Forbidden.new("target group must be in the same authority") unless group.authority_id == authority.id raise Error::Forbidden.new("target group must participate in the 'signage' subsystem") unless group.subsystems.includes?("signage") group @@ -303,16 +299,37 @@ module PlaceOS::Api # Playlist Revisions # ================== + # `media` is hydrated by the controller before the revision is rendered to JSON. + # Once the caller has access to the playlist they're allowed to see every item + # the revision references — collaborators don't need each item separately + # shared with their group. + class ::PlaceOS::Model::Playlist::Revision + @[JSON::Field(key: "media", ignore_deserialize: true)] + property media : Array(::PlaceOS::Model::Playlist::Item)? = nil + end + # get the current list of media for the playlist @[AC::Route::GET("/:id/media")] def media : ::PlaceOS::Model::Playlist::Revision - media_revisions(1).first? || ::PlaceOS::Model::Playlist::Revision.new + revision = current_playlist.revisions.limit(1).first? || ::PlaceOS::Model::Playlist::Revision.new + hydrate_media!([revision]) + revision end # returns the previous versions of a playlist @[AC::Route::GET("/:id/media/revisions")] def media_revisions(limit : Int32 = 10) : Array(::PlaceOS::Model::Playlist::Revision) - current_playlist.revisions.limit(limit).to_a + revisions = current_playlist.revisions.limit(limit).to_a + hydrate_media!(revisions) + revisions + end + + private def hydrate_media!(revisions : Array(::PlaceOS::Model::Playlist::Revision)) : Nil + ids = revisions.flat_map(&.items).uniq! + items_by_id = ids.empty? ? {} of String => ::PlaceOS::Model::Playlist::Item : ::PlaceOS::Model::Playlist::Item.where(id: ids).to_a.index_by { |i| i.id.as(String) } + revisions.each do |rev| + rev.media = rev.items.compact_map { |item_id| items_by_id[item_id]? } + end end # provide an update list of media for a playlist diff --git a/src/placeos-rest-api/controllers/systems.cr b/src/placeos-rest-api/controllers/systems.cr index 7efa8ea7..94153e96 100644 --- a/src/placeos-rest-api/controllers/systems.cr +++ b/src/placeos-rest-api/controllers/systems.cr @@ -249,7 +249,7 @@ module PlaceOS::Api @[AC::Param::Info(description: "return systems which are in the zones provided", example: "zone-1234,zone-4567")] zone_id : Array(String)? = nil, @[AC::Param::Info(description: "return systems anchored by this group's GroupZone rows; non-support callers must have Read on the group")] - group_id : String? = nil, + group_id : UUID? = nil, @[AC::Param::Info(description: "return systems in any zone the caller can reach (transitively) within the given subsystem code, e.g. 'signage'")] subsystem : String? = nil, @[AC::Param::Info(description: "return systems which are public", example: "true")] @@ -278,13 +278,12 @@ module PlaceOS::Api # scope clause is added. scope_zones = [] of String - if (gid_str = group_id) - gid = UUID.new(gid_str) + if group_id unless user_support? - perms = group_memberships(current_user)[gid]? || ::PlaceOS::Model::Permissions::None + perms = group_memberships(current_user)[group_id]? || ::PlaceOS::Model::Permissions::None raise Error::Forbidden.new unless perms.read? end - scope_zones.concat(::PlaceOS::Model::GroupZone.where(group_id: gid).to_a.map(&.zone_id)) + scope_zones.concat(::PlaceOS::Model::GroupZone.where(group_id: group_id).to_a.map(&.zone_id)) end if (sub = subsystem) diff --git a/src/placeos-rest-api/controllers/zones.cr b/src/placeos-rest-api/controllers/zones.cr index 93f19f34..75512f4b 100644 --- a/src/placeos-rest-api/controllers/zones.cr +++ b/src/placeos-rest-api/controllers/zones.cr @@ -151,7 +151,7 @@ module PlaceOS::Api @[AC::Param::Info(description: "return zones with particular tags", example: "building,level")] tags : Array(String)? = nil, @[AC::Param::Info(description: "filter to zones a group has direct GroupZone anchors on; non-support callers must have Read on the group. Forces include_children_count=true so callers can drill down.")] - group_id : String? = nil, + group_id : UUID? = nil, @[AC::Param::Info(description: "include children_count for each zone (useful for tree views)", example: "true")] include_children_count : Bool = false, ) : Array(::PlaceOS::Model::Zone) @@ -160,13 +160,12 @@ module PlaceOS::Api # ends up unconstrained). group_zone_ids = nil if group_id - gid = UUID.new(group_id) unless user_support? - perms = group_memberships(current_user)[gid]? || ::PlaceOS::Model::Permissions::None + perms = group_memberships(current_user)[group_id]? || ::PlaceOS::Model::Permissions::None raise Error::Forbidden.new unless perms.read? end group_zone_ids = ::PlaceOS::Model::GroupZone - .where(group_id: gid) + .where(group_id: group_id) .to_a .map(&.zone_id) if group_zone_ids.empty?