Skip to content

Cleanup stale unconfirmed users#181

Open
1amKhush wants to merge 3 commits intobtcpayserver:masterfrom
1amKhush:feat/user-cleanup
Open

Cleanup stale unconfirmed users#181
1amKhush wants to merge 3 commits intobtcpayserver:masterfrom
1amKhush:feat/user-cleanup

Conversation

@1amKhush
Copy link

Added scheduled cleanup for stale unconfirmed user accounts.

Changes Introduced

  • Added CreatedAt column to AspNetUsers via migration :
    (PluginBuilder/Data/Scripts/20.UnconfirmedUserCleanup.sql)
  • Implemented UserCleanupRunner to handle deletion logic :
    (PluginBuilder/Services/UserCleanupRunner.cs)
  • Added a weekly hosted service to trigger cleanup :
    (PluginBuilder/HostedServices/UserCleanupHostedService.cs)
  • Registered cleanup components in dependency injection :
    (PluginBuilder/Program.cs)
  • Added unit tests covering deletion and retention scenarios :
    (PluginBuilder.Tests/PluginTests/UserCleanupTests.cs)

Cleanup Criteria
A user is deleted only if all of the following conditions are met:

  • EmailConfirmed = false
  • CreatedAt is older than 30 days
  • No associated roles (AspNetUserRoles)
  • No plugin ownership (users_plugins)
  • No involvement in reviews (plugin_reviewers, plugin_reviews)
  • Not linked to any listing reviews (plugin_listing_requests.reviewed_by)

@coderabbitai
Copy link

coderabbitai bot commented Mar 25, 2026

Note

.coderabbit.yaml has unrecognized properties

CodeRabbit is using all valid settings from your configuration. Unrecognized properties (listed below) have been ignored and may indicate typos or deprecated fields that can be removed.

⚠️ Parsing warnings (1)
Validation error: Unrecognized key(s) in object: 'auto_resolve_threads'
⚙️ Configuration instructions
  • Please see the configuration documentation for more information.
  • You can also validate your configuration using the online YAML validator.
  • If your editor has YAML language server enabled, you can add the path at the top of this file to enable auto-completion and validation: # yaml-language-server: $schema=https://coderabbit.ai/integrations/schema.v2.json

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro

Run ID: 43a8dbd1-2529-4617-8700-522f98d587c8

📥 Commits

Reviewing files that changed from the base of the PR and between 7be1cd8 and 0d26419.

📒 Files selected for processing (1)
  • PluginBuilder/Services/UserCleanupRunner.cs

Walkthrough

Adds a CreatedAt TIMESTAMPTZ column and index to AspNetUsers. Introduces UserCleanupRunner to delete unconfirmed users older than 30 days while excluding users with roles, plugin ownership/links, review or listing associations. Adds UserCleanupHostedService that runs the runner on a 7-day interval and registers the runner and hosted service in DI. Adds an integration test (UserCleanupTests) that verifies only stale unconfirmed users without retention links are deleted.

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~20 minutes

Possibly related issues

Possibly related PRs

Suggested reviewers

  • TChukwuleta
  • thgO-O
  • rockstardev
🚥 Pre-merge checks | ✅ 2 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (2 passed)
Check name Status Explanation
Title check ✅ Passed The title 'Cleanup stale unconfirmed users' directly and concisely summarizes the main objective of the changeset—implementing scheduled cleanup for stale unconfirmed user accounts.
Description check ✅ Passed The description is well-structured and comprehensively related to the changeset, covering all major changes: database migration, cleanup runner logic, hosted service scheduling, DI registration, and tests.

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

✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests

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.

❤️ Share

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

Copy link

@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 (1)
PluginBuilder/Services/UserCleanupRunner.cs (1)

31-44: Minor: Inconsistent indentation.

Lines 32-44 have extra indentation compared to the surrounding code (double-indented when single indent is expected).

Proposed fix
         var threshold = DateTimeOffset.UtcNow - StaleThreshold;
