add relative_link to link field #254
Open
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
Provide functionality to link internal pages not directly accessible via 'internal_link', e.g. apphook-urls.
Reviewer's GuideThis PR adds support for a new “relative_link” type, extending the existing link widget, form field, validators, model helpers, and tests to handle relative URL paths as a first-class link option. Entity relationship diagram for link_types and allowed_link_typeserDiagram
LINK_TYPE {
internal_link varchar
relative_link varchar
external_link varchar
file_link varchar
anchor varchar
mailto varchar
tel varchar
}
ALLOWED_LINK_TYPE {
internal_link varchar
relative_link varchar
external_link varchar
file_link varchar
anchor varchar
mailto varchar
tel varchar
}
LINK_TYPE ||--|| ALLOWED_LINK_TYPE : "is allowed"
Class diagram for LinkFormField and RelativeURLValidator changesclassDiagram
class LinkFormField {
+external_link_validators: list
+relative_link_validators: list
+internal_link_validators: list
+file_link_validators: list
+anchor_validators: list
+prepare_value(value: dict): list[str | None]
}
class RelativeURLValidator {
+message: str
+code: str
+allowed_link_types: list
+__call__(value: str)
}
LinkFormField --> RelativeURLValidator : uses
Class diagram for LinkDict changesclassDiagram
class LinkDict {
+__init__(initial=None, **kwargs)
+url: str
+type: str
}
LinkDict : +relative_link
LinkDict : +external_link
LinkDict : +internal_link
LinkDict : +file_link
LinkDict : +anchor
File-Level Changes
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:
- Fix the typo in RelativeURLValidator’s message (change 'realtive' to 'relative').
- The .format(example_uri_scheme) call in the relative link widget help text is redundant (there’s no placeholder) and can be removed.
- Consider extracting the list of link types into a shared constant to avoid having to update fields, widgets, helpers, and CSS in multiple places when adding new types.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- Fix the typo in RelativeURLValidator’s message (change 'realtive' to 'relative').
- The .format(example_uri_scheme) call in the relative link widget help text is redundant (there’s no placeholder) and can be removed.
- Consider extracting the list of link types into a shared constant to avoid having to update fields, widgets, helpers, and CSS in multiple places when adding new types.
## Individual Comments
### Comment 1
<location> `djangocms_link/validators.py:117` </location>
<code_context>
+
+@deconstructible
+class RelativeURLValidator:
+ message = _("Enter a valid realtive link")
+ code = "invalid"
+
</code_context>
<issue_to_address>
**issue (typo):** Typo in validator message: 'realtive' should be 'relative'.
Fixing the typo ensures error messages are clear and understandable.
```suggestion
message = _("Enter a valid relative link")
```
</issue_to_address>
### Comment 2
<location> `djangocms_link/helpers.py:52-53` </location>
<code_context>
if link_field_value["external_link"].startswith("tel:"):
return link_field_value["external_link"].replace(" ", "")
return link_field_value["external_link"] or None
+ elif "relative_link" in link_field_value:
+ return link_field_value["relative_link"] or None
</code_context>
<issue_to_address>
**suggestion:** Relative links are now returned directly; consider normalization for leading/trailing whitespace.
Strip whitespace from relative links before returning to prevent subtle bugs.
```suggestion
elif "relative_link" in link_field_value:
relative_link = link_field_value["relative_link"]
if relative_link is not None:
relative_link = relative_link.strip()
return relative_link or None
```
</issue_to_address>
### Comment 3
<location> `tests/test_fields.py:104` </location>
<code_context>
self.assertEqual(form.cleaned_data["link_field"], value)
check_value({"internal_link": f"cms.page:{self.page.id}", "anchor": "#anchor"})
+ check_value({"relative_link": "/some/path"})
check_value({"external_link": "https://example.com"})
check_value({"external_link": "#anchor"})
</code_context>
<issue_to_address>
**suggestion (testing):** Consider adding tests for invalid relative links and edge cases.
Adding tests for invalid relative links, such as those missing a leading slash, containing unsafe characters, or being excessively long, will help verify the validator's error handling.
```suggestion
check_value({"relative_link": "/some/path"})
# Invalid relative link: missing leading slash
with self.assertRaises(forms.ValidationError):
check_value({"relative_link": "some/path"})
# Invalid relative link: unsafe characters
with self.assertRaises(forms.ValidationError):
check_value({"relative_link": "/some/<path>"})
# Invalid relative link: excessively long path
long_path = "/" + "a" * 2048
with self.assertRaises(forms.ValidationError):
check_value({"relative_link": long_path})
```
</issue_to_address>
### Comment 4
<location> `djangocms_link/fields.py:153` </location>
<code_context>
# Configure the LinkWidget
link_types = {
"internal_link": _("Internal link"),
+ "relative_link": _("Relative link"),
"external_link": _("External link/anchor"),
}
</code_context>
<issue_to_address>
**issue (complexity):** Consider refactoring link type handling into a single configuration dictionary to centralize logic and reduce duplication.
```markdown
Rather than sprinkling `relative_link` (and any future link types) through `link_types`, `allowed_link_types`, `widgets`, `validators` and `prepare_value`, you can collapse all of that into a single data‐driven mapping.
1) Define a top‐level config dict:
```python
LINK_TYPE_CONFIG = {
'internal_link': {
'label': _('Internal link'),
'widget_cls': LinkAutoCompleteWidget,
'widget_attrs': {
'widget': 'internal_link',
'data-help': _('Select from available internal destinations…'),
'data-placeholder': _('Select internal destination'),
},
'validator_cls': None,
},
'external_link': {
'label': _('External link/anchor'),
'widget_cls': URLInput,
'widget_attrs': {
'widget': 'external_link',
'placeholder': _('https://example.com or #anchor'),
'data-help': _('Provide a link to an external URL…'),
},
'validator_cls': ExtendedURLValidator,
},
'relative_link': {
'label': _('Relative link'),
'widget_cls': TextInput,
'widget_attrs': {
'widget': 'relative_link',
'placeholder': _('/some/path - optionally append #anchor'),
'data-help': _('Provide a relative link…'),
},
'validator_cls': RelativeURLValidator,
},
# file_link, anchor, mailto, tel, etc.
}
```
2) Generate `link_types`, `widgets` and `validators` by iterating that dict:
```python
allowed = set(getattr(settings, 'DJANGOCMS_LINK_ALLOWED_LINK_TYPES', LINK_TYPE_CONFIG.keys()))
link_types = {key: cfg['label'] for key, cfg in LINK_TYPE_CONFIG.items() if key in allowed}
# in LinkWidget.__init__:
self.widgets = [
cfg['widget_cls'](attrs=cfg['widget_attrs'])
for key, cfg in LINK_TYPE_CONFIG.items()
if key in allowed
]
# in LinkFormField:
self.validators = [
cfg['validator_cls'](allowed_link_types=allowed)
for key, cfg in LINK_TYPE_CONFIG.items()
if key in allowed and cfg['validator_cls']
]
```
3) Simplify `prepare_value` to a loop instead of multiple `elif`s:
```python
def prepare_value(self, value: dict) -> list[str | None]:
if isinstance(value, list):
return value
buckets = [None] * len(self.widget.widgets)
for key in LINK_TYPE_CONFIG:
if key in value:
buckets[0] = key
buckets[self._get_pos(key)] = value[key]
break
return buckets
```
This removes all of the duplicated boilerplate whenever you add a new link type (just one entry in `LINK_TYPE_CONFIG`).
</issue_to_address>
### Comment 5
<location> `djangocms_link/validators.py:115` </location>
<code_context>
return super().__call__(value)
+
+
+@deconstructible
+class RelativeURLValidator:
+ message = _("Enter a valid realtive link")
</code_context>
<issue_to_address>
**issue (complexity):** Consider refactoring the validation logic to use early returns and a helper function for error handling to improve readability.
Here’s one way to collapse the nested logic into a straight‐line of early checks plus a single helper for raising errors. This keeps every check, but it’s easier to scan and maintains the same behavior:
```python
@deconstructible
class RelativeURLValidator:
message = _("Enter a valid relative link")
code = "invalid"
def __init__(self, allowed_link_types: list = None, **kwargs):
self.allowed_link_types = allowed_link_types
super().__init__(**kwargs)
def __call__(self, value: str):
# simple helper to reduce repetition
def _fail():
raise ValidationError(self.message, code=self.code, params={"value": value})
if not isinstance(value, str):
_fail()
if len(value) > URLValidator.max_length:
_fail()
if URLValidator.unsafe_chars.intersection(value):
_fail()
# must start with “/”
if not value.startswith("/"):
_fail()
# if caller has restricted link types, enforce “relative_link”
if self.allowed_link_types and "relative_link" not in self.allowed_link_types:
_fail()
return value
```
Steps to apply:
1. Extract a small `_fail()` inner function so you don’t repeat the `ValidationError` call.
2. Turn each condition into an early return (`_fail()`), so you avoid nested `and`/`or` logic.
3. Split the “must start with `/`” check from the “allowed_link_types” check to keep each test atomic.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
Comment on lines
+52
to
+53
| elif "relative_link" in link_field_value: | ||
| return link_field_value["relative_link"] or None |
There was a problem hiding this comment.
suggestion: Relative links are now returned directly; consider normalization for leading/trailing whitespace.
Strip whitespace from relative links before returning to prevent subtle bugs.
Suggested change
| elif "relative_link" in link_field_value: | |
| return link_field_value["relative_link"] or None | |
| elif "relative_link" in link_field_value: | |
| relative_link = link_field_value["relative_link"] | |
| if relative_link is not None: | |
| relative_link = relative_link.strip() | |
| return relative_link or None |
| return super().__call__(value) | ||
|
|
||
|
|
||
| @deconstructible |
There was a problem hiding this comment.
issue (complexity): Consider refactoring the validation logic to use early returns and a helper function for error handling to improve readability.
Here’s one way to collapse the nested logic into a straight‐line of early checks plus a single helper for raising errors. This keeps every check, but it’s easier to scan and maintains the same behavior:
@deconstructible
class RelativeURLValidator:
message = _("Enter a valid relative link")
code = "invalid"
def __init__(self, allowed_link_types: list = None, **kwargs):
self.allowed_link_types = allowed_link_types
super().__init__(**kwargs)
def __call__(self, value: str):
# simple helper to reduce repetition
def _fail():
raise ValidationError(self.message, code=self.code, params={"value": value})
if not isinstance(value, str):
_fail()
if len(value) > URLValidator.max_length:
_fail()
if URLValidator.unsafe_chars.intersection(value):
_fail()
# must start with “/”
if not value.startswith("/"):
_fail()
# if caller has restricted link types, enforce “relative_link”
if self.allowed_link_types and "relative_link" not in self.allowed_link_types:
_fail()
return valueSteps to apply:
- Extract a small
_fail()inner function so you don’t repeat theValidationErrorcall. - Turn each condition into an early return (
_fail()), so you avoid nestedand/orlogic. - Split the “must start with
/” check from the “allowed_link_types” check to keep each test atomic.
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.
Description
Provide functionality to link internal pages not directly accessible via 'internal_link', e.g. apphook-urls.
Checklist
masterSlack to find a “pr review buddy” who is going to review my pull request.
Summary by Sourcery
Add full support for a new "relative_link" link type by extending the link widget, form field, validators, models, helpers, and tests to handle relative URLs.
New Features:
Enhancements:
Tests: