Fix NestBot calendar events feature branch merge conflicts #2230
Conversation
* feat: Implement GraphQL support for badges - Add BadgeNode GraphQL type with fields: id, name, description, weight, css_class - Add badges query to return all badges ordered by weight and name - Add badges field to UserNode to expose user badges sorted by weight - Add comprehensive test coverage for BadgeQueries and UserNode badges field - Integrate BadgeQueries into NestQuery schema Fixes OWASP#1764 * fix: Update UserNode test to include badges field * done with changes suggested by coderabbit with all checks passing * fixed some more coderabbit suggestion * Update code * Refactor badges field resolution in UserNode and BadgeQueries for improved clarity and performance * updated the suggested changes by @arkid15r 1.deleted the badge queries - which is just querying all badges (not useful) 2.delete the test case for it 3.merge user_badge_field_test.py to user_test.py 4.updated user_badge.py to query for getting badges for each user * done with coderabbit suggestions * Remove BadgeQueries import from NestQuery class * Remove BadgeQueries inheritance from NestQuery class * Update badges resolver to sort by badge weight in descending order * Update code --------- Co-authored-by: Arkadii Yakovets <[email protected]> Co-authored-by: Kate Golovanova <[email protected]>
* Add google libs * Add model * Update logic and add tests * Update tests, apply make-check and suggestions * Skip sonar and apply rabbit suggestion * Update poetry lock * Add migrations * Change user to member * Update env.example * Update poetry.lock * Update tokens to binary field * Improve auth logic * Apply migrations * Apply rabbit's suggestions * Separate google client from slack.GoogleAuth model * Convert singleton to factory * Update auth_uri * Apply suggestions * Add meta class * Update refresh logic and tests * Make google auth credentials secret * Update code * Apply suggestions * Update tests and member related name * Clean up migrations * Add kms key id to env * Add AWS to settings * Add KMS client, encryption and decryption * Separate kms client from GoogleAuth model * Update kms client * Update tests * Update refresh and tests * Apply rabbit suggestion * Apply check-spelling * Apply rabbit suggestion and add google client test * Add custom field for kms * Update tests * Apply suggestions * Update tests * Guard authenticate method with KMS * Update tests * Add migrations * Update settings * Clean up migrations * Add google callback * Add google sign in button * Apply suggestions * Apply suggestion * Update tests * Change name * Add migrations * Refactor google and kms clients * Update tests * Add sign in command * Update tests * Add handler test * Clean up migrations * Apply suggestions * Update backend/tests/apps/nest/models/google_account_authorization_test.py Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com> * Apply rabbit's suggestion * Update poetry lock * Update code * Apply suggestions --------- Co-authored-by: Arkadii Yakovets <[email protected]> Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
* Use singleton * Add thread locking
…r models (OWASP#2128) * Update event * Add slack member and channel id fields * Apply rabbit's suggestion * Apply suggestions * Add null possibility to channel in remainder * Apply rabbit suggestions * Use channel id and add tests * Apply suggestions and add reminder shedule --------- Co-authored-by: Kate Golovanova <[email protected]>
Summary by CodeRabbit
WalkthroughIntroduces user badge GraphQL support and related model relation changes, updates top contributors’ id derivation, adjusts REST member endpoint metadata, modifies frontend Sentry/release build configuration and Docker secrets, updates CI workflows to pass RELEASE_VERSION, bumps a frontend dependency, and adds tests. Some files contain unresolved merge conflict markers. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested labels
Suggested reviewers
✨ Finishing Touches
🧪 Generate 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. 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: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
backend/apps/github/api/internal/queries/repository_contributor.py (1)
53-64: Use composite, stable ID for membership nodes
The node includes project-specific fields (project_key/project_name), so it represents a person-in-project membership—not a standalone user. Currentlyid=loginwill collide when the same user appears in multiple projects. Update the resolver inbackend/apps/github/api/internal/queries/repository_contributor.py(lines 53–64) to buildidfrom bothloginandproject_key(e.g."${login}@${project_key}").backend/apps/nest/management/commands/nest_update_badges.py (1)
43-58: Inactive staff with existing badge are never reactivated (logic bug).
employees_without_badgeexcludes users who already have the badge, even ifis_active=False, so staff who were deactivated won’t be reactivated.Apply:
- employees_without_badge = User.objects.filter( - is_owasp_staff=True, - ).exclude( - user_badges__badge=badge, - ) + # Staff who either don't have the badge or have it but inactive. + employees_needing_activation = ( + User.objects.filter(is_owasp_staff=True) + .exclude(user_badges__badge=badge, user_badges__is_active=True) + .distinct() + ) count = employees_without_badge.count() - if count: - for user in employees_without_badge: - user_badge, created = UserBadge.objects.get_or_create(user=user, badge=badge) - if not user_badge.is_active: - user_badge.is_active = True - user_badge.save(update_fields=["is_active"]) + if count: + for user in employees_needing_activation: + user_badge, _ = UserBadge.objects.get_or_create(user=user, badge=badge) + if not user_badge.is_active: + user_badge.is_active = True + user_badge.save(update_fields=["is_active"])
🧹 Nitpick comments (11)
backend/apps/github/api/internal/queries/repository_contributor.py (2)
41-51: Normalizeexcluded_usernamesto avoid case-mismatch leaks.GitHub logins are case-insensitive; normalize once before passing to the model.
top_contributors = RepositoryContributor.get_top_contributors( + excluded_usernames=[u.lower() for u in excluded_usernames] if excluded_usernames else None, chapter=chapter, committee=committee, - excluded_usernames=excluded_usernames, has_full_name=has_full_name,
53-64: Harden dict access to avoidKeyErroron optional fields.If
name(oravatar_url) is missing for some contributors, this will raise. Use.get()for soft fields.- avatar_url=tc["avatar_url"], + avatar_url=tc.get("avatar_url"), contributions_count=tc["contributions_count"], id=..., login=tc["login"], - name=tc["name"], + name=tc.get("name"),frontend/__tests__/unit/pages/ProjectsHealthDashboardMetrics.test.tsx (1)
55-55: Make useSearchParams mock seedable per test.Using a bare new URLSearchParams() returns an empty set; seed query strings where needed for filter/sort tests.
Apply:
-jest.mock('next/navigation', () => ({ - useSearchParams: jest.fn(() => new URLSearchParams()), - useRouter: jest.fn(() => ({ - push: jest.fn(), - replace: jest.fn(), - })), -})) +jest.mock('next/navigation', () => { + let qs = '' + return { + useSearchParams: jest.fn(() => new URLSearchParams(qs)), + useRouter: jest.fn(() => ({ push: jest.fn(), replace: jest.fn() })), + // helper for tests: + __setSearchParams: (v: string) => (qs = v), + } +})Then in tests that need params:
const nav = jest.requireMock('next/navigation') nav.__setSearchParams('level=Flagship&sort=desc')backend/apps/nest/management/commands/nest_update_badges.py (1)
61-76: Optional: bulk flip for performance.You can reduce per-user queries by bulk-updating existing relations, then only creating missing ones.
- non_employees = User.objects.filter( + non_employees = User.objects.filter( is_owasp_staff=False, user_badges__badge=badge, ).distinct() removed_count = non_employees.count() if removed_count: UserBadge.objects.filter( user_id__in=non_employees.values_list("id", flat=True), badge=badge, ).update(is_active=False) + + # (Optional) Bulk activate existing user_badges for staff, then create missing. + UserBadge.objects.filter(badge=badge, user__is_owasp_staff=True, is_active=False).update(is_active=True) + missing_staff = ( + User.objects.filter(is_owasp_staff=True) + .exclude(user_badges__badge=badge) + .values_list("id", flat=True) + ) + UserBadge.objects.bulk_create([UserBadge(user_id=uid, badge=badge, is_active=True) for uid in missing_staff]).github/workflows/run-ci-cd.yaml (3)
349-351: Passing RELEASE_VERSION as a build secret works; consider build-arg for non-secret values.Since release version isn’t sensitive, using a Docker build-arg simplifies local builds and avoids secret mounts.
- secrets: | - RELEASE_VERSION=${{ needs.set-release-version.outputs.release_version }} - SENTRY_AUTH_TOKEN=${{ secrets.SENTRY_AUTH_TOKEN }} + build-args: | + RELEASE_VERSION=${{ env.RELEASE_VERSION }} + secrets: | + SENTRY_AUTH_TOKEN=${{ secrets.SENTRY_AUTH_TOKEN }}Companion Dockerfile change (builder stage):
- RUN --mount=type=secret,id=RELEASE_VERSION \ - --mount=type=secret,id=SENTRY_AUTH_TOKEN \ - export NEXT_SENTRY_AUTH_TOKEN=$(cat /run/secrets/SENTRY_AUTH_TOKEN) && \ - export RELEASE_VERSION=$(cat /run/secrets/RELEASE_VERSION) && \ - pnpm run build + ARG RELEASE_VERSION + RUN --mount=type=secret,id=SENTRY_AUTH_TOKEN \ + export NEXT_SENTRY_AUTH_TOKEN="$(cat /run/secrets/SENTRY_AUTH_TOKEN)" && \ + export RELEASE_VERSION="${RELEASE_VERSION:-dev}" && \ + pnpm run build
669-671: Mirror the same release version handling in production build.Keep staging/production consistent: prefer build-arg for RELEASE_VERSION; keep SENTRY token as a secret.
- secrets: | - RELEASE_VERSION=${{ needs.set-release-version.outputs.release_version }} - SENTRY_AUTH_TOKEN=${{ secrets.SENTRY_AUTH_TOKEN }} + build-args: | + RELEASE_VERSION=${{ env.RELEASE_VERSION }} + secrets: | + SENTRY_AUTH_TOKEN=${{ secrets.SENTRY_AUTH_TOKEN }}
130-135: Typo in job name (“Denendencies”).Minor polish.
- scan-ci-dependencies: - name: Run CI Denendencies Scan + scan-ci-dependencies: + name: Run CI Dependencies Scanbackend/apps/github/api/internal/nodes/user.py (1)
31-43: Resolver logic looks good; watch for N+1 on user lists.If this field is requested on many users, consider prefetching
user_badges.select_related("badge")via a DataLoader/prefetch to avoid per-user queries.frontend/docker/Dockerfile (2)
37-41: Make secret mounts resilient for local builds.If secrets aren’t provided, the build can fail. Provide fallbacks and make SENTRY token optional.
-RUN --mount=type=secret,id=RELEASE_VERSION \ - --mount=type=secret,id=SENTRY_AUTH_TOKEN \ - export NEXT_SENTRY_AUTH_TOKEN=$(cat /run/secrets/SENTRY_AUTH_TOKEN) && \ - export RELEASE_VERSION=$(cat /run/secrets/RELEASE_VERSION) && \ - pnpm run build +RUN --mount=type=secret,id=RELEASE_VERSION,required=false \ + --mount=type=secret,id=SENTRY_AUTH_TOKEN,required=false \ + sh -eu -c '\ + if [ -f /run/secrets/SENTRY_AUTH_TOKEN ]; then \ + export NEXT_SENTRY_AUTH_TOKEN="$(cat /run/secrets/SENTRY_AUTH_TOKEN)"; \ + fi; \ + if [ -f /run/secrets/RELEASE_VERSION ]; then \ + export RELEASE_VERSION="$(cat /run/secrets/RELEASE_VERSION)"; \ + else \ + export RELEASE_VERSION="dev"; \ + fi; \ + pnpm run build \ + '
60-60: Avoid failing if .env isn’t present.
rm .envcan error; make it idempotent.-RUN mkdir -p /app/.next/cache && chown -R nextjs:nodejs /app/.next/cache && chmod -R 755 /app/.next/cache && rm .env +RUN mkdir -p /app/.next/cache && chown -R nextjs:nodejs /app/.next/cache && chmod -R 755 /app/.next/cache && rm -f .envbackend/tests/apps/github/api/internal/nodes/user_test.py (1)
84-123: Good behavior test for badges resolver.Covers call chain and ordering. Consider an extra case where two badges share the same weight to assert secondary sort by name.
+ def test_badges_resolver_secondary_sort_by_name(self): + a = Mock(); a.weight = 5; a.name = "Alpha" + b = Mock(); b.weight = 5; b.name = "Beta" + ua, ub = Mock(), Mock() + ua.badge, ub.badge = a, b + mock_user, mock_qs = Mock(), Mock() + mock_qs.filter.return_value.order_by.return_value = [ua, ub] + mock_user.user_badges.select_related.return_value = mock_qs + assert UserNode.badges(mock_user) == [a, b]
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (4)
backend/poetry.lockis excluded by!**/*.lockcspell/pnpm-lock.yamlis excluded by!**/pnpm-lock.yamlfrontend/pnpm-lock.yamlis excluded by!**/pnpm-lock.yamlfrontend/src/types/__generated__/graphql.tsis excluded by!**/__generated__/**
📒 Files selected for processing (15)
.github/workflows/label-pull-requests.yaml(0 hunks).github/workflows/run-ci-cd.yaml(2 hunks)backend/apps/github/api/internal/nodes/user.py(2 hunks)backend/apps/github/api/internal/queries/repository_contributor.py(1 hunks)backend/apps/github/api/rest/v1/member.py(0 hunks)backend/apps/github/models/repository_contributor.py(0 hunks)backend/apps/nest/api/internal/nodes/badge.py(1 hunks)backend/apps/nest/management/commands/nest_update_badges.py(2 hunks)backend/apps/nest/migrations/0005_alter_userbadge_user.py(1 hunks)backend/apps/nest/models/user_badge.py(1 hunks)backend/tests/apps/github/api/internal/nodes/user_test.py(3 hunks)frontend/__tests__/unit/pages/ProjectsHealthDashboardMetrics.test.tsx(1 hunks)frontend/docker/Dockerfile(1 hunks)frontend/next.config.ts(1 hunks)frontend/package.json(1 hunks)
💤 Files with no reviewable changes (3)
- backend/apps/github/models/repository_contributor.py
- .github/workflows/label-pull-requests.yaml
- backend/apps/github/api/rest/v1/member.py
🧰 Additional context used
🧠 Learnings (2)
📚 Learning: 2025-07-13T07:31:06.511Z
Learnt from: Rajgupta36
PR: OWASP/Nest#1717
File: frontend/src/components/ModuleCard.tsx:53-55
Timestamp: 2025-07-13T07:31:06.511Z
Learning: In Next.js 13+ app router, useRouter from 'next/navigation' does not provide asPath or query properties. Use useParams to extract route parameters and usePathname to get the current pathname instead.
Applied to files:
frontend/__tests__/unit/pages/ProjectsHealthDashboardMetrics.test.tsx
📚 Learning: 2025-07-11T15:57:56.648Z
Learnt from: Rajgupta36
PR: OWASP/Nest#1717
File: backend/apps/mentorship/graphql/queries/module.py:39-50
Timestamp: 2025-07-11T15:57:56.648Z
Learning: In the OWASP Nest mentorship GraphQL queries, Strawberry GraphQL automatically converts between Django Module instances and ModuleNode types, so methods can return Module instances directly without manual conversion even when typed as ModuleNode.
Applied to files:
backend/apps/nest/api/internal/nodes/badge.py
🧬 Code graph analysis (4)
backend/apps/nest/api/internal/nodes/badge.py (2)
backend/apps/nest/models/badge.py (1)
Badge(10-41)frontend/src/types/__generated__/graphql.ts (1)
Node(300-303)
backend/apps/github/api/internal/nodes/user.py (1)
backend/apps/nest/api/internal/nodes/badge.py (1)
BadgeNode(19-20)
backend/apps/nest/management/commands/nest_update_badges.py (2)
backend/apps/github/models/user.py (1)
User(19-159)backend/apps/nest/api/internal/nodes/user.py (1)
is_owasp_staff(14-16)
backend/tests/apps/github/api/internal/nodes/user_test.py (1)
backend/apps/github/api/internal/nodes/user.py (2)
UserNode(28-67)badges(32-42)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: Run frontend unit tests
- GitHub Check: Run frontend e2e tests
- GitHub Check: Run backend tests
🔇 Additional comments (8)
frontend/package.json (2)
42-42: Patch bump to apexcharts looks fine.Single patch update within the same major/minor; low risk.
42-42: Peer dependency range is compatible: [email protected] declares apexcharts ≥4.0.0, so [email protected] satisfies this range. No changes required.frontend/next.config.ts (1)
44-44: Confirmsilentsupport: withSentryConfig in @sentry/nextjs@^10.10.0 supports thesilent: booleanoption to suppress SDK/build logs.backend/apps/github/api/internal/nodes/user.py (1)
7-7: Import looks correct and scoped.backend/apps/nest/models/user_badge.py (1)
41-46: Reverse relation rename aligns with usage.
related_name="user_badges"matches query paths elsewhere.backend/apps/nest/api/internal/nodes/badge.py (1)
1-21: BadgeNode mapping is clean and minimal.backend/apps/nest/migrations/0005_alter_userbadge_user.py (1)
7-24: Migration correctly updates reverse relation to user_badges.backend/tests/apps/github/api/internal/nodes/user_test.py (1)
20-42: Schema expectations updated appropriately to include badges.
| avatar_url=tc["avatar_url"], | ||
| contributions_count=tc["contributions_count"], | ||
| id=tc["id"], | ||
| id=tc["login"], |
There was a problem hiding this comment.
Do not use login as GraphQL id (unstable; cache-colliding across contexts).
- GitHub usernames can be renamed, breaking node identity.
- Same user across different projects will share the same
idwhile carrying different fields (project_key,project_name), causing GraphQL cache corruption.
Prefer a stable backend identifier (GitHub id/node_id) or, if this node represents a “contributor membership,” compose the id with scope (e.g., project/repo) to ensure uniqueness. Temporary safe fallback keeps behavior while enabling a future model fix.
Apply this diff:
- id=tc["login"],
+ # Prefer stable unique ids; fall back to login only if unavailable.
+ id=str(tc.get("id") or tc.get("node_id") or f"{tc['login']}|{tc.get('project_key') or ''}"),Follow-up (outside this file): reintroduce a stable identifier in RepositoryContributor.get_top_contributors (include id or node_id) so the fallback can be removed.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| id=tc["login"], | |
| # Prefer stable unique ids; fall back to login+scope only if unavailable. | |
| # Prefer stable unique ids; fall back to login only if unavailable. | |
| id=str( | |
| tc.get("id") | |
| or tc.get("node_id") | |
| or f"{tc['login']}|{tc.get('project_key') or ''}" | |
| ), |
| release: { | ||
| name: process.env.RELEASE_VERSION, | ||
| }, | ||
| silent: isLocal, | ||
| telemetry: false, |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Only set Sentry release when RELEASE_VERSION is defined.
Prevents an undefined release name in CI/local.
- release: {
- name: process.env.RELEASE_VERSION,
- },
+ ...(process.env.RELEASE_VERSION
+ ? { release: { name: process.env.RELEASE_VERSION } }
+ : {}),📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| release: { | |
| name: process.env.RELEASE_VERSION, | |
| }, | |
| silent: isLocal, | |
| telemetry: false, | |
| ...(process.env.RELEASE_VERSION | |
| ? { release: { name: process.env.RELEASE_VERSION } } | |
| : {}), | |
| silent: isLocal, | |
| telemetry: false, |
🤖 Prompt for AI Agents
In frontend/next.config.ts around lines 41 to 45, the Sentry release is
unconditionally set to process.env.RELEASE_VERSION which can be undefined;
change the config so the release field is only added when
process.env.RELEASE_VERSION is defined (e.g., conditionally include the release
object or set release.name only when the env var exists) to avoid an undefined
release name in CI/local.
| ...(process.env.NEXT_SENTRY_AUTH_TOKEN | ||
| ? { | ||
| authToken: process.env.NEXT_SENTRY_AUTH_TOKEN, | ||
| // https://docs.sentry.io/platforms/javascript/guides/nextjs/sourcemaps/ | ||
| sourcemaps: { | ||
| assets: ['**/*.js', '**/*.js.map'], | ||
| deleteSourcemapsAfterUpload: true, | ||
| assets: ['./.next/**'], | ||
| deleteSourcemapsAfterUpload: false, | ||
| disable: false, | ||
| ignore: ['**/node_modules/**'], | ||
| }, | ||
| }), | ||
| } | ||
| : {}), | ||
| }) |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Narrow sourcemap upload globs and avoid keeping maps in the artifact.
Uploading ./.next/** is overly broad and keeping maps increases risk and build size. Prefer targeted assets and delete after upload; disable uploads locally or without token.
- ...(process.env.NEXT_SENTRY_AUTH_TOKEN
- ? {
- authToken: process.env.NEXT_SENTRY_AUTH_TOKEN,
- // https://docs.sentry.io/platforms/javascript/guides/nextjs/sourcemaps/
- sourcemaps: {
- assets: ['./.next/**'],
- deleteSourcemapsAfterUpload: false,
- disable: false,
- ignore: ['**/node_modules/**'],
- },
- }
- : {}),
+ ...(process.env.NEXT_SENTRY_AUTH_TOKEN
+ ? {
+ authToken: process.env.NEXT_SENTRY_AUTH_TOKEN,
+ sourcemaps: {
+ assets: ['.next/static/chunks/**', '.next/static/css/**'],
+ deleteSourcemapsAfterUpload: true,
+ disable: isLocal, // skip uploads locally
+ ignore: ['**/node_modules/**'],
+ },
+ }
+ : { sourcemaps: { disable: true } }),Additionally, consider aligning Next’s public sourcemaps with Sentry usage:
// outside this hunk, recommend:
productionBrowserSourceMaps: isLocal🤖 Prompt for AI Agents
In frontend/next.config.ts around lines 47 to 59, the current sourcemaps upload
uses the overly broad './.next/**' glob and keeps maps in the build artifact;
narrow the upload globs to only the required client JS assets (e.g.
.next/static/chunks/** and .next/static/media/** or equivalent bundles), set
deleteSourcemapsAfterUpload to true so sourcemaps are removed from the artifact,
ensure uploads are disabled when running locally or when NEXT_SENTRY_AUTH_TOKEN
is not present (already gated by the token but also add an explicit disable flag
for local/dev), and align Next config by setting productionBrowserSourceMaps
appropriately (e.g. tied to isLocal or a env flag) so public browser sourcemaps
behavior matches Sentry uploads.
arkid15r
merged commit 5c940d6
into
OWASP:feature/nestbot-calendar-events
3 participants
Proposed change
Resolves #(put the issue number here)
Add the PR description here.
Checklist
make check-testlocally; all checks and tests passed.