Repository navigation
Conversation
1 of 2 tasks
Contributor
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughAdds Strawberry-Django relay nodes and GraphQL query resolvers for BoardCandidateClaim and BoardCandidateClaimEvidence, enforces owner vs. public access, adds an ChangesGraphQL Queries and Access Control for Candidate Claims
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
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. Comment |
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## feature/bod-candidate-transparency #4832 +/- ##
======================================================================
- Coverage 98.88% 98.85% -0.04%
======================================================================
Files 542 546 +4
Lines 17253 17324 +71
Branches 2469 2474 +5
======================================================================
+ Hits 17061 17126 +65
- Misses 103 109 +6
Partials 89 89
Flags with carried forward coverage won't be shown. Click here to find out more.
Continue to review full report in Codecov by Harness.
🚀 New features to boost your workflow:
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
backend/tests/unit/apps/owasp/models/board_candidate_claim_test.py (1)
376-393: 🛠️ Refactor suggestion | 🟠 Major | ⚡ Quick winAdd a regression test for board attachment after initial draft creation.
Current coverage only validates create-time assignment with
board_idalready present. Please add a case where an existing draft withorder=0transitions fromboard_id=Noneto a concrete board and verifyorderis assigned. This protects the real workflow path and prevents regression.🤖 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 `@backend/tests/unit/apps/owasp/models/board_candidate_claim_test.py` around lines 376 - 393, Add a regression unit test that covers the transition of an existing draft BoardCandidateClaim from board_id=None to a real board so order is assigned on update; create a BoardCandidateClaim instance with status=BoardCandidateClaim.Status.DRAFT, candidate_id set, board_id=None and order=0 (simulate existing DB object with pk set), patch BoardCandidateClaim.objects.filter().aggregate() to return {"max_order": N} like in the existing test, then set claim.board_id to a concrete value, call claim.save(), and assert claim.order == N+1; reference the test function pattern in board_candidate_claim_test.py and the BoardCandidateClaim.save behavior to ensure you replicate the mocks for TimestampedModel.save and full_clean used in the other test.backend/apps/owasp/models/board_candidate_claim.py (1)
20-23:⚠️ Potential issue | 🟠 Major | 🏗️ Heavy lift
max(order)+1is race-prone without DB uniqueness protection.Concurrent inserts for the same
(candidate_id, board_id)can compute the same max and persist duplicateordervalues. This should be guarded by a database-level unique constraint and atomic assignment/retry strategy.Suggested direction
class Meta: db_table = "owasp_board_candidate_claim" indexes = [ models.Index(fields=["candidate", "status"], name="owasp_claim_candidate_status"), models.Index(fields=["board", "status"], name="owasp_claim_board_status"), ] + constraints = [ + models.UniqueConstraint( + fields=["candidate", "board", "order"], + name="owasp_claim_candidate_board_order_uniq", + ) + ]And compute
orderinside an atomic block with retry onIntegrityError.Also applies to: 125-135
🤖 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 `@backend/apps/owasp/models/board_candidate_claim.py` around lines 20 - 23, Add a DB-level uniqueness constraint for the per-(candidate,board) ordering and make the order assignment atomic with retry: update the model (BoardCandidateClaim) indexes block to include a UniqueConstraint on ["candidate","board","order"] (in addition to the existing Indexes) and change the claim-creation logic (the function that currently computes max(order)+1 around lines ~125-135) to compute order inside a transaction/locking block (e.g., select_for_update or an atomic transaction) and retry on IntegrityError so concurrent inserts cannot produce duplicate order values.
🤖 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 `@backend/apps/owasp/api/internal/queries/board_candidate_claim_evidence.py`:
- Line 38: The current is_self computation uses claim.candidate.member.login
directly which can raise AttributeError when claim.candidate or
claim.candidate.member is None; change the guard so you first verify
claim.candidate and claim.candidate.member exist (and member.login is present)
before comparing to str(user). In other words, update the is_self logic to
short-circuit to False when claim.candidate or claim.candidate.member is null
(use a safe existence check or getattr) and only perform the str(user) ==
claim.candidate.member.login comparison when the member/login is available.
In `@backend/apps/owasp/migrations/0074_boardcandidateclaim_order.py`:
- Around line 12-20: Add a migrations.RunPython backfill that assigns
deterministic sequential order values per (candidate_id, board_id) partition
before relying on the new BoardCandidateClaim.order semantics: implement a
forwards function (e.g., backfill_boardcandidateclaim_order) that queries
BoardCandidateClaim via the migration ORM, groups rows by candidate_id and
board_id, sorts each group by a stable key (created_at or id) and assigns
incremental PositiveSmallInteger values starting at 0 (use bulk_update or raw
SQL for efficiency), and include a simple reverse function (no-op or reset to 0)
for migrations.RunPython; register this RunPython in the migration alongside the
AddField so existing rows receive stable ordering before you add any uniqueness
constraint.
In `@backend/apps/owasp/models/board_candidate_claim.py`:
- Around line 125-135: The current logic in BoardCandidateClaim only sets
self.order when not self.pk, so if a draft was created with board_id=None later
setting board_id leaves order at 0; update the save() (or pre-save) logic in
BoardCandidateClaim to assign order whenever board_id and candidate_id are
present and either the instance is new (self.pk is None) or the board_id has
changed on an existing record (compare self.board_id to the value loaded from DB
for this pk). Keep the same aggregation using
BoardCandidateClaim.objects.filter(...).aggregate(max_order=models.Max("order"))
to compute (max_order or 0) + 1 and set self.order before saving. Ensure you
reference self.candidate_id and self.board_id for the lookup and only run when
those are truthy.
---
Outside diff comments:
In `@backend/apps/owasp/models/board_candidate_claim.py`:
- Around line 20-23: Add a DB-level uniqueness constraint for the
per-(candidate,board) ordering and make the order assignment atomic with retry:
update the model (BoardCandidateClaim) indexes block to include a
UniqueConstraint on ["candidate","board","order"] (in addition to the existing
Indexes) and change the claim-creation logic (the function that currently
computes max(order)+1 around lines ~125-135) to compute order inside a
transaction/locking block (e.g., select_for_update or an atomic transaction) and
retry on IntegrityError so concurrent inserts cannot produce duplicate order
values.
In `@backend/tests/unit/apps/owasp/models/board_candidate_claim_test.py`:
- Around line 376-393: Add a regression unit test that covers the transition of
an existing draft BoardCandidateClaim from board_id=None to a real board so
order is assigned on update; create a BoardCandidateClaim instance with
status=BoardCandidateClaim.Status.DRAFT, candidate_id set, board_id=None and
order=0 (simulate existing DB object with pk set), patch
BoardCandidateClaim.objects.filter().aggregate() to return {"max_order": N} like
in the existing test, then set claim.board_id to a concrete value, call
claim.save(), and assert claim.order == N+1; reference the test function pattern
in board_candidate_claim_test.py and the BoardCandidateClaim.save behavior to
ensure you replicate the mocks for TimestampedModel.save and full_clean used in
the other test.
🪄 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: ASSERTIVE
Plan: Pro
Run ID: 1a449aae-c7c2-41a0-a044-2abce040c581
📒 Files selected for processing (11)
backend/apps/owasp/api/internal/nodes/board_candidate_claim.pybackend/apps/owasp/api/internal/nodes/board_candidate_claim_evidence.pybackend/apps/owasp/api/internal/queries/__init__.pybackend/apps/owasp/api/internal/queries/board_candidate_claim.pybackend/apps/owasp/api/internal/queries/board_candidate_claim_evidence.pybackend/apps/owasp/migrations/0074_boardcandidateclaim_order.pybackend/apps/owasp/models/board_candidate_claim.pybackend/apps/owasp/models/board_of_directors.pybackend/tests/unit/apps/owasp/api/internal/queries/board_candidate_claim_evidence_test.pybackend/tests/unit/apps/owasp/api/internal/queries/board_candidate_claim_test.pybackend/tests/unit/apps/owasp/models/board_candidate_claim_test.py
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
6 issues found across 11 files
Confidence score: 2/5
- There is a high-impact authorization risk in
backend/apps/owasp/api/internal/queries/board_candidate_claim.py: self-authorization matches Nest username to GitHub login, which can allow private claim access on username collisions. backend/apps/owasp/models/board_candidate_claim.pyintroduces multiple data-integrity/regression risks:bulk_savebypassessave()/full_clean()(skipping validation/lock/order logic), draft claims can keeporder=0after later board assignment, andmax(order)+1is non-atomic under concurrency.backend/apps/owasp/api/internal/queries/board_candidate_claim_evidence.pymay dereference null (claim.candidate.member.login), andbackend/apps/owasp/models/board_of_directors.pyaddsget_candidate(login)without direct test coverage, increasing uncertainty.- Pay close attention to
backend/apps/owasp/api/internal/queries/board_candidate_claim.py,backend/apps/owasp/models/board_candidate_claim.py,backend/apps/owasp/api/internal/queries/board_candidate_claim_evidence.py,backend/apps/owasp/models/board_of_directors.py- authorization correctness, validation/order invariants, null-safety, and untested query behavior.
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
rudransh-shrivastava
marked this pull request as ready for review
June 5, 2026 07:11
4 tasks done
rudransh-shrivastava
added
the
gsoc2026:rudransh-shrivastava
arkid15r
requested changes
Jun 9, 2026
rudransh-shrivastava
marked this pull request as draft
June 10, 2026 06:10
rudransh-shrivastava
dismissed stale reviews from coderabbitai[bot] and cubic-dev-ai[bot]
via
June 10, 2026 06:10
47ef143
|
rudransh-shrivastava
marked this pull request as ready for review
June 10, 2026 12:08
Collaborator
Author
|
I don't know why pre-commit is failing. I will sync feature branch with main after this PR is merged. |
| if not is_self and claim.status != BoardCandidateClaim.Status.APPROVED: | ||
| return [] | ||
|
|
||
| return claim.evidences.filter(is_removed=False) |
Collaborator
Author
There was a problem hiding this comment.
I made this one liner, will add this change in another PR.
arkid15r
approved these changes
Jun 12, 2026
arkid15r
merged commit Jun 12, 2026
42b076f
into
OWASP:feature/bod-candidate-transparency
53 of 56 checks passed
3 of 4 tasks
1 of 2 tasks
rudransh-shrivastava
added a commit
to rudransh-shrivastava/Nest
that referenced
this pull request
Jun 17, 2026
* add candidate claim graphql queries * add tests for get_candidate * add tests for nodes * guard null member * expose nest_created/updated_at as created/updated_at fields * sort alphabetically * address review comments * fix missing created_at
rudransh-shrivastava
added a commit
to rudransh-shrivastava/Nest
that referenced
this pull request
Jun 17, 2026
* add candidate claim graphql queries * add tests for get_candidate * add tests for nodes * guard null member * expose nest_created/updated_at as created/updated_at fields * sort alphabetically * address review comments * fix missing created_at
4 tasks done
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Proposed change
Resolves #4791
Add GraphQL queries for candidate claim data.
Checklist
make check-testlocally: all warnings addressed, tests passed