-                // Protect any unconfirmed user that has roles or owns/reviews plugins.
-                var deletedCount = await conn.ExecuteAsync(
-                        """
-                        DELETE FROM "AspNetUsers" u
-                        WHERE u."EmailConfirmed" = FALSE
-                            AND u."CreatedAt" < `@Threshold`
-                            AND NOT EXISTS (SELECT 1 FROM "AspNetUserRoles" ur WHERE ur."UserId" = u."Id")
-                            AND NOT EXISTS (SELECT 1 FROM users_plugins up WHERE up.user_id = u."Id")
-                            AND NOT EXISTS (SELECT 1 FROM plugin_reviewers pr WHERE pr.user_id = u."Id")
-                            AND NOT EXISTS (SELECT 1 FROM plugin_reviews pr WHERE pr.user_id = u."Id")
-                            AND NOT EXISTS (SELECT 1 FROM plugin_listing_requests plr WHERE plr.reviewed_by = u."Id")
-                        """,
-                        new { Threshold = threshold });
+        // Protect any unconfirmed user that has roles or owns/reviews plugins.
+        var deletedCount = await conn.ExecuteAsync(
+            """
+            DELETE FROM "AspNetUsers" u
+            WHERE u."EmailConfirmed" = FALSE
+              AND u."CreatedAt" < `@Threshold`
+              AND NOT EXISTS (SELECT 1 FROM "AspNetUserRoles" ur WHERE ur."UserId" = u."Id")
+              AND NOT EXISTS (SELECT 1 FROM users_plugins up WHERE up.user_id = u."Id")
+              AND NOT EXISTS (SELECT 1 FROM plugin_reviewers pr WHERE pr.user_id = u."Id")
+              AND NOT EXISTS (SELECT 1 FROM plugin_reviews pr WHERE pr.user_id = u."Id")
+              AND NOT EXISTS (SELECT 1 FROM plugin_listing_requests plr WHERE plr.reviewed_by = u."Id")
+            """,
+            new { Threshold = threshold });
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@PluginBuilder/Services/UserCleanupRunner.cs` around lines 31 - 44, The SQL
delete block assigned to deletedCount is indented one level too deep; reformat
the block so its indentation matches the surrounding method body (use single
indent rather than double). Locate the var threshold = DateTimeOffset.UtcNow -
StaleThreshold; and the await conn.ExecuteAsync("""...""", new { Threshold =
threshold }); call and adjust the multiline SQL string and its parameters to
align with the method's standard indentation, keeping the content unchanged and
preserving the ExecuteAsync call and StaleThreshold reference.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.

Inline comments:
In `@PluginBuilder.Tests/PluginTests/UserCleanupTests.cs`:
- Around line 69-76: The test inserts into plugin_reviews using a non-existent
reviewer_id column via creating a plugin_reviewers row; instead, change the test
to insert directly into plugin_reviews using the user_id column: remove the
conn.QuerySingleAsync call that creates plugin_reviewers and replace the INSERT
into plugin_reviews in UserCleanupTests (references: reviewPlugin,
staleReviewerKeep, conn.NewPlugin) to supply `@UserId` = staleReviewerKeep and
insert into (plugin_slug, user_id, rating, body) so the INSERT targets the
correct column.

In `@PluginBuilder/HostedServices/UserCleanupHostedService.cs`:
- Around line 1-2: The file is missing the Microsoft.Extensions.Logging
namespace required for ILogger<UserCleanupHostedService>; add a using directive
for Microsoft.Extensions.Logging at the top of the file so the
ILogger<UserCleanupHostedService> reference in the UserCleanupHostedService
class resolves correctly and the file compiles.

---

Nitpick comments:
In `@PluginBuilder/Services/UserCleanupRunner.cs`:
- Around line 31-44: The SQL delete block assigned to deletedCount is indented
one level too deep; reformat the block so its indentation matches the
surrounding method body (use single indent rather than double). Locate the var
threshold = DateTimeOffset.UtcNow - StaleThreshold; and the await
conn.ExecuteAsync("""...""", new { Threshold = threshold }); call and adjust the
multiline SQL string and its parameters to align with the method's standard
indentation, keeping the content unchanged and preserving the ExecuteAsync call
and StaleThreshold reference.

ℹ️ Review info
⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro

Run ID: 42c231af-8e26-4ffa-b02a-20e5d31bf580

📥 Commits

Reviewing files that changed from the base of the PR and between b6d1e12 and 47e1e19.

📒 Files selected for processing (5)
  • PluginBuilder.Tests/PluginTests/UserCleanupTests.cs
  • PluginBuilder/Data/Scripts/20.UnconfirmedUserCleanup.sql
  • PluginBuilder/HostedServices/UserCleanupHostedService.cs
  • PluginBuilder/Program.cs
  • PluginBuilder/Services/UserCleanupRunner.cs

Comment on lines +69 to +76
const string reviewPlugin = "review-linked-plugin";
await conn.NewPlugin(reviewPlugin, staleOwnerKeep);
var reviewerId = await conn.QuerySingleAsync<long>(
"INSERT INTO plugin_reviewers (user_id, source) VALUES (@UserId, 'system') RETURNING id",
new { UserId = staleReviewerKeep });
await conn.ExecuteAsync(
"INSERT INTO plugin_reviews (plugin_slug, reviewer_id, rating, body) VALUES (@PluginSlug, @ReviewerId, 5, 'ok')",
new { PluginSlug = reviewPlugin, ReviewerId = reviewerId });
Copy link

@coderabbitai coderabbitai bot Mar 25, 2026

Choose a reason for hiding this comment

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

⚠️ Potential issue | 🟠 Major

🧩 Analysis chain

🏁 Script executed:

#!/bin/bash
# Check actual plugin_reviews schema for column names
rg -A 20 'CREATE TABLE "plugin_reviews"' --type sql

# Check if reviewer_id exists or if it should be user_id
rg -n "reviewer_id|user_id" PluginBuilder/Data/Scripts/14.PluginRatingReviews.sql

Repository: btcpayserver/btcpayserver-plugin-builder

Length of output: 188


Test code has SQL error: plugin_reviews table uses user_id, not reviewer_id.

Line 75 attempts to insert into plugin_reviews(plugin_slug, reviewer_id, rating, body), but the schema shows plugin_reviews has a user_id column (NOT NULL), not reviewer_id. The test will fail with a SQL error about the non-existent column.

Correct the insert to use user_id directly instead of creating a plugin_reviewers record and using its ID:

INSERT INTO plugin_reviews (plugin_slug, user_id, rating, body) 
VALUES (`@PluginSlug`, `@UserId`, 5, 'ok')
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@PluginBuilder.Tests/PluginTests/UserCleanupTests.cs` around lines 69 - 76,
The test inserts into plugin_reviews using a non-existent reviewer_id column via
creating a plugin_reviewers row; instead, change the test to insert directly
into plugin_reviews using the user_id column: remove the conn.QuerySingleAsync
call that creates plugin_reviewers and replace the INSERT into plugin_reviews in
UserCleanupTests (references: reviewPlugin, staleReviewerKeep, conn.NewPlugin)
to supply `@UserId` = staleReviewerKeep and insert into (plugin_slug, user_id,
rating, body) so the INSERT targets the correct column.

