feat(gdi_userportal): honor accept-language for enhanced package resp… #214
Conversation
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
Reviewer's GuideThis PR adds full multilingual support to enhanced package responses by honoring the Accept-Language header, extends the scheming schema with GDI-specific and translated fields, and updates core translation utilities, action handlers, plugin hooks, and corresponding tests. File-Level Changes
Possibly linked issues
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey there - I've reviewed your changes - here's some feedback:
- Instead of passing the raw Accept-Language header through, use a proper parser to extract and prioritize supported language codes (e.g. handling q-values and language subtags) before calling replace_package.
- The massive scheming YAML could be refactored using YAML anchors, includes, or split into smaller modular files to avoid repetition and improve maintainability of the field definitions.
- Consider adding unit tests for replace_search_facets (including translations and fallback logic) to ensure facet titles are correctly localized and fallback as expected.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- Instead of passing the raw Accept-Language header through, use a proper parser to extract and prioritize supported language codes (e.g. handling q-values and language subtags) before calling replace_package.
- The massive scheming YAML could be refactored using YAML anchors, includes, or split into smaller modular files to avoid repetition and improve maintainability of the field definitions.
- Consider adding unit tests for replace_search_facets (including translations and fallback logic) to ensure facet titles are correctly localized and fallback as expected.
## Individual Comments
### Comment 1
<location> `ckanext/gdi_userportal/tests/test_translation_utils.py:68-84` </location>
<code_context>
+ }
+
+
+def test_replace_package_prefers_requested_language():
+ package = deepcopy(_base_package())
+
+ result = replace_package(package, translation_dict={}, lang="nl")
+
+ assert result["title"] == "Nederlandse titel"
+ assert result["notes"] == "Nederlandse toelichting"
+ assert result["provenance"] == "Nederlandse herkomst"
+ assert result["population_coverage"] == "Nederlandse dekking"
+ assert result["publisher_note"] == "Nederlandse uitgeversnotitie"
+
+ resource = result["resources"][0]
+ assert resource["name"] == "Nederlandse resource"
+ assert resource["rights"] == "Nederlandse rechten"
+
+ attribution_agent = result["qualified_attribution"][0]["agent"][0]
+ assert attribution_agent["name"] == "Nederlandse agent"
+
</code_context>
<issue_to_address>
**suggestion (testing):** Missing test for edge cases where the requested language is present but the value is empty or None.
Add a test where the requested language key exists but its value is empty or None to verify correct fallback behavior.
```suggestion
def test_replace_package_prefers_requested_language():
package = deepcopy(_base_package())
result = replace_package(package, translation_dict={}, lang="nl")
assert result["title"] == "Nederlandse titel"
assert result["notes"] == "Nederlandse toelichting"
assert result["provenance"] == "Nederlandse herkomst"
assert result["population_coverage"] == "Nederlandse dekking"
assert result["publisher_note"] == "Nederlandse uitgeversnotitie"
resource = result["resources"][0]
assert resource["name"] == "Nederlandse resource"
assert resource["rights"] == "Nederlandse rechten"
attribution_agent = result["qualified_attribution"][0]["agent"][0]
assert attribution_agent["name"] == "Nederlandse agent"
def test_replace_package_requested_language_empty_or_none():
package = deepcopy(_base_package())
# Set the requested language values to empty string and None
package["title"] = {"en": "English title", "nl": ""}
package["notes"] = {"en": "English notes", "nl": None}
package["provenance"] = {"en": "English provenance", "nl": ""}
package["population_coverage"] = {"en": "English coverage", "nl": None}
package["publisher_note"] = {"en": "English publisher note", "nl": ""}
package["resources"][0]["name"] = {"en": "English resource", "nl": ""}
package["resources"][0]["rights"] = {"en": "English rights", "nl": None}
package["qualified_attribution"][0]["agent"][0]["name"] = {"en": "English agent", "nl": ""}
result = replace_package(package, translation_dict={}, lang="nl")
# Should fallback to English when nl is empty or None
assert result["title"] == "English title"
assert result["notes"] == "English notes"
assert result["provenance"] == "English provenance"
assert result["population_coverage"] == "English coverage"
assert result["publisher_note"] == "English publisher note"
resource = result["resources"][0]
assert resource["name"] == "English resource"
assert resource["rights"] == "English rights"
attribution_agent = result["qualified_attribution"][0]["agent"][0]
assert attribution_agent["name"] == "English agent"
```
</issue_to_address>
### Comment 2
<location> `ckanext/gdi_userportal/tests/test_translation_utils.py:87-83` </location>
<code_context>
+ assert attribution_agent["name"] == "Nederlandse agent"
+
+
+def test_replace_package_falls_back_to_default_language():
+ package = deepcopy(_base_package())
+
+ result = replace_package(package, translation_dict={}, lang="fr")
+
+ assert result["title"] == "English Title"
+ assert result["notes"] == "English Notes"
+ assert result["provenance"] == "English provenance"
+ assert result["population_coverage"] == "English coverage"
+ assert result["publisher_note"] == "English publisher note"
+
+ resource = result["resources"][0]
+ assert resource["name"] == "English resource"
+ assert resource["rights"] == "English rights"
+
+ attribution_agent = result["qualified_attribution"][0]["agent"][0]
+ assert attribution_agent["name"] == "English agent"
</code_context>
<issue_to_address>
**suggestion (testing):** Missing test for scenario where neither requested nor default language is present in the translation dict.
Please add a test where both the requested and default languages are absent, but another language exists, to confirm correct fallback behavior.
</issue_to_address>
### Comment 3
<location> `ckanext/gdi_userportal/helpers.py:64-70` </location>
<code_context>
def _is_missing_value(value: Any) -> bool:
if value is None:
return True
if isinstance(value, str):
return value.strip() == ""
if isinstance(value, dict):
if not value:
return True
return all(_is_missing_value(v) for v in value.values())
if isinstance(value, (list, tuple, set)):
if not value:
return True
return all(_is_missing_value(v) for v in value)
return False
</code_context>
<issue_to_address>
**issue (code-quality):** We've found these issues:
- Lift code into else after jump in control flow [×2] ([`reintroduce-else`](https://docs.sourcery.ai/Reference/Default-Rules/refactorings/reintroduce-else/))
- Replace if statement with if expression [×2] ([`assign-if-exp`](https://docs.sourcery.ai/Reference/Default-Rules/refactorings/assign-if-exp/))
</issue_to_address>
### Comment 4
<location> `ckanext/gdi_userportal/helpers.py:109-110` </location>
<code_context>
def scheming_missing_required_fields(
pages: list[dict[str, Any]],
data: dict[str, Any] | None = None,
package_id: str | None = None,
) -> list[list[str]]:
"""Return a list of missing required fields grouped per form page.
This helper acts as the base implementation expected by
``ckanext-fluent``. It mirrors the behaviour from the forked
ckanext-scheming version previously used in this project and makes sure
chained helpers can extend the result again.
"""
data_dict = _ensure_data_dict(data, package_id)
missing_per_page: list[list[str]] = []
for page in pages or []:
page_missing: list[str] = []
for field in page.get("fields", []):
# Ignore non-required fields early.
if not tk.h.scheming_field_required(field):
continue
value = _extract_field_value(field, data_dict)
# Repeating subfields can contain a list of child values; treat the
# field as present when at least one entry contains data.
if field.get("repeating_subfields") and isinstance(value, list):
if any(not _is_missing_value(item) for item in value):
continue
elif not _is_missing_value(value):
continue
field_name = field.get("field_name")
if field_name:
page_missing.append(field_name)
missing_per_page.append(page_missing)
return missing_per_page
</code_context>
<issue_to_address>
**suggestion (code-quality):** Use named expression to simplify assignment and conditional ([`use-named-expression`](https://docs.sourcery.ai/Reference/Default-Rules/refactorings/use-named-expression/))
```suggestion
if field_name := field.get("field_name"):
```
</issue_to_address>
### Comment 5
<location> `ckanext/gdi_userportal/logic/action/translation_utils.py:184` </location>
<code_context>
def _apply_translated_properties(data: Any, preferred_lang: str, fallback_lang: str = DEFAULT_FALLBACK_LANG):
if isinstance(data, dict):
for key, value in list(data.items()):
if isinstance(value, dict):
_apply_translated_properties(value, preferred_lang, fallback_lang)
elif isinstance(value, list):
data[key] = [
_apply_translated_properties(item, preferred_lang, fallback_lang)
if isinstance(item, (dict, list))
else item
for item in value
]
for key, value in list(data.items()):
if key.endswith(TRANSLATED_SUFFIX) and isinstance(value, dict):
base_key = key[:-len(TRANSLATED_SUFFIX)]
data[base_key] = _select_translated_value(value, preferred_lang, fallback_lang)
elif key in LANGUAGE_VALUE_FIELDS and isinstance(value, dict):
data[key] = _select_translated_value(value, preferred_lang, fallback_lang)
return data
if isinstance(data, list):
return [
_apply_translated_properties(item, preferred_lang, fallback_lang)
if isinstance(item, (dict, list))
else item
for item in data
]
return data
</code_context>
<issue_to_address>
**issue (code-quality):** We've found these issues:
- Add guard clause ([`last-if-guard`](https://docs.sourcery.ai/Reference/Default-Rules/refactorings/last-if-guard/))
- Lift code into else after jump in control flow ([`reintroduce-else`](https://docs.sourcery.ai/Reference/Default-Rules/refactorings/reintroduce-else/))
- Replace if statement with if expression ([`assign-if-exp`](https://docs.sourcery.ai/Reference/Default-Rules/refactorings/assign-if-exp/))
</issue_to_address>
### Comment 6
<location> `ckanext/gdi_userportal/logic/action/translation_utils.py:237-239` </location>
<code_context>
def _has_content(value: Any) -> bool:
if value is None:
return False
if isinstance(value, str):
return bool(value.strip())
if isinstance(value, (list, dict)):
return bool(value)
return True
</code_context>
<issue_to_address>
**suggestion (code-quality):** We've found these issues:
- Lift code into else after jump in control flow ([`reintroduce-else`](https://docs.sourcery.ai/Reference/Default-Rules/refactorings/reintroduce-else/))
- Replace if statement with if expression ([`assign-if-exp`](https://docs.sourcery.ai/Reference/Default-Rules/refactorings/assign-if-exp/))
```suggestion
return bool(value) if isinstance(value, (list, dict)) else True
```
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
Co-authored-by: sourcery-ai[bot] <58596630+sourcery-ai[bot]@users.noreply.github.com>
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.
…onses
Summary by Sourcery
Implement comprehensive multilingual support by extending scheming schemas with translated fields, updating translation utilities to honor the Accept-Language header, integrating template helpers, and adding related tests
New Features:
Enhancements:
Tests: