Code

Opened 4 years ago

Closed 4 years ago

Last modified 3 years ago

#14386 closed (fixed)

Standardize Site/RequestSite access; looser coupling of sites framework in contrib

Reported by: gabrielhurley Owned by: gabrielhurley
Component: Contrib apps Version: master
Severity: Keywords:
Cc: Triage Stage: Unreviewed
Has patch: yes Needs documentation: no
Needs tests: no Patch needs improvement: no
Easy pickings: UI/UX:

Description

A few days ago I started this thread on the Django Developers list in order to solve a number of tickets at the same time. The gist:

All of these tickets relate to needing access to the current domain, whether that's through a Site instance, or through a RequestSite instance. To facilitate this, the provided patch adds a utility function, django.contrib.sites.models.get_current_site which takes a request as an argument and returns either a Site or RequestSite instance.

Using that utility function, a lot of repetitious code could be removed and streamlined, and several tickets were trivial to resolve.

Included in this patch:

  • Adds the django.contrib.sites.models.get_current_site function.
  • Provides a note in the contrib.sites docs regarding the function.
  • Converts to contrib.sites tests from doctests to unit tests and adds a test for get_current_site
  • Resolves #8960 by allowing django.contrib.contenttypes.views.shortcut to use either a Site or RequesSite object.
  • Converts contrib.contenttypes tests from doctests to unit tests and adds a test for #8960.
  • Resolves #10235 and #10909 by using get_current_site in the Sitemaps framework.
  • Adds test case for #10235/#10909.
  • Updates django.contrib.syndication.views to use `get_current_site.
  • Updates django.contrib.gis.sitemaps.views to use get_current_site as per #10608.
  • Updates django.contrib.auth.views.login to use get_current_site.

The patch passes the full test suite under WinXP/Py2.6/SQLite.

This patch would completely fix #8960, #10235, and #10909. The remainder of #10608 is actually invalid, because the remaining places where contrib apps rely on contrib.sites the model has a direct relation to Site, or there is no way to access a request object to use RequestSite. Any further decoupling in contrib would require backwards-incompatible rewrites.

Attachments (1)

14386.diff (21.6 KB) - added by gabrielhurley 4 years ago.
Updated to fix #14377 as well. Uses get_current_site() in contrib.auth.views.logout to make site accessible to logout template as in login view.

Download all attachments as: .zip

Change History (10)

comment:1 Changed 4 years ago by gabrielhurley

  • Needs documentation unset
  • Needs tests unset
  • Patch needs improvement unset
  • Status changed from new to assigned

I'm not sure why Trac isn't displaying the diff for this patch, but the code *is* in there... ::sigh::

Changed 4 years ago by gabrielhurley

Updated to fix #14377 as well. Uses get_current_site() in contrib.auth.views.logout to make site accessible to logout template as in login view.

comment:2 Changed 4 years ago by gabrielhurley

Since apparently overwriting an old patch file in Trac overwrites messages added with it, I'll add a permanent note that this patch now solves #13845 (using get_current_site in contrib.auth.forms.PasswordResetForm.save), and #14377 (using get_current_site in contrib.auth.views.logout to make the current site accessible in the logout template as it is in the login view/template).

Complete list of tickets affected currently: #8960, #10235, #10909, #10608, #13845, #14377.

comment:3 Changed 4 years ago by lukeplant

I will commit this shortly. I've made some tweaks to the patch, which I'll document here for your benefit:

  1. In the tests, settings.DEBUG should be restored to it's original value in a tearDown method, rather than in the test and rather than to the value 'False'. This is to avoid relying on assumptions about settings files or other code - it might be safe now, but that could change.
  1. Similarly, in the tests, Site._meta.installed should also be restored in the tearDown method, and again to the original value, and the test should set it to True if it is assuming that it is installed.
  1. According to PEP8, comments should start with a # followed by a space - the space was missing in some of the comments. Also, according to PEP8 there should be be two lines between top level definitions in modules - a convention we haven't followed properly so far, but are trying to do with new files.
  1. I moved a tearDown method to after the setup, since they belong together.
  1. There was an unneeded import in one test, and a remaining unneeded import of Site/RequestSite in auth.views, and one in FlatPageSitemap
  1. Technically I think #13845 and #14377 should have had separate patches with tests. I added a test for #14377 since it was easy to do.
  1. Whitespace - we like trailing spaces removed in the Django code base. It's admittedly hard to see in some editors, Emacs has an option for seeing it.

BTW, Trac didn't show the patch because of a bug where it fails to parse "\ No newline at end of file" in the patch, which wasn't your fault.

Notwithstanding those comments, thanks for putting all this together, great work.

comment:4 Changed 4 years ago by lukeplant

  • Resolution set to fixed
  • Status changed from assigned to closed

(In [13980]) Fixed #14386, #8960, #10235, #10909, #10608, #13845, #14377 - standardize Site/RequestSite usage in various places.

Many thanks to gabrielhurley for putting most of this together. Also to
bmihelac, arthurk, qingfeng, hvendelbo, petr.pulc@…, Hraban for
reports and some initial patches.

The patch also contains some whitespace/PEP8 fixes.

comment:5 Changed 4 years ago by gabrielhurley

Thanks for the final cleanup (and commit) on the patch, Luke. The inconsistent whitespace on comments and the like are what I get when writing too much code too late at night. Good point on TearDown, too.

As for rolling the other two tickets into it later, I did that simply because the solution to them relied on the same get_current_site method, and I wasn't sure if this would get committed soon or days/weeks from now. Didn't want to write a patch for those that wouldn't apply cleanly until this one was in and have it end up getting messy. I certainly see your point though.

Thanks!

comment:6 Changed 4 years ago by lukeplant

(In [13987]) [1.2.X] Fixed #14386, #8960, #10235, #10909, #10608, #13845, #14377 - standardize Site/RequestSite usage in various places.

Many thanks to gabrielhurley for putting most of this together. Also to
bmihelac, arthurk, qingfeng, hvendelbo, petr.pulc@…, Hraban for
reports and some initial patches.

The patch also contains some whitespace/PEP8 fixes.

Backport of [13980] from trunk. Some items could be considered features
(i.e. supporting RequestSite in various contrib apps), others are definite
bugs, but it was much more robust to backport all these tightly related
changes together.

comment:7 Changed 4 years ago by lukeplant

(In [14141]) Fixed #14433 - replaced a thread-unsafe solution to #10235 introduced in [13980]

This patch also addresses sitemap code found in contrib/gis, which [13980]
did not.

Thanks to gabrielhurley for the initial patch.

Refs #10235, #14386

comment:8 Changed 4 years ago by lukeplant

(In [14142]) [1.2.X] Fixed #14433 - replaced a thread-unsafe solution to #10235 introduced in [13980]

This patch also addresses sitemap code found in contrib/gis, which [13980]
did not.

Thanks to gabrielhurley for the initial patch.

Refs #10235, #14386

Backport of [14141] from trunk.

comment:5 Changed 3 years ago by jacob

  • milestone 1.3 deleted

Milestone 1.3 deleted

Add Comment

Modify Ticket

Change Properties
<Author field>
Action
as closed
as The resolution will be set. Next status will be 'closed'
The resolution will be deleted. Next status will be 'new'
Author


E-mail address and user name can be saved in the Preferences.

 
Note: See TracTickets for help on using tickets.