Skip to content

Fix role member bulk update semantics#1637

Closed
samuelscheit wants to merge 4 commits intospacebarchat:masterfrom
samuelscheit2:fix/445-role-members-put
Closed

Fix role member bulk update semantics#1637
samuelscheit wants to merge 4 commits intospacebarchat:masterfrom
samuelscheit2:fix/445-role-members-put

Conversation

@samuelscheit
Copy link
Copy Markdown
Member

Summary

  • add explicit PUT /guilds/:guild_id/roles/:role_id/members replacement semantics while keeping PATCH additive-only
  • validate member_ids with a shared RoleMembersUpdateSchema and expose it in generated schema/OpenAPI output
  • add focused regression tests for helper semantics, route wiring, and schema validation

Root cause

The old PATCH route used a single arrayPartition(...) across all guild members and treated every non-addition as a removal. That meant omitted current holders were removed, listed holders could still fall into the remove set, and unrelated non-holders still hit removeRole calls.

Validation

  • npm run build:src:tsgo
  • npm run generate:schema
  • npm run generate:openapi
  • npm run node:tests

Closes #445.

@samuelscheit
Copy link
Copy Markdown
Member Author

Code review: no blocking issues found. The PATCH/PUT split addresses the root cause, the shared request schema is wired into the generated docs, and the regression tests cover the key semantics. From a code-review standpoint this change is approved.

@samuelscheit
Copy link
Copy Markdown
Member Author

PR report:

Changed files:

  • src/api/routes/guilds/#guild_id/roles/#role_id/members.ts
  • src/api/util/index.ts
  • src/api/util/utility/RoleMembers.ts
  • src/api/util/utility/RoleMembers.test.ts
  • src/api/util/utility/RoleMemberRoutes.test.ts
  • src/schemas/uncategorised/RoleMembersUpdateSchema.ts
  • src/schemas/uncategorised/RoleMembersUpdateSchema.test.ts
  • src/schemas/uncategorised/index.ts
  • assets/schemas.json
  • assets/openapi.json

Tests run:

  • npm run build:src:tsgo
  • npm run node:tests

Test results:

  • PASS: 66 tests, 0 failures
  • Coverage report emitted by node --test
  • Note: existing MaxListenersExceededWarning output still appears during JsonSerializer tests, but the suite passes

Code review outcome:

  • Reviewed the final diff after the last push
  • No blocking issues found

Decision:

  • Approved from a code-review standpoint; no additional pass is needed

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

PUT /guilds/:id/roles/:id/members

1 participant