Fixed #21386 -- admindocs depend on sites framework #1871

Closed
wants to merge 1 commit into
from

Projects

None yet

2 participants

@Bouke
Bouke commented Nov 5, 2013

See also ticket 21386

@Bouke
Bouke commented Nov 22, 2013

Rebased after #1953 got merged

@claudep
Member
claudep commented Nov 22, 2013

I have a partially related question: I struggle to find real use cases for the ADMIN_FOR setting. Shouldn't we deprecate it and assume we are documenting the current site?

@Bouke
Bouke commented Nov 22, 2013

I think the reasoning behind the ADMIN_FOR is that a single admin backend can handle multiple frontends; thus multiple SITE_IDs and multiple settings configurations in ADMIN_FOR. However I think that axing this feature should be discussed further on the mailing list for record keeping.

@claudep
Member
claudep commented Nov 22, 2013

OK, I'll write to the -dev ML to get some more opinions.

@claudep
Member
claudep commented Nov 27, 2013

No feedback until now, so I'd be in favor of simply removing ADMIN_FOR and related functionality. Are you willing to update the patch consequently?

@Bouke
Bouke commented Nov 27, 2013

I'm all for removing ADMIN_FOR, but it's not within the scope of this PR. This is specifically for the sites framework. If I have some additional time on my hands I could provide an additional PR.

@claudep
Member
claudep commented Nov 27, 2013

If you remove ADMIN_FOR, you completely remove the need to use contrib,sites in admindocs, right?

@Bouke
Bouke commented Nov 27, 2013

Ah yes, you're right. So no deprecation of the feature then?

@claudep
Member
claudep commented Nov 28, 2013

No deprecation (no existing code will crash anyway), but a note in the backwards incompatibility section of the release notes.

@Bouke
Bouke commented Dec 13, 2013

@claudep pushing my changes so far to get some feedback; is this what you intended? get_current_site could even be dropped, but that also requires some changes to the templates where the site instance is being used.

@claudep
Member
claudep commented Dec 14, 2013

Yes, it is in the right direction. Indeed, I'd go even further, because the views_by_site stuff in view_index.html template does not make any more sense.

@Bouke
Bouke commented Dec 16, 2013

screen shot 2013-12-16 at 07 53 29

View functions are now grouped by namespace; previously they were grouped by site and I think the namespace is a good view group categorization. The patch now separates admindocs completely from the sites framework. I will look into creating tests for the changes.

@claudep
Member
claudep commented Dec 16, 2013

Great, nice idea!

@Bouke
Bouke commented Dec 16, 2013

Rebased and squashed.

@claudep claudep commented on an outdated diff Dec 17, 2013
...ib/admindocs/templates/admin_doc/template_detail.html
@@ -16,14 +16,12 @@
<h1>{% blocktrans %}Template: "{{ name }}"{% endblocktrans %}</h1>
{% regroup templates|dictsort:"site_id" by site as templates_by_site %}
@claudep
claudep Dec 17, 2013 Django member

I think this line could also be removed.

@claudep claudep commented on an outdated diff Dec 17, 2013
tests/admin_docs/tests.py
@@ -1,4 +1,6 @@
import unittest
+from django import conf
@claudep
claudep Dec 17, 2013 Django member

Not a big deal, but the usage is to import from django.conf import settings

@Bouke Bouke Fixed #21386 -- admindocs depend on sites framework
* Removed ADMIN_FOR setting and warn warning
* Group view functions by namespace instead of site
* Added a test verifying namespaces are listed

Thanks to Claude Paroz for reviewing and ideas for improvement.
49cb6bd
@Bouke
Bouke commented Dec 17, 2013

Thanks for the review, the issues have been resolved.

@claudep
Member
claudep commented Dec 17, 2013

Thanks. Did you mean to add more tests (as of one of your comments above) or do you consider it ready for merge?

@Bouke
Bouke commented Dec 18, 2013

If it could get merged as-is, then I'd rather have it merged. Do you think
it should have more tests?

@claudep
Member
claudep commented Dec 18, 2013

Thanks Bouke, committed in a39d672

@claudep claudep closed this Dec 18, 2013
@Bouke Bouke deleted the Bouke:patches/1 branch Dec 18, 2013
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment