Skip to content

Conversation

@JoeyStk
Copy link
Contributor

@JoeyStk JoeyStk commented Dec 21, 2025

Short description

This PR attempts to refactor the method generate_unique_slug in utils/slug_utils.py. We came across this method during our work on the Django update. There were two main issues I saw with the method: firstly I thought the CC was quite high (17) and secondly I thought the function was a bit too long (97 LOC). I tried to reduce both.

Proposed changes

  • Reduce CC to 5
  • Reduce LOC to 68

Side effects

  • As with all refactorings, I tried my best to pay attention to all the details, but of course there is a high chance of human error, so horough testing is highly appreciated.

Faithfulness to issue description and design

There are no intended deviations from the issue and design.

How to test

  • Check if a new slug is properly generated
  • Check in case a slug already exists, the new slug is changed to SLUG-2
  • Check in case a slug is only used in a previous version of this page, that no error occurs

Resolved issues

Fixes: /


Pull Request Review Guidelines

@JoeyStk JoeyStk marked this pull request as draft December 21, 2025 13:12
@JoeyStk JoeyStk force-pushed the refactor-generate-unique-slug-2 branch from abaf41f to da570c6 Compare December 22, 2025 12:11
@JoeyStk JoeyStk marked this pull request as ready for review December 22, 2025 12:20
@PeterNerlich
Copy link
Contributor

I stumbled across #3837's failing test ValidationError: Slug kann nicht aus dem Titel abgeleitet werden., got curious and started digging. I think this PR would be fitting to address that problem as well:

Four years ago, in ac63eca, we split generate_unique_slug(form_object, foreign_model) into the outer generate_unique_slug_helper(form_object, foreign_model) and the inner generate_unique_slug(kwargs). The outer helper was quite simple, just preparing the kwargs dict and calling the inner function with it. I find the naming a bit unlucky, since the helper now sounds like a function you shouldn't need to do anything with because it is just called for some inner workings, when in fact it is exactly the other way around.
My expectation would be that any and all calls to generate_unique_slug(form_object, foreign_model) should have been just moved over to generate_unique_slug_helper(form_object, foreign_model), a trivial change only requiring the change of the function.

However, it seems that today we still call the inner generate_unique_slug(kwargs) with a hand crafted dict in four places:

  • integreat_cms/cms/models/events/event_translation.py:172
  • integreat_cms/cms/models/pages/page_translation.py:428
  • integreat_cms/cms/models/pois/poi_translation.py:193 and
  • integreat_cms/cms/models/abstract_content_model.py:211.

At least PageTranslation.clean() is not including cleaned_data in kwargs, which is why in the test that is an empty dict, so the key for the fallback of "title" cannot be found so its value is not available to derive a slug from.

Without having looked deeper into it, I expect that we will find that replacing those calls with just handing form_object and foreign_model to the helper will solve this problem.

if not object_instance:
return qs

if foreign_model in content_models:
Copy link
Contributor

Choose a reason for hiding this comment

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

Aah, I remember why that had to be in here, because we cannot just pass objects without an ID with the new django version anymore.
In that case, the foreign object (page, event or location) does not exist in the db yet, meaning that we are trying to create it. When it doesn't exist yet, we also don't have any other slugs in the database to exclude when looking for naming conflicts, wo skipping that step and running the lines following is valid.

Suggested change
if foreign_model in content_models:
if (
foreign_model in content_models
and (foreign_object or object_instance.foreign_object).id
):

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.

4 participants