Add backend tests for certificate generation - #5342
Conversation
|
Contribution validation failed:
|
|
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:
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
Summary by CodeRabbit
WalkthroughThe change adds backend tests for OWASP contributor certificates, score recalculation, GraphQL access, management commands, certificate providers, and CRP model representations. Final-batch certificate issuance now records unexpected failures. ChangesOWASP certificate and scoring coverage
Estimated code review effort: 3 (Moderate) | ~25 minutes Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches 💡 1🛠️ Fix failing CI checks 💡
🧪 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
backend/src/apps/owasp/utils/score_calculator.py (1)
305-355: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winExtract the duplicated batch persist-and-issue logic.
The mid-loop batch block (lines 305-330) and the final leftover-batch block (lines 331-355) now contain the same bulk-save-then-issue-certificates sequence, including identical
except CertificateIssuanceErrorandexcept Exceptionhandling. This change makes both blocks fully mirror each other. Extract a single helper method that takescontribution_scores,pending_scores, andfailed_certificates, and call it from both places, to keep the exception-handling policy in one location and prevent future edits from drifting out of sync again.♻️ Proposed refactor
+ def _persist_and_issue_certificates( + self, + contribution_scores: list[ContributionScore], + pending_scores: list[ContributionScore], + failed_certificates: list[tuple[str, Exception]], + ) -> None: + BulkSaveModel.bulk_save( + ContributionScore, contribution_scores, fields=["value", "tier"] + ) + for pending_score in pending_scores: + try: + Certificate.issue_certificate( + pending_score.github_user, + pending_score.value, + TierChoices(pending_score.tier), + ) + except CertificateIssuanceError as e: + logger.exception( + "Failed to issue certificate for user %s", + pending_score.github_user.login, + ) + failed_certificates.append((pending_score.github_user.login, e)) + except Exception as e: + logger.exception( + "Unexpected certificate processing error for user %s", + pending_score.github_user.login, + ) + failed_certificates.append((pending_score.github_user.login, e)) + if len(contribution_scores) >= self.BATCH_SIZE: - BulkSaveModel.bulk_save( - ContributionScore, contribution_scores, fields=["value", "tier"] - ) - for pending_score in pending_scores: - try: - ... - except Exception as e: - ... - failed_certificates.append((pending_score.github_user.login, e)) + self._persist_and_issue_certificates( + contribution_scores, pending_scores, failed_certificates + ) pending_scores.clear() contribution_scores.clear() if contribution_scores: - BulkSaveModel.bulk_save( - ContributionScore, contribution_scores, fields=["value", "tier"] - ) - for pending_score in pending_scores: - try: - ... - except Exception as e: - ... - failed_certificates.append((pending_score.github_user.login, e)) + self._persist_and_issue_certificates( + contribution_scores, pending_scores, failed_certificates + ) pending_scores.clear() contribution_scores.clear()🤖 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/src/apps/owasp/utils/score_calculator.py` around lines 305 - 355, Extract the duplicated bulk-save and certificate-issuance sequence into a single helper method that accepts contribution_scores, pending_scores, and failed_certificates. Move the existing exception handling into that helper, then replace both the mid-loop batch block and final leftover-batch block with calls to it while preserving the existing clearing behavior.
🤖 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/tests/unit/apps/owasp/models/crp/certificate_test.py`:
- Line 1: Prepend the project's standard copyright header to each new test file:
backend/tests/unit/apps/owasp/models/crp/certificate_test.py lines 1-1,
backend/tests/unit/apps/owasp/utils/certificate_provider_test.py lines 1-1,
backend/tests/unit/apps/owasp/utils/score_calculator_test.py lines 1-1, and
backend/tests/unit/apps/owasp/management/commands/owasp_crp_recalculate_scores_test.py
lines 1-1; leave the existing test imports and contents unchanged.
In `@backend/tests/unit/apps/owasp/models/crp/scoring_weight_test.py`:
- Around line 13-14: Update the test around the ScoringWeight string
representation to remove the get_event_type_display patch and invoke the real
EventTypeChoices.PR_MERGED display path. Keep the assertion verifying the
expected event label and “25 points” suffix so incorrect choice labels are
detected.
In `@backend/tests/unit/apps/owasp/utils/score_calculator_test.py`:
- Around line 189-431: Extract the repeated queryset setup from the six
recalculate_all tests into a shared helper or fixture, such as
_mock_users_queryset(mock_user_class, users), and centralize the empty
PullRequest and Issue queryset configuration as well. Update each named test to
reuse the shared setup while preserving its user list and count behavior.
- Around line 432-460: Update ContributionScoreCalculator.recalculate_user to
pass a TierChoices value, matching recalculate_all’s existing TierChoices(tier)
pattern, when calling Certificate.issue_certificate. Preserve the returned tier
string and score persistence behavior, and update test_recalculate_user to
assert the certificate call receives the TierChoices representation.
---
Outside diff comments:
In `@backend/src/apps/owasp/utils/score_calculator.py`:
- Around line 305-355: Extract the duplicated bulk-save and certificate-issuance
sequence into a single helper method that accepts contribution_scores,
pending_scores, and failed_certificates. Move the existing exception handling
into that helper, then replace both the mid-loop batch block and final
leftover-batch block with calls to it while preserving the existing clearing
behavior.
🪄 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 Plus
Run ID: 9a116b17-b343-40c0-bea1-ed1d1b2435d5
📒 Files selected for processing (10)
backend/src/apps/owasp/utils/score_calculator.pybackend/tests/unit/apps/owasp/api/internal/nodes/certificate_test.pybackend/tests/unit/apps/owasp/api/internal/queries/certificate_test.pybackend/tests/unit/apps/owasp/management/commands/owasp_crp_recalculate_scores_test.pybackend/tests/unit/apps/owasp/models/crp/certificate_test.pybackend/tests/unit/apps/owasp/models/crp/contribution_score_test.pybackend/tests/unit/apps/owasp/models/crp/leaderboard_snapshot_test.pybackend/tests/unit/apps/owasp/models/crp/scoring_weight_test.pybackend/tests/unit/apps/owasp/utils/certificate_provider_test.pybackend/tests/unit/apps/owasp/utils/score_calculator_test.py
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
All reported issues were addressed across 10 files
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
backend/src/apps/owasp/utils/score_calculator.py (1)
305-355: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winExtract the duplicated certificate-issuance loop into a helper method.
The mid-batch flush (lines 309-327) and the final-batch flush (lines 335-353) contain the identical try/except block for issuing certificates and recording failures. This diff added the generic
except Exceptionclause to both blocks, completing the duplication. Extract a shared helper, for example_issue_certificates_for_batch(pending_scores, failed_certificates), and call it from both flush points to keep the two code paths from diverging in future changes.♻️ Proposed refactor
+ def _issue_certificates_for_batch( + self, + pending_scores: list[ContributionScore], + failed_certificates: list[tuple[str, Exception]], + ) -> None: + """Issue certificates for a batch of pending scores, recording failures.""" + for pending_score in pending_scores: + try: + Certificate.issue_certificate( + pending_score.github_user, + pending_score.value, + TierChoices(pending_score.tier), + ) + except CertificateIssuanceError as e: + logger.exception( + "Failed to issue certificate for user %s", + pending_score.github_user.login, + ) + failed_certificates.append((pending_score.github_user.login, e)) + except Exception as e: + logger.exception( + "Unexpected certificate processing error for user %s", + pending_score.github_user.login, + ) + failed_certificates.append((pending_score.github_user.login, e)) + if len(contribution_scores) >= self.BATCH_SIZE: BulkSaveModel.bulk_save( ContributionScore, contribution_scores, fields=["value", "tier"] ) - for pending_score in pending_scores: - try: - Certificate.issue_certificate( - pending_score.github_user, - pending_score.value, - TierChoices(pending_score.tier), - ) - except CertificateIssuanceError as e: - logger.exception( - "Failed to issue certificate for user %s", - pending_score.github_user.login, - ) - failed_certificates.append((pending_score.github_user.login, e)) - except Exception as e: - logger.exception( - "Unexpected certificate processing error for user %s", - pending_score.github_user.login, - ) - failed_certificates.append((pending_score.github_user.login, e)) + self._issue_certificates_for_batch(pending_scores, failed_certificates) pending_scores.clear() contribution_scores.clear() if contribution_scores: BulkSaveModel.bulk_save( ContributionScore, contribution_scores, fields=["value", "tier"] ) - for pending_score in pending_scores: - try: - Certificate.issue_certificate( - pending_score.github_user, - pending_score.value, - TierChoices(pending_score.tier), - ) - except CertificateIssuanceError as e: - logger.exception( - "Failed to issue certificate for user %s", - pending_score.github_user.login, - ) - failed_certificates.append((pending_score.github_user.login, e)) - except Exception as e: - logger.exception( - "Unexpected certificate processing error for user %s", - pending_score.github_user.login, - ) - failed_certificates.append((pending_score.github_user.login, e)) + self._issue_certificates_for_batch(pending_scores, failed_certificates) pending_scores.clear() contribution_scores.clear()🤖 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/src/apps/owasp/utils/score_calculator.py` around lines 305 - 355, Extract the duplicated certificate-processing try/except loop into a shared helper such as _issue_certificates_for_batch, preserving both CertificateIssuanceError and generic exception handling and failed_certificates recording. Replace the loops in both the mid-batch and final flush paths of the score-calculation method with calls to this helper.
🤖 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.
Outside diff comments:
In `@backend/src/apps/owasp/utils/score_calculator.py`:
- Around line 305-355: Extract the duplicated certificate-processing try/except
loop into a shared helper such as _issue_certificates_for_batch, preserving both
CertificateIssuanceError and generic exception handling and failed_certificates
recording. Replace the loops in both the mid-batch and final flush paths of the
score-calculation method with calls to this helper.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 707a4d31-674d-491d-9ca4-e2033b3439f2
📒 Files selected for processing (8)
backend/src/apps/owasp/utils/score_calculator.pybackend/tests/unit/apps/owasp/api/internal/nodes/certificate_test.pybackend/tests/unit/apps/owasp/management/commands/owasp_crp_recalculate_scores_test.pybackend/tests/unit/apps/owasp/models/crp/__init__.pybackend/tests/unit/apps/owasp/models/crp/certificate_test.pybackend/tests/unit/apps/owasp/models/crp/leaderboard_snapshot_test.pybackend/tests/unit/apps/owasp/models/crp/scoring_weight_test.pybackend/tests/unit/apps/owasp/utils/score_calculator_test.py
3 similar comments
|
Hi @arkid15r pr is ready for review please let me know if any changes required Thanks! |
Signed-off-by: Anurag Yadav <anuragyadav2787@gmail.com>
Signed-off-by: Anurag Yadav <anuragyadav2787@gmail.com>
612a03c to
ede615d
Compare
arkid15r
left a comment
arkid15r
left a comment
There was a problem hiding this comment.
I don't see these tests ran succesfuly.
Hi @arkid15r the test are getting pass locally and the ci failure is related to sync so are you talking about the Test run by CI?? |
Signed-off-by: Anurag Yadav <anuragyadav2787@gmail.com>
…nto score-calculation-test
a67a449
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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/tests/unit/apps/owasp/models/crp/__init__.py`:
- Line 1: Add the repository-standard copyright header to the package
initializer to resolve Ruff CPY001, or explicitly exempt this initializer in the
Ruff configuration if omitting the header is intentional.
🪄 Autofix
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 Plus
Run ID: db125d5d-fa2b-4b7d-8b1f-e5618800ad44
📒 Files selected for processing (1)
backend/tests/unit/apps/owasp/models/crp/__init__.py
Hi @arkid15r i checked the latest CI run the workflow is triggering correctly but the backend tests aren't being reached because trivy and the dependency audit are failing due to a dependency version mismatch so to fix this i have raised pr #5408 to fix that |
|
Note GitHub couldn't provide a complete incremental comparison for this pull request, so CodeRabbit is performing a full review instead. This review may take a little longer. |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## feature/contributor-recognition-program #5342 +/- ##
===========================================================================
+ Coverage 97.93% 98.73% +0.79%
===========================================================================
Files 555 555
Lines 17695 17701 +6
Branches 2524 2525 +1
===========================================================================
+ Hits 17330 17477 +147
+ Misses 263 122 -141
Partials 102 102
Flags with carried forward coverage won't be shown. Click here to find out more.
... and 7 files with indirect coverage changes Continue to review full report in Codecov by Harness.
🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 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/tests/unit/apps/owasp/api/internal/queries/certificate_test.py`:
- Around line 16-18: Update the CertificateQuery tests to use the defined
my_certificates field and resolver instead of my_certificate, including all
affected assertions and method calls in the covered test cases. Adjust expected
results to assert [] when github_user is absent and the ordered QuerySet when it
is present.
In `@backend/tests/unit/apps/owasp/models/crp/certificate_test.py`:
- Line 46: Rename every unused patch-injected mock parameter by prefixing it
with an underscore while preserving decorator order and test behavior: update
mock_exit and mock_enter in certificate_test.py at lines 46, 65, 82, and 102;
update mock_load and the other unused injected mocks in score_calculator_test.py
at lines 59, 75, 103, 109, 116, 144, 166, 196, 228-230, 270-272, 306-310, 349,
380-384, 419-423, and 451. Do not alter mocks that are actually referenced.
In `@backend/tests/unit/apps/owasp/utils/score_calculator_test.py`:
- Around line 18-25: Remove the unused mock_weights pytest fixture and remove
its parameter from test_load_scoring_weights. Leave the test’s locally created
weight mocks and remaining setup unchanged.
🪄 Autofix
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 Plus
Run ID: 9aae6fb2-a04c-42ff-a9ac-2cc002790017
📒 Files selected for processing (11)
backend/src/apps/owasp/utils/score_calculator.pybackend/tests/unit/apps/owasp/api/internal/nodes/certificate_test.pybackend/tests/unit/apps/owasp/api/internal/queries/certificate_test.pybackend/tests/unit/apps/owasp/management/commands/owasp_crp_recalculate_scores_test.pybackend/tests/unit/apps/owasp/models/crp/__init__.pybackend/tests/unit/apps/owasp/models/crp/certificate_test.pybackend/tests/unit/apps/owasp/models/crp/contribution_score_test.pybackend/tests/unit/apps/owasp/models/crp/leaderboard_snapshot_test.pybackend/tests/unit/apps/owasp/models/crp/scoring_weight_test.pybackend/tests/unit/apps/owasp/utils/certificate_provider_test.pybackend/tests/unit/apps/owasp/utils/score_calculator_test.py
Signed-off-by: Anurag Yadav <anuragyadav2787@gmail.com>
Signed-off-by: Anurag Yadav <anuragyadav2787@gmail.com>
| pending_score.github_user.login, | ||
| ) | ||
| failed_certificates.append((pending_score.github_user.login, e)) | ||
| except Exception as e: |
There was a problem hiding this comment.
What's the reason for this wide exception catch here?
There was a problem hiding this comment.
I added this earlier because some tests were failing and removing them was causing the coverage to drop but while checking it again i found the issue and fixed it so the tests are no longer failing
Signed-off-by: Anurag Yadav <anuragyadav2787@gmail.com>
f139892
Signed-off-by: Anurag Yadav <anuragyadav2787@gmail.com>
|
2 participants
Proposed change
Add backend tests for certificate generation
Resolves #5199
Checklist