Code

Opened 5 years ago

Closed 4 years ago

Last modified 4 years ago

#11358 closed (fixed)

Flatpage sitemap shows all pages, even private pages

Reported by: dburke Owned by: kmtracey
Component: Contrib apps Version: 1.0
Severity: Keywords: sprintSep2010
Cc: Triage Stage: Ready for checkin
Has patch: yes Needs documentation: no
Needs tests: no Patch needs improvement: no
Easy pickings: UI/UX:

Description

I tried searching trac for this issue, but wasn't able to turn up anything relevant.

Flatpages allow you to make a page visible only to logged in users, but the flatpage sitemap reveals the url. Would it be possible to add a .filter(registration_required=False) to the queryset that populates the flatpage sitemap?

in django.contrib.sitemaps:

class FlatPageSitemap(Sitemap):
    def items(self):
        from django.contrib.sites.models import Site
        current_site = Site.objects.get_current()
        return current_site.flatpage_set.filter(registration_required=False)

Attachments (2)

11358.patch (1.3 KB) - added by mlavin 4 years ago.
Patch against [13710] for change and doc change
11358_with_test.patch (3.7 KB) - added by mlavin 4 years ago.
Patch against [13714] to add tests and doc change

Download all attachments as: .zip

Change History (15)

comment:1 Changed 5 years ago by kmpm

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

You almost answered it yourself.
I don't know if its worth doing a modification to sitemaps just for this.
Save your class but with another name like "FlatPagePublicSitemap" in a file called "sitemaps.py" for example.
Include ( from sitemaps include FlatPagePublicSitemap ) and use that class in your urls.py instead of the ordinary FlagPageSitemap.

Is that a good enough solution for you?

comment:2 Changed 5 years ago by dburke

Yeah, this solution is fine. Should be made a little more explicit in the Documentation? Or should we hope that google helps direct people seeking this solution here?

comment:3 Changed 4 years ago by russellm

  • Triage Stage changed from Unreviewed to Accepted

A sitemap exists for the benefit of public spiders and crawlers, so there isn't much point having a sitemap that includes private pages.

comment:4 Changed 4 years ago by mlavin

  • Owner changed from nobody to mlavin

Changed 4 years ago by mlavin

Patch against [13710] for change and doc change

comment:5 Changed 4 years ago by mlavin

  • Has patch set

comment:6 Changed 4 years ago by mtredinnick

The code change looks fine, however I'd like a small tweak in the documentation: rather then explaining the effect in code (registration_required=False), try to do it in English (e.g. "pages which are visible to all users, including those not logged in" or something a little more concise). It can disrupt the reading flow to have to switch to understanding that flatpages model has an attribute called registration_required and so on.

Are there any existing sitemap tests? If so, they might need to be updated here. If not, have a think about if there's some way to test them even a little bit. It might be hard to do so, but shouldn't be impossible (using test.Client to pull back a sample of the sitemap and examine the contents, perhaps?)

Changed 4 years ago by mlavin

Patch against [13714] to add tests and doc change

comment:7 Changed 4 years ago by mlavin

  • Keywords sprintSep2010 added

comment:8 Changed 4 years ago by mlavin

  • Owner changed from mlavin to nobody
  • Triage Stage changed from Accepted to Ready for checkin

comment:9 Changed 4 years ago by kmtracey

  • Owner changed from nobody to kmtracey

comment:10 Changed 4 years ago by kmtracey

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

(In [13734]) Fixed #11358: Don't include private flatpages in sitemap. Thanks dburke and mlavin.

comment:11 follow-up: Changed 4 years ago by lukeplant

Is there a reason this wasn't backported? I'm trying to backport another patch, and I've got a conflict because this hasn't been backported to 1.2.X.

comment:12 in reply to: ↑ 11 Changed 4 years ago by kmtracey

Replying to lukeplant:

Is there a reason this wasn't backported? I'm trying to backport another patch, and I've got a conflict because this hasn't been backported to 1.2.X.

Hectic sprint day oversight, I think. On first read I think I thought this was more of a feature than a bugfix...on re-looking at it it seems to be more of a bugfix, so should probably be backported.

comment:13 Changed 4 years ago by lukeplant

I backported it now - [13985]

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.