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.

Sign up for GitHub

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

Jump to bottom

Use object instance when getting _meta info in admin #331

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

Use object instance when getting _meta info in admin #331

wants to merge 1 commit into from

Conversation

frennkie
Copy link
Collaborator

@frennkie frennkie commented Sep 22, 2020 •
edited

Make sure to use the object instance in admin.py in order to get the correct app_label and model_name. This is important when a subclass (e.g. of Newsletter) is used.

fix #330

@frennkie frennkie marked this pull request as draft September 22, 2020 17:55
@frennkie
Copy link
Collaborator Author

frennkie commented Sep 22, 2020 •
edited

Changed to Draft as tests are failing (worked on my machine).. will look into it.

@frennkie
Use object instance when getting _meta info in admin
6a25089 
Make sure to use the object instance in admin.py in order to
get the correct app_label and model_name. This is important
when a subclass (e.g. of Newsletter) is used.
@coveralls
Copy link

Coverage Status

Coverage remained the same at 88.389% when pulling 6a25089 on frennkie:fix-app-label into 57ac58d on dokterbob:master.

@frennkie frennkie marked this pull request as ready for review September 22, 2020 18:06
@frennkie
Copy link
Collaborator Author

frennkie commented Sep 22, 2020 •
edited

The tests prevented me from making a mistake. I don't think this can be "fixed". But other line is being fixed in this PR. Makes it easer to subclass.

@dokterbob
Copy link
Collaborator

The tests prevented me from making a mistake. I don't think this can be "fixed". But other line is being fixed in this PR. Makes it easer to subclass.

Why can't it be fixed? It seems obj is a class instance (just from looking at it), so it does seem it would have the same behaviour. Could you verify this, for completeness sake?

Thanks! Great contribs! Forgot how much fun Open Source was. 🙌

@dokterbob dokterbob added this to the 1.0b1 milestone Oct 24, 2020
@frennkie
Copy link
Collaborator Author

frennkie commented Oct 25, 2020 •
edited

Why can't it be fixed? It seems obj is a class instance (just from looking at it), so it does seem it would have the same behaviour.

Yes, obj is an model instance. It has the opts attribute. The way the other classes are passed into model appears to be different. Even it I instantiate them (run model().opts) there is no opts attribute.. :-/

Update: I think I found it.. the opts attribute is not on the Model but on the ModelAdmin and set when that is instatiated: https://github.com/django/django/blob/master/django/contrib/admin/options.py#L589

@dokterbob
Copy link
Collaborator

Right. Thanks for the feedback.

Are the API's used here documented API's? If not, they might be eligible to change and thus they might add additional maintenance load. In such case, perhaps it's wise to keep this work alive in a branch or a fork, until a stable API allows us to implement this well. I feel that subclassing the Newsletter model is a rather limited use case, as most people requiring such functionality have been making the modifications in their own forks of the project.

What are your thoughts about this?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
1.0b1
Development

Successfully merging this pull request may close these issues.

App Label used from Newsletter class instead of instance
3 participants
@frennkie @coveralls @dokterbob