Added sponsors sorting based on their type #1031
Conversation
Summary by CodeRabbit
WalkthroughThis pull request introduces the Changes
Possibly related PRs
Suggested labels
Suggested reviewers
✨ Finishing Touches
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
Documentation and Community
|
![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 (1)
frontend/src/components/LogoCarousel.tsx (1)
18-22: Consider making the sponsor type prioritization more resilient.The current implementation assumes that sponsor types will always be uppercase and exactly match the keys in the priority object. This might cause issues if the data format changes.
Consider making the sorting more resilient by:
- const priority = { DIAMOND: 1, PLATINUM: 2, GOLD: 3, SILVER: 4, SUPPORTER: 5, NOT_A_SPONSOR: 6 } + const priority = { + DIAMOND: 1, + PLATINUM: 2, + GOLD: 3, + SILVER: 4, + SUPPORTER: 5, + NOT_A_SPONSOR: 6, + } + + const getSponsorPriority = (type: string) => { + const upperType = type?.toUpperCase(); + return priority[upperType] || 999; // Default to lowest priority if type not found + } + + const sortedSponsors = [...sponsors].sort( + (a, b) => getSponsorPriority(a.sponsorType) - getSponsorPriority(b.sponsorType) + )Also, please verify if "NOT_A_SPONSOR" is an actual value that will be used in the system or if it can be removed.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
backend/apps/owasp/graphql/nodes/sponsor.py(1 hunks)frontend/src/api/queries/homeQueries.ts(1 hunks)frontend/src/components/LogoCarousel.tsx(1 hunks)frontend/src/types/home.ts(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (5)
- GitHub Check: CodeQL (python)
- GitHub Check: CodeQL (javascript-typescript)
- GitHub Check: Run frontend e2e tests
- GitHub Check: Run backend tests
- GitHub Check: Run frontend unit tests
🔇 Additional comments (5)
backend/apps/owasp/graphql/nodes/sponsor.py (1)
15-15: LGTM! The addition of the sponsor_type field enhances the GraphQL node.The addition of the "sponsor_type" field to the SponsorNode's Meta class fields tuple is a good enhancement that makes this attribute available via GraphQL. This change is necessary to support the new sorting functionality implemented in the frontend.
frontend/src/types/home.ts (1)
38-38: LGTM! The TypeScript type definition is consistent with the backend changes.The addition of the
sponsorTypeproperty to theSponsorTypeinterface provides type safety for the frontend when working with the new field. The naming follows frontend conventions (camelCase) and correctly corresponds to the snake_case field added in the backend.frontend/src/api/queries/homeQueries.ts (1)
56-56: LGTM! GraphQL query updated to request the new field.The addition of the
sponsorTypefield to the sponsors section of the GraphQL query allows the frontend to retrieve this data from the backend. This change aligns well with the backend and TypeScript type changes.frontend/src/components/LogoCarousel.tsx (2)
24-59: LGTM! Carousel implementation now uses sorted sponsors.The implementation correctly creates a copy of the sponsors array before sorting it and uses the sorted array in the render logic. This ensures sponsors are displayed according to their priority.
60-75: LGTM! Informative section adds context and clear call-to-action.The addition of an informational section with descriptive text and a "Become a sponsor" link provides good context for users and a clear call-to-action for potential sponsors.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
backend/apps/owasp/graphql/queries/sponsor.py (1)
17-25: Good implementation of priority-based sorting for sponsors.The implementation correctly sorts sponsors based on their type using a priority mapping dictionary, which is a clean approach. The diamond sponsors get the highest priority (lowest number), followed by platinum, gold, silver, etc.
However, there's a potential issue: if a sponsor has a
sponsor_typethat's not in thepriority_orderdictionary, aKeyErrorwill be raised. Consider handling this edge case:- return sorted(Sponsor.objects.all(), key=lambda x: priority_order[x.sponsor_type]) + return sorted(Sponsor.objects.all(), key=lambda x: priority_order.get(x.sponsor_type, 999)) # Use high number for unknown typesThis would place any sponsors with unknown types at the end of the list instead of crashing.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
backend/apps/owasp/graphql/queries/sponsor.py(1 hunks)frontend/src/components/LogoCarousel.tsx(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- frontend/src/components/LogoCarousel.tsx
⏰ Context from checks skipped due to timeout of 90000ms (5)
- GitHub Check: Run backend tests
- GitHub Check: CodeQL (python)
- GitHub Check: Run frontend unit tests
- GitHub Check: CodeQL (javascript-typescript)
- GitHub Check: Run frontend e2e tests
arkid15r
changed the title
|
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
frontend/__tests__/e2e/pages/Contribute.spec.ts (1)
20-20: Consider making time-based tests more resilient.The test has been updated from expecting "2 months ago" to "3 months ago". While this fixes the test in the short term, time-based tests can become fragile as they'll need updates whenever the mock data's timestamp changes.
Consider one of these approaches for more resilient tests:
- Use a regex or partial match instead of exact text
- Mock the date in your tests to ensure consistent behavior
- Test the format pattern rather than the specific output value
- await expect(page.getByText('3 months ago')).toBeVisible() + await expect(page.getByText(/\d+ months? ago/)).toBeVisible()
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
frontend/__tests__/e2e/pages/Contribute.spec.ts(1 hunks)frontend/src/components/LogoCarousel.tsx(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- frontend/src/components/LogoCarousel.tsx
⏰ Context from checks skipped due to timeout of 90000ms (5)
- GitHub Check: CodeQL (python)
- GitHub Check: CodeQL (javascript-typescript)
- GitHub Check: Run frontend e2e tests
- GitHub Check: Run frontend unit tests
- GitHub Check: Run backend tests
🔇 Additional comments (1)
frontend/__tests__/e2e/pages/Contribute.spec.ts (1)
1-69:❓ Verification inconclusive
Note: Test update seems unrelated to PR objective.
This test update appears to be unrelated to the main PR objective of "Added sorting based on types" and adding a
sponsorTypefield. It's merely adjusting the expected text in a time-based test.Make sure this change is intentional and verify whether:
- The mock data in
mockContributeDatawas updated to change the timestamp- The method for calculating relative times was modified
- This change is necessary to accommodate the sponsor type sorting functionality
🏁 Script executed:
#!/bin/bash # Check if mockContributeData was modified in this PR git diff --name-only HEAD^ HEAD | grep -q "mockContributeData" && echo "mockContributeData was modified" || echo "mockContributeData was not modified" # Look for the mock data definition to understand how timestamp is set cd frontend && grep -r "mockContributeData" --include="*.ts" . | grep -v "import"Length of output: 574
Action Required: Verify Time Display Update in Contribute Page Tests
The current test update changes the expected relative time text ("3 months ago") without any modification to
mockContributeData. Please confirm the following:
- Relative Time Calculation: Has the method for calculating relative times been updated? If so, please ensure the test reflects the new logic.
- Intentional Update for Sponsor Sorting: Is this expected text update an intentional side-effect of the sponsor type sorting changes, or was it introduced inadvertently?
* Added sorting based on types * shifted sorting logic in the backend ' * Update code --------- Co-authored-by: Arkadii Yakovets <[email protected]>
2 participants
Resolves #1012
Add the PR description here.