-
-
Notifications
You must be signed in to change notification settings - Fork 32.8k
Fixed #2259 -- Made manually specified primary keys readonly in the admin. #19675
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
base: main
Are you sure you want to change the base?
Conversation
…omatic zoom issue in mobile Safari.
…dmin change forms.
094817a
to
7a78c93
Compare
@@ -418,7 +418,22 @@ def get_readonly_fields(self, request, obj=None): | |||
""" | |||
Hook for specifying custom readonly fields. | |||
""" | |||
return self.readonly_fields |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It works on my test project, and it should not affect existing overrides. It also provides the option to fall back to the initial behavior:
def get_readonly_fields(self, request, obj=None):
return self.readonly_fields
django/contrib/admin/options.py
Outdated
|
||
# Only add PK to readonly if: | ||
# - editing an existing object | ||
# - PK is not auto-created |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Any particular reason why an auto_created
pk needs to be excluded?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
auto_created pks are already not editable in the admin.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you for the PR
This doesn't seem to be working fully for inlines.
Given the models:
class State(models.Model):
label = models.CharField(max_length=255)
class StatePKModel(models.Model):
id = models.IntegerField(primary_key=True)
state = models.ForeignKey(State, related_name="pk_state_model", on_delete=models.CASCADE)
and admin
class StatePKModelInline(admin.TabularInline):
model = StatePKModel
@admin.register(State)
class StateAdmin(admin.ModelAdmin):
inlines = [StatePKModelInline,]
label { | ||
font-size: 1rem; | ||
} | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This change looks to be unrelated to the ticket
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That was my bad. Fixed this css thing (was from a year old PR). Also, I confirmed with a test that manually defined Pks are readonly in inlines when editing existing objects.
django/contrib/admin/options.py
Outdated
@@ -418,7 +418,22 @@ def get_readonly_fields(self, request, obj=None): | |||
""" | |||
Hook for specifying custom readonly fields. | |||
""" | |||
return self.readonly_fields | |||
readonly = self.readonly_fields | |||
pk_field = self.model._meta.pk |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This should account for composite primary keys now and use _meta.pk_fields
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks! Updated to use _meta.pk_fields for future composite PK support, with fallback to _meta.pk for compatibility.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Updated to use _meta.pk_fields for future composite PK support, with fallback to _meta.pk for compatibility.
No need for a fallback, see the docs.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
fallback removed.
893d265
to
262ca22
Compare
262ca22
to
2153973
Compare
This is still not fixed unfortunately |
Thanks for the feedback. Just to clarify -- the original Trac ticket #2259 doesn't mention inlines at all. The bug, as described and discussed over the years, is focused on making manually defined primary key fields read-only in the Django admin when editing existing objects. The root issue was that those fields were editable, changing them would silently insert a new object instead of updating the original -- without any error or indication. This has been fixed by marking such fields as read-only in ModelAdmin when editing. inlines were only brought up in two comments (comments 29 and 54) over the 19 years of discussion on the ticket. So I'm a little confused about why inlines are being brought up. That being said. I can investigate a follow up fix for inlines. |
acbe1c3
to
2085066
Compare
So I tried making manually-defined primary key fields (including composite PKs) readyonly when editing inlines by overriding InlineModelAdmin.get_readonly_fields() and adding a help method to identify and return PK field names. This worked but lead to the PK fields being entirely hidden in the Django Admin. Which I'm fairly sure you all don't want. I'm happy with the resolution as it stands and would rather not pursue additional inline-specific logic here. |
This issue, as described, is present for inlines. As you mentioned, it has been mentioned a couple of times in the ticket discussion. This was also a reason why some PRs previously didn't continue as they were missing a solution for this problem |
Trac ticket number
ticket-2259
Branch description
When editing a model instance in the Django admin, manually specified primary key fields (e.g.
CharField(primary_key=True)
) should not be editable. Without this fix, users can mistakenly change a model's PK, which can cause errors or lead to duplicate records.This patch updates
BaseModelAdmin.get_readonly_fields()
to automatically mark such PKs as readonly if they are not auto-generated and not already in the readonly fields.This change was verified against existing admin inline tests (test_char_pk_inline, test_integer_pk_inline), which initially failed due to the patch, and the logic was adjusted to restore their passing state.
Checklist
main
branch.