Code

Opened 5 years ago

Closed 4 years ago

Last modified 4 years ago

#10608 closed (fixed)

Support RequestSite along with Sites.objects.get_current() in contrib

Reported by: hvendelbo Owned by: nobody
Component: Contrib apps Version: 1.1
Severity: Keywords:
Cc: arikon Triage Stage: Accepted
Has patch: yes Needs documentation: no
Needs tests: no Patch needs improvement: yes
Easy pickings: UI/UX:

Description

Most contrib apps assume that the Site model from contrib.sites is used. The login function from auth does a test to see if the Site model is installed, and otherwise
resolves the site using django.contrib.sites.models.RequestSite

This patch duplicates the login function behaviour in
auth.forms, comments.feeds, contenttypes.views, gis.sitemaps.views, sitemaps, and sitemaps.views

Attachments (1)

django_requestsite.patch (6.8 KB) - added by hvendelbo 5 years ago.

Download all attachments as: .zip

Change History (13)

Changed 5 years ago by hvendelbo

comment:1 Changed 5 years ago by hvendelbo

  • Needs documentation unset
  • Needs tests unset
  • Patch needs improvement unset

In admindocs views there is a similar switch which defaults to GenericSite().
It looks like it can be replaced by the same code that I added in the other spots

comment:2 Changed 5 years ago by jacob

  • milestone set to 1.2
  • Triage Stage changed from Unreviewed to Accepted

comment:3 Changed 5 years ago by arikon

  • Cc arikon added

comment:4 Changed 5 years ago by Alex

  • Patch needs improvement set

There are a number of places where this patch will do nothing more than raise a NameError since request isn't in scope.

comment:5 Changed 5 years ago by arthurk

Even if the request would be in scope, it wouldn't work correctly for some methods. For example the FlatPageSitemap method returns current_site.flatpage_set.all(). It would result in an error if the Sites framework wouldn't be installed, because the Request object doesn't have a flatpage_set.

comment:6 Changed 5 years ago by veena

The management of the "if statements" in applications will be annoying, don't you think?

Shouldn't there be a Site class not inherited from django.db.models.Model but provide API for getting data? Then if site app is installed it can use database as a storage for site data. If site app is not installed, it will use RequestSite class. Of course, request object is need to be passed.

It forces not to use construction like "current_site.flatpage_set.all()" but "Flatpage.objects.filter(sites=site)" and also "if statements" don't need to be used.

comment:7 Changed 5 years ago by arthurk

veena, please see my second patch in #10235. It adds a get_current method that will return the appropriate object.

comment:8 Changed 4 years ago by ubernostrum

  • milestone 1.2 deleted

1.2 is feature-frozen, moving this feature request off the milestone.

comment:9 Changed 4 years ago by floledermann

  • Version changed from 1.0 to 1.1

I would classify this as a bug, not a feature. Without using sites the "View on site" link is generated in the admin, but fails with an error when clicked - isn't that a bug?

However this should probably be broken down into several bugs dealing with each contrib app, then the consequences of this bug could also be discussed in more detail. Too late for 1.2 anyway, probably.

comment:10 Changed 4 years ago by floledermann

Btw the bug for the "View on site" issue is #8960 - also still not resolved, and much older than this issue.

comment:11 Changed 4 years ago by lukeplant

  • Resolution set to fixed
  • Status changed from new 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:12 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.

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.