Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughReplaces generic Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant NewsView
participant NewsEndpoint
participant EntitySearch as "@search_entity"
participant NewsFolderSearch as "@search_newsfolder_for_entity"
participant NewsItemsSearch as "@search_newsitems"
Client->>NewsView: request news for entity_uid
NewsView->>NewsEndpoint: request folders for entity_uid
NewsEndpoint->>EntitySearch: GET `@search_entity?UID=...&metadata_fields=UID`
EntitySearch-->>NewsEndpoint: return entity metadata
NewsEndpoint->>NewsFolderSearch: GET `@search_newsfolder_for_entity?portal_type=imio.news.NewsFolder&metadata_fields=UID&entity_uid=...`
NewsFolderSearch-->>NewsEndpoint: return folder UIDs & titles
NewsEndpoint->>NewsEndpoint: choose selected_folder (existing or "administration" fallback)
NewsEndpoint->>NewsItemsSearch: GET `@search_newsitems?entity_uid=...&selected_news_folders=...`
NewsItemsSearch-->>NewsEndpoint: return news items
NewsEndpoint-->>NewsView: deliver assembled news items (and warning if selection changed)
NewsView-->>Client: render items (or show warning)
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 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: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (4)
src/imio/smartweb/core/contents/rest/news/endpoint.py (2)
35-47: 🛠️ Refactor suggestion | 🟠 Major
_get_news_folders_uids_and_title_from_entityis duplicated verbatim inview.pyThe method body in
BaseNewsEndpoint(lines 35-47) is identical to the one inNewsView(view.py lines 20-31). Both will diverge independently and double the surface area for bugs.Consider extracting it to a module-level utility function in
utils.py(or a shared mixin) and calling it from both locations.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/imio/smartweb/core/contents/rest/news/endpoint.py` around lines 35 - 47, The method _get_news_folders_uids_and_title_from_entity is duplicated in BaseNewsEndpoint and NewsView; extract the shared logic into a single reusable function (e.g., get_news_folders_uids_and_titles) in a new or existing utils module (or a shared mixin class) and replace both implementations to call that utility; ensure the new function accepts the same entity_uid parameter and returns (uids, mapping) and update BaseNewsEndpoint._get_news_folders_uids_and_title_from_entity and NewsView to delegate to the new utility to avoid divergence.
35-47:⚠️ Potential issue | 🔴 CriticalMissing null/empty guards on
get_json()responses cause runtime crashes
get_json()returnsNoneon timeout or non-200 responses (seeutils.py). Both calls lack guards, sodata.get("items")[0]raisesAttributeErrorwhendataisNone, andIndexErrorwhen the items list is empty.🛡️ Proposed fix
def _get_news_folders_uids_and_title_from_entity(self, entity_uid): url = f"{NEWS_URL}/@search_entity?UID={entity_uid}&metadata_fields=UID" data = get_json(url) - entity_uid = data.get("items")[0].get("UID") + if not data or not data.get("items"): + return [], {} + entity_uid = data["items"][0].get("UID") url_to_get_news_folders = ( f"{data.get('items')[0].get('@id')}" f"/@search_newsfolder_for_entity?portal_type=imio.news.NewsFolder&metadata_fields=UID&entity_uid={entity_uid}" ) data = get_json(url_to_get_news_folders) - uids = [item["UID"] for item in data["items"]] - data = {item["UID"]: item["title"] for item in data["items"]} + if not data or not data.get("items"): + return [], {} + uids = [item["UID"] for item in data["items"]] + data = {item["UID"]: item["title"] for item in data["items"]} return uids, data🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/imio/smartweb/core/contents/rest/news/endpoint.py` around lines 35 - 47, The _get_news_folders_uids_and_title_from_entity function calls get_json twice without guarding against None or empty "items"; add null/empty checks after each get_json call (the first call returning data used to compute entity_uid and '@id', and the second call used to build uids and the title map) and handle error cases by either raising a clear exception or returning ([], {}) (and log the failure) instead of accessing data.get("items")[0] or iterating empty lists; ensure you reference get_json, the local variables entity_uid, url_to_get_news_folders, uids, and the dict comprehension that builds title mapping when adding these checks.src/imio/smartweb/core/tests/test_rest.py (1)
493-508:⚠️ Potential issue | 🟡 MinorStale
# UNDO CACHEcomment is now misleadingThe comment block at lines 493–494 suggests
entity_uidwas previously excluded, but it is now an active part of the URL (line 508). The comment no longer matches reality and should be removed or updated.✏️ Proposed fix
- # UNDO CACHE - # "entity_uid=7c69f9a738ec497c819725c55888ee32&" self.assertEqual( url, "http://localhost:8080/Plone/@search_newsitems?"🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/imio/smartweb/core/tests/test_rest.py` around lines 493 - 508, Remove or update the stale comment "# UNDO CACHE" that incorrectly claims entity_uid was excluded; locate the assertion using assertEqual comparing url (the URL string construction in the test) and either delete the "# UNDO CACHE" line or replace it with a short accurate note indicating that entity_uid is included in the URL (keep the rest of the assertEqual and URL string unchanged).src/imio/smartweb/core/tests/test_section_news.py (1)
54-91:⚠️ Potential issue | 🔴 CriticalMock URLs are missing
review_state,entity_uid, andcache_keyparameters — tests will not matchThe view's
itemsproperty builds the query URL with these parameters (view.py lines 61, 70, 71):"review_state=published", f"entity_uid={entity_uid}", f"cache_key={modified_hash}",None of them appear in the mock URLs registered via
m.get(url, ...). Sincerequests_mockmatches the full URL (including query string), the actual HTTP call will not match the registered mock and the test will raise a connection error or silently fall through.The
cache_keyis dynamic (based onmodification_date), so it cannot be hardcoded; consider usingrequests_mockadapter options to match on a URL prefix or use a real adapter pattern. Thereview_stateandentity_uidparameters can and should be added to the mock URLs onceentity_uidis known in the test context (or patched via the registry).This same gap applies to all mock URLs in
test_news(line 68, 77),test_news_modified(lines 111, 119, 135),test_orientation(line 159), andtest_show_categories_or_topics(line 182).🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/imio/smartweb/core/tests/test_section_news.py` around lines 54 - 91, The registered request mocks for the news tests don't include the query params review_state, entity_uid and the dynamic cache_key so requests_mock fails to match the actual URL built by NewsView (see NewsView._get_news_folders_uids_and_title_from_entity and news_view.items usage); fix by updating the mocked URLs for the tests (e.g., those using self.json_news and self.json_specific_news in test_news/test_news_modified/test_orientation/test_show_categories_or_topics) to include review_state and entity_uid, and handle cache_key by matching the URL prefix or using requests_mock's URL-prefix/pattern matching (or a custom matcher) instead of hardcoding the full querystring so the dynamic cache_key does not break the mock.
♻️ Duplicate comments (1)
src/imio/smartweb/core/contents/sections/news/view.py (1)
20-31:⚠️ Potential issue | 🔴 CriticalIdentical
_get_news_folders_uids_and_title_from_entityin bothview.pyandendpoint.py— DRY violationSame null/empty guard issue applies here as in
endpoint.py(see comment there). Bothget_json()calls lack protection againstNoneor emptyitems.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/imio/smartweb/core/contents/sections/news/view.py` around lines 20 - 31, The function _get_news_folders_uids_and_title_from_entity has duplicated logic and fails to guard against get_json returning None or data["items"] being empty; update the function to handle missing/empty responses from both get_json calls (the first call that sets data and entity_uid and the second call that builds uids and data dict) by checking that data and data.get("items") are present and non-empty before indexing [0] or iterating, return sensible defaults (e.g., ([], {}) ) when no items are found, and consolidate duplicated behavior with the same safe checks used in endpoint.py to avoid the DRY violation.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@CHANGES.rst`:
- Around line 8-9: Fix the typo and ambiguous punctuation in the changelog entry
for WEB-4366: change "spicialized" to "specialized" and remove/clarify the
trailing question mark so the sentence reads clearly (e.g., "Refactor caching
with more specialized endpoints to avoid incorrect or random entity/newsfolders
retrieval from cache" or "…to avoid incorrect or random retrieval of
entities/newsfolders from cache"); update the entry associated with WEB-4366
(the line containing "[boulch]") accordingly.
In `@src/imio/smartweb/core/contents/rest/news/endpoint.py`:
- Line 88: Remove the debug print that leaks to stdout in the query_url handler:
delete the line print("URL = {}".format(url)) in
src/imio/smartweb/core/contents/rest/news/endpoint.py (inside the query_url
function) and, if logging the URL is desired, replace it with a proper
logger.debug call (e.g., use the module/class logger) so it doesn't spam stdout
in production.
- Around line 58-62: The fallback to uids[0] is unreachable because the list
comprehension [k for k, v in data.items() if "administration" in v.lower()][0]
will raise IndexError before the subsequent if not item check; replace that
pattern with next((k for k, v in data.items() if "administration" in v.lower()),
None) to get a safe default, then if selected_item not in uids set selected_item
= found_item if found_item is not None else uids[0]; update the variable names
used in the block (selected_item, uids, data) and remove the now-dead if not
item: branch to satisfy RUF015 and prevent crashes.
In `@src/imio/smartweb/core/contents/sections/news/view.py`:
- Around line 40-51: Replace the unsafe list indexing with a safe generator
lookup: use item = next((k for k, v in data.items() if "administration" in
v.lower()), None) then check if item is None and set selected_item =
f"selected_news_folders={uids[0]}" else set selected_item =
f"selected_news_folders={item}"; keep the rest (current_lang, self._issue
assignment using translate and _) unchanged. Ensure you reference
self.context.related_news, data, uids, item, and selected_item when making the
change.
---
Outside diff comments:
In `@src/imio/smartweb/core/contents/rest/news/endpoint.py`:
- Around line 35-47: The method _get_news_folders_uids_and_title_from_entity is
duplicated in BaseNewsEndpoint and NewsView; extract the shared logic into a
single reusable function (e.g., get_news_folders_uids_and_titles) in a new or
existing utils module (or a shared mixin class) and replace both implementations
to call that utility; ensure the new function accepts the same entity_uid
parameter and returns (uids, mapping) and update
BaseNewsEndpoint._get_news_folders_uids_and_title_from_entity and NewsView to
delegate to the new utility to avoid divergence.
- Around line 35-47: The _get_news_folders_uids_and_title_from_entity function
calls get_json twice without guarding against None or empty "items"; add
null/empty checks after each get_json call (the first call returning data used
to compute entity_uid and '@id', and the second call used to build uids and the
title map) and handle error cases by either raising a clear exception or
returning ([], {}) (and log the failure) instead of accessing
data.get("items")[0] or iterating empty lists; ensure you reference get_json,
the local variables entity_uid, url_to_get_news_folders, uids, and the dict
comprehension that builds title mapping when adding these checks.
In `@src/imio/smartweb/core/tests/test_rest.py`:
- Around line 493-508: Remove or update the stale comment "# UNDO CACHE" that
incorrectly claims entity_uid was excluded; locate the assertion using
assertEqual comparing url (the URL string construction in the test) and either
delete the "# UNDO CACHE" line or replace it with a short accurate note
indicating that entity_uid is included in the URL (keep the rest of the
assertEqual and URL string unchanged).
In `@src/imio/smartweb/core/tests/test_section_news.py`:
- Around line 54-91: The registered request mocks for the news tests don't
include the query params review_state, entity_uid and the dynamic cache_key so
requests_mock fails to match the actual URL built by NewsView (see
NewsView._get_news_folders_uids_and_title_from_entity and news_view.items
usage); fix by updating the mocked URLs for the tests (e.g., those using
self.json_news and self.json_specific_news in
test_news/test_news_modified/test_orientation/test_show_categories_or_topics) to
include review_state and entity_uid, and handle cache_key by matching the URL
prefix or using requests_mock's URL-prefix/pattern matching (or a custom
matcher) instead of hardcoding the full querystring so the dynamic cache_key
does not break the mock.
---
Duplicate comments:
In `@src/imio/smartweb/core/contents/sections/news/view.py`:
- Around line 20-31: The function _get_news_folders_uids_and_title_from_entity
has duplicated logic and fails to guard against get_json returning None or
data["items"] being empty; update the function to handle missing/empty responses
from both get_json calls (the first call that sets data and entity_uid and the
second call that builds uids and data dict) by checking that data and
data.get("items") are present and non-empty before indexing [0] or iterating,
return sensible defaults (e.g., ([], {}) ) when no items are found, and
consolidate duplicated behavior with the same safe checks used in endpoint.py to
avoid the DRY violation.
ℹ️ Review info
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
CHANGES.rstsrc/imio/smartweb/core/contents/rest/news/endpoint.pysrc/imio/smartweb/core/contents/sections/news/view.pysrc/imio/smartweb/core/tests/test_rest.pysrc/imio/smartweb/core/tests/test_section_news.py
CHANGES.rst
Outdated
Comment on lines
+8
to
+9
| - WEB-4366 : Refactor caching with more spicialized endpoints to avoid incorrect/random? entity/newsfolders retrieval from cache | ||
| [boulch] |
There was a problem hiding this comment.
Typo: "spicialized" → "specialized"; clarify the trailing ?
The changelog entry has a misspelling and ambiguous wording.
✏️ Proposed fix
-- WEB-4366 : Refactor caching with more spicialized endpoints to avoid incorrect/random? entity/newsfolders retrieval from cache
+- WEB-4366 : Refactor caching with more specialized endpoints to avoid incorrect/random entity/newsfolders retrieval from cache🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@CHANGES.rst` around lines 8 - 9, Fix the typo and ambiguous punctuation in
the changelog entry for WEB-4366: change "spicialized" to "specialized" and
remove/clarify the trailing question mark so the sentence reads clearly (e.g.,
"Refactor caching with more specialized endpoints to avoid incorrect or random
entity/newsfolders retrieval from cache" or "…to avoid incorrect or random
retrieval of entities/newsfolders from cache"); update the entry associated with
WEB-4366 (the line containing "[boulch]") accordingly.
| params.append("b_size={}".format(self.batch_size)) | ||
| params = self.construct_query_string(params) | ||
| url = f"{NEWS_URL}/{self.remote_endpoint}?{params}" | ||
| print("URL = {}".format(url)) |
There was a problem hiding this comment.
Remove debug print statement before merging
print("URL = {}".format(url)) will emit to stdout on every query_url access in production.
🐛 Proposed fix
- print("URL = {}".format(url))
+ # Remove or replace with: logger.debug("News query URL: %s", url)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/imio/smartweb/core/contents/rest/news/endpoint.py` at line 88, Remove the
debug print that leaks to stdout in the query_url handler: delete the line
print("URL = {}".format(url)) in
src/imio/smartweb/core/contents/rest/news/endpoint.py (inside the query_url
function) and, if logging the URL is desired, replace it with a proper
logger.debug call (e.g., use the module/class logger) so it doesn't spam stdout
in production.
There was a problem hiding this comment.
Pull request overview
Refactors Smartweb’s news fetching to use more specialized remote REST endpoints and include entity_uid in requests, aiming to prevent incorrect/random entity/newsfolder cache hits.
Changes:
- Switch news search calls from
@searchto@search_newsitemsand addentity_uidas a query parameter. - Introduce entity/newsfolder lookup via
@search_entityand@search_newsfolder_for_entity, and re-enable fallback selection logic when the configured newsfolder is missing. - Update tests and changelog to reflect the new endpoints/parameters.
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 8 comments.
Show a summary per file
| File | Description |
|---|---|
src/imio/smartweb/core/contents/sections/news/view.py |
Moves entity/newsfolder lookup to specialized endpoints; adds entity_uid to newsitems search; re-enables missing-newsfolder fallback. |
src/imio/smartweb/core/contents/rest/news/endpoint.py |
Updates REST endpoint to call @search_newsitems, adds entity_uid, and re-enables missing-newsfolder fallback in query building. |
src/imio/smartweb/core/tests/test_section_news.py |
Updates mocked URLs/method patch target for the refactor (but currently missing the new entity_uid param in mocks). |
src/imio/smartweb/core/tests/test_rest.py |
Updates expected REST URL to @search_newsitems and includes entity_uid. |
CHANGES.rst |
Adds a WEB-4366 changelog entry for the caching refactor. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
39
to
+45
| selected_item = f"selected_news_folders={self.context.related_news}" | ||
| if self.context.related_news not in uids: | ||
| item = [k for k, v in data.items() if "administration" in v.lower()][0] | ||
| if not item: | ||
| selected_item = f"selected_news_folders={uids[0]}" | ||
| selected_item = f"selected_news_folders={item}" | ||
| current_lang = api.portal.get_current_language()[:2] |
There was a problem hiding this comment.
Fallback selection logic has two issues: (1) [...][0] will raise IndexError if no newsfolder title contains "administration"; (2) even when the if not item branch runs, selected_item is immediately overwritten by selected_item = f"selected_news_folders={item}", so the uids[0] fallback is never actually used. Consider using next((k for k,v in data.items() if ...), None) and only overriding selected_item when a match is found; also handle the case where uids is empty.
Comment on lines
35
to
43
| def _get_news_folders_uids_and_title_from_entity(self, entity_uid): | ||
| url = f"{NEWS_URL}/@search?UID={entity_uid}" | ||
| url = f"{NEWS_URL}/@search_entity?UID={entity_uid}&metadata_fields=UID" | ||
|
|
||
| data = get_json(url) | ||
| entity_uid = data.get("items")[0].get("UID") | ||
| url_to_get_news_folders = ( | ||
| f"{data.get('items')[0].get('@id')}" | ||
| "/@search?portal_type=imio.news.NewsFolder&depth=1&metadata_fields=UID" | ||
| f"/@search_newsfolder_for_entity?portal_type=imio.news.NewsFolder&metadata_fields=UID&entity_uid={entity_uid}" | ||
| ) |
There was a problem hiding this comment.
_get_news_folders_uids_and_title_from_entity assumes the remote @search_entity response has a non-empty items list and that the first item contains UID and @id. If the registry entity UID is wrong or the remote endpoint returns no results, this will raise (IndexError/AttributeError). Please add guards and return a safe default (or raise a controlled error) when items is missing/empty.
Comment on lines
+59
to
+62
| item = [k for k, v in data.items() if "administration" in v.lower()][0] | ||
| if not item: | ||
| selected_item = uids[0] | ||
| selected_item = item |
There was a problem hiding this comment.
Fallback selection has the same issue as in NewsView: [...][0] will raise if no title contains "administration", and selected_item is always overwritten by selected_item = item, making the uids[0] fallback ineffective. Use a safe lookup (e.g., next(..., None)) and only overwrite selected_item when a match exists; also handle empty uids.
Suggested change
| item = [k for k, v in data.items() if "administration" in v.lower()][0] | |
| if not item: | |
| selected_item = uids[0] | |
| selected_item = item | |
| admin_uid = next( | |
| (k for k, v in data.items() if "administration" in v.lower()), | |
| None, | |
| ) | |
| if admin_uid is not None: | |
| selected_item = admin_uid | |
| elif uids: | |
| selected_item = uids[0] |
| params.append("b_size={}".format(self.batch_size)) | ||
| params = self.construct_query_string(params) | ||
| url = f"{NEWS_URL}/{self.remote_endpoint}?{params}" | ||
| print("URL = {}".format(url)) |
There was a problem hiding this comment.
Leftover debug output: print("URL = ...") will write to stdout in production and during tests, and can leak request details in logs. Please remove it or replace with the project’s configured logger at an appropriate level (likely debug).
Suggested change
| print("URL = {}".format(url)) |
Comment on lines
+68
to
69
| url = "http://localhost:8080/Plone/@search_newsitems?selected_news_folders=64f4cbee9a394a018a951f6d94452914&portal_type=imio.news.NewsItem&metadata_fields=container_uid&metadata_fields=category_title&metadata_fields=local_category&metadata_fields=topics&metadata_fields=has_leadimage&metadata_fields=modified&metadata_fields=effective&metadata_fields=UID&sort_limit=6&translated_in_en=1&sort_on=effective&sort_order=descending" | ||
| m.get(url, text=json.dumps(self.json_news)) |
There was a problem hiding this comment.
NewsView.items() now always adds an entity_uid=... parameter to the query, so this mocked URL is missing entity_uid=7c69f9a738ec497c819725c55888ee32 (from the testing registry). Without it, requests_mock won’t match and the test will fail; update this URL (and the other similar ones in this file) to include the entity_uid parameter.
| "bfe2b4391a0f4a8db6d8b7fed63d1c4a", | ||
| ] | ||
| url = "http://localhost:8080/Plone/@search?UID=aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa&UID=bfe2b4391a0f4a8db6d8b7fed63d1c4a&portal_type=imio.news.NewsItem&metadata_fields=container_uid&metadata_fields=category_title&metadata_fields=local_category&metadata_fields=topics&metadata_fields=has_leadimage&metadata_fields=modified&metadata_fields=effective&metadata_fields=UID&sort_limit=6&translated_in_en=1" | ||
| url = "http://localhost:8080/Plone/@search_newsitems?UID=aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa&UID=bfe2b4391a0f4a8db6d8b7fed63d1c4a&portal_type=imio.news.NewsItem&metadata_fields=container_uid&metadata_fields=category_title&metadata_fields=local_category&metadata_fields=topics&metadata_fields=has_leadimage&metadata_fields=modified&metadata_fields=effective&metadata_fields=UID&sort_limit=6&translated_in_en=1" |
There was a problem hiding this comment.
Same as above for the specific-UID query: NewsView.items() appends entity_uid=... to the request params even when specific_related_newsitems is set. This mocked URL should include entity_uid=7c69f9a738ec497c819725c55888ee32 to match the actual request.
Suggested change
| url = "http://localhost:8080/Plone/@search_newsitems?UID=aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa&UID=bfe2b4391a0f4a8db6d8b7fed63d1c4a&portal_type=imio.news.NewsItem&metadata_fields=container_uid&metadata_fields=category_title&metadata_fields=local_category&metadata_fields=topics&metadata_fields=has_leadimage&metadata_fields=modified&metadata_fields=effective&metadata_fields=UID&sort_limit=6&translated_in_en=1" | |
| url = "http://localhost:8080/Plone/@search_newsitems?UID=aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa&UID=bfe2b4391a0f4a8db6d8b7fed63d1c4a&portal_type=imio.news.NewsItem&metadata_fields=container_uid&metadata_fields=category_title&metadata_fields=local_category&metadata_fields=topics&metadata_fields=has_leadimage&metadata_fields=modified&metadata_fields=effective&metadata_fields=UID&sort_limit=6&translated_in_en=1&entity_uid=7c69f9a738ec497c819725c55888ee32" |
CHANGES.rst
Outdated
| ------------------- | ||
|
|
||
| - Nothing changed yet. | ||
| - WEB-4366 : Refactor caching with more spicialized endpoints to avoid incorrect/random? entity/newsfolders retrieval from cache |
There was a problem hiding this comment.
Typo in changelog entry: “spicialized” should be “specialized”.
Suggested change
| - WEB-4366 : Refactor caching with more spicialized endpoints to avoid incorrect/random? entity/newsfolders retrieval from cache | |
| - WEB-4366 : Refactor caching with more specialized endpoints to avoid incorrect/random? entity/newsfolders retrieval from cache |
Comment on lines
+20
to
27
| def _get_news_folders_uids_and_title_from_entity(self, entity_uid): | ||
| url = f"{NEWS_URL}/@search_entity?UID={entity_uid}&metadata_fields=UID" | ||
| data = get_json(url) | ||
| entity_uid = data.get("items")[0].get("UID") | ||
| url_to_get_news_folders = ( | ||
| f"{data.get('items')[0].get('@id')}" | ||
| "/@search?portal_type=imio.news.NewsFolder&depth=1&metadata_fields=UID" | ||
| f"/@search_newsfolder_for_entity?portal_type=imio.news.NewsFolder&metadata_fields=UID&entity_uid={entity_uid}" | ||
| ) |
There was a problem hiding this comment.
_get_news_folders_uids_and_title_from_entity assumes get_json(url) returns a dict with a non-empty items list and that the first item contains UID and @id. If the entity UID is missing/invalid or the remote endpoint returns an empty result, this will raise (IndexError/AttributeError) and break section rendering; please add defensive checks (and return ([], {}) or a clear warning) before dereferencing items[0].
…incorrect/random? entity/newsfolders retrieval from cache
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
src/imio/smartweb/core/contents/rest/news/endpoint.py (1)
35-47:⚠️ Potential issue | 🔴 CriticalGuard
get_jsonresults before indexingitems.Line 39 and Line 41 assume non-empty responses. If the auth source times out or returns no entity, this path crashes instead of degrading gracefully.
🐛 Proposed fix
def _get_news_folders_uids_and_title_from_entity(self, entity_uid): url = f"{NEWS_URL}/@search_entity?UID={entity_uid}&metadata_fields=UID" - data = get_json(url) - entity_uid = data.get("items")[0].get("UID") + data = get_json(url) or {} + items = data.get("items") or [] + if not items: + return [], {} + entity_uid = items[0].get("UID") url_to_get_news_folders = ( - f"{data.get('items')[0].get('@id')}" + f"{items[0].get('@id')}" f"/@search_newsfolder_for_entity?portal_type=imio.news.NewsFolder&metadata_fields=UID&entity_uid={entity_uid}" ) - data = get_json(url_to_get_news_folders) - uids = [item["UID"] for item in data["items"]] - data = {item["UID"]: item["title"] for item in data["items"]} + data = get_json(url_to_get_news_folders) or {} + items = data.get("items") or [] + uids = [item["UID"] for item in items] + data = {item["UID"]: item["title"] for item in items} return uids, data🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/imio/smartweb/core/contents/rest/news/endpoint.py` around lines 35 - 47, In _get_news_folders_uids_and_title_from_entity validate the responses from get_json before indexing "items": after the first get_json(url) check that data is a dict and data.get("items") is a non-empty list (otherwise return empty uids and dict or raise a clear exception), only then extract the entity UID and build url_to_get_news_folders; after the second get_json(url_to_get_news_folders) likewise ensure data.get("items") exists and is iterable before constructing uids and the title mapping, using .get() safely throughout to avoid IndexError/TypeError when the auth source times out or returns no entity.src/imio/smartweb/core/contents/sections/news/view.py (1)
20-31:⚠️ Potential issue | 🔴 CriticalSection helper needs the same defensive checks for empty/failed JSON responses.
Line 23 and Line 25 can throw when auth-source responses are missing or empty, causing section rendering to fail.
🐛 Proposed fix
def _get_news_folders_uids_and_title_from_entity(self, entity_uid): url = f"{NEWS_URL}/@search_entity?UID={entity_uid}&metadata_fields=UID" - data = get_json(url) - entity_uid = data.get("items")[0].get("UID") + data = get_json(url) or {} + items = data.get("items") or [] + if not items: + return [], {} + entity_uid = items[0].get("UID") url_to_get_news_folders = ( - f"{data.get('items')[0].get('@id')}" + f"{items[0].get('@id')}" f"/@search_newsfolder_for_entity?portal_type=imio.news.NewsFolder&metadata_fields=UID&entity_uid={entity_uid}" ) - data = get_json(url_to_get_news_folders) - uids = [item["UID"] for item in data["items"]] - data = {item["UID"]: item["title"] for item in data["items"]} + data = get_json(url_to_get_news_folders) or {} + items = data.get("items") or [] + uids = [item["UID"] for item in items] + data = {item["UID"]: item["title"] for item in items} return uids, data🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/imio/smartweb/core/contents/sections/news/view.py` around lines 20 - 31, The helper _get_news_folders_uids_and_title_from_entity should defensively validate JSON responses from get_json: check that the first get_json(url) returns a dict with a non-empty "items" list and that items[0] contains "UID" and "@id" before accessing them (use a local found_uid instead of overwriting entity_uid); if missing/invalid, return ([], {}). Do the same after fetching url_to_get_news_folders: ensure the response is a dict with a non-empty "items" list and that each item has "UID" and "title" before building uids and the dict; on any failure return ([], {}) to avoid exceptions in rendering. Ensure references to NEWS_URL, _get_news_folders_uids_and_title_from_entity, get_json, and url_to_get_news_folders are used to locate the changes.
♻️ Duplicate comments (1)
src/imio/smartweb/core/contents/rest/news/endpoint.py (1)
58-67:⚠️ Potential issue | 🔴 CriticalEmpty-folder fallback can still crash (
uids[0]).Line 65 can raise
IndexErrorwhen no folder UID is returned. Also avoid emittingselected_news_folders=Nonein Line 67.🐛 Proposed fix
selected_item = self.context.selected_news_folder if selected_item not in uids: item = next( (k for k, v in data.items() if "administration" in v.lower()), - uids[0] if uids else None, + None, ) - selected_item = item if item else None - if not selected_item: - selected_item = uids[0] - params = [ - "selected_news_folders={}".format(selected_item), + selected_item = item or (uids[0] if uids else None) + params = [] + if selected_item: + params.append("selected_news_folders={}".format(selected_item)) + params += [ "portal_type=imio.news.NewsItem", "metadata_fields=category",🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/imio/smartweb/core/contents/rest/news/endpoint.py` around lines 58 - 67, The current fallback uses uids[0] and may raise IndexError when uids is empty and can also emit selected_news_folders=None; update the selection logic around selected_item and uids so you never index uids when it's falsy and you never format "selected_news_folders=None". Specifically, in the block that computes selected_item (referencing selected_item, uids, data), ensure you only use uids[0] when uids is non-empty, and if no valid UID is found set selected_item to an empty string (or omit it). When building params (where "selected_news_folders={}" is formatted), only append that param if selected_item is truthy (non-empty) to avoid emitting None or causing IndexError.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/imio/smartweb/core/tests/test_section_news.py`:
- Line 68: The test URL fixtures in
src/imio/smartweb/core/tests/test_section_news.py are missing the new entity_uid
query parameter introduced by NewsView.items; update each affected expected URL
(the ones at the indicated locations and any other updated fixtures in that
file) to include the entity_uid=<expected_entity_uid> parameter (matching how
NewsView.items builds URLs) so the tests assert the entity-scoped filtering
behavior; adjust all occurrences listed (including lines around 68, 77, 111,
119, 135, 159, 182) to append the entity_uid parameter in the same format as
other query params.
---
Outside diff comments:
In `@src/imio/smartweb/core/contents/rest/news/endpoint.py`:
- Around line 35-47: In _get_news_folders_uids_and_title_from_entity validate
the responses from get_json before indexing "items": after the first
get_json(url) check that data is a dict and data.get("items") is a non-empty
list (otherwise return empty uids and dict or raise a clear exception), only
then extract the entity UID and build url_to_get_news_folders; after the second
get_json(url_to_get_news_folders) likewise ensure data.get("items") exists and
is iterable before constructing uids and the title mapping, using .get() safely
throughout to avoid IndexError/TypeError when the auth source times out or
returns no entity.
In `@src/imio/smartweb/core/contents/sections/news/view.py`:
- Around line 20-31: The helper _get_news_folders_uids_and_title_from_entity
should defensively validate JSON responses from get_json: check that the first
get_json(url) returns a dict with a non-empty "items" list and that items[0]
contains "UID" and "@id" before accessing them (use a local found_uid instead of
overwriting entity_uid); if missing/invalid, return ([], {}). Do the same after
fetching url_to_get_news_folders: ensure the response is a dict with a non-empty
"items" list and that each item has "UID" and "title" before building uids and
the dict; on any failure return ([], {}) to avoid exceptions in rendering.
Ensure references to NEWS_URL, _get_news_folders_uids_and_title_from_entity,
get_json, and url_to_get_news_folders are used to locate the changes.
---
Duplicate comments:
In `@src/imio/smartweb/core/contents/rest/news/endpoint.py`:
- Around line 58-67: The current fallback uses uids[0] and may raise IndexError
when uids is empty and can also emit selected_news_folders=None; update the
selection logic around selected_item and uids so you never index uids when it's
falsy and you never format "selected_news_folders=None". Specifically, in the
block that computes selected_item (referencing selected_item, uids, data),
ensure you only use uids[0] when uids is non-empty, and if no valid UID is found
set selected_item to an empty string (or omit it). When building params (where
"selected_news_folders={}" is formatted), only append that param if
selected_item is truthy (non-empty) to avoid emitting None or causing
IndexError.
ℹ️ Review info
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
CHANGES.rstsrc/imio/smartweb/core/contents/rest/news/endpoint.pysrc/imio/smartweb/core/contents/sections/news/view.pysrc/imio/smartweb/core/tests/test_rest.pysrc/imio/smartweb/core/tests/test_section_news.py
🚧 Files skipped from review as they are similar to previous changes (1)
- CHANGES.rst
| self.assertEqual(news_view.items, []) | ||
|
|
||
| url = "http://localhost:8080/Plone/@search?selected_news_folders=64f4cbee9a394a018a951f6d94452914&portal_type=imio.news.NewsItem&metadata_fields=container_uid&metadata_fields=category_title&metadata_fields=local_category&metadata_fields=topics&metadata_fields=has_leadimage&metadata_fields=modified&metadata_fields=effective&metadata_fields=UID&sort_limit=6&translated_in_en=1&sort_on=effective&sort_order=descending" | ||
| url = "http://localhost:8080/Plone/@search_newsitems?selected_news_folders=64f4cbee9a394a018a951f6d94452914&portal_type=imio.news.NewsItem&metadata_fields=container_uid&metadata_fields=category_title&metadata_fields=local_category&metadata_fields=topics&metadata_fields=has_leadimage&metadata_fields=modified&metadata_fields=effective&metadata_fields=UID&sort_limit=6&translated_in_en=1&sort_on=effective&sort_order=descending" |
There was a problem hiding this comment.
Section-news URL fixtures should assert entity_uid as well.
These updated expectations verify the endpoint rename, but they still omit the new entity_uid parameter added by NewsView.items (Line 71 in src/imio/smartweb/core/contents/sections/news/view.py). Please include it in expected URLs so regressions on entity-scoped filtering are caught.
✅ Minimal direction
- ...&metadata_fields=UID&sort_limit=6&translated_in_en=1...
+ ...&metadata_fields=UID&entity_uid={entity_uid}&sort_limit=6&translated_in_en=1...Apply the same adjustment to each updated URL fixture in this file.
Also applies to: 77-77, 111-111, 119-119, 135-135, 159-159, 182-182
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/imio/smartweb/core/tests/test_section_news.py` at line 68, The test URL
fixtures in src/imio/smartweb/core/tests/test_section_news.py are missing the
new entity_uid query parameter introduced by NewsView.items; update each
affected expected URL (the ones at the indicated locations and any other updated
fixtures in that file) to include the entity_uid=<expected_entity_uid> parameter
(matching how NewsView.items builds URLs) so the tests assert the entity-scoped
filtering behavior; adjust all occurrences listed (including lines around 68,
77, 111, 119, 135, 159, 182) to append the entity_uid parameter in the same
format as other query params.
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
src/imio/smartweb/core/tests/test_section_external_content.py (1)
220-257: Add timeout-path tests for ODWB-backed plugins.The production code handles request timeouts, but these branches are not covered yet. Adding them would lock down failure behavior and prevent regressions.
🧪 Suggested test additions
+import requests + @@ class TestSectionExternalContent(ImioSmartwebTestCase): @@ + def test_idelux_waste_plugin_timeout(self): + sec = api.content.create( + container=self.page, type="imio.smartweb.SectionExternalContent", id="sec" + ) + odwb_url = "https://www.odwb.be/api/explore/v2.1/catalog/datasets/guide-de-tri1/records" + sec.external_content_url = odwb_url + + with requests_mock_module.Mocker() as m: + m.get(f"{odwb_url}?limit=-1", exc=requests.exceptions.Timeout) + section_view = queryMultiAdapter((sec, self.request), name="view") + self.assertIsNone(section_view.contents) + + def test_inbw_containers_affluence_plugin_timeout(self): + sec = api.content.create( + container=self.page, type="imio.smartweb.SectionExternalContent", id="sec" + ) + odwb_url = "https://www.odwb.be/api/explore/v2.1/catalog/datasets/affluence/records?dataset=affluence" + sec.external_content_url = odwb_url + + with requests_mock_module.Mocker() as m: + m.get(f"{odwb_url}&limit=-1", exc=requests.exceptions.Timeout) + section_view = queryMultiAdapter((sec, self.request), name="view") + self.assertIsNone(section_view.contents)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/imio/smartweb/core/tests/test_section_external_content.py` around lines 220 - 257, Add two new tests mirroring test_idelux_waste_plugin and test_inbw_containers_affluence_plugin that exercise the timeout branch: in each test create the same SectionExternalContent (sec), set external_content_url to the same odwb endpoints, then use requests_mock_module.Mocker() to register the GET for the plugin URL to raise requests.exceptions.Timeout (or requests.exceptions.ReadTimeout) instead of returning JSON; call section_view = queryMultiAdapter((sec, self.request), name="view") and access section_view.contents to ensure the code handles the timeout without raising and returns a safe/falsy result (assert that section_view.contents is falsy), and also verify section_view.get_plugin() and section_view.which_plugin() still identify the plugin (use the same plugin interface checks IIdeluxWastePlugin and IInbwContainersAffluencePlugin and which_plugin() string checks as in the existing tests).src/imio/smartweb/core/tests/test_utils.py (1)
146-153: Strengthen theget_ts_api_urlsuccess assertion and add a non-wcscase.Current substring checks are permissive; asserting the exact URL gives tighter regression protection. Also add coverage for non-
wcsservices to catch service-branch edge cases.Suggested improvement
def test_get_ts_api_url_valid_url(self): with patch("imio.smartweb.core.utils.get_value_from_registry") as mock_registry: mock_registry.return_value = "https://mysite.guichet-citoyen.be" result = get_ts_api_url("wcs") - self.assertIsNotNone(result) - self.assertIn("-formulaires.guichet-citoyen.be", result) - self.assertIn("/api", result) + self.assertEqual( + result, "https://mysite-formulaires.guichet-citoyen.be/api" + )🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/imio/smartweb/core/tests/test_utils.py` around lines 146 - 153, Update the test_get_ts_api_url_valid_url test to assert the exact expected URL rather than loose substrings and add a new assertion covering a non-"wcs" service branch: when patching imio.smartweb.core.utils.get_value_from_registry to return "https://mysite.guichet-citoyen.be", assert that get_ts_api_url("wcs") equals the full expected string (e.g. "https://mysite-formulaires.guichet-citoyen.be/api") and add an additional call asserting get_ts_api_url("other_service") returns the correctly formed URL for non-wcs services; keep using the existing patch on get_value_from_registry and the same test function (test_get_ts_api_url_valid_url) or add a sibling test to cover the non-wcs case.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/imio/smartweb/core/tests/test_utils.py`:
- Around line 159-164: The test test_populate_procedure_button_text currently
mutates the global registry key "smartweb.procedure_button_text" via
populate_procedure_button_text() and doesn't restore it; modify the test to read
and save the original registry value (via
getUtility(IRegistry).get("smartweb.procedure_button_text")) before calling
populate_procedure_button_text(), run the assertions, and then restore the saved
value in a finally block (or tearDown) so the registry is returned to its prior
state even if assertions fail.
- Around line 154-157: The test test_get_plausible_vars_not_set is flaky because
get_plausible_vars() reads SMARTWEB_PLAUSIBLE_* env vars; make the test
deterministic by ensuring those env vars are removed/isolated before calling
get_plausible_vars(): in the test function (test_get_plausible_vars_not_set)
temporarily clear or monkeypatch os.environ entries for the SMARTWEB_PLAUSIBLE_*
keys the code checks (or use pytest's monkeypatch.delenv for each relevant key)
so the function only sees registry defaults (None), then call
get_plausible_vars() and assertIsNone(result), restoring the environment
afterwards.
---
Nitpick comments:
In `@src/imio/smartweb/core/tests/test_section_external_content.py`:
- Around line 220-257: Add two new tests mirroring test_idelux_waste_plugin and
test_inbw_containers_affluence_plugin that exercise the timeout branch: in each
test create the same SectionExternalContent (sec), set external_content_url to
the same odwb endpoints, then use requests_mock_module.Mocker() to register the
GET for the plugin URL to raise requests.exceptions.Timeout (or
requests.exceptions.ReadTimeout) instead of returning JSON; call section_view =
queryMultiAdapter((sec, self.request), name="view") and access
section_view.contents to ensure the code handles the timeout without raising and
returns a safe/falsy result (assert that section_view.contents is falsy), and
also verify section_view.get_plugin() and section_view.which_plugin() still
identify the plugin (use the same plugin interface checks IIdeluxWastePlugin and
IInbwContainersAffluencePlugin and which_plugin() string checks as in the
existing tests).
In `@src/imio/smartweb/core/tests/test_utils.py`:
- Around line 146-153: Update the test_get_ts_api_url_valid_url test to assert
the exact expected URL rather than loose substrings and add a new assertion
covering a non-"wcs" service branch: when patching
imio.smartweb.core.utils.get_value_from_registry to return
"https://mysite.guichet-citoyen.be", assert that get_ts_api_url("wcs") equals
the full expected string (e.g.
"https://mysite-formulaires.guichet-citoyen.be/api") and add an additional call
asserting get_ts_api_url("other_service") returns the correctly formed URL for
non-wcs services; keep using the existing patch on get_value_from_registry and
the same test function (test_get_ts_api_url_valid_url) or add a sibling test to
cover the non-wcs case.
ℹ️ Review info
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/imio/smartweb/core/tests/test_section_external_content.pysrc/imio/smartweb/core/tests/test_utils.py
Comment on lines
+154
to
+157
| def test_get_plausible_vars_not_set(self): | ||
| # By default, registry values are None, so get_plausible_vars returns None | ||
| result = get_plausible_vars() | ||
| self.assertIsNone(result) |
There was a problem hiding this comment.
Make test_get_plausible_vars_not_set deterministic (avoid env-driven flakiness).
This test can fail when SMARTWEB_PLAUSIBLE_* is present in the test process environment, because get_plausible_vars() prioritizes env vars over registry values.
Proposed fix
def test_get_plausible_vars_not_set(self):
- # By default, registry values are None, so get_plausible_vars returns None
- result = get_plausible_vars()
- self.assertIsNone(result)
+ with patch.dict(
+ "os.environ",
+ {
+ "SMARTWEB_PLAUSIBLE_URL": "",
+ "SMARTWEB_PLAUSIBLE_SITE": "",
+ "SMARTWEB_PLAUSIBLE_TOKEN": "",
+ },
+ ), patch("plone.api.portal.get_registry_record") as mock_registry:
+ mock_registry.return_value = None
+ result = get_plausible_vars()
+ self.assertIsNone(result)📝 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 test_get_plausible_vars_not_set(self): | |
| # By default, registry values are None, so get_plausible_vars returns None | |
| result = get_plausible_vars() | |
| self.assertIsNone(result) | |
| def test_get_plausible_vars_not_set(self): | |
| with patch.dict( | |
| "os.environ", | |
| { | |
| "SMARTWEB_PLAUSIBLE_URL": "", | |
| "SMARTWEB_PLAUSIBLE_SITE": "", | |
| "SMARTWEB_PLAUSIBLE_TOKEN": "", | |
| }, | |
| ), patch("plone.api.portal.get_registry_record") as mock_registry: | |
| mock_registry.return_value = None | |
| result = get_plausible_vars() | |
| self.assertIsNone(result) |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/imio/smartweb/core/tests/test_utils.py` around lines 154 - 157, The test
test_get_plausible_vars_not_set is flaky because get_plausible_vars() reads
SMARTWEB_PLAUSIBLE_* env vars; make the test deterministic by ensuring those env
vars are removed/isolated before calling get_plausible_vars(): in the test
function (test_get_plausible_vars_not_set) temporarily clear or monkeypatch
os.environ entries for the SMARTWEB_PLAUSIBLE_* keys the code checks (or use
pytest's monkeypatch.delenv for each relevant key) so the function only sees
registry defaults (None), then call get_plausible_vars() and
assertIsNone(result), restoring the environment afterwards.
Comment on lines
+159
to
+164
| def test_populate_procedure_button_text(self): | ||
| populate_procedure_button_text() | ||
| registry = getUtility(IRegistry) | ||
| labels = registry.get("smartweb.procedure_button_text") | ||
| self.assertIsNotNone(labels) | ||
| self.assertGreater(len(labels), 0) |
There was a problem hiding this comment.
Restore registry state in test_populate_procedure_button_text to keep tests isolated.
This test writes a global registry key and does not restore it, which can leak state to later tests.
Proposed fix
def test_populate_procedure_button_text(self):
- populate_procedure_button_text()
registry = getUtility(IRegistry)
+ original = registry.get("smartweb.procedure_button_text", None)
+
+ def _restore():
+ if original is None:
+ try:
+ del registry["smartweb.procedure_button_text"]
+ except Exception:
+ pass
+ else:
+ registry["smartweb.procedure_button_text"] = original
+
+ self.addCleanup(_restore)
+ populate_procedure_button_text()
labels = registry.get("smartweb.procedure_button_text")
self.assertIsNotNone(labels)
self.assertGreater(len(labels), 0)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/imio/smartweb/core/tests/test_utils.py` around lines 159 - 164, The test
test_populate_procedure_button_text currently mutates the global registry key
"smartweb.procedure_button_text" via populate_procedure_button_text() and
doesn't restore it; modify the test to read and save the original registry value
(via getUtility(IRegistry).get("smartweb.procedure_button_text")) before calling
populate_procedure_button_text(), run the assertions, and then restore the saved
value in a finally block (or tearDown) so the registry is returned to its prior
state even if assertions fail.
There was a problem hiding this comment.
🧹 Nitpick comments (2)
src/imio/smartweb/core/tests/test_section_external_content.py (1)
143-290: Consider extracting a small section factory helper to reduce test duplication.The repeated
SectionExternalContentcreation and URL/params wiring appears in many tests; a helper would improve readability and maintenance.♻️ Example refactor
class TestSectionExternalContent(ImioSmartwebTestCase): layer = IMIO_SMARTWEB_CORE_FUNCTIONAL_TESTING def setUp(self): self.request = self.layer["request"] self.portal = self.layer["portal"] setRoles(self.portal, TEST_USER_ID, ["Manager"]) self.page = api.content.create( container=self.portal, type="imio.smartweb.Page", id="page", ) + + def _create_external_section(self, url=None, params=None): + sec = api.content.create( + container=self.page, type="imio.smartweb.SectionExternalContent", id="sec" + ) + if url is not None: + sec.external_content_url = url + if params is not None: + sec.external_content_params = params + return sec🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/imio/smartweb/core/tests/test_section_external_content.py` around lines 143 - 290, Many tests duplicate creating a SectionExternalContent and setting external_content_url/params; add a small factory helper (e.g., create_section_external_content or make_section) inside the test class to centralize api.content.create(..., type="imio.smartweb.SectionExternalContent") and optional url/params wiring; update tests like test_cognitoform_plugin, test_arcgis_plugin, test_giveaday_plugin, test_idelux_waste_plugin, test_inbw_containers_affluence_plugin, test_odwb_widget_plugin, and test_external_content_view_methods to call this helper instead of repeating creation and assignment, and ensure the helper returns the created sec for further modifications and assertions.src/imio/smartweb/core/tests/test_search_endpoint.py (1)
1-416: Good test coverage for the caching refactor.This new test module provides solid coverage for the search endpoint functionality, aligning well with the PR objective of refactoring caching with specialized endpoints. The tests cover:
- Helper functions (cache keys, view lookups, navigation root)
ExtendedSearchHandlermethods including URL construction and translation handling- REST service delegation
- Browser view error handling and URL generation
Consider adding a test for the
_adapt_resultmethod with a non-French language parameter to verify the language is passed through to_adapt_result_values.💡 Optional: Add language parameter test for _adapt_result
def test_adapt_result_with_language_passes_to_values(self): handler = ExtendedSearchHandler(self.portal, self.request) mock_result = { "items": [ { "@type": "imio.news.NewsItem", "container_uid": "some-uid", "UID": "item-uid", "path_string": "/plone/news/item", "modified": "2021-09-14T08:00:00", "title_nl": "Dutch title", "description_nl": "Dutch desc", } ], "items_total": 1, } result = handler._adapt_result(mock_result, "news", language="nl") self.assertEqual(result["items"][0]["title"], "Dutch title")🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/imio/smartweb/core/tests/test_search_endpoint.py` around lines 1 - 416, Add a new unit test in TestExtendedSearchHandler that verifies ExtendedSearchHandler._adapt_result forwards the language parameter to _adapt_result_values: create mock_result with one imio.news.NewsItem containing title_nl/description_nl, call handler._adapt_result(mock_result, "news", language="nl"), and assert the returned item's "title" and "description" are the Dutch values and the language-specific keys are removed; reference ExtendedSearchHandler, _adapt_result, and _adapt_result_values to locate the code under test.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@src/imio/smartweb/core/tests/test_search_endpoint.py`:
- Around line 1-416: Add a new unit test in TestExtendedSearchHandler that
verifies ExtendedSearchHandler._adapt_result forwards the language parameter to
_adapt_result_values: create mock_result with one imio.news.NewsItem containing
title_nl/description_nl, call handler._adapt_result(mock_result, "news",
language="nl"), and assert the returned item's "title" and "description" are the
Dutch values and the language-specific keys are removed; reference
ExtendedSearchHandler, _adapt_result, and _adapt_result_values to locate the
code under test.
In `@src/imio/smartweb/core/tests/test_section_external_content.py`:
- Around line 143-290: Many tests duplicate creating a SectionExternalContent
and setting external_content_url/params; add a small factory helper (e.g.,
create_section_external_content or make_section) inside the test class to
centralize api.content.create(..., type="imio.smartweb.SectionExternalContent")
and optional url/params wiring; update tests like test_cognitoform_plugin,
test_arcgis_plugin, test_giveaday_plugin, test_idelux_waste_plugin,
test_inbw_containers_affluence_plugin, test_odwb_widget_plugin, and
test_external_content_view_methods to call this helper instead of repeating
creation and assignment, and ensure the helper returns the created sec for
further modifications and assertions.
ℹ️ Review info
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
src/imio/smartweb/core/tests/test_search_endpoint.pysrc/imio/smartweb/core/tests/test_section_external_content.pysrc/imio/smartweb/core/tests/test_utils.py
🚧 Files skipped from review as they are similar to previous changes (1)
- src/imio/smartweb/core/tests/test_utils.py
There was a problem hiding this comment.
🧹 Nitpick comments (3)
src/imio/smartweb/core/tests/test_section_external_content.py (2)
228-233: Assert outbound ODWB request details explicitly.Right now the tests only assert returned content/plugin type. Add request-history assertions so regressions in query construction (
limit=-1, existing query preservation) fail fast.Suggested test hardening
with requests_mock_module.Mocker() as m: m.get(f"{odwb_url}?limit=-1", json=mock_data) section_view = queryMultiAdapter((sec, self.request), name="view") result = section_view.contents self.assertEqual(result, mock_data) + self.assertTrue(m.called) + self.assertEqual(m.call_count, 1) + self.assertEqual(m.request_history[0].qs.get("limit"), ["-1"]) self.assertTrue(IIdeluxWastePlugin.providedBy(section_view.get_plugin()))with requests_mock_module.Mocker() as m: m.get(f"{odwb_url}&limit=-1", json=mock_data) section_view = queryMultiAdapter((sec, self.request), name="view") import json result = section_view.contents self.assertEqual(result, json.dumps(mock_data)) + self.assertTrue(m.called) + self.assertEqual(m.call_count, 1) + self.assertEqual(m.request_history[0].qs.get("dataset"), ["affluence"]) + self.assertEqual(m.request_history[0].qs.get("limit"), ["-1"])Also applies to: 244-246
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/imio/smartweb/core/tests/test_section_external_content.py` around lines 228 - 233, Add assertions that the mocked ODWB request was made with the expected query string and parameters: after creating the mocker and calling section_view.contents, inspect the request history on the requests_mock_module.Mocker instance (e.g., m.request_history or m.last_request) to assert the request URL equals odwb_url with the query "limit=-1" and that any pre-existing query params on odwb_url are preserved; apply the same pattern to the analogous assertions in the other test block mentioned (lines 244-246) so the test fails if query construction changes.
232-233: Normalize payload assertions to one contract style.Line 232 asserts a dict, while Line 250 asserts a JSON string for very similar ODWB payload flows. This makes tests brittle to serialization refactors rather than behavior.
Suggested normalization pattern
+import json ... - result = section_view.contents - self.assertEqual(result, mock_data) + payload = section_view.contents + if isinstance(payload, str): + payload = json.loads(payload) + self.assertEqual(payload, mock_data)Apply the same pattern to both tests so they validate data semantics consistently.
Also applies to: 249-250
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/imio/smartweb/core/tests/test_section_external_content.py` around lines 232 - 233, The two assertions in this test file use inconsistent payload contracts (one compares a dict: assertEqual(result, mock_data) and another compares a JSON string elsewhere) which makes tests brittle; pick a single canonical contract (preferably compare deserialized Python objects for semantic equality) and update the assertions that check the ODWB payload (e.g., the variable result and the other assertion at lines 249-250) to parse any JSON payloads to dicts before asserting equality against mock_data, keeping the plugin presence assertion (IIdeluxWastePlugin.providedBy(section_view.get_plugin())) unchanged.src/imio/smartweb/core/tests/test_init.py (1)
131-158: Consider using underscore prefix for unused parameters.The
TrackedImage.getFirstBytesmethod has unused*args, **kwargsparameters flagged by static analysis. Since these are needed to match the interface signature but intentionally unused, prefix them with underscores.♻️ Suggested fix
- def getFirstBytes(self, *args, **kwargs): + def getFirstBytes(self, *_args, **_kwargs): return b"data"🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/imio/smartweb/core/tests/test_init.py` around lines 131 - 158, The test's TrackedImage.getFirstBytes currently declares used-but-unused parameters as *args, **kwargs; change the signature to use underscore-prefixed names (e.g., getFirstBytes(self, *_args, **_kwargs)) so the parameters still match the expected interface but are marked as intentionally unused; update only the signature in the TrackedImage class inside the test_init.py snippet.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@src/imio/smartweb/core/tests/test_init.py`:
- Around line 131-158: The test's TrackedImage.getFirstBytes currently declares
used-but-unused parameters as *args, **kwargs; change the signature to use
underscore-prefixed names (e.g., getFirstBytes(self, *_args, **_kwargs)) so the
parameters still match the expected interface but are marked as intentionally
unused; update only the signature in the TrackedImage class inside the
test_init.py snippet.
In `@src/imio/smartweb/core/tests/test_section_external_content.py`:
- Around line 228-233: Add assertions that the mocked ODWB request was made with
the expected query string and parameters: after creating the mocker and calling
section_view.contents, inspect the request history on the
requests_mock_module.Mocker instance (e.g., m.request_history or m.last_request)
to assert the request URL equals odwb_url with the query "limit=-1" and that any
pre-existing query params on odwb_url are preserved; apply the same pattern to
the analogous assertions in the other test block mentioned (lines 244-246) so
the test fails if query construction changes.
- Around line 232-233: The two assertions in this test file use inconsistent
payload contracts (one compares a dict: assertEqual(result, mock_data) and
another compares a JSON string elsewhere) which makes tests brittle; pick a
single canonical contract (preferably compare deserialized Python objects for
semantic equality) and update the assertions that check the ODWB payload (e.g.,
the variable result and the other assertion at lines 249-250) to parse any JSON
payloads to dicts before asserting equality against mock_data, keeping the
plugin presence assertion
(IIdeluxWastePlugin.providedBy(section_view.get_plugin())) unchanged.
ℹ️ Review info
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
src/imio/smartweb/core/tests/test_init.pysrc/imio/smartweb/core/tests/test_search_endpoint.pysrc/imio/smartweb/core/tests/test_section_external_content.pysrc/imio/smartweb/core/tests/test_utils.py
🚧 Files skipped from review as they are similar to previous changes (1)
- src/imio/smartweb/core/tests/test_utils.py
boulch
force-pushed
the
ram_cache
branch
7 times, most recently
from
6f7ec7c to
678b869
Compare
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
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.
…incorrect/random? entity/newsfolders retrieval from cache
Summary by CodeRabbit
Bug Fixes
User Experience
Tests
Changelog