Copy link
Author

Choose a reason for hiding this comment

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

@coderabbitai

The current schema, updated through PluginBuilder/Data/Scripts/17.AlterPluginReviewTable.sql, adds the plugin_reviewers table and makes reviewer_id mandatory in plugin_reviews.

The test flow in PluginBuilder.Tests/PluginTests/UserCleanupTests.cs seems correct to me. It creates a plugin_reviewers entry and inserts into plugin_reviews using reviewer_id, plugin_slug, rating, and body.

Switching to user_id only would not match the current migrated schema.

Copy link

Choose a reason for hiding this comment

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

🧩 Analysis chain

🏁 Script executed:

#!/bin/bash
# Verify the schema in migration 17
cat PluginBuilder/Data/Scripts/17.AlterPluginReviewTable.sql

# Also check migration 14 for baseline
cat PluginBuilder/Data/Scripts/14.PluginRatingReviews.sql

Length of output: 2463


@1amKhush, you're absolutely right — I apologize for the incorrect review. The script output from 17.AlterPluginReviewTable.sql confirms the schema was restructured: plugin_reviews.reviewer_id is now a NOT NULL BIGINT foreign key referencing plugin_reviewers.id, and user_id was made nullable and effectively replaced. The test flow — inserting into plugin_reviewers first and then using the returned id as reviewer_id in plugin_reviews — is correct and matches the current migrated schema.

