Conversation
…) each authentic source in the HTML and XML sitemaps.
📝 WalkthroughWalkthroughChangesConfigurable sitemap sources
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant SiteAdmin
participant PloneRegistry
participant SitemapViews
participant RESTEndpoints
SiteAdmin->>PloneRegistry: Save source settings
SitemapViews->>PloneRegistry: Read enabled sources and limits
SitemapViews->>RESTEndpoints: Request items with batch and sort parameters
RESTEndpoints-->>SitemapViews: Return sitemap items
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
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: 2
🧹 Nitpick comments (5)
src/imio/smartweb/core/tests/test_rest_views.py (1)
243-256: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winAssert the propagated endpoint arguments.
mock_callis only a stub, so this test verifiesview.b_sizebut not the batch size actually passed throughget_endpoint_data. A regression could leaveview.b_sizeatDEFAULT_BATCH_SIZEwhile sending the sitemap cap (50) to the endpoint. Assert the mock call arguments for the expected batch and sort values.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/imio/smartweb/core/tests/test_rest_views.py` around lines 243 - 256, The test test_seo_hidden_react_links_calls_endpoint_with_correct_arity must also verify the arguments propagated to the external endpoint. After invoking view(), inspect mock_call and assert it received view.DEFAULT_BATCH_SIZE along with the expected SEO sort field and sort order, while preserving the existing view.b_size assertion.src/imio/smartweb/core/upgrades/upgrades.py (1)
321-336: 🔒 Security & Privacy | 🔵 Trivial | ⚡ Quick winConsider adopting the admin user for this privileged registry/profile-import step.
This upgrade step deletes a registry record and re-runs a GenericSetup profile import — operations that typically need elevated permissions. As per path instructions, upgrade steps should use
api.env.adopt_user(username='admin')when elevated permissions are needed, and this function doesn't do so.🔐 Proposed fix
def add_sitemap_authentic_sources_registry(context): """...""" - registry = api.portal.get_tool("portal_registry") - if "smartweb.sitemap_authentic_sources" in registry: - del registry.records["smartweb.sitemap_authentic_sources"] - logger.info("Removed obsolete smartweb.sitemap_authentic_sources record.") - portal_setup = api.portal.get_tool("portal_setup") - portal_setup.runImportStepFromProfile(PROFILEID, "plone.app.registry") - logger.info("smartweb.sitemap_authentic_sources registry record ensured.") + with api.env.adopt_user(username="admin"): + registry = api.portal.get_tool("portal_registry") + if "smartweb.sitemap_authentic_sources" in registry: + del registry.records["smartweb.sitemap_authentic_sources"] + logger.info("Removed obsolete smartweb.sitemap_authentic_sources record.") + portal_setup = api.portal.get_tool("portal_setup") + portal_setup.runImportStepFromProfile(PROFILEID, "plone.app.registry") + logger.info("smartweb.sitemap_authentic_sources registry record ensured.")🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/imio/smartweb/core/upgrades/upgrades.py` around lines 321 - 336, Wrap the privileged registry deletion and profile import in add_sitemap_authentic_sources_registry with api.env.adopt_user(username='admin'). Keep both operations, including their logging, inside the adopted-user context so the upgrade reliably runs with elevated permissions.Source: Path instructions
src/imio/smartweb/core/browser/sitemap.py (2)
214-232: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick win
objects()(XML sitemap) lacks theif not data: continueguard thatsiteMap()(HTML sitemap) has.In
CatalogSiteMap.siteMap()(Line 263-264),format_sitemap_itemsis only invoked whendatais truthy. Here,format_sitemap_items(items, ...)runs unconditionally even whenget_endpoint_datareturns{}(empty/failed endpoint), which could emit a spurious sitemap entry for an authentic source with no items — asymmetric behavior between the XML and HTML sitemap outputs for the same underlying data. Please confirmformat_sitemap_items([], ...)doesn't add an unwanted entry, or add the same guard here for consistency.♻️ Proposed fix for symmetry with `siteMap()`
data = get_endpoint_data( obj, obj.REQUEST, source_cfg.get("max_items"), sort_on, sort_order, ) + if not data: + continue items = data.get("items", [])[: source_cfg.get("max_items")] yield from format_sitemap_items(items, obj.absolute_url())🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/imio/smartweb/core/browser/sitemap.py` around lines 214 - 232, Update CatalogSiteMap.objects around get_endpoint_data so it skips the source when data is empty before slicing items or calling format_sitemap_items, matching the guard used by siteMap(). Preserve processing and formatting for truthy endpoint responses.
33-72: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winFallback
max_items: 50duplicatesMAX_SITEMAP_ITEMSdefined incontrolpanel_siteadmin.py.If the cap is ever changed in one place, the other will silently drift out of sync.
♻️ Proposed fix to share the constant
+from imio.smartweb.core.browser.controlpanel_siteadmin import MAX_SITEMAP_ITEMS ... if rows is None: rows = [ { "source_type": t, "enabled": True, - "max_items": 50, + "max_items": MAX_SITEMAP_ITEMS, "item_filter": "most_recent", } for t in AUTHENTIC_SOURCE_TYPES ]🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/imio/smartweb/core/browser/sitemap.py` around lines 33 - 72, Update get_sitemap_sources_config so its fallback configuration reuses the existing MAX_SITEMAP_ITEMS constant from controlpanel_siteadmin.py instead of the hardcoded max_items value 50. Preserve the current fallback behavior and avoid duplicating the cap in sitemap.py.src/imio/smartweb/core/tests/test_sitemap.py (1)
218-222: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valuePrefer
next(...)over single-element list slicing (Ruff RUF015).All four sites build a list comprehension only to index
[0];next(...)avoids materializing the full list.♻️ Proposed refactor (example for one site)
- directory_entry = [ - c - for c in sitemap.siteMap().get("children") - if c.get("Title") == "directory view" - ][0] + directory_entry = next( + c + for c in sitemap.siteMap().get("children") + if c.get("Title") == "directory view" + )Apply the same pattern to the other three flagged sites.
Also applies to: 228-232, 337-341, 383-387
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/imio/smartweb/core/tests/test_sitemap.py` around lines 218 - 222, Replace the single-element list comprehensions indexed with [0] in the sitemap tests with next(...) over the corresponding child iterators, preserving the existing Title == "Folder" filtering and selected entry behavior. Apply this consistently to folder_entry at all four flagged sites.Source: Linters/SAST tools
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/imio/smartweb/core/browser/controlpanel_siteadmin.py`:
- Around line 273-288: Update the sitemap validation in applyChanges so missing
source_type values are rejected through the existing status-message path without
passing None and strings to sorted(). Normalize or otherwise compare
source_types safely, while preserving the requirement that each
SITEMAP_SOURCE_VOCABULARY.by_value entry appears exactly once.
In `@src/imio/smartweb/core/tests/test_rest_views.py`:
- Around line 249-256: Clear the reused request’s batching parameters between
tests so state from test_seo_hidden_react_links_batching cannot affect
test_seo_html. Update the shared setUp request initialization, or add equivalent
cleanup, to remove/reset b_start and b_size before each test while preserving
the SEO view’s DEFAULT_BATCH_SIZE assertion.
---
Nitpick comments:
In `@src/imio/smartweb/core/browser/sitemap.py`:
- Around line 214-232: Update CatalogSiteMap.objects around get_endpoint_data so
it skips the source when data is empty before slicing items or calling
format_sitemap_items, matching the guard used by siteMap(). Preserve processing
and formatting for truthy endpoint responses.
- Around line 33-72: Update get_sitemap_sources_config so its fallback
configuration reuses the existing MAX_SITEMAP_ITEMS constant from
controlpanel_siteadmin.py instead of the hardcoded max_items value 50. Preserve
the current fallback behavior and avoid duplicating the cap in sitemap.py.
In `@src/imio/smartweb/core/tests/test_rest_views.py`:
- Around line 243-256: The test
test_seo_hidden_react_links_calls_endpoint_with_correct_arity must also verify
the arguments propagated to the external endpoint. After invoking view(),
inspect mock_call and assert it received view.DEFAULT_BATCH_SIZE along with the
expected SEO sort field and sort order, while preserving the existing
view.b_size assertion.
In `@src/imio/smartweb/core/tests/test_sitemap.py`:
- Around line 218-222: Replace the single-element list comprehensions indexed
with [0] in the sitemap tests with next(...) over the corresponding child
iterators, preserving the existing Title == "Folder" filtering and selected
entry behavior. Apply this consistently to folder_entry at all four flagged
sites.
In `@src/imio/smartweb/core/upgrades/upgrades.py`:
- Around line 321-336: Wrap the privileged registry deletion and profile import
in add_sitemap_authentic_sources_registry with
api.env.adopt_user(username='admin'). Keep both operations, including their
logging, inside the adopted-user context so the upgrade reliably runs with
elevated permissions.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 17fd9b3a-4714-4ac1-8a14-c8ed6b02acae
📒 Files selected for processing (14)
CHANGES.rstsrc/imio/smartweb/core/browser/controlpanel_siteadmin.pysrc/imio/smartweb/core/browser/sitemap.pysrc/imio/smartweb/core/contents/rest/base.pysrc/imio/smartweb/core/contents/rest/directory/endpoint.pysrc/imio/smartweb/core/contents/rest/events/endpoint.pysrc/imio/smartweb/core/contents/rest/news/endpoint.pysrc/imio/smartweb/core/contents/rest/view.pysrc/imio/smartweb/core/profiles/default/metadata.xmlsrc/imio/smartweb/core/tests/test_rest.pysrc/imio/smartweb/core/tests/test_rest_views.pysrc/imio/smartweb/core/tests/test_sitemap.pysrc/imio/smartweb/core/upgrades/configure.zcmlsrc/imio/smartweb/core/upgrades/upgrades.py
Comment on lines
273
to
+288
| def applyChanges(self, data): | ||
| # Guard: the sitemap grid must list each authentic source exactly once | ||
| # (source_type is editable to satisfy the widget, so we validate it). | ||
| sitemap_rows = data.get("sitemap_authentic_sources") | ||
| if sitemap_rows is not None: | ||
| source_types = [row.get("source_type") for row in sitemap_rows] | ||
| if sorted(source_types) != sorted(SITEMAP_SOURCE_VOCABULARY.by_value): | ||
| IStatusMessage(self.request).addStatusMessage( | ||
| _( | ||
| "The sitemap configuration must list each authentic " | ||
| "source exactly once." | ||
| ), | ||
| type="error", | ||
| ) | ||
| return False | ||
|
|
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
sorted() can crash on a None source_type instead of showing the status error.
source_type is required=False, so row.get("source_type") can legitimately be None for a row (e.g. the hidden input isn't posted). sorted(source_types) then mixes str and None, which raises TypeError: '<' not supported between instances of 'NoneType' and 'str' in Python 3 — turning the intended graceful rejection into an unhandled 500 for the site admin instead of the status message.
🛡️ Proposed fix to sort safely
sitemap_rows = data.get("sitemap_authentic_sources")
if sitemap_rows is not None:
source_types = [row.get("source_type") for row in sitemap_rows]
- if sorted(source_types) != sorted(SITEMAP_SOURCE_VOCABULARY.by_value):
+ if sorted(source_types, key=lambda v: (v is None, v)) != sorted(
+ SITEMAP_SOURCE_VOCABULARY.by_value
+ ):📝 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.
Suggested change
| def applyChanges(self, data): | |
| # Guard: the sitemap grid must list each authentic source exactly once | |
| # (source_type is editable to satisfy the widget, so we validate it). | |
| sitemap_rows = data.get("sitemap_authentic_sources") | |
| if sitemap_rows is not None: | |
| source_types = [row.get("source_type") for row in sitemap_rows] | |
| if sorted(source_types) != sorted(SITEMAP_SOURCE_VOCABULARY.by_value): | |
| IStatusMessage(self.request).addStatusMessage( | |
| _( | |
| "The sitemap configuration must list each authentic " | |
| "source exactly once." | |
| ), | |
| type="error", | |
| ) | |
| return False | |
| def applyChanges(self, data): | |
| # Guard: the sitemap grid must list each authentic source exactly once | |
| # (source_type is editable to satisfy the widget, so we validate it). | |
| sitemap_rows = data.get("sitemap_authentic_sources") | |
| if sitemap_rows is not None: | |
| source_types = [row.get("source_type") for row in sitemap_rows] | |
| if sorted(source_types, key=lambda v: (v is None, v)) != sorted( | |
| SITEMAP_SOURCE_VOCABULARY.by_value | |
| ): | |
| IStatusMessage(self.request).addStatusMessage( | |
| _( | |
| "The sitemap configuration must list each authentic " | |
| "source exactly once." | |
| ), | |
| type="error", | |
| ) | |
| return False |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/imio/smartweb/core/browser/controlpanel_siteadmin.py` around lines 273 -
288, Update the sitemap validation in applyChanges so missing source_type values
are rejected through the existing status-message path without passing None and
strings to sorted(). Normalize or otherwise compare source_types safely, while
preserving the requirement that each SITEMAP_SOURCE_VOCABULARY.by_value entry
appears exactly once.
Comment on lines
+249
to
+256
| view = queryMultiAdapter((self.directory_view, self.request), name="seo_html") | ||
| view() # must not raise TypeError | ||
| self.assertEqual(view.total, 1) | ||
| self.assertGreaterEqual(len(view.get_data), 1) | ||
| # seo_html must keep its own (larger) batch size, NOT the sitemap | ||
| # control-panel max_items cap (50) — otherwise SEO discovery of the | ||
| # long tail via /seo_html would be truncated. | ||
| self.assertEqual(view.b_size, view.DEFAULT_BATCH_SIZE) |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Locate the test class and related fixtures/helpers.
rg -n "class TestSeoHiddenReactLinks|b_size|b_start|seo_html|DEFAULT_BATCH_SIZE|queryMultiAdapter|layer\\[\"request\"\\]" src/imio/smartweb/core/tests/test_rest_views.py src/imio/smartweb/core/tests -S
printf '\n--- file outline ---\n'
ast-grep outline src/imio/smartweb/core/tests/test_rest_views.py --view expanded
printf '\n--- surrounding lines ---\n'
sed -n '1,340p' src/imio/smartweb/core/tests/test_rest_views.pyRepository: IMIO/imio.smartweb.core
Length of output: 50379
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect base test case and layer setup for request reuse/reset behavior.
rg -n "ImioSmartwebTestCase|FUNCTIONAL_TESTING|ACCEPTANCE_TESTING|INTEGRATION_TESTING|request\.form|layer\[\"request\"\]|setUp\\(|tearDown\\(" src/imio/smartweb/core/tests src/imio/smartweb/core -S
# Find the base test case definition and test layer helpers.
fd -a ".*test.*py$" src/imio/smartweb/core | sed -n '1,120p'Repository: IMIO/imio.smartweb.core
Length of output: 50379
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect base test case and layer setup for request reuse/reset behavior.
rg -n "ImioSmartwebTestCase|FUNCTIONAL_TESTING|ACCEPTANCE_TESTING|INTEGRATION_TESTING|request\\.form|layer\\[\"request\"\\]|setUp\\(|tearDown\\(" src/imio/smartweb/core/tests src/imio/smartweb/core -S
# Find the base test case definition and test layer helpers.
fd -a ".*test.*py$" src/imio/smartweb/core | sed -n '1,120p'Repository: IMIO/imio.smartweb.core
Length of output: 50379
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '%s\n' '--- test_rest_views.py relevant slice ---'
sed -n '147,270p' src/imio/smartweb/core/tests/test_rest_views.py
printf '\n%s\n' '--- testing.py relevant slice ---'
sed -n '1,180p' src/imio/smartweb/core/testing.pyRepository: IMIO/imio.smartweb.core
Length of output: 9651
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '%s\n' '--- seo_html implementation slice ---'
sed -n '90,130p' src/imio/smartweb/core/contents/rest/view.py
printf '\n%s\n' '--- directory endpoint / batch handling slice ---'
sed -n '1,140p' src/imio/smartweb/core/contents/rest/directory/endpoint.py 2>/dev/null || trueRepository: IMIO/imio.smartweb.core
Length of output: 5284
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '%s\n' '--- testing.py relevant slice ---'
sed -n '1,180p' src/imio/smartweb/core/testing.py
printf '\n%s\n' '--- test_rest_views.py relevant slice ---'
sed -n '147,270p' src/imio/smartweb/core/tests/test_rest_views.py
printf '\n%s\n' '--- seo_html implementation slice ---'
sed -n '90,130p' src/imio/smartweb/core/contents/rest/view.pyRepository: IMIO/imio.smartweb.core
Length of output: 11211
Clear the batching params between tests. self.layer["request"] is reused in setUp, and test_seo_hidden_react_links_batching leaves b_start/b_size behind, so the default-size assertion can become order-dependent.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/imio/smartweb/core/tests/test_rest_views.py` around lines 249 - 256,
Clear the reused request’s batching parameters between tests so state from
test_seo_hidden_react_links_batching cannot affect test_seo_html. Update the
shared setUp request initialization, or add equivalent cleanup, to remove/reset
b_start and b_size before each test while preserving the SEO view’s
DEFAULT_BATCH_SIZE assertion.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary by CodeRabbit
New Features
Bug Fixes
Upgrades