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

Use ModelAdmin.opts for correct reverse in admin #332

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

Conversation

frennkie
Copy link
Collaborator

@frennkie frennkie commented Sep 22, 2020

Make sure to use ModelAdmin.opts 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.

@frennkie frennkie force-pushed the fix-hardcoded-admin-links branch 2 times, most recently from 10c3a25 to 637dddb Compare September 22, 2020 19:45
@coveralls
Copy link

coveralls commented Sep 22, 2020

Coverage Status

Coverage decreased (-0.06%) to 88.327% when pulling ebc42b9 on frennkie:fix-hardcoded-admin-links into 57ac58d on dokterbob:master.

@coveralls
Copy link

Coverage Status

Coverage decreased (-0.07%) to 88.317% when pulling 637dddb on frennkie:fix-hardcoded-admin-links into 57ac58d on dokterbob:master.

@dokterbob
Copy link
Collaborator

Hi @frennkie,

Thanks for the contrib! I'm sure this is useful and the code seems ready to merge as it is. However, as I am not currently aware of this pattern, I would assume there are a fair amount of fellow developers who aren't.

Could I ask you to provide some reference, either here (so I'll add it to the commit msg) or as a comment to the code, of either the relevant section in the Django doc's, or perhaps another place where this is documented? If not, would it be possible to add a ~ one sentence rationale explaining the behaviour in addition to the objective (which you already did)?

This is all just to facilitate future contribution, as well as the overall didactic quality of the code.

@dokterbob dokterbob added this to the 1.0b1 milestone Oct 24, 2020
@frennkie frennkie force-pushed the fix-hardcoded-admin-links branch 2 times, most recently from 7132e99 to 338404b Compare November 6, 2020 21:53
Make sure to use ModelAdmin.opts 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
@frennkie frennkie force-pushed the fix-hardcoded-admin-links branch from 338404b to dc35e54 Compare November 6, 2020 22:01
@frennkie
Copy link
Collaborator Author

frennkie commented Nov 6, 2020

I removed one commit that was actually not directly related and two changes that might be (a bit) disputable.

Now all this does is use ModelAdmin.opts.app_label and ModelAdmin.opts.model_name instead of hard-coding the label and name.

I have to admit that "my heart isn't into this so much anymore".. ;-D

The reason I started this was that I wanted to subclass some a class which proved to be really hard. I ended up using a ProxyModel instead only overriding the methods that I needed to change in my project (most prominently Submission.send_message) - which is looking fine so far.

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