Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix: Remove get_absolute_url from both Alias and AliasContent models #199

Merged
merged 24 commits into from
Mar 5, 2024

Conversation

fsbraun
Copy link
Member

@fsbraun fsbraun commented Nov 12, 2023

Description

According to the Django docs, get_absolute_url should calculate the canonical URL of an object. I argue that aliases should not have a canonical URL, since

  • they represent fragments which are not to be offered to the end user independently
  • all URL returned by the current implementation refer to admin sites, which - by definition - are unreachable for non-staff users.
  • the "View on site" link in the admin's object tools

Fixes the invalid edit alias URL in the plugin menu (if no alias is available in the current language).

Related resources

tests/test_admin.py Fixed Show fixed Hide fixed
Copy link
Contributor

@Aiky30 Aiky30 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This could be left behind from where previously there was a custom admin UI solution for managing Alias. We opted sometime ago to use the native django admin like the other packages do.

The one exception to the point that you make on the fact that Alias are not viewable outright is that the toolbar does treat them as individually viewable items. I'd check that the toolbar continues to function for Alias with regards to editing an Alias. If this works fine then I see no issue removing.

Some of the tests that have been changed could be regression, the side frame actiona removal and also the fact that the alias does have a complex matrix of when to show draft and live content in the page edit mode. The absolute url could drive that although it's been some time since I worked / looked at that code.

@fsbraun
Copy link
Member Author

fsbraun commented Nov 18, 2023

@Aiky30 Preview and edit endpoints are not affected by the PR, of course. And they continue to work.

Copy link
Contributor

@Aiky30 Aiky30 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The only other thing I can think of regarding this change is a strange matrix of scenarios that exist when trying to render an Alias. I think it went:

Draft Page renders Draft Alias
Preview Page renders Draft Alias
Published Page renders Published Alias

I'm not sure if the get absolute url was used for this mechanism. I'm so far away from the CMS these days that I'm not much use.

I know that FIL internal codebase depends on this so people may want this to be investigated further.

tests/test_views.py Outdated Show resolved Hide resolved
@fsbraun fsbraun merged commit e0e4bbb into django-cms:master Mar 5, 2024
12 checks passed
@joshyu joshyu mentioned this pull request Aug 20, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants