diff --git a/CHANGELOG.rst b/CHANGELOG.rst index 837f5e43..e4eaef54 100644 --- a/CHANGELOG.rst +++ b/CHANGELOG.rst @@ -2,6 +2,10 @@ Changelog ========= +* Fix the ``admin_register`` fixer to avoid rewriting when there are duplicate ``ModelAdmin`` classes in the file. + + `Issue #471 `__. + 1.19.0 (2024-06-27) ------------------- diff --git a/src/django_upgrade/fixers/admin_register.py b/src/django_upgrade/fixers/admin_register.py index 8323eb42..a42cbbf4 100644 --- a/src/django_upgrade/fixers/admin_register.py +++ b/src/django_upgrade/fixers/admin_register.py @@ -45,7 +45,9 @@ def __init__(self, parent: ast.AST, lineno: int) -> None: self.model_names_per_site: dict[str, set[str]] = {} -decorable_admins: MutableMapping[State, dict[str, AdminDetails]] = WeakKeyDictionary() +decorable_admins: MutableMapping[State, dict[str, AdminDetails | None]] = ( + WeakKeyDictionary() +) # Name of site to set of unregistered model names, or True if potentially all # models have been unregistered unregistered_site_models: MutableMapping[State, dict[str, set[str] | Literal[True]]] = ( @@ -67,9 +69,14 @@ def visit_ClassDef( parents: tuple[ast.AST, ...], ) -> Iterable[tuple[Offset, TokenFunc]]: if _is_django_admin_imported(state) and not uses_full_super_in_init_or_new(node): - decorable_admins.setdefault(state, {})[node.name] = AdminDetails( - parents[-1], node.lineno - ) + admin_detailses = decorable_admins.setdefault(state, {}) + if node.name in admin_detailses: + # Duplicate name, ignore + admin_detailses[node.name] = None + return + else: + admin_detailses[node.name] = AdminDetails(parents[-1], node.lineno) + if not node.decorator_list: offset = ast_start_offset(node) decorated = False @@ -125,7 +132,7 @@ def update_class_def( tokens: list[Token], i: int, *, name: str, state: State, decorated: bool ) -> None: admin_details = decorable_admins.get(state, {})[name] - if not admin_details.model_names_per_site: + if admin_details is None or not admin_details.model_names_per_site: return if decorated: @@ -221,7 +228,12 @@ def visit_Call( admin_details.model_names_per_site.setdefault(site_name, set()).update( model_names ) - yield ast_start_offset(node), partial(erase_node, node=parents[-1]) + yield ast_start_offset(parents[-1]), partial( + remove_register, + name=admin_name, + state=state, + node=parents[-1], + ) elif node.func.attr == "unregister" and ( ( # admin.site.unregister(...) isinstance(node.func.value, ast.Attribute) @@ -266,6 +278,16 @@ def visit_Call( existing_names.update(unregistered_names) +def remove_register( + tokens: list[Token], i: int, *, name: str, state: State, node: ast.AST +) -> None: + admin_details = decorable_admins.get(state, {})[name] + if admin_details is None: + return + + erase_node(tokens, i, node=node) + + site_definitions: MutableMapping[ast.Module, dict[str, int | None]] = ( WeakKeyDictionary() ) diff --git a/tests/fixers/test_admin_register.py b/tests/fixers/test_admin_register.py index 05b730bb..4196b68d 100644 --- a/tests/fixers/test_admin_register.py +++ b/tests/fixers/test_admin_register.py @@ -65,6 +65,20 @@ class AuthorAdmin(admin.ModelAdmin): ) +def test_register_assigned(): + check_noop( + """\ + from django.contrib import admin + from myapp.models import Author + + class AuthorAdmin(admin.ModelAdmin): + pass + + value = admin.site.register(Author, AuthorAdmin) + """, + ) + + def test_py2_style_init_super(): check_noop( """\ @@ -669,6 +683,75 @@ class MyCustomAdmin: ) +def test_duplicate_model_admins_register_one(): + check_noop( + """\ + from django.contrib import admin + + class BookAdmin(admin.ModelAdmin): + pass + + admin.site.register(Book, BookAdmin) + + class BookAdmin(admin.ModelAdmin): + pass + """, + ) + + +def test_duplicate_model_admins_register_two(): + check_noop( + """\ + from django.contrib import admin + + class BookAdmin(admin.ModelAdmin): + pass + + class BookAdmin(admin.ModelAdmin): + pass + + admin.site.register(Book, BookAdmin) + """, + ) + + +def test_duplicate_model_admins_with_intermediate(): + check_transformed( + """\ + from django.contrib import admin + + class BookAdmin(admin.ModelAdmin): + pass + + class DuckAdmin(admin.ModelAdmin): + pass + + admin.site.register(Duck, DuckAdmin) + + class BookAdmin(admin.ModelAdmin): + pass + + admin.site.register(Book, BookAdmin) + """, + """\ + from django.contrib import admin + + class BookAdmin(admin.ModelAdmin): + pass + + @admin.register(Duck) + class DuckAdmin(admin.ModelAdmin): + pass + + + class BookAdmin(admin.ModelAdmin): + pass + + admin.site.register(Book, BookAdmin) + """, + ) + + def test_complete(): check_transformed( """\