(´• ω •`) 🐇


✏️ Learnings added
Learnt from: 1amKhush
Repo: btcpayserver/btcpayserver-plugin-builder PR: 181
File: PluginBuilder.Tests/PluginTests/UserCleanupTests.cs:69-76
Timestamp: 2026-03-26T16:05:10.265Z
Learning: In `btcpayserver/btcpayserver-plugin-builder`, migration `PluginBuilder/Data/Scripts/17.AlterPluginReviewTable.sql` restructured the reviews schema: it creates a `plugin_reviewers` table (with `id BIGSERIAL PRIMARY KEY`, `user_id TEXT UNIQUE`, `source TEXT`, etc.), adds a `reviewer_id BIGINT NOT NULL` FK column to `plugin_reviews` referencing `plugin_reviewers.id`, and drops the old `user_id`-based unique constraint. Any insert into `plugin_reviews` must now supply `reviewer_id` (the PK from `plugin_reviewers`), not `user_id` directly.

Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.


🧠 Learnings used
Learnt from: TChukwuleta
Repo: btcpayserver/btcpayserver-plugin-builder PR: 120
File: PluginBuilder/Util/Extensions/NpgsqlConnectionExtensions.cs:458-471
Timestamp: 2025-11-17T17:09:46.830Z
Learning: The `SetPluginReviewerDisplayInfo` method in `NpgsqlConnectionExtensions.cs` is specifically designed for updating existing review data during migration, where all records have a non-NULL `user_id`. It is not used for newly imported external reviews that might have NULL `user_id`.

Learnt from: TChukwuleta
Repo: btcpayserver/btcpayserver-plugin-builder PR: 105
File: PluginBuilder/Util/Extensions/NpgsqlConnectionExtensions.cs:99-110
Timestamp: 2025-11-17T07:51:08.020Z
Learning: In the BTCPay Server Plugin Builder project, X/Twitter account verification is not currently enforced in the `IsSocialAccountsVerified` method in `PluginBuilder/Util/Extensions/NpgsqlConnectionExtensions.cs`. The method only validates email confirmation, GitHub gist URL, and Nostr npub for the plugin listing request flow.

Learnt from: TChukwuleta
Repo: btcpayserver/btcpayserver-plugin-builder PR: 120
File: PluginBuilder/Util/Extensions/NpgsqlConnectionExtensions.cs:416-423
Timestamp: 2025-11-17T17:13:46.880Z
Learning: The `GetPluginReviews` method in `NpgsqlConnectionExtensions.cs` does not need to select the `id` column because the review ID is provided through other means (route parameters, form data) when needed by operations like delete or voting. The method is primarily used for display purposes.

Copy link
Author

Choose a reason for hiding this comment

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

@coderabbitai Can i leave the code regarding the migrated schema as it is or should i make any changes then?

Copy link

Choose a reason for hiding this comment

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

@1amKhush, you can leave the code exactly as it is — no changes are needed. The test correctly follows the current migrated schema:

  1. Inserts into plugin_reviewers to get a generated id (BIGSERIAL PRIMARY KEY).
  2. Uses that id as reviewer_id (the NOT NULL BIGINT FK) when inserting into plugin_reviews.

My original review comment was incorrect. The migration in 17.AlterPluginReviewTable.sql already established reviewer_id as the required column, and your test handles that properly. ✅


🧠 Learnings used
Learnt from: TChukwuleta
Repo: btcpayserver/btcpayserver-plugin-builder PR: 120
File: PluginBuilder/Util/Extensions/NpgsqlConnectionExtensions.cs:458-471
Timestamp: 2025-11-17T17:09:46.830Z
Learning: The `SetPluginReviewerDisplayInfo` method in `NpgsqlConnectionExtensions.cs` is specifically designed for updating existing review data during migration, where all records have a non-NULL `user_id`. It is not used for newly imported external reviews that might have NULL `user_id`.

Learnt from: TChukwuleta
Repo: btcpayserver/btcpayserver-plugin-builder PR: 105
File: PluginBuilder/Util/Extensions/NpgsqlConnectionExtensions.cs:99-110
Timestamp: 2025-11-17T07:51:08.020Z
Learning: In the BTCPay Server Plugin Builder project, X/Twitter account verification is not currently enforced in the `IsSocialAccountsVerified` method in `PluginBuilder/Util/Extensions/NpgsqlConnectionExtensions.cs`. The method only validates email confirmation, GitHub gist URL, and Nostr npub for the plugin listing request flow.

Learnt from: 1amKhush
Repo: btcpayserver/btcpayserver-plugin-builder PR: 181
File: PluginBuilder.Tests/PluginTests/UserCleanupTests.cs:69-76
Timestamp: 2026-03-26T16:05:10.265Z
Learning: In `btcpayserver/btcpayserver-plugin-builder`, migration `PluginBuilder/Data/Scripts/17.AlterPluginReviewTable.sql` restructured the reviews schema: it creates a `plugin_reviewers` table (with `id BIGSERIAL PRIMARY KEY`, `user_id TEXT UNIQUE`, `source TEXT`, etc.), adds a `reviewer_id BIGINT NOT NULL` FK column to `plugin_reviews` referencing `plugin_reviewers.id`, and drops the old `user_id`-based unique constraint. Any insert into `plugin_reviews` must now supply `reviewer_id` (the PK from `plugin_reviewers`), not `user_id` directly.

Learnt from: TChukwuleta
Repo: btcpayserver/btcpayserver-plugin-builder PR: 120
File: PluginBuilder/Util/Extensions/NpgsqlConnectionExtensions.cs:416-423
Timestamp: 2025-11-17T17:13:46.880Z
Learning: The `GetPluginReviews` method in `NpgsqlConnectionExtensions.cs` does not need to select the `id` column because the review ID is provided through other means (route parameters, form data) when needed by operations like delete or voting. The method is primarily used for display purposes.

Copy link
Author

Choose a reason for hiding this comment

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

@coderabbitai check for other stale/redundant database code which has become stale after similar migrations

Copy link

Choose a reason for hiding this comment

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

🧩 Analysis chain

🏁 Script executed:

#!/bin/bash
# Check all SQL/C# references to plugin_reviews and plugin_reviewers to find stale code

echo "=== All migration scripts ==="
ls PluginBuilder/Data/Scripts/

echo ""
echo "=== Migration 17: AlterPluginReviewTable.sql ==="
cat PluginBuilder/Data/Scripts/17.AlterPluginReviewTable.sql

echo ""
echo "=== Migration 14: PluginRatingReviews.sql ==="
cat PluginBuilder/Data/Scripts/14.PluginRatingReviews.sql

echo ""
echo "=== All references to plugin_reviews (non-migration) ==="
rg -n "plugin_reviews" --type cs -A 3 -B 1

echo ""
echo "=== All references to plugin_reviewers (non-migration) ==="
rg -n "plugin_reviewers" --type cs -A 3 -B 1

echo ""
echo "=== Check UserCleanupRunner.cs DELETE query ==="
cat PluginBuilder/Services/UserCleanupRunner.cs

echo ""
echo "=== NpgsqlConnectionExtensions references to plugin_reviews ==="
rg -n "plugin_reviews\|plugin_reviewers\|user_id\|reviewer_id" PluginBuilder/Util/Extensions/NpgsqlConnectionExtensions.cs -A 3 -B 1

echo ""
echo "=== All SQL in C# files referencing old user_id column in plugin_reviews context ==="
rg -n "plugin_reviews" --type cs -C 5

echo ""
echo "=== Check for helpful_voters references ==="
rg -n "helpful_voters" --type cs -C 3

echo ""
echo "=== Check all migration scripts for context ==="
ls PluginBuilder/Data/Scripts/ | sort -V

echo ""
echo "=== Migration 18, 19, 20 if present ==="
for f in 18 19 20; do
  file=$(ls PluginBuilder/Data/Scripts/${f}.* 2>/dev/null | head -1)
  if [ -n "$file" ]; then
    echo "--- $file ---"
    cat "$file"
  fi
done

Length of output: 43025


@1amKhush, I found two stale patterns from migration 17 — both outside the files changed in this PR:


1. UpsertPluginReview still writes user_id into plugin_reviews (NpgsqlConnectionExtensions.cs, ~line 541)

After migration 17, plugin_reviews.user_id was made nullable and the canonical link is now reviewer_id → plugin_reviewers.id. However, UpsertPluginReview still populates the old column:

INSERT INTO plugin_reviews
    (plugin_slug, user_id, rating, body, plugin_version, reviewer_id, ...)
VALUES
    (`@plugin_slug`, `@user_id`, `@rating`, ..., `@reviewer_id`, ...)

Writing user_id here is redundant — the unique constraint is now on (plugin_slug, reviewer_id), not (plugin_slug, user_id). The column still exists and the FK still exists (see issue 2), so it doesn't break anything, but it's a ghost of the pre-migration-17 schema being maintained unnecessarily.


2. fk_plugin_reviews_user FK was never dropped in migration 17

Migration 14 created:

CONSTRAINT fk_plugin_reviews_user
    FOREIGN KEY (user_id) REFERENCES "AspNetUsers" ("Id") ON DELETE CASCADE

Migration 17 only dropped the unique constraint (plugin_reviews_plugin_slug_user_id_key) but did not drop this FK. This means:

  • The ON DELETE CASCADE on plugin_reviews.user_id → AspNetUsers.Id still exists in the DB, which is a silent secondary cascade path alongside the new plugin_reviewers ON DELETE CASCADE added in migration 17.
  • The UpsertPluginReview call continues to populate user_id, so this FK is still being fed — meaning it's not dormant, just undocumented.

A follow-up migration should ideally:

-- Drop stale FK and stop writing user_id to plugin_reviews
ALTER TABLE plugin_reviews DROP CONSTRAINT IF EXISTS fk_plugin_reviews_user;

And UpsertPluginReview should stop inserting user_id.


What's correct in this PR ✅

UserCleanupRunner.cs is correctly written — it guards against plugin_reviewers.user_id (not the stale plugin_reviews.user_id), which is the right check post-migration-17.

Would you like me to open a follow-up GitHub issue to track the cleanup of the stale user_id column writes and the undropped FK?


🧠 Learnings used
Learnt from: TChukwuleta
Repo: btcpayserver/btcpayserver-plugin-builder PR: 120
File: PluginBuilder/Util/Extensions/NpgsqlConnectionExtensions.cs:458-471
Timestamp: 2025-11-17T17:09:46.830Z
Learning: The `SetPluginReviewerDisplayInfo` method in `NpgsqlConnectionExtensions.cs` is specifically designed for updating existing review data during migration, where all records have a non-NULL `user_id`. It is not used for newly imported external reviews that might have NULL `user_id`.

Learnt from: TChukwuleta
Repo: btcpayserver/btcpayserver-plugin-builder PR: 105
File: PluginBuilder/Util/Extensions/NpgsqlConnectionExtensions.cs:99-110
Timestamp: 2025-11-17T07:51:08.020Z
Learning: In the BTCPay Server Plugin Builder project, X/Twitter account verification is not currently enforced in the `IsSocialAccountsVerified` method in `PluginBuilder/Util/Extensions/NpgsqlConnectionExtensions.cs`. The method only validates email confirmation, GitHub gist URL, and Nostr npub for the plugin listing request flow.

Learnt from: 1amKhush
Repo: btcpayserver/btcpayserver-plugin-builder PR: 181
File: PluginBuilder.Tests/PluginTests/UserCleanupTests.cs:69-76
Timestamp: 2026-03-26T16:05:10.265Z
Learning: In `btcpayserver/btcpayserver-plugin-builder`, migration `PluginBuilder/Data/Scripts/17.AlterPluginReviewTable.sql` restructured the reviews schema: it creates a `plugin_reviewers` table (with `id BIGSERIAL PRIMARY KEY`, `user_id TEXT UNIQUE`, `source TEXT`, etc.), adds a `reviewer_id BIGINT NOT NULL` FK column to `plugin_reviews` referencing `plugin_reviewers.id`, and drops the old `user_id`-based unique constraint. Any insert into `plugin_reviews` must now supply `reviewer_id` (the PK from `plugin_reviewers`), not `user_id` directly.

Learnt from: TChukwuleta
Repo: btcpayserver/btcpayserver-plugin-builder PR: 120
File: PluginBuilder/Util/Extensions/NpgsqlConnectionExtensions.cs:416-423
Timestamp: 2025-11-17T17:13:46.880Z
Learning: The `GetPluginReviews` method in `NpgsqlConnectionExtensions.cs` does not need to select the `id` column because the review ID is provided through other means (route parameters, form data) when needed by operations like delete or voting. The method is primarily used for display purposes.

Copy link
Author

Choose a reason for hiding this comment

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

Copy link

Choose a reason for hiding this comment

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

Seems like the humans are having a chat. I'll hop back into my burrow for now. If you need me again, just tag @coderabbitai in a new comment, and I'll come hopping out!

Copy link
Collaborator

@thgO-O thgO-O left a comment

Choose a reason for hiding this comment

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

Thanks for working on this one @1amKhush. Can you please check coderabbit review?

@1amKhush
Copy link
Author

1amKhush commented Mar 26, 2026

@thgO-O I reviewed the issues highlighted by CodeRabbit and came to a different conclusion. It seems the review may not fully align with the current code. Could you please take a look and share your thoughts?

  • Missing using directive for ILogger<T> appears to be missing because it is available via implicit/global usings. I can add local ones but i would just be increasing the lines of code. (I also ran a fresh build of PluginBuilder.csproj and it succeeded, confirming no unresolved logger namespace issue.)

  • I think the current schema introduces plugin_reviewers and enforces reviewer_id in plugin_reviews

Copy link
Collaborator

@thgO-O thgO-O left a comment

Choose a reason for hiding this comment

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

well done, mate! I Made a few comments, but is almost good to go

Copy link

@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: 1

🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.

Inline comments:
In `@PluginBuilder/Services/UserCleanupRunner.cs`:
- Around line 28-42: The ExecuteAsync call in UserCleanupRunner (after awaiting
_connectionFactory.Open) does not forward the cancellationToken, so DELETE may
continue despite cancellation; fix by creating a Dapper CommandDefinition with
the SQL, parameters (new { Threshold = threshold }) and the cancellationToken
and pass that CommandDefinition into conn.ExecuteAsync (referencing ExecuteAsync
and CommandDefinition in the UserCleanupRunner.cs method) so the token is
honored.
🪄 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: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro

Run ID: d2c448c4-3f22-4ce6-8ee8-3555a273652a

📥 Commits

Reviewing files that changed from the base of the PR and between 47e1e19 and 7be1cd8.

📒 Files selected for processing (4)
  • PluginBuilder.Tests/PluginTests/UserCleanupTests.cs
  • PluginBuilder/Data/Scripts/20.UnconfirmedUserCleanup.sql
  • PluginBuilder/HostedServices/UserCleanupHostedService.cs
  • PluginBuilder/Services/UserCleanupRunner.cs
✅ Files skipped from review due to trivial changes (2)
  • PluginBuilder/Data/Scripts/20.UnconfirmedUserCleanup.sql
  • PluginBuilder.Tests/PluginTests/UserCleanupTests.cs
🚧 Files skipped from review as they are similar to previous changes (1)
  • PluginBuilder/HostedServices/UserCleanupHostedService.cs

@1amKhush 1amKhush requested a review from thgO-O March 26, 2026 18:06
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.

3 participants