Skip to content

feat: suspend banned user#177

Open
alexey-yarmosh wants to merge 6 commits into
username-changefrom
suspend-banned-users
Open

feat: suspend banned user#177
alexey-yarmosh wants to merge 6 commits into
username-changefrom
suspend-banned-users

Conversation

@alexey-yarmosh
Copy link
Copy Markdown
Member

Fixes #165

FYI @jimaek
suspended status is reset to active if user exists in github, so if you want to ban manually use any other status, e.g. archived.

@coderabbitai
Copy link
Copy Markdown

coderabbitai Bot commented Jun 5, 2026

Review Change Stack

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

Run ID: baa03585-01b6-4f80-a6da-a8ff3290b292

📥 Commits

Reviewing files that changed from the base of the PR and between 7ce8bdc and c70f730.

📒 Files selected for processing (1)
  • src/extensions/operations/remove-banned-users-cron-handler/src/repositories/github.ts

Walkthrough

This PR migrates the banned GitHub users workflow from immediate deletion to a three-phase lifecycle: suspend users whose GitHub IDs no longer exist, reactivate them if they return, and delete after 365 days. The database schema adds date_updated and suspended_at fields for tracking. The removeBannedUsers cron handler is refactored to partition users into action groups and execute batch Directus operations. GitHub validation is consolidated into a bulk-check function using GraphQL and REST APIs. The signup hook is simplified by removing manual date_created assignment, now handled by Directus.

Suggested reviewers

  • MartinKolarik
  • PavelKopecky
