fix(copi): prevent duplicate votes via unique constraint and safe upsert

[prajakta128]

Resolves #2288

Summary

Both toggle_vote and toggle_continue_vote used a check-then-act pattern (check if a vote exists, then separately insert or delete) with no database constraint backing it up. Concurrent requests could both pass the check and create duplicate votes.

continue_votes already had a unique constraint on (player_id, game_id), but votes had none, and both handlers still relied on the racy in-memory check rather than the database.

Changes

Scoped to two files:

• New migration: unique index on votes(player_id, dealt_card_id)
• toggle_continue_vote: replaced Enum.find + delete! with Repo.delete_all matching on the actual keys (handling {n, _} when n > 0, not just {1, _}), and replaced the plain insert with on_conflict: :nothing, conflict_target: [:player_id, :game_id]
• handle_toggle_vote: same pattern applied to card voting, using the new votes unique constraint as conflict_target

Testing

Existing tests for both handlers continue to pass unchanged — their assertions check the resulting vote count/state, not the specific error path, so they remain valid under the new atomic-upsert behavior. Opening as a draft PR to let CI confirm before requesting review.

Related

Two prior attempts (#2289, #2291) addressed this correctly in principle but were closed after months due to scope creep (rate limiting, an unrelated game-deletion handler, Python test file changes) and unresolved review comments (coveralls.json edits,
delete_all matching only {1, _}). This PR is scoped strictly to the migration and the two handlers.

@sydseter — you mentioned a migration script was already finished;
I wasn't able to locate it, so I've written one myself here. Happy to align with yours if it differs, just let me know.

[Copilot]

Pull request overview

This PR fixes a race condition in the Copi voting system by enforcing uniqueness at the database layer and switching vote toggles to safe, conflict-tolerant writes, preventing duplicate votes under concurrent requests.

Changes:

• Adds a unique index on votes(player_id, dealt_card_id) to enforce one vote per player per dealt card.
• Updates toggle_continue_vote to delete via key-based Repo.delete_all/2 and insert with on_conflict: :nothing + conflict_target.
• Updates card voting (handle_toggle_vote) to use the same delete_all + safe insert pattern with the new unique index as the conflict target.

Reviewed changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated 3 comments.

File	Description
copi.owasp.org/priv/repo/migrations/20260807120000_add_unique_constraint_to_votes.exs	Adds a DB-level unique index to prevent duplicate card votes.
copi.owasp.org/lib/copi_web/live/player_live/show.ex	Reworks vote toggling to use atomic delete-by-keys and safe upserts under concurrency.

Suppressed comments (1)

copi.owasp.org/lib/copi_web/live/player_live/show.ex:244

• With on_conflict: :nothing, Repo.insert/2 can return {:ok, struct} even when no row was inserted (conflict/no-op). Logging "Vote added successfully" in the {:ok, _vote} branch is therefore potentially misleading and makes debugging race conditions harder. Consider explicitly distinguishing the no-op conflict case (e.g., id == nil) from a true insert.

              {:ok, _vote} ->
                Logger.debug("Vote added successfully for player_id: #{player.id}, dealt_card_id: #{card_id}, game_id: #{game.id}")

              {:error, changeset} ->
                Logger.warning("Voting failed for player_id: #{inspect(player.id)}, dealt_card_id: #{inspect(card_id)}, game_id: #{inspect(game.id)}, errors: #{inspect(changeset.errors)}")

---

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

[sydseter]

@prajakta128 Please have a look at the comments