Feature : add related slack channels to chapters and projects #2865
Conversation
Summary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings. WalkthroughAdds GenericRelation fields to Project and Chapter models, exposes a new Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes
Suggested reviewers
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (2)
⏰ 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). (6)
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: 0
🧹 Nitpick comments (5)
frontend/src/components/CardDetailsPage.tsx (1)
120-148: ExtendingSocialLinkstoprojecttype is aligned and guardedThe condition now including
type === 'project'reuses the existing SocialLinks rendering path for chapters/committees and is still safely guarded bysocialLinkstruthiness, withSocialLinksitself no-op’ing on an empty array. This is a straightforward and correct extension of the existing behavior.If you touch this area again, consider replacing the repeated
type === ...checks with an['chapter', 'committee', 'project'].includes(type)style predicate for slightly clearer intent.backend/tests/apps/owasp/models/project_test.py (1)
4-9: Good coverage ofsocial_channelsGenericRelationThe new
test_social_channels_accurately verifies the presence and configuration of thesocial_channelsGenericRelation onProject, including model type and content-type/object-id field names, which should catch regressions in the relation wiring.You might rename the test to something like
test_social_channels_field_configuration(dropping the trailing underscore) to better reflect its purpose and match existing naming style.Also applies to: 136-143
frontend/__tests__/unit/components/CardDetailsPage.test.tsx (1)
675-695: New test for project social links covers the added behaviorThe
renders social links for project type when socialLinks are providedtest accurately asserts that the Social Links heading appears and that three links are rendered for a project with three URLs, mirroring the existing chapter/committee tests and giving good regression coverage.Because you assert on
getAllByRole('link'), this test may become brittle if additional unrelated links are added to the card; using a more specific container or test id around the Social Links section could make it more resilient.backend/apps/owasp/api/internal/nodes/project.py (1)
114-127:social_urlsresolver is functionally correct for Slack channelsThe resolver walks active
social_channels, filters for Slack entries with a non-emptyslack_channel_id, and returns well-formedapp_redirectURLs, which matches the requirement to expose related Slack channels; given it’s used for a single-project query, the simple loop is acceptable from a performance standpoint.If there’s a chance of duplicate
EntityChannelrecords pointing to the same Slack channel, you may want to de-duplicateurls(e.g., via aset) or enforce uniqueness at the DB layer to avoid repeated badges in the UI.backend/apps/owasp/models/chapter.py (1)
71-77: Consider aligning field naming with Project model for consistency.The implementation is correct, but there's a naming inconsistency:
Projectusessocial_channelswhileChapteruseschannels. Since both serve the same purpose (linking toEntityChannelfor social/Slack channels), consistent naming would improve code maintainability and reduce cognitive load when working across both models.If you agree, consider renaming to match:
- # related channels from EntityChannel - channels = GenericRelation( + # related channels from EntityChannel + social_channels = GenericRelation( EntityChannel, content_type_field="entity_type", object_id_field="entity_id", - related_query_name="chapter", + related_query_name="chapter", )Note: If you choose to rename, ensure any existing usages of
chapter.channels(e.g., in API resolvers or templates) are updated accordingly.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (2)
frontend/src/types/__generated__/graphql.tsis excluded by!**/__generated__/**frontend/src/types/__generated__/projectQueries.generated.tsis excluded by!**/__generated__/**
📒 Files selected for processing (10)
backend/apps/owasp/api/internal/nodes/project.py(1 hunks)backend/apps/owasp/models/chapter.py(2 hunks)backend/apps/owasp/models/project.py(2 hunks)backend/tests/apps/owasp/api/internal/nodes/project_test.py(2 hunks)backend/tests/apps/owasp/models/project_test.py(2 hunks)frontend/__tests__/unit/components/CardDetailsPage.test.tsx(1 hunks)frontend/src/app/projects/[projectKey]/page.tsx(1 hunks)frontend/src/components/CardDetailsPage.tsx(1 hunks)frontend/src/server/queries/projectQueries.ts(1 hunks)frontend/src/types/project.ts(1 hunks)
🧰 Additional context used
🧠 Learnings (7)
📚 Learning: 2025-07-12T17:36:57.255Z
Learnt from: Rajgupta36
Repo: OWASP/Nest PR: 1717
File: frontend/__tests__/unit/pages/createProgram.test.tsx:70-86
Timestamp: 2025-07-12T17:36:57.255Z
Learning: When testing React page components that use mocked form components, validation logic should be tested at the form component level, not the page level. Page-level tests should focus on authentication, role checking, submission handling, and navigation logic.
Applied to files:
frontend/__tests__/unit/components/CardDetailsPage.test.tsx
📚 Learning: 2025-06-20T16:12:59.256Z
Learnt from: ahmedxgouda
Repo: OWASP/Nest PR: 1633
File: frontend/src/components/HealthMetrics.tsx:30-30
Timestamp: 2025-06-20T16:12:59.256Z
Learning: In the DetailsCard component (frontend/src/components/CardDetailsPage.tsx), there's a length check before rendering HealthMetrics: `healthMetricsData.length > 0`. This ensures that when HealthMetrics is rendered, the data array has at least one element, making accessing data[0] safe within the HealthMetrics component.
Applied to files:
frontend/src/components/CardDetailsPage.tsx
📚 Learning: 2025-06-20T16:12:59.256Z
Learnt from: ahmedxgouda
Repo: OWASP/Nest PR: 1633
File: frontend/src/components/HealthMetrics.tsx:30-30
Timestamp: 2025-06-20T16:12:59.256Z
Learning: In the DetailsCard component (frontend/src/components/CardDetailsPage.tsx), there's a safety check that ensures HealthMetrics component is only rendered when healthMetricsData exists and has at least one element: `healthMetricsData && healthMetricsData.length > 0`. This makes accessing data[0] safe within the HealthMetrics component.
Applied to files:
frontend/src/components/CardDetailsPage.tsx
📚 Learning: 2025-08-31T13:47:15.861Z
Learnt from: rudransh-shrivastava
Repo: OWASP/Nest PR: 2155
File: frontend/src/server/queries/programsQueries.ts:81-81
Timestamp: 2025-08-31T13:47:15.861Z
Learning: In frontend/src/server/queries/programsQueries.ts, GET_PROGRAM_DETAILS is actively used in frontend/src/app/my/mentorship/programs/[programKey]/edit/page.tsx for program editing functionality and cannot be removed. It serves a different purpose than GET_PROGRAM_ADMIN_DETAILS, providing comprehensive program information needed for editing.
Applied to files:
frontend/src/server/queries/projectQueries.ts
📚 Learning: 2025-08-31T13:47:15.861Z
Learnt from: rudransh-shrivastava
Repo: OWASP/Nest PR: 2155
File: frontend/src/server/queries/programsQueries.ts:81-81
Timestamp: 2025-08-31T13:47:15.861Z
Learning: In frontend/src/server/queries/programsQueries.ts, GET_PROGRAM_DETAILS and GET_PROGRAM_ADMIN_DETAILS are two separate queries serving different purposes: GET_PROGRAM_DETAILS fetches comprehensive program information while GET_PROGRAM_ADMIN_DETAILS fetches only admin-related details. These queries cannot be removed or merged as they serve different use cases in the application.
Applied to files:
frontend/src/server/queries/projectQueries.ts
📚 Learning: 2025-08-31T13:47:15.861Z
Learnt from: rudransh-shrivastava
Repo: OWASP/Nest PR: 2155
File: frontend/src/server/queries/programsQueries.ts:81-81
Timestamp: 2025-08-31T13:47:15.861Z
Learning: In frontend/src/server/queries/programsQueries.ts, GET_PROGRAM_DETAILS and GET_PROGRAM_ADMIN_DETAILS are two separate queries serving different purposes: GET_PROGRAM_DETAILS fetches comprehensive program information while GET_PROGRAM_ADMIN_DETAILS fetches only admin-related details.
Applied to files:
frontend/src/server/queries/projectQueries.ts
📚 Learning: 2025-09-10T03:14:06.506Z
Learnt from: arkid15r
Repo: OWASP/Nest PR: 1995
File: backend/apps/github/models/issue.py:61-65
Timestamp: 2025-09-10T03:14:06.506Z
Learning: When refactoring from ManyToManyField to GenericForeignKey + GenericRelation in Django, all standard relationship operations (.all(), .add(), .select_related(), .order_by()) continue to work unchanged. GenericRelation automatically handles setting content_type and object_id when using .add().
Applied to files:
backend/apps/owasp/models/project.py
🧬 Code graph analysis (4)
backend/apps/owasp/models/chapter.py (1)
backend/apps/owasp/models/entity_channel.py (1)
EntityChannel(8-66)
frontend/src/app/projects/[projectKey]/page.tsx (1)
backend/apps/github/models/repository.py (1)
project(162-164)
backend/tests/apps/owasp/models/project_test.py (1)
backend/apps/owasp/models/entity_channel.py (1)
EntityChannel(8-66)
backend/apps/owasp/models/project.py (1)
backend/apps/owasp/models/entity_channel.py (1)
EntityChannel(8-66)
🔇 Additional comments (7)
frontend/src/app/projects/[projectKey]/page.tsx (1)
91-111: PropagatingsocialUrlsintoDetailsCardlooks correctPassing
socialLinks={project.socialUrls}cleanly wires the new GraphQL field into the shared details component, and the downstream component already guards rendering on presence/length of the array, so this should be safe even when the field is null/undefined or empty.frontend/src/types/project.ts (1)
18-48:socialUrlstype addition is consistent with usageAdding
socialUrls?: string[]toProjectmatches the GraphQL selection andDetailsCardprops, and keeping it optional preserves backward compatibility with callers that don’t request this field.frontend/src/server/queries/projectQueries.ts (1)
3-128: GraphQL query now correctly fetchessocialUrlsIncluding
socialUrlsinGET_PROJECT_DATAkeeps the client query in sync with the new ProjectNode field and theProjecttype, enabling the project page to render social links without affecting other queries likeGET_PROJECT_METADATA.backend/tests/apps/owasp/api/internal/nodes/project_test.py (1)
16-41: ProjectNode test suite correctly extended forsocial_urlsAdding
"social_urls"toexpected_field_namesand the dedicatedtest_resolve_social_urlsthat checks the type aslist[str]keeps the public API contract ofProjectNodewell-specified and ensures the new field can’t silently change or disappear.Also applies to: 108-111
backend/apps/owasp/models/project.py (2)
22-22: LGTM!The import is correctly placed alongside other model imports.
130-136: LGTM!The
GenericRelationsetup is correct and follows Django conventions. The field configuration properly maps toEntityChannel'sentity_typeandentity_idfields, and therelated_query_name="project"enables efficient reverse lookups. Based on learnings, this pattern supports all standard relationship operations.backend/apps/owasp/models/chapter.py (1)
17-17: LGTM!The import is correctly placed with other model imports.
|
@ahmedxgouda should I keep working on this ? As you have assigned this to yourself |
@ishanBahuguna yes. Ahmed is an assignee for the code review. |
|
ahmedxgouda
left a comment
ahmedxgouda
left a comment
There was a problem hiding this comment.
Thanks for working in this. You didn't update the chapter graphql resolvers with the new EntityChannel field data. Also, please look into the bellow comments.
| for ec in self.social_channels.filter(is_active=True): | ||
| channel = ec.channel | ||
| if ( | ||
| ec.platform == "slack" |
There was a problem hiding this comment.
| for ec in self.social_channels.filter(is_active=True): | |
| channel = ec.channel | |
| if ( | |
| ec.platform == "slack" | |
| for entity_channel in self.social_channels.filter(is_active=True): | |
| channel = entity_channel.channel | |
| if ( | |
| entity_channel.platform == "slack" |
| ) | ||
|
|
||
| # related channels from EntityChannel | ||
| social_channels = GenericRelation( |
There was a problem hiding this comment.
Why this named social_channels and it is named in Chapter model: channels?
There was a problem hiding this comment.
@ahmedxgouda I forget to remove it from chapter page.
I'm am a bit confused regarding updating the chapter graphql resolver , the chapter page already shows the related communication channel on frontend . So do I still need to update the resolver?
There was a problem hiding this comment.
It seems we will migrate to using EntityChannel field instead of urls JSON field. At least that what I understood from the issue description. Maybe @arkid15r could confirm that.
3 participants
Proposed change
Resolves #2624
Backend:
Projectmodel to support Slack channel URLs using EntityChannel.Frontend:
While working on the issue I found that the
Chapterpage is already displaying the slack channels on frontendChecklist
make check-testlocally and all tests passed