Implement certificate metadata issuance system#4922
Conversation
|
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:
WalkthroughIntroduces the OWASP Contributor Recognition Program (CRP) scoring and certificate infrastructure. Four CRP model ChangesCRP Contribution Scoring & Certificate Issuance
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related issues
Suggested reviewers
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 7
🤖 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/admin/contribution_score.py`:
- Around line 56-61: The exception handling in the batch admin action for score
recalculation is too narrow. Change the except clause from catching only
ValueError and TypeError to catching the broader Exception class instead. This
allows the batch operation to continue processing remaining contributors even if
an individual score recalculation fails for any reason, rather than aborting the
entire action. The logging statement is already in place and will capture all
exceptions, so only the except clause needs to be modified.
In `@backend/apps/owasp/score_calculator.py`:
- Around line 89-99: The `calculate()` method is being called once per user in
`recalculate_all()`, causing three separate COUNT queries per user (from
`count_merged_pull_requests()`, `count_opened_pull_requests()`, and
`count_opened_issues()`). Refactor `recalculate_all()` to pre-aggregate counts
grouped by `author_id` in a single batch query instead of iterating through
users and calling `calculate()` individually. Build a dictionary of aggregated
counts keyed by author_id, then compute scores for all users from this
pre-aggregated data, eliminating the query amplification of 3 queries per user.
In `@backend/apps/owasp/services/certificate_providers.py`:
- Line 1: The services directory is missing an __init__.py file, which is
required to make it a proper Python package. Create a new __init__.py file in
the services directory with a simple docstring identifying it as the OWASP
services package to enable proper module imports from this directory.
- Line 11: The import of TierChoices at the top level should be moved into the
TYPE_CHECKING block since it is only used in type annotations (not at runtime).
Remove the import statement `from apps.owasp.models.crp.recognition_enums import
TierChoices` from the unconditional imports section and add it within the `if
TYPE_CHECKING:` block at the top of the file alongside other type-checking
imports. This avoids the runtime import overhead while keeping TierChoices
available for type annotations.
- Around line 69-73: The ValueError raised in the provider class lookup section
violates Ruff rules TRY003 and EM102 because the exception message uses an
f-string literal directly in the raise statement. Create a variable to hold the
error message first, then use that variable in the raise ValueError statement.
Specifically, assign the message containing the provider_type to a variable (for
example, msg or error_message) before the raise statement, then pass that
variable to ValueError instead of the inline f-string.
In `@backend/apps/owasp/services/certificate_service.py`:
- Line 1: The file has three Ruff linting blockers that must be fixed before
merge. First, create a missing `__init__.py` file in the
`backend/apps/owasp/services/` directory to resolve the INP001 implicit
namespace package error. Second, at the locations indicated (lines 9-9 and
44-45), fix the TC001 error by creating custom exception classes instead of
using generic exceptions, and fix the TRY401 error by ensuring proper exception
handling patterns (such as using `raise ... from e` syntax where appropriate).
Review the code at those line ranges and replace generic exception handling with
properly structured custom exceptions and correct error chaining.
- Around line 42-46: The issue_certificate method currently logs errors and
returns silently when certificate provider initialization or issuance fails,
preventing callers from knowing whether the operation succeeded. Modify the
method to expose failures to callers by changing its return type to bool (or
raising a domain exception). At the exception handling block where
CertificateProviderFactory.get_provider() fails (lines 42-46), return False
instead of silently returning. Apply the same fix at lines 54-61 where the
issuance failure is also caught and silently returned. This allows admin and
command flows to accurately track and report failed certificate issuance
attempts.
🪄 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: a9a876d0-c836-42d9-b903-25320d44546f
📒 Files selected for processing (19)
backend/apps/github/models/generic_issue_model.pybackend/apps/owasp/Makefilebackend/apps/owasp/admin/certificate.pybackend/apps/owasp/admin/contribution_score.pybackend/apps/owasp/admin/leaderboard_snapshot.pybackend/apps/owasp/admin/scoring_weight.pybackend/apps/owasp/management/commands/owasp_crp_recognition_recalculate_scores.pybackend/apps/owasp/migrations/0074_alter_certificate_table_and_more.pybackend/apps/owasp/models/__init__.pybackend/apps/owasp/models/crp/__init__.pybackend/apps/owasp/models/crp/certificate.pybackend/apps/owasp/models/crp/contribution_score.pybackend/apps/owasp/models/crp/leaderboard_snapshot.pybackend/apps/owasp/models/crp/recognition_enums.pybackend/apps/owasp/models/crp/scoring_weight.pybackend/apps/owasp/score_calculator.pybackend/apps/owasp/services/certificate_providers.pybackend/apps/owasp/services/certificate_service.pycspell/custom-dict.txt
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
4 issues found and verified against the latest diff
Confidence score: 2/5
- In
backend/apps/owasp/score_calculator.py, the opened-issue scoring path filters to completed issues, so validissue_openedcontributions are dropped and contributors can be assigned the wrong score/tier; this is the highest user-impact risk in the PR — fix the filter logic and add a regression test for opened-but-not-completed issues before merging. - In
backend/apps/owasp/services/certificate_service.py, certificate issuance is non-atomic, so concurrent requests for the same user+tier can race and try to create duplicate active certificates, causing non-deterministic outcomes and noisy failures — enforce atomicity (transaction/locking or uniqueness-safe upsert) before merge. - In
backend/apps/owasp/admin/contribution_score.py, batch admin exception handling is too narrow, so one unexpected DB/service/runtime error can halt processing for the rest of the selected contributors — broaden error handling so failures are isolated per contributor and the batch can complete. - In
backend/apps/owasp/score_calculator.py,recalculate_allhas N+1 query behavior that will degrade noticeably as contributor counts grow, risking slow admin operations and timeouts at scale — switch to batched aggregation/annotation or prefetch-based counting as a follow-up before this grows further.
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/apps/owasp/services/certificate_providers.py (1)
44-48:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winMake provider writes idempotent at the DB write step.
LocalCertificateProvider.issue()performs a plaincreate(). BecauseCertificateServicedoes a separate pre-check, concurrent issuers can still race and hitunique_active_cert_per_tier. Preferget_or_create(...)(or handleIntegrityErroras already-issued) to make issuance atomic/idempotent.Suggested fix
+from django.db import IntegrityError @@ def issue(self, user: User, score: int, tier: TierChoices) -> None: @@ - Certificate.objects.create( - github_user=user, - score=score, - tier=tier, - ) + try: + Certificate.objects.get_or_create( + github_user=user, + tier=tier, + is_revoked=False, + defaults={"score": score}, + ) + except IntegrityError: + # Concurrent issuer won the race; treat as already issued. + return🤖 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/services/certificate_providers.py` around lines 44 - 48, The `Certificate.objects.create()` call in the `LocalCertificateProvider.issue()` method is not idempotent and can race when multiple concurrent issuers pass the pre-check in `CertificateService`. Replace the `create()` call with `get_or_create()` using the appropriate unique fields (github_user and tier) to make the write atomic and idempotent, ensuring concurrent calls safely handle the case where the certificate already exists rather than raising an IntegrityError from the unique_active_cert_per_tier constraint.
♻️ Duplicate comments (1)
backend/apps/owasp/services/certificate_service.py (1)
22-22:⚠️ Potential issue | 🟠 Major | 🏗️ Heavy liftExpose issuance failure to callers instead of logging-and-returning.
issue_certificate()currently swallows provider resolution and issuance failures. Downstream recalculation flows continue and can report success while certificate metadata is not persisted. Return a status (bool) or raise a domain exception so callers can track/report failures explicitly.Suggested direction
- def issue_certificate(cls, user: User, score: int, tier: TierChoices) -> None: + def issue_certificate(cls, user: User, score: int, tier: TierChoices) -> bool: @@ if has_active_cert: - return + return True @@ except ValueError: logger.exception("Failed to resolve certificate provider") - return + return False @@ try: provider.issue(user, score, tier) except Exception: logger.exception( "Failed to issue %s certificate for user %s", tier, user.login, ) + return False + return TrueThen update callers in
backend/apps/owasp/score_calculator.pyto handle the returned status.Also applies to: 41-45, 53-61
🤖 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/services/certificate_service.py` at line 22, The issue_certificate() method in certificate_service.py currently suppresses provider resolution and issuance failures by logging them and returning silently, causing downstream flows to report success even though certificate metadata was not persisted. Modify issue_certificate() to return a bool status (True for success, False for failure) or raise a domain exception instead of silently returning on errors. Update all failure points in the method (including the provider resolution failure around line 41-45 and issuance failures around line 53-61) to explicitly signal failure rather than logging-and-returning. Then update all callers of issue_certificate() in backend/apps/owasp/score_calculator.py to check the returned status or handle the raised exception, so they can properly track and report certificate issuance failures instead of proceeding as if the operation succeeded.
🤖 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/apps/owasp/services/certificate_providers.py`:
- Around line 44-48: The `Certificate.objects.create()` call in the
`LocalCertificateProvider.issue()` method is not idempotent and can race when
multiple concurrent issuers pass the pre-check in `CertificateService`. Replace
the `create()` call with `get_or_create()` using the appropriate unique fields
(github_user and tier) to make the write atomic and idempotent, ensuring
concurrent calls safely handle the case where the certificate already exists
rather than raising an IntegrityError from the unique_active_cert_per_tier
constraint.
---
Duplicate comments:
In `@backend/apps/owasp/services/certificate_service.py`:
- Line 22: The issue_certificate() method in certificate_service.py currently
suppresses provider resolution and issuance failures by logging them and
returning silently, causing downstream flows to report success even though
certificate metadata was not persisted. Modify issue_certificate() to return a
bool status (True for success, False for failure) or raise a domain exception
instead of silently returning on errors. Update all failure points in the method
(including the provider resolution failure around line 41-45 and issuance
failures around line 53-61) to explicitly signal failure rather than
logging-and-returning. Then update all callers of issue_certificate() in
backend/apps/owasp/score_calculator.py to check the returned status or handle
the raised exception, so they can properly track and report certificate issuance
failures instead of proceeding as if the operation succeeded.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 1fb6b41b-3da5-4fc4-96a8-4e19a4068991
📒 Files selected for processing (3)
backend/apps/owasp/services/__init__.pybackend/apps/owasp/services/certificate_providers.pybackend/apps/owasp/services/certificate_service.py
There was a problem hiding this comment.
0 issues found across 3 files (changes from recent commits).
Requires human review: Auto-approval blocked by 4 unresolved issues from previous reviews.
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/apps/owasp/score_calculator.py (1)
364-376:⚠️ Potential issue | 🟠 Major | ⚡ Quick winInconsistent certificate failure handling between single-user and bulk flows.
recalculate_all()(lines 309–319, 328–338) catches and logs certificate issuance failures but continues processing, treating certificate errors as non-critical. However,recalculate_user()at line 375 propagatesCertificateIssuanceErrorto the caller, aborting the operation. This inconsistency means direct callers ofrecalculate_user()must handle certificate exceptions, while the method signature and docstring don't document this behavior. Align the two flows: either wrap line 375 intry/except(matching bulk behavior) or document the raised exception.🛡️ Align error handling with bulk flow
"tier": tier, }, ) - CertificateService.issue_certificate(user, total_score, tier) + try: + CertificateService.issue_certificate(user, total_score, tier) + except Exception: + logger.exception( + "Failed to issue certificate for user %s", + user.login, + ) logger.info(🤖 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/score_calculator.py` around lines 364 - 376, The recalculate_user() method calls CertificateService.issue_certificate() without exception handling, allowing CertificateIssuanceError to propagate to callers, while recalculate_all() catches and logs certificate failures to continue processing. Wrap the CertificateService.issue_certificate(user, total_score, tier) call in a try/except block that catches CertificateIssuanceError, logs the error (matching the approach used in recalculate_all()), and allows the method to complete successfully, ensuring consistent non-critical error handling across both flows.
🤖 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/apps/owasp/score_calculator.py`:
- Around line 364-376: The recalculate_user() method calls
CertificateService.issue_certificate() without exception handling, allowing
CertificateIssuanceError to propagate to callers, while recalculate_all()
catches and logs certificate failures to continue processing. Wrap the
CertificateService.issue_certificate(user, total_score, tier) call in a
try/except block that catches CertificateIssuanceError, logs the error (matching
the approach used in recalculate_all()), and allows the method to complete
successfully, ensuring consistent non-critical error handling across both flows.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: fd029e87-02cd-4787-be91-a73730e2c9ac
📒 Files selected for processing (4)
backend/apps/owasp/admin/contribution_score.pybackend/apps/owasp/exceptions.pybackend/apps/owasp/score_calculator.pybackend/apps/owasp/services/certificate_service.py
There was a problem hiding this comment.
2 issues found across 4 files (changes from recent commits).
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="backend/apps/owasp/score_calculator.py">
<violation number="1" location="backend/apps/owasp/score_calculator.py:315">
P2: Catching broad `Exception` and continuing hides certificate issuance failures from control flow. This can leave users without certificates while recalculation reports success.</violation>
</file>
Tip: Review your code locally with the cubic CLI to iterate faster.
Re-trigger cubic
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
backend/apps/owasp/score_calculator.py (1)
35-45:⚠️ Potential issue | 🟠 Major | 🏗️ Heavy liftHonor
ScoringWeight.daily_capbefore persisting scores.
load_scoring_weights()discardsdaily_cap, andcalculate_score()multiplies already-collapsed totals by the per-event score. Any configured cap is therefore ignored, soContributionScore.value, tiers, and certificate issuance can all overshoot the scoring policy defined inbackend/apps/owasp/models/crp/scoring_weight.py. This needs a per-day aggregation path (or the unsupported field removed) before release.Also applies to: 69-75
🤖 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/score_calculator.py` around lines 35 - 45, The load_scoring_weights() method currently returns only event_type and score while discarding the daily_cap field from ScoringWeight, causing daily caps to be ignored during score calculation. Update load_scoring_weights() to include the daily_cap value in the returned dictionary, then modify the calculate_score() method (which uses these weights around lines 69-75) to implement per-day aggregation that respects the daily_cap constraint, ensuring ContributionScore.value never exceeds the configured daily caps for each event type.
🤖 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/score_calculator.py`:
- Around line 94-99: The bucket key is labeled "issue_opened" but the underlying
query logic filters for state_reason=COMPLETED and applies the date window to
created_at, creating a mismatch between the key name and what actually gets
scored. Additionally, this causes the date window to miss issues completed
within the window if they were opened earlier. You need to choose one consistent
contract—either track opened issues using created_at, or track completed issues
using the appropriate completion date field—and then make the bucket key name,
the state_reason predicate (or equivalent completion filter), and the date field
all align with that choice. Ensure this consistency is applied across all
affected locations where this metric is defined and scored.
---
Outside diff comments:
In `@backend/apps/owasp/score_calculator.py`:
- Around line 35-45: The load_scoring_weights() method currently returns only
event_type and score while discarding the daily_cap field from ScoringWeight,
causing daily caps to be ignored during score calculation. Update
load_scoring_weights() to include the daily_cap value in the returned
dictionary, then modify the calculate_score() method (which uses these weights
around lines 69-75) to implement per-day aggregation that respects the daily_cap
constraint, ensuring ContributionScore.value never exceeds the configured daily
caps for each event type.
🪄 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: 19d5bc96-19ad-4df2-bce2-388efc2e0339
📒 Files selected for processing (1)
backend/apps/owasp/score_calculator.py
There was a problem hiding this comment.
1 issue found across 1 file (changes from recent commits).
Tip: Review your code locally with the cubic CLI to iterate faster.
Re-trigger cubic
There was a problem hiding this comment.
1 issue found across 2 files (changes from recent commits).
Tip: Review your code locally with the cubic CLI to iterate faster.
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 (2)
backend/apps/owasp/score_calculator.py (1)
304-321:⚠️ Potential issue | 🟠 Major | 🏗️ Heavy liftKeep score updates and certificate issuance in the same failure unit.
BulkSaveModel.bulk_save(...)commits the newContributionScorerows beforeCertificateService.issue_certificate(...)runs. The new catch-and-continue block then records the provider error and moves on, so a provider outage leaves users with upgraded tiers but no matchingCertificatemetadata until someone manually reruns the job. That breaks the persistence contract this CRP flow is introducing.Based on
backend/apps/owasp/services/certificate_service.py:21-68, the transaction boundary only covers the certificate write, not the preceding score save.Also applies to: 324-341
🤖 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/score_calculator.py` around lines 304 - 321, The BulkSaveModel.bulk_save() call for ContributionScore is executed before attempting to issue certificates, which means if certificate issuance fails, the score updates have already been persisted, breaking the consistency contract. Move the BulkSaveModel.bulk_save(ContributionScore, ...) call to execute only after the certificate issuance loop completes successfully (after the for loop over pending_certificates), so that score updates and certificate issuance remain in the same transactional failure unit. This same change is needed at the second occurrence of this pattern mentioned in lines 324-341.backend/apps/owasp/management/commands/owasp_crp_recognition_recalculate_scores.py (1)
20-39:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winOnly print the green success banner on the success path.
When
failed_count > 0, this command still emits a styled SUCCESS message and then exits withCommandError. That makes failure runs look healthy in CI and operator logs at a glance.Suggested change
- self.stdout.write( - self.style.SUCCESS( - f"Score recalculation complete:\n" - f" - Total users: {result['total']}\n" - f" - Created: {result['created']}\n" - f" - Updated: {result['updated']}\n" - f" - Failed: {result['failed_count']}" - ) - ) - failed_count = result.get("failed_count", 0) if failed_count > 0: failures = result.get("failures", []) failed_users = [username for username, _ in failures] failed_str = ", ".join(failed_users) self.stdout.write( self.style.WARNING(f"Failed to issue certificates for: {failed_str}") ) error_msg = f"Failed to issue certificates for {failed_count} user(s)" raise CommandError(error_msg) + + self.stdout.write( + self.style.SUCCESS( + f"Score recalculation complete:\n" + f" - Total users: {result['total']}\n" + f" - Created: {result['created']}\n" + f" - Updated: {result['updated']}\n" + f" - Failed: {result['failed_count']}" + ) + )🤖 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/management/commands/owasp_crp_recognition_recalculate_scores.py` around lines 20 - 39, The success message is printed unconditionally even when the command fails (when failed_count > 0), making failures appear successful in logs. Move the self.stdout.write call with self.style.SUCCESS containing the score recalculation summary into a conditional block that only executes when failed_count equals 0. Keep the failure handling path (the check for failed_count > 0, the WARNING message, and the CommandError) separate and only execute that path when there are actual failures.
🤖 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/apps/owasp/management/commands/owasp_crp_recognition_recalculate_scores.py`:
- Around line 20-39: The success message is printed unconditionally even when
the command fails (when failed_count > 0), making failures appear successful in
logs. Move the self.stdout.write call with self.style.SUCCESS containing the
score recalculation summary into a conditional block that only executes when
failed_count equals 0. Keep the failure handling path (the check for
failed_count > 0, the WARNING message, and the CommandError) separate and only
execute that path when there are actual failures.
In `@backend/apps/owasp/score_calculator.py`:
- Around line 304-321: The BulkSaveModel.bulk_save() call for ContributionScore
is executed before attempting to issue certificates, which means if certificate
issuance fails, the score updates have already been persisted, breaking the
consistency contract. Move the BulkSaveModel.bulk_save(ContributionScore, ...)
call to execute only after the certificate issuance loop completes successfully
(after the for loop over pending_certificates), so that score updates and
certificate issuance remain in the same transactional failure unit. This same
change is needed at the second occurrence of this pattern mentioned in lines
324-341.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 49b7efbb-2956-40d4-91be-229009ba6778
📒 Files selected for processing (2)
backend/apps/owasp/management/commands/owasp_crp_recognition_recalculate_scores.pybackend/apps/owasp/score_calculator.py
There was a problem hiding this comment.
0 issues found across 3 files (changes from recent commits).
Requires human review: Auto-approval blocked by 1 unresolved issue from previous reviews.
Re-trigger cubic
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## feature/contributor-recognition-program #4922 +/- ##
===========================================================================
- Coverage 98.74% 97.95% -0.79%
===========================================================================
Files 547 551 +4
Lines 17185 17386 +201
Branches 2417 2434 +17
===========================================================================
+ Hits 16969 17031 +62
- Misses 127 266 +139
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:
|
arkid15r
left a comment
arkid15r
left a comment
There was a problem hiding this comment.
Please resolve conflicts
|
Hi @arkid15r, the pr is ready for review. the current ci/cd failure seems unrelated to my changes Thanks! |
| ) | ||
| ) | ||
|
|
||
| failed_count = result.get("failed_count", 0) |
There was a problem hiding this comment.
Consider walrus operator and more pythonic way to check if it's truthy (just if failed_count).
| ValueError: If the configured provider is unknown. | ||
|
|
||
| """ | ||
| provider_type = getattr(settings, "CERTIFICATE_PROVIDER", "local") |
There was a problem hiding this comment.
Let's make settings.CERTIFICATE_PROVIDER usable here (w/o getattr)
| has_active_cert = Certificate.objects.filter( | ||
| github_user=user, | ||
| tier=tier, | ||
| is_revoked=False, | ||
| ).exists() | ||
|
|
||
| if has_active_cert: | ||
| return |
There was a problem hiding this comment.
Use := to combine those 2.
| """Abstract base class for certificate providers.""" | ||
|
|
||
| @abstractmethod | ||
| def issue(self, user: User, score: int, tier: TierChoices) -> None: |
There was a problem hiding this comment.
This name is somewhat confusing, could you use something like issue_certificate ?
Thanks
| ) | ||
| try: | ||
| provider.issue(user, score, tier) | ||
| except Exception as e: |
There was a problem hiding this comment.
Could you narrow down the exception types here?
There was a problem hiding this comment.
I had narrowed it to IntegrityError based on the current implementation but keeping it broad here seems fine since the service layer is responsible for wrapping provider side failures cubic dev AI also pointed out the same regarding preserving the service error boundary #4922 (comment)
| pending_score.value, | ||
| TierChoices(pending_score.tier), | ||
| ) | ||
| except Exception as e: |
There was a problem hiding this comment.
It's not encouraged to have that wide exceptions everywhere -- not a good practice.
There was a problem hiding this comment.
Sorry for this i will take care from next time
There was a problem hiding this comment.
2 issues found across 5 files (changes from recent commits).
Tip: Review your code locally with the cubic CLI to iterate faster.
Re-trigger cubic
|
2 participants
Proposed change
This PR adds the backend flow for issuing contributor certificates by creating and storing certificate metadata when a contributor reaches a recognition tier.
Resolves #4921
Checklist
make check-testlocally: all warnings addressed, tests passed