🚥 Pre-merge checks | ✅ 4
✅ Passed checks (4 passed)
Check name Status Explanation
Title check ✅ Passed The title 'feat: suspend banned user' directly captures the main objective: suspending users instead of deleting them when banned on GitHub.
Description check ✅ Passed The description references issue #165 and explains the suspension behavior, including the key detail that suspended status resets to active if user exists on GitHub.
Linked Issues check ✅ Passed The PR meets all primary coding requirements: users are suspended instead of deleted [#165], suspended users are reactivated if found on GitHub [#165], date_updated field is added to the schema [#165], and users suspended for >1 year are deleted [#165].
Out of Scope Changes check ✅ Passed All changes directly support the issue #165 objectives: schema updates for suspension tracking, hook modifications to remove automatic date_created setting, and cron handler refactoring to implement three-phase moderation workflow.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing Touches
📝 Generate docstrings
  • Create stacked PR
  • Commit on current branch
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch suspend-banned-users

Comment @coderabbitai help to get the list of available commands and usage tips.

Copy link
Copy Markdown

@coderabbitai coderabbitai Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 2

🧹 Nitpick comments (2)
snapshots/collections-schema.yml (1)

964-964: ⚡ Quick win

Make suspended_at system-managed (readonly).

meta.readonly: false allows manual writes to the suspension timestamp, which can skew delayed-deletion timing. Prefer keeping this field readonly and only updated by the lifecycle job/service.

Suggested diff
-      readonly: false
+      readonly: true
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@snapshots/collections-schema.yml` at line 964, The field definition for
suspended_at currently has meta.readonly: false which allows manual writes;
change the schema for the suspended_at field to make it system-managed by
setting meta.readonly: true (and update any associated description or comments
if present) so only the lifecycle job/service updates suspended_at; locate the
suspended_at field in the collection schema (search for suspended_at and
meta.readonly) and flip false to true to enforce read-only behavior.
src/extensions/operations/remove-banned-users-cron-handler/test/api.test.ts (1)

56-62: ⚡ Quick win

Isolate HTTP mocks per test with afterEach cleanup.

Add afterEach(() => nock.cleanAll()) so one failing case cannot leak interceptors into subsequent tests.

🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@src/extensions/operations/remove-banned-users-cron-handler/test/api.test.ts`
around lines 56 - 62, The test suite currently calls nock.cleanAll() in an
after() hook which can let HTTP interceptors leak between tests; add an
afterEach(() => { nock.cleanAll(); }) (alongside the existing beforeEach(() => {
sinon.resetHistory(); })) so each test is isolated, ensuring interceptors
created in individual tests are removed immediately after each test and cannot
affect later tests (refer to the existing beforeEach and after hooks in the test
file).
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Inline comments:
In
`@src/extensions/operations/remove-banned-users-cron-handler/src/actions/remove-banned-users.ts`:
- Around line 22-23: The deletion logic currently uses isSuspensionExpired
(based on suspended_at) to decide removals, but the requirement is to delete
users whose date_updated is older than one year; update the condition in the
removal routine (where isSuspensionExpired is used to push into toDelete) to
instead check date_updated age (e.g., compute now - user.date_updated > 1 year)
and push those users to toDelete; replace or add a new helper (e.g.,
isDateUpdatedOlderThanOneYear) and ensure both places referenced (the block
using isSuspensionExpired and the similar check around lines 41-42) use the
date_updated check rather than suspended_at.

In
`@src/extensions/operations/remove-banned-users-cron-handler/src/repositories/github.ts`:
- Around line 63-66: The graphql call using graphql<GraphqlData>(query, {
...variables, headers: { Authorization: `Bearer ${env.GITHUB_ACCESS_TOKEN}` } })
has no timeout/abort and can hang; wrap the request with an AbortController (or
use AbortSignal.timeout(ms)) and pass the signal via the graphql request option
(e.g., include request: { signal: controller.signal } or request: { signal:
AbortSignal.timeout(… ) }), ensuring you create/clear the controller and choose
a sensible timeout value so the cron job doesn't stall; update the call site
where graphql is invoked and keep the existing variables and headers intact
while adding the request signal.

---

Nitpick comments:
In `@snapshots/collections-schema.yml`:
- Line 964: The field definition for suspended_at currently has meta.readonly:
false which allows manual writes; change the schema for the suspended_at field
to make it system-managed by setting meta.readonly: true (and update any
associated description or comments if present) so only the lifecycle job/service
updates suspended_at; locate the suspended_at field in the collection schema
(search for suspended_at and meta.readonly) and flip false to true to enforce
read-only behavior.

In `@src/extensions/operations/remove-banned-users-cron-handler/test/api.test.ts`:
- Around line 56-62: The test suite currently calls nock.cleanAll() in an
after() hook which can let HTTP interceptors leak between tests; add an
afterEach(() => { nock.cleanAll(); }) (alongside the existing beforeEach(() => {
sinon.resetHistory(); })) so each test is isolated, ensuring interceptors
created in individual tests are removed immediately after each test and cannot
affect later tests (refer to the existing beforeEach and after hooks in the test
file).
🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

  • Push a commit to this branch (recommended)
  • Create a new PR with the fixes

ℹ️ Review info
⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

Run ID: 01fb89b5-133d-468c-b65a-ea6db1da6281

📥 Commits

Reviewing files that changed from the base of the PR and between 4e86387 and 7ce8bdc.

⛔ Files ignored due to path filters (1)
  • pnpm-lock.yaml is excluded by !**/pnpm-lock.yaml
📒 Files selected for processing (11)
  • snapshots/collections-schema.yml
  • src/extensions/hooks/sign-up/src/index.ts
  • src/extensions/hooks/sign-up/test/index.test.ts
  • src/extensions/operations/remove-banned-users-cron-handler/package.json
  • src/extensions/operations/remove-banned-users-cron-handler/src/actions/remove-banned-users.ts
  • src/extensions/operations/remove-banned-users-cron-handler/src/api.ts
  • src/extensions/operations/remove-banned-users-cron-handler/src/app.ts
  • src/extensions/operations/remove-banned-users-cron-handler/src/repositories/directus.ts
  • src/extensions/operations/remove-banned-users-cron-handler/src/repositories/github.ts
  • src/extensions/operations/remove-banned-users-cron-handler/src/types.ts
  • src/extensions/operations/remove-banned-users-cron-handler/test/api.test.ts
💤 Files with no reviewable changes (1)
  • src/extensions/hooks/sign-up/src/index.ts

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.

2 participants