feat: add backend endpoint for sitemap generation #1740
Conversation
Signed-off-by: bandhan-majumder <[email protected]>
Signed-off-by: bandhan-majumder <[email protected]>
Summary by CodeRabbit
Summary by CodeRabbit
WalkthroughThis change migrates sitemap generation from a management command to Django backend endpoints. It introduces a new Changes
Assessment against linked issues
Assessment against linked issues: Out-of-scope changes
Possibly related PRs
📜 Recent review detailsConfiguration used: .coderabbit.yaml 📒 Files selected for processing (4)
🚧 Files skipped from review as they are similar to previous changes (4)
⏰ 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). (4)
✨ 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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. 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
|
Signed-off-by: bandhan-majumder <[email protected]>
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (4)
backend/apps/core/urls.py (1)
22-49: Consider optimizing individual sitemap endpoints.The URL patterns are well-structured, but the individual sitemap endpoints recreate sitemap class instances instead of reusing the existing
sitemapsdictionary.path( "sitemap/static.xml", - cached_sitemap_view(sitemaps={"static": StaticSitemap}), + cached_sitemap_view(sitemaps={"static": sitemaps["static"]}), name="sitemap-static", ), path( "sitemap/projects.xml", - cached_sitemap_view(sitemaps={"projects": ProjectSitemap}), + cached_sitemap_view(sitemaps={"projects": sitemaps["projects"]}), name="sitemap-projects", ), path( "sitemap/chapters.xml", - cached_sitemap_view(sitemaps={"chapters": ChapterSitemap}), + cached_sitemap_view(sitemaps={"chapters": sitemaps["chapters"]}), name="sitemap-chapters", ), path( "sitemap/committees.xml", - cached_sitemap_view(sitemaps={"committees": CommitteeSitemap}), + cached_sitemap_view(sitemaps={"committees": sitemaps["committees"]}), name="sitemap-committees", ), path( "sitemap/users.xml", - cached_sitemap_view(sitemaps={"users": UserSitemap}), + cached_sitemap_view(sitemaps={"users": sitemaps["users"]}), name="sitemap-users", ),This would ensure consistency and reduce redundancy by reusing the centralized sitemap definitions.
backend/apps/core/sitemaps.py (3)
98-100: Optimize database filtering with model-level method.The current approach filters in Python after fetching all active records. Consider implementing this logic at the model level for better performance.
def items(self): """Return list of active and indexable chapters for sitemap generation.""" - return [ - c for c in Chapter.objects.filter(is_active=True) if getattr(c, "is_indexable", True) - ] + # Consider adding this as a model manager or queryset method + chapters = Chapter.objects.filter(is_active=True) + if hasattr(Chapter, 'is_indexable'): + chapters = chapters.filter(is_indexable=True) + return list(chapters)
124-126: Apply same optimization suggestion for Committee filtering.The same performance optimization applies here as mentioned for ChapterSitemap.
80-82: Consider varying change frequencies based on content type.All dynamic sitemaps use "weekly" as the change frequency. Consider using different frequencies based on how often each content type actually changes (e.g., projects might change more frequently than committees).
Also applies to: 106-108, 132-134, 156-158
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
backend/.env.example(1 hunks)backend/apps/core/sitemaps.py(1 hunks)backend/apps/core/urls.py(1 hunks)backend/settings/base.py(1 hunks)backend/settings/urls.py(2 hunks)backend/templates/sitemap.xml(1 hunks)
🧰 Additional context used
🧠 Learnings (1)
backend/settings/urls.py (1)
Learnt from: Rajgupta36
PR: OWASP/Nest#1717
File: backend/apps/mentorship/graphql/queries/module.py:39-50
Timestamp: 2025-07-11T15:57:56.607Z
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.
🧬 Code Graph Analysis (2)
backend/apps/core/urls.py (1)
backend/apps/core/sitemaps.py (6)
ChapterSitemap(93-116)CommitteeSitemap(119-142)ProjectSitemap(69-90)StaticSitemap(27-58)UserSitemap(145-166)cached_sitemap_view(170-177)
backend/apps/core/sitemaps.py (3)
backend/apps/github/graphql/queries/user.py (1)
user(40-56)backend/apps/owasp/graphql/queries/chapter.py (1)
chapter(14-19)backend/apps/owasp/graphql/queries/committee.py (1)
committee(14-28)
🔇 Additional comments (14)
backend/settings/urls.py (2)
10-10: LGTM: Proper import addition for URL inclusion.The addition of
includeto the imports follows Django conventions and is necessary for the new URL pattern.
28-28: LGTM: Correct URL pattern for core app integration.The URL pattern correctly includes the core app's URLs at the root path, which is the appropriate way to integrate sitemap endpoints into the main URL configuration.
backend/.env.example (1)
22-22: LGTM: Proper environment variable documentation.The environment variable addition is well-documented with an appropriate default value of 86400 seconds (24 hours) for sitemap caching, and follows the established naming convention.
backend/templates/sitemap.xml (1)
1-12: LGTM: Well-structured sitemap template following protocol standards.The template correctly implements the sitemap protocol with:
- Proper XML declaration and namespace
- Conditional rendering for optional elements (lastmod, changefreq, priority)
- Appropriate URL escaping for security
- Clean Django template syntax
backend/apps/core/urls.py (2)
1-12: LGTM: Well-structured sitemap URL configuration.The imports and module structure follow Django conventions and properly organize the sitemap functionality.
14-20: LGTM: Clean sitemap dictionary organization.The sitemaps dictionary provides a clear mapping between sitemap keys and their corresponding classes, making the configuration easy to understand and maintain.
backend/apps/core/sitemaps.py (8)
3-3: Modern Python datetime import looks good.The use of
datetime.UTCis the modern Python 3.9+ approach for timezone-aware datetime objects.
18-24: Static routes configuration is well-structured.The priority values (0.6-0.9) and change frequencies are appropriately configured for different content types.
154-154: User location attribute is correctly implemented.The use of
obj.loginfor user URLs is consistent with the GraphQL queries shown in the relevant code snippets.
170-177: Cached sitemap view implementation is well-designed.The caching implementation using Django's
cache_pagedecorator with configurable timeout is appropriate for sitemap performance optimization.
78-78: Verifiednest_keyexists on Project model
TheProjectmodel inbackend/apps/owasp/models/project.pydefines anest_keyproperty, so no changes are necessary.
130-130: Nest key exists on Committee modelConfirmed that
nest_keyis defined on theCommitteemodel inbackend/apps/owasp/models/committee.py(around line 58). No further changes are needed.• Definition:
backend/apps/owasp/models/committee.py:58–def nest_key(self): ...
104-104: Verified nest_key property on Chapter modelThe
Chaptermodel inbackend/apps/owasp/models/chapter.pydefines a@property def nest_keyat lines 70–73, confirming the attribute exists and returns the expected key:70: @property 71: def nest_key(self): 72: """Get Nest key.""" 73: return self.key.replace("www-chapter-", "")No further changes needed.
74-74: Project.is_indexable is defined via mixin – no changes needed.The
Projectmodel inherits fromProjectIndexMixin→RepositoryBasedEntityModelMixin, which provides the@property def is_indexable, so usinggetattr(p, "is_indexable", True)is safe and no missing attribute.
Signed-off-by: bandhan-majumder <[email protected]>
bandhan-majumder
changed the title
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
backend/apps/core/sitemaps.py (1)
46-60: Database query optimization still needed despite past improvements.While the refactoring with
path_to_modeldictionary addresses the previous review, the implementation still executes separate database queries for each static route during sitemap generation. This could be further optimized.Consider precomputing all lastmod values in a single operation:
def lastmod(self, item): """Return the last modification date for a static route item.""" - path_to_model = { - "/projects": Project, - "/contribute": Project, - "/chapters": Chapter, - "/committees": Committee, - "/members": User, - } - model = path_to_model.get(item["path"]) - if model: - lastmod = model.objects.aggregate(latest=Max("updated_at"))["latest"] - else: - lastmod = None + if not hasattr(self, '_cached_lastmod'): + self._cached_lastmod = { + "/projects": Project.objects.aggregate(latest=Max("updated_at"))["latest"], + "/contribute": Project.objects.aggregate(latest=Max("updated_at"))["latest"], + "/chapters": Chapter.objects.aggregate(latest=Max("updated_at"))["latest"], + "/committees": Committee.objects.aggregate(latest=Max("updated_at"))["latest"], + "/members": User.objects.aggregate(latest=Max("updated_at"))["latest"], + } + lastmod = self._cached_lastmod.get(item["path"]) return lastmod or datetime.now(UTC)
🧹 Nitpick comments (2)
backend/apps/core/sitemaps.py (2)
56-59: Apply static analysis suggestion for cleaner code.The ternary operator suggestion from Ruff would improve readability.
- if model: - lastmod = model.objects.aggregate(latest=Max("updated_at"))["latest"] - else: - lastmod = None + lastmod = model.objects.aggregate(latest=Max("updated_at"))["latest"] if model else None
63-68: Helper function could be optimized with dict lookup.The linear search through STATIC_ROUTES could be optimized for better performance.
+# Create a lookup dictionary for better performance +_STATIC_PRIORITY_MAP = {route["path"]: route["priority"] for route in STATIC_ROUTES} + def get_static_priority(path): """Get the priority for a static route based on its path.""" - for route in STATIC_ROUTES: - if route["path"] == path: - return route["priority"] - return 0.7 # fallback priority + return _STATIC_PRIORITY_MAP.get(path, 0.7) # fallback priority
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
backend/apps/core/sitemaps.py(1 hunks)backend/settings/base.py(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- backend/settings/base.py
🧰 Additional context used
🧬 Code Graph Analysis (1)
backend/apps/core/sitemaps.py (4)
backend/apps/github/graphql/queries/user.py (1)
user(40-56)backend/apps/owasp/graphql/queries/chapter.py (1)
chapter(14-19)backend/apps/owasp/graphql/queries/committee.py (1)
committee(14-28)backend/apps/github/models/repository.py (1)
path(154-156)
🪛 Ruff (0.12.2)
backend/apps/core/sitemaps.py
56-59: Use ternary operator lastmod = model.objects.aggregate(latest=Max("updated_at"))["latest"] if model else None instead of if-else-block
Replace if-else-block with lastmod = model.objects.aggregate(latest=Max("updated_at"))["latest"] if model else None
(SIM108)
🔇 Additional comments (7)
backend/apps/core/sitemaps.py (7)
1-16: LGTM! Clean import structure.The imports are well-organized and include all necessary Django sitemap components and model imports.
18-24: Well-structured static route configuration.The priority values are appropriate, with projects having the highest priority (0.9) and contribute having the lowest (0.6). The change frequencies align well with expected content update patterns.
156-156: Consistent URL construction pattern.User sitemap uses
obj.logininstead ofobj.nest_keylike other models. This appears intentional based on the User model structure but creates inconsistency.
172-179: Well-implemented caching mechanism.The cached sitemap view properly uses Django's cache_page decorator with configurable timeout. This will significantly improve performance for sitemap requests.
80-80: nest_key Consistency VerifiedAll three models—Project, Chapter, and Committee—implement
nest_keyand correctly strip their respective prefixes (www-project-,www-chapter-,www-committee-). The sitemap entries at lines 80, 106, and 132 can safely useobj.nest_keyfor URL construction.
76-76: All Project models provideis_indexablevia mixin—no action needed
- The
RepositoryBasedEntityModelMixinin
backend/apps/owasp/models/mixins/common.pydefines
@property def is_indexable(self)ProjectIndexMixin(inbackend/apps/owasp/models/mixins/project.py) inherits that mixin- The
Projectmodel (inbackend/apps/owasp/models/project.py) subclassesProjectIndexMixinSince
is_indexableis always present onProject, thegetattr(..., True)fallback is purely defensive. No changes required.
92-92: All sitemap target models define bothcreated_atandupdated_atVerified that every model used in
lastmodhas the required fields:
- User (
apps/github/models/user.py) inheritscreated_at&updated_atfromGenericUserModel- Chapter & Committee (
apps/owasp/models/common.py) inherit those fields fromRepositoryBasedEntityModel- Project (
apps/owasp/models/project.py) explicitly declarescreated_at&updated_atNo changes needed.
arkid15r
left a comment
arkid15r
left a comment
There was a problem hiding this comment.
@bandhan-majumder I refactored it a bit but don't have more time to finish it.
I think it should be a separate app (Django app on github/owasp/slack level). Could you refactor it and add some tests?
Thank you!
// Reusing internal Django tools for this is lit -- I totally forgot it existed.
Signed-off-by: bandhan-majumder <[email protected]>
Signed-off-by: bandhan-majumder <[email protected]>
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (4)
backend/tests/apps/sitemaps/sitemaps_test.py (1)
173-184: Good test coverage for cached sitemap view functionality.The test verifies that the cached_sitemap_view returns a callable and that the returned view produces the expected HTTP response. The mocking approach is appropriate.
However, there's a trailing whitespace issue on line 184 that should be fixed.
- self.assertEqual(response.content, b"ok") + self.assertEqual(response.content, b"ok")Also add a newline at the end of the file.
backend/apps/sitemaps/sitemaps.py (3)
25-58: Good implementation of StaticSitemap class.The class properly implements all required sitemap methods. The path_to_model mapping approach for determining last modification dates is clever and efficient.
However, consider that the
/contributepath is mapped to theProjectmodel, which may not be semantically correct if the contribute page has different content that's not directly related to projects.Consider if the mapping for
/contribute→Projectis semantically correct, or if it should have its own logic or fallback to current datetime.
69-162: Excellent implementation of dynamic sitemap classes.All four sitemap classes follow a consistent pattern and implement the required methods correctly:
- Proper filtering for active, indexable objects
- Correct URL path generation
- Appropriate change frequency and priority settings
- Good fallback logic for last modification dates
The filtering logic matches the patterns seen in the GraphQL queries and ensures only relevant objects are included in sitemaps.
Consider optimizing the
items()methods by using database-level filtering instead of list comprehensions:def items(self): """Return list of indexable projects for sitemap generation.""" return Project.objects.filter(is_active=True, is_indexable=True)This would be more efficient for large datasets, assuming
is_indexableis a database field rather than a property.
165-173: Good caching implementation for performance optimization.The cached_sitemap_view function properly implements caching using Django's cache_page decorator. The 24-hour cache duration is appropriate for sitemap data.
Consider making the cache duration configurable through settings:
from django.conf import settings @cache_page(getattr(settings, 'SITEMAP_CACHE_TIMEOUT', 86400)) def _view(request: HttpRequest): return sitemap(request, sitemaps=sitemaps, **kwargs)This would align with the PR objective of making the cache timeout configurable via the
DJANGO_SITEMAP_CACHE_TIMEOUTenvironment variable.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
backend/apps/sitemaps/sitemaps.py(1 hunks)backend/apps/sitemaps/templates/sitemap.xml(1 hunks)backend/apps/sitemaps/urls.py(1 hunks)backend/settings/base.py(1 hunks)backend/settings/urls.py(2 hunks)backend/tests/apps/sitemaps/sitemaps_test.py(1 hunks)
✅ Files skipped from review due to trivial changes (3)
- backend/settings/base.py
- backend/apps/sitemaps/templates/sitemap.xml
- backend/apps/sitemaps/urls.py
🚧 Files skipped from review as they are similar to previous changes (1)
- backend/settings/urls.py
🧰 Additional context used
🧬 Code Graph Analysis (2)
backend/tests/apps/sitemaps/sitemaps_test.py (1)
backend/apps/sitemaps/sitemaps.py (32)
StaticSitemap(25-58)lastmod(44-58)lastmod(88-90)lastmod(112-114)lastmod(136-138)lastmod(160-162)items(28-30)items(72-74)items(96-98)items(120-122)items(144-146)location(32-34)location(76-78)location(100-102)location(124-126)location(148-150)changefreq(36-38)changefreq(80-82)changefreq(104-106)changefreq(128-130)changefreq(152-154)priority(40-42)priority(84-86)priority(108-110)priority(132-134)priority(156-158)get_static_priority(61-66)ProjectSitemap(69-90)ChapterSitemap(93-114)CommitteeSitemap(117-138)UserSitemap(141-162)cached_sitemap_view(166-173)
backend/apps/sitemaps/sitemaps.py (4)
backend/apps/github/graphql/queries/user.py (1)
user(40-56)backend/apps/owasp/graphql/queries/chapter.py (1)
chapter(14-19)backend/apps/owasp/graphql/queries/committee.py (1)
committee(14-28)backend/apps/github/models/repository.py (1)
path(154-156)
🪛 Ruff (0.12.2)
backend/tests/apps/sitemaps/sitemaps_test.py
25-25: Use a regular assert instead of unittest-style assertIsNotNone
Replace assertIsNotNone(...) with assert ...
(PT009)
28-28: Use a regular assert instead of unittest-style assertEqual
Replace assertEqual(...) with assert ...
(PT009)
32-32: Use a regular assert instead of unittest-style assertEqual
Replace assertEqual(...) with assert ...
(PT009)
36-36: Use a regular assert instead of unittest-style assertEqual
Replace assertEqual(...) with assert ...
(PT009)
40-40: Use a regular assert instead of unittest-style assertEqual
Replace assertEqual(...) with assert ...
(PT009)
44-44: Use a regular assert instead of unittest-style assertEqual
Replace assertEqual(...) with assert ...
(PT009)
45-45: Use a regular assert instead of unittest-style assertEqual
Replace assertEqual(...) with assert ...
(PT009)
47-47: Use a regular assert instead of unittest-style assertEqual
Replace assertEqual(...) with assert ...
(PT009)
51-51: Argument name Project should be lowercase
(N803)
55-55: Use a regular assert instead of unittest-style assertEqual
Replace assertEqual(...) with assert ...
(PT009)
60-60: Use a regular assert instead of unittest-style assertEqual
Replace assertEqual(...) with assert ...
(PT009)
65-65: Use a regular assert instead of unittest-style assertEqual
Replace assertEqual(...) with assert ...
(PT009)
70-70: Use a regular assert instead of unittest-style assertEqual
Replace assertEqual(...) with assert ...
(PT009)
76-76: Use a regular assert instead of unittest-style assertEqual
Replace assertEqual(...) with assert ...
(PT009)
78-78: Use a regular assert instead of unittest-style assertEqual
Replace assertEqual(...) with assert ...
(PT009)
82-82: Argument name Chapter should be lowercase
(N803)
86-86: Use a regular assert instead of unittest-style assertEqual
Replace assertEqual(...) with assert ...
(PT009)
91-91: Use a regular assert instead of unittest-style assertEqual
Replace assertEqual(...) with assert ...
(PT009)
96-96: Use a regular assert instead of unittest-style assertEqual
Replace assertEqual(...) with assert ...
(PT009)
101-101: Use a regular assert instead of unittest-style assertEqual
Replace assertEqual(...) with assert ...
(PT009)
107-107: Use a regular assert instead of unittest-style assertEqual
Replace assertEqual(...) with assert ...
(PT009)
109-109: Use a regular assert instead of unittest-style assertEqual
Replace assertEqual(...) with assert ...
(PT009)
113-113: Argument name Committee should be lowercase
(N803)
117-117: Use a regular assert instead of unittest-style assertEqual
Replace assertEqual(...) with assert ...
(PT009)
122-122: Use a regular assert instead of unittest-style assertEqual
Replace assertEqual(...) with assert ...
(PT009)
127-127: Use a regular assert instead of unittest-style assertEqual
Replace assertEqual(...) with assert ...
(PT009)
132-132: Use a regular assert instead of unittest-style assertEqual
Replace assertEqual(...) with assert ...
(PT009)
138-138: Use a regular assert instead of unittest-style assertEqual
Replace assertEqual(...) with assert ...
(PT009)
140-140: Use a regular assert instead of unittest-style assertEqual
Replace assertEqual(...) with assert ...
(PT009)
144-144: Argument name User should be lowercase
(N803)
148-148: Use a regular assert instead of unittest-style assertEqual
Replace assertEqual(...) with assert ...
(PT009)
153-153: Use a regular assert instead of unittest-style assertEqual
Replace assertEqual(...) with assert ...
(PT009)
158-158: Use a regular assert instead of unittest-style assertEqual
Replace assertEqual(...) with assert ...
(PT009)
163-163: Use a regular assert instead of unittest-style assertEqual
Replace assertEqual(...) with assert ...
(PT009)
169-169: Use a regular assert instead of unittest-style assertEqual
Replace assertEqual(...) with assert ...
(PT009)
171-171: Use a regular assert instead of unittest-style assertEqual
Replace assertEqual(...) with assert ...
(PT009)
177-177: Use a regular assert instead of unittest-style assertTrue
Replace assertTrue(...) with assert ...
(PT009)
183-183: Use a regular assert instead of unittest-style assertEqual
Replace assertEqual(...) with assert ...
(PT009)
184-184: Use a regular assert instead of unittest-style assertEqual
Replace assertEqual(...) with assert ...
(PT009)
184-184: Trailing whitespace
Remove trailing whitespace
(W291)
184-184: No newline at end of file
Add trailing newline
(W292)
🪛 GitHub Actions: Run CI/CD
backend/tests/apps/sitemaps/sitemaps_test.py
[error] 27-27: Ruff PT009: Use a regular assert instead of unittest-style assertIsNotNone.
[error] 30-30: Ruff PT009: Use a regular assert instead of unittest-style assertEqual.
[error] 34-34: Ruff PT009: Use a regular assert instead of unittest-style assertEqual.
[error] 38-38: Ruff PT009: Use a regular assert instead of unittest-style assertEqual.
[error] 42-42: Ruff PT009: Use a regular assert instead of unittest-style assertEqual.
[error] 46-49: Ruff PT009: Use a regular assert instead of unittest-style assertEqual in multiple test methods.
[error] 53-53: Ruff N803: Argument name Project should be lowercase.
[error] 57-57: Ruff PT009: Use a regular assert instead of unittest-style assertEqual.
[error] 62-72: Ruff PT009: Use a regular assert instead of unittest-style assertEqual in multiple test methods.
[error] 78-80: Ruff PT009: Use a regular assert instead of unittest-style assertEqual in multiple test methods.
[error] 84-84: Ruff N803: Argument name Chapter should be lowercase.
[error] 88-88: Ruff PT009: Use a regular assert instead of unittest-style assertEqual.
[error] 93-103: Ruff PT009: Use a regular assert instead of unittest-style assertEqual in multiple test methods.
[error] 109-111: Ruff PT009: Use a regular assert instead of unittest-style assertEqual in multiple test methods.
[error] 115-115: Ruff N803: Argument name Committee should be lowercase.
[error] 119-119: Ruff PT009: Use a regular assert instead of unittest-style assertEqual.
[error] 124-134: Ruff PT009: Use a regular assert instead of unittest-style assertEqual in multiple test methods.
[error] 140-142: Ruff PT009: Use a regular assert instead of unittest-style assertEqual in multiple test methods.
[error] 146-146: Ruff N803: Argument name User should be lowercase.
[error] 150-150: Ruff PT009: Use a regular assert instead of unittest-style assertEqual.
[error] 155-165: Ruff PT009: Use a regular assert instead of unittest-style assertEqual in multiple test methods.
[error] 171-173: Ruff PT009: Use a regular assert instead of unittest-style assertEqual in multiple test methods.
[error] 179-179: Ruff PT009: Use a regular assert instead of unittest-style assertTrue.
[error] 185-186: Ruff PT009: Use a regular assert instead of unittest-style assertEqual in multiple assertions.
[error] Ruff failed with exit code 1. 42 errors found (3 fixed, 39 remaining). Some fixes require --unsafe-fixes option.
🔇 Additional comments (7)
backend/tests/apps/sitemaps/sitemaps_test.py (4)
1-8: LGTM: Imports are well-organized and appropriate.The imports include all necessary modules for comprehensive testing of the sitemap functionality.
9-41: Comprehensive test coverage for StaticSitemap class.The test methods properly mock database aggregates and verify all sitemap methods. The approach of testing each method individually provides good coverage.
42-47: LGTM: get_static_priority function is well-tested.Tests cover both known paths and the fallback behavior for unknown paths, ensuring the function behaves correctly in all scenarios.
49-171: Excellent test coverage for all dynamic sitemap classes.The test classes follow a consistent pattern and thoroughly test all aspects of the sitemap functionality:
- Model filtering (active, indexable, non-bot for users)
- URL path generation using nest_key/login
- Change frequency and priority assignment
- Last modification date fallback logic
The mocking approach is appropriate and the test logic is sound.
backend/apps/sitemaps/sitemaps.py (3)
1-23: LGTM: Well-structured imports and static routes configuration.The imports include all necessary Django sitemap components and the static routes are properly configured with reasonable priority values and change frequencies.
61-67: LGTM: Simple and effective helper function.The function correctly retrieves priority values from static routes with an appropriate fallback value.
1-174: Excellent migration from management command to dynamic sitemap endpoints.This implementation successfully achieves the PR objectives:
- ✅ Uses Django's built-in sitemap framework
- ✅ Implements dynamic sitemap generation
- ✅ Includes caching for performance optimization
- ✅ Provides priority settings for routes
- ✅ Covers both static and dynamic content
The code is well-structured, follows Django conventions, and provides a solid foundation for sitemap generation. The replacement of the previous management command approach with dynamic endpoints is a good architectural decision.
Signed-off-by: bandhan-majumder <[email protected]>
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
backend/tests/apps/sitemaps/urls_test.py (3)
1-6: Remove unused imports.Static analysis correctly identifies unused imports that should be removed to clean up the code.
import unittest from unittest.mock import patch, MagicMock -from django.urls import reverse, resolve -from django.test import RequestFactory from apps.sitemaps import urls as sm_urls
25-27: Use unittest assertion methods for better error messages.Consider using
self.assertIn()instead of plainassertstatements for better test failure messages and consistency with unittest conventions.- for name in expected_names: - assert name in found_names + for name in expected_names: + self.assertIn(name, found_names, f"Expected URL pattern '{name}' not found in registered patterns")
28-28: Fix formatting issues.Remove trailing whitespace and add a newline at the end of the file as indicated by static analysis.
- +
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
backend/tests/apps/sitemaps/sitemaps_test.py(1 hunks)backend/tests/apps/sitemaps/urls_test.py(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- backend/tests/apps/sitemaps/sitemaps_test.py
🧰 Additional context used
🪛 Ruff (0.12.2)
backend/tests/apps/sitemaps/urls_test.py
4-4: django.urls.reverse imported but unused
Remove unused import
(F401)
4-4: django.urls.resolve imported but unused
Remove unused import
(F401)
5-5: django.test.RequestFactory imported but unused
Remove unused import: django.test.RequestFactory
(F401)
28-28: Blank line contains whitespace
Remove whitespace from blank line
(W293)
28-28: No newline at end of file
Add trailing newline
(W292)
🔇 Additional comments (3)
backend/tests/apps/sitemaps/urls_test.py (3)
8-8: Clean test class definition.The class name follows proper naming conventions and inherits from the appropriate base class.
9-16: Good mocking and module reloading strategy.The approach to patch
cached_sitemap_viewand reload the module ensures the test runs in isolation without executing actual view logic, which is appropriate for URL pattern testing.
17-24: Comprehensive coverage of expected sitemap routes.The expected names list covers all the necessary sitemap types (index, chapters, committees, projects, static, users) which aligns well with the PR objective of implementing Django's sitemap framework.
Signed-off-by: bandhan-majumder <[email protected]>
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
backend/tests/apps/sitemaps/sitemaps_test.py (1)
15-19: Patch-stack parameter order is fragile & can confuse future maintainersWith four stacked
@patchdecorators, the order of the injected mocks is reversed (bottom decorator → first argument). The current signature is correct but not obvious at a glance. A small re-ordering (or use ofpatch.multiple) would improve readability and reduce the risk of accidental argument mis-ordering during future edits.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
backend/tests/apps/sitemaps/sitemaps_test.py(1 hunks)
🧰 Additional context used
🪛 Ruff (0.12.2)
backend/tests/apps/sitemaps/sitemaps_test.py
47-47: Use a regular assert instead of unittest-style assertAlmostEqual
(PT009)
48-48: Use a regular assert instead of unittest-style assertAlmostEqual
(PT009)
51-51: Use a regular assert instead of unittest-style assertAlmostEqual
(PT009)
193-193: No newline at end of file
Add trailing newline
(W292)
Signed-off-by: bandhan-majumder <[email protected]>
|
@arkid15r |
| def test_urlpatterns_registered(self, mock_cached_sitemap_view): | ||
| dummy_view = MagicMock() | ||
| mock_cached_sitemap_view.return_value = dummy_view | ||
| import importlib |
There was a problem hiding this comment.
Update the prompt to use top-level import only.
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (8)
backend/apps/sitemap/views/member.py (1)
8-15: Consider adding standard sitemap methods for completeness.The sitemap class is missing common sitemap methods like
priority,changefreq, andlastmod. While these are optional, they provide better SEO value and consistency with other sitemap classes in the codebase.class MemberSitemap(Sitemap): """Member sitemap.""" prefix = "/members" def items(self): """Return list of members for sitemap generation.""" return [u for u in User.objects.filter(is_bot=False) if u.is_indexable] + + def lastmod(self, obj): + """Return the last modification date for a member.""" + return obj.updated_at + + def changefreq(self, obj): + """Return the change frequency for a member.""" + return 'weekly' + + def priority(self, obj): + """Return the priority score for a member.""" + return 0.7backend/apps/sitemap/views/project.py (1)
8-15: Consider adding standard sitemap methods for consistency.Like MemberSitemap, this class would benefit from standard sitemap methods (
priority,changefreq,lastmod) for better SEO value and consistency with the codebase pattern.class ProjectSitemap(Sitemap): """Project sitemap.""" prefix = "/projects" def items(self): """Return list of projects for sitemap generation.""" return [p for p in Project.objects.filter(is_active=True) if p.is_indexable] + + def lastmod(self, obj): + """Return the last modification date for a project.""" + return obj.updated_at + + def changefreq(self, obj): + """Return the change frequency for a project.""" + return 'weekly' + + def priority(self, obj): + """Return the priority score for a project.""" + return 0.9 # Projects might have higher priority than membersbackend/tests/apps/sitemap/views/member_test.py (1)
15-20: Clarify that location() method testing relies on Django's default behavior.The test calls
sitemap.location(obj)but theMemberSitemapclass doesn't define alocation()method. This test is actually verifying Django's default sitemap behavior, which callsget_absolute_url()on the object. Consider adding a comment to clarify this dependency.def test_location(self): + # Note: MemberSitemap inherits Django's default location() method + # which calls get_absolute_url() on the object obj = MagicMock() obj.get_absolute_url.return_value = "/members/qux" sitemap = MemberSitemap() assert sitemap.location(obj) == "/members/qux"backend/tests/apps/sitemap/urls_test.py (1)
29-31: Avoid accessing private attributes for more robust testing.The current implementation accesses
p.pattern._routewhich is a private attribute and could change in future Django versions. Consider using a more stable approach to extract URL patterns.- found_paths = {p.pattern._route for p in sitemap_urls.urlpatterns} + found_paths = {str(p.pattern) for p in sitemap_urls.urlpatterns} for path in expected_paths: assert path in found_pathsAlternatively, use Django's URL resolver for more robust testing:
+from django.urls import reverse +from django.test import TestCase - found_paths = {p.pattern._route for p in sitemap_urls.urlpatterns} - for path in expected_paths: - assert path in found_paths + # Test that each expected path can be resolved + for path in expected_paths: + try: + # This will raise NoReverseMatch if the URL pattern doesn't exist + resolved = reverse('django.contrib.sitemaps.views.sitemap', kwargs={'section': path.replace('.xml', '').replace('sitemap/', '').replace('sitemap', 'index')}) + assert resolved is not None + except: + # For paths that don't follow the standard pattern, check string presence + found_paths = {str(p.pattern) for p in sitemap_urls.urlpatterns} + assert path in found_pathsbackend/apps/sitemap/views/static.py (1)
39-42: Consider handling potential None values from aggregation.While unlikely, the Max aggregation could return None if no records exist in the model. Consider adding a null check for robustness.
return ( - model.objects.aggregate(latest=Max("updated_at"))["latest"] + model.objects.aggregate(latest=Max("updated_at"))["latest"] or datetime.now(UTC) if (model := path_to_model.get(item["path"])) else datetime.now(UTC) )backend/apps/sitemap/views/base.py (2)
12-18: Consider using a dictionary for O(1) static route priority lookup.The current implementation iterates through STATIC_ROUTES for each priority lookup, which is O(n). Consider creating a path-to-priority mapping for better performance when serving sitemaps with many items.
class BaseSitemap(Sitemap): """Base sitemap class.""" change_frequency = "weekly" prefix = "" STATIC_ROUTES = ( {"path": "/chapters", "changefreq": "weekly", "priority": 0.8}, {"path": "/committees", "changefreq": "monthly", "priority": 0.8}, {"path": "/contribute", "changefreq": "daily", "priority": 0.6}, {"path": "/members", "changefreq": "daily", "priority": 0.7}, {"path": "/projects", "changefreq": "weekly", "priority": 0.9}, ) + + _PRIORITY_MAP = {route["path"]: route["priority"] for route in STATIC_ROUTES} def get_static_priority(self, path): """Get the priority for a static route based on its path.""" - for route in self.STATIC_ROUTES: - if route["path"] == path: - return route["priority"] - - return 0.7 # Fallback priority. + return self._PRIORITY_MAP.get(path, 0.7)
36-38: Handle potential AttributeError for objects without timestamp attributes.The
lastmodmethod assumes objects haveupdated_atandcreated_atattributes. Consider adding error handling for objects that might not have these attributes.def lastmod(self, obj): """Return the last modification date for an object.""" - return obj.updated_at or obj.created_at + return getattr(obj, 'updated_at', None) or getattr(obj, 'created_at', None)backend/tests/apps/sitemap/views/static_test.py (1)
19-28: Consider more specific assertions for lastmod test.The test only asserts that
result is not None, but could verify that the mocked datetime is actually returned for better test confidence.def test_lastmod(self, mock_user, mock_project, mock_committee, mock_chapter, sitemap): dt = timezone.now() mock_chapter.return_value = {"latest": dt} mock_committee.return_value = {"latest": dt} mock_project.return_value = {"latest": dt} mock_user.return_value = {"latest": dt} for item in sitemap.STATIC_ROUTES: result = sitemap.lastmod(item) - assert result is not None + # For routes that map to models, should return the mocked datetime + if item["path"] in ["/chapters", "/committees", "/contribute", "/members", "/projects"]: + assert result == dt + else: + assert result is not None
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (25)
backend/apps/github/models/common.py(1 hunks)backend/apps/github/models/user.py(3 hunks)backend/apps/owasp/models/chapter.py(2 hunks)backend/apps/owasp/models/project.py(1 hunks)backend/apps/sitemap/templates/sitemap.xml(1 hunks)backend/apps/sitemap/urls.py(1 hunks)backend/apps/sitemap/views/__init__.py(1 hunks)backend/apps/sitemap/views/base.py(1 hunks)backend/apps/sitemap/views/chapter.py(1 hunks)backend/apps/sitemap/views/committee.py(1 hunks)backend/apps/sitemap/views/member.py(1 hunks)backend/apps/sitemap/views/project.py(1 hunks)backend/apps/sitemap/views/static.py(1 hunks)backend/settings/base.py(1 hunks)backend/settings/urls.py(2 hunks)backend/tests/apps/sitemap/urls_test.py(1 hunks)backend/tests/apps/sitemap/views/base_test.py(1 hunks)backend/tests/apps/sitemap/views/cached_sitemap_view_test.py(1 hunks)backend/tests/apps/sitemap/views/chapter_test.py(1 hunks)backend/tests/apps/sitemap/views/committee_test.py(1 hunks)backend/tests/apps/sitemap/views/member_test.py(1 hunks)backend/tests/apps/sitemap/views/project_test.py(1 hunks)backend/tests/apps/sitemap/views/static_test.py(1 hunks)proxy/production.conf(1 hunks)proxy/staging.conf(1 hunks)
✅ Files skipped from review due to trivial changes (8)
- proxy/production.conf
- backend/apps/github/models/common.py
- backend/apps/owasp/models/project.py
- proxy/staging.conf
- backend/apps/sitemap/views/chapter.py
- backend/apps/sitemap/urls.py
- backend/apps/sitemap/views/init.py
- backend/apps/sitemap/templates/sitemap.xml
🚧 Files skipped from review as they are similar to previous changes (2)
- backend/settings/urls.py
- backend/settings/base.py
🧰 Additional context used
🧠 Learnings (1)
backend/apps/github/models/user.py (1)
Learnt from: Rajgupta36
PR: OWASP/Nest#1717
File: backend/apps/mentorship/graphql/mutations/module.py:28-29
Timestamp: 2025-07-16T13:49:58.622Z
Learning: In the OWASP Nest mentorship system, mentors can be created with only github_user initially (without nest_user) when assigned to modules. This allows mentors to be assigned before they've signed into the system. When these users eventually sign in, the nest_user is automatically linked to the existing github_user, creating a complete mentor profile. This design provides flexibility in mentor assignment workflows.
🧬 Code Graph Analysis (8)
backend/apps/owasp/models/chapter.py (3)
backend/apps/github/models/user.py (2)
get_absolute_url(95-97)nest_url(58-60)backend/apps/owasp/models/project.py (3)
get_absolute_url(307-309)nest_url(176-178)nest_key(171-173)backend/apps/common/utils.py (1)
get_absolute_url(48-58)
backend/tests/apps/sitemap/views/project_test.py (7)
backend/apps/sitemap/views/project.py (2)
ProjectSitemap(8-15)items(13-15)backend/tests/apps/sitemap/views/chapter_test.py (2)
test_items(10-15)test_location(17-20)backend/tests/apps/sitemap/views/committee_test.py (2)
test_items(10-15)test_location(17-20)backend/tests/apps/sitemap/views/member_test.py (2)
test_items(8-13)test_location(15-20)backend/apps/sitemap/views/base.py (2)
items(32-34)location(40-42)backend/apps/sitemap/views/member.py (1)
items(13-15)backend/apps/owasp/models/project.py (1)
get_absolute_url(307-309)
backend/apps/github/models/user.py (2)
backend/apps/common/utils.py (1)
get_absolute_url(48-58)backend/apps/github/models/common.py (1)
nest_key(35-37)
backend/apps/sitemap/views/committee.py (5)
backend/apps/owasp/graphql/queries/committee.py (1)
committee(14-28)backend/apps/sitemap/views/base.py (2)
BaseSitemap(6-46)items(32-34)backend/apps/sitemap/views/member.py (1)
items(13-15)backend/apps/sitemap/views/chapter.py (1)
items(12-14)backend/apps/sitemap/views/project.py (1)
items(13-15)
backend/tests/apps/sitemap/views/member_test.py (6)
backend/tests/apps/sitemap/views/static_test.py (3)
sitemap(10-11)test_items(29-30)test_location(32-34)backend/apps/sitemap/views/member.py (2)
MemberSitemap(8-15)items(13-15)backend/tests/apps/sitemap/views/chapter_test.py (2)
test_items(10-15)test_location(17-20)backend/tests/apps/sitemap/views/committee_test.py (2)
test_items(10-15)test_location(17-20)backend/tests/apps/sitemap/views/project_test.py (2)
test_items(8-13)test_location(15-20)backend/apps/github/models/user.py (1)
get_absolute_url(95-97)
backend/apps/sitemap/views/member.py (1)
backend/apps/github/models/user.py (1)
User(19-149)
backend/apps/sitemap/views/static.py (4)
backend/apps/github/models/user.py (1)
User(19-149)backend/apps/owasp/models/chapter.py (1)
Chapter(20-212)backend/apps/owasp/models/project.py (1)
Project(26-358)backend/apps/sitemap/views/base.py (6)
BaseSitemap(6-46)changefreq(28-30)location(40-42)items(32-34)lastmod(36-38)priority(44-46)
backend/tests/apps/sitemap/views/static_test.py (5)
backend/apps/sitemap/views/static.py (6)
StaticSitemap(14-47)lastmod(29-43)items(25-27)location(21-23)changefreq(17-19)priority(45-47)backend/apps/sitemap/views/committee.py (1)
items(12-14)backend/apps/sitemap/views/member.py (1)
items(13-15)backend/apps/sitemap/views/chapter.py (1)
items(12-14)backend/apps/sitemap/views/project.py (1)
items(13-15)
⏰ 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). (4)
- GitHub Check: Run frontend unit tests
- GitHub Check: Run frontend e2e tests
- GitHub Check: Run backend tests
- GitHub Check: CodeQL (javascript-typescript)
🔇 Additional comments (13)
backend/apps/owasp/models/chapter.py (1)
13-13: LGTM! Clean implementation following established patterns.The addition of the
nest_urlproperty is consistent with the same pattern implemented in theProjectandUsermodels, properly utilizing the sharedget_absolute_urlutility function and the existingnest_keyproperty.Also applies to: 74-78
backend/apps/github/models/user.py (1)
8-8: LGTM! Consistent implementation across models.The addition of both the
nest_urlproperty andget_absolute_urlmethod follows the same pattern implemented in theProjectmodel. Theget_absolute_urlmethod provides a standard interface for URL retrieval while thenest_urlproperty handles the actual URL construction using the shared utility function.Also applies to: 57-61, 95-98
backend/apps/sitemap/views/committee.py (1)
7-14: LGTM! Follows established sitemap patterns.The
CommitteeSitemapimplementation is consistent with other sitemap classes (ChapterSitemap,MemberSitemap,ProjectSitemap), correctly filtering active committees that are indexable and inheriting common functionality fromBaseSitemap.backend/tests/apps/sitemap/views/cached_sitemap_view_test.py (1)
10-24: LGTM! Well-structured test with proper mocking.The test effectively verifies the
cached_sitemap_viewfunctionality by:
- Confirming it returns a callable view
- Simulating HTTP requests using
RequestFactory- Properly mocking internal dependencies to isolate the test
- Asserting both status code and response content
backend/tests/apps/sitemap/views/project_test.py (2)
7-13: LGTM! Proper test implementation with effective mocking.The test correctly mocks the
Projectmodel's filter method and verifies that only indexable objects are returned by theitems()method, following the same pattern as other sitemap test classes.
15-20: No changes needed: ProjectSitemap location is correctThe
ProjectSitemapclass inherits directly from Django’s built-inSitemap, whose defaultlocation()returnsobj.get_absolute_url(). The test correctly verifies that behavior, and the difference fromBaseSitemap-based classes (which useobj.nest_key) is intentional.• File: backend/apps/sitemap/views/project.py
–class ProjectSitemap(Sitemap):
– No override oflocation(), so it usesget_absolute_url()Test for
location()is valid as written.backend/tests/apps/sitemap/urls_test.py (1)
15-19: Excellent handling of module caching edge case.The module reloading logic with the explanatory comment is well-implemented and addresses a real testing challenge when patching already-imported modules.
backend/apps/sitemap/views/static.py (2)
29-43: Excellent implementation with efficient database queries.The
lastmodmethod implementation is well-designed with proper use of Django ORM aggregation and clean fallback handling. The walrus operator usage makes the code concise while maintaining readability.
31-37: Timezones are already consistent—no changes needed.All
DateTimeFields operate underUSE_TZ = TruewithTIME_ZONE = "UTC"(backend/settings/base.py), and the fallback usesdatetime.now(UTC). This ensures both database timestamps and the fallback are timezone‐aware in UTC.backend/tests/apps/sitemap/views/base_test.py (1)
1-48: Well-structured test coverage for BaseSitemap.The test suite effectively covers all core methods of the BaseSitemap class with appropriate mocking and clear assertions. The tests verify both happy path and edge cases (unknown paths, missing timestamps).
backend/tests/apps/sitemap/views/chapter_test.py (1)
1-44: Comprehensive test coverage for ChapterSitemap with proper mocking.The tests correctly verify ChapterSitemap functionality including model filtering, URL construction, and timestamp handling. The patch target
"apps.sitemap.views.chapter.Chapter"appropriately mocks the Chapter model import.backend/tests/apps/sitemap/views/committee_test.py (1)
1-41: Consistent and thorough test coverage for CommitteeSitemap.The test structure mirrors the chapter tests appropriately, with correct URL path formatting (
"/committees/baz") and proper model mocking. All core sitemap methods are well-tested.backend/tests/apps/sitemap/views/static_test.py (1)
15-28: Patch decorator order is correct — The@patchdecorators apply bottom-to-top (User → Project → Committee → Chapter) and the parametersmock_user, mock_project, mock_committee, mock_chapteralign perfectly. No changes needed.
|
Thanks you so much. I will keep the formatting pattern in mind from the next time. Also, there were changes in fetching query which makes it more optimized rather than my brute force approach. Will apply them from the next time. There are also some changes in files which I am not getting why (django is new to me), I will figure out them myself. |
Feel free to DM in Slack if you need help understanding the code. |
* feat: add backend endpoint for sitemap generation Signed-off-by: bandhan-majumder <[email protected]> * fix: docs and format Signed-off-by: bandhan-majumder <[email protected]> * remove: prev sitemap generate functions Signed-off-by: bandhan-majumder <[email protected]> * rabbit fixes Signed-off-by: bandhan-majumder <[email protected]> * Update code * refactor: sitemap files Signed-off-by: bandhan-majumder <[email protected]> * add test Signed-off-by: bandhan-majumder <[email protected]> * more test and fixes Signed-off-by: bandhan-majumder <[email protected]> * sonar fix Signed-off-by: bandhan-majumder <[email protected]> * ruff fix Signed-off-by: bandhan-majumder <[email protected]> * ruff fix * Update code * Update tests --------- Signed-off-by: bandhan-majumder <[email protected]> Co-authored-by: Arkadii Yakovets <[email protected]> Co-authored-by: Arkadii Yakovets <[email protected]>
2 participants
Resolves #1554
Add the PR description here.
Using django's build in sitemap to generate sitemap dynamically for different routes. Using a template
sitemap.xmland caching them for 24 hours with the help of an envDJANGO_SITEMAP_CACHE_TIMEOUTwhich has the default value of 24 hours. THe sitemaps have the priority listed.I have removed the previous generate_sitemap function and tests
Tests:-
