Canonical sites for photos and galleries https://github.com/jdriscoll/django-photologue/issues/135

[b-jam]

For users without multisite, they will not notice any differnence. There is a migration + datamigration to set canonical site for all their existing galleries and photos. It also will automatically set canonical site in the post_save just how it is currently.

For users with multisite, they will see a new field canonical site. I was not sure whether a data migration is needed for them, as it would have to choose a random site essentially. Thoughts?

I was thinking about adding validation, like if canonical site is not in sites, then automatically add it. But I just settled with a warning message in admin save. If the canonical site is not in sites, it will give a 404.

All tests are passing.

in get_canonical_url, I'm not sure whether it needs something more to set the scheme if https.

[coveralls]

Coverage decreased (-0.39%) to 59.36% when pulling 9d596afe7e82102d41728720fa9ad4deb0f5cb76 on pa-jama:master into b183f06e22ed97b8db99d9126e4aea6e68005e66 on jdriscoll:master.

[richardbarran]

Hi Pa-jama, I've just been reading your pull request - I haven't tested it yet. I like it a lot! There's a few small additions that I can think of:

• adding some help_text to explain what the new canonical_site field is.
• writing a new unit test (or 2) for the sitemap.
• the warning messages are not marked for i18n.

I can write those additions, but it will be a week or so before I find the time to do so. If you want to make these additions yourself in the meantime, please go ahead!

As for get_canonical_url(), I guess that catering for https is an edge case, and someone who needs that can easily extend Gallery and Photo to provide that functionality.

[b-jam]

Thanks! Same here, a bit tied up for the next 2 days, but I'll work on those things if you haven't got to it already.

Also I'm currently working on a news sitemap for galleries, so I could add that as well https://support.google.com/news/publisher/answer/74288?hl=en

[b-jam]

Hey so I started on this, but realised I didn't understand factory-boy well enough to write the tests. The other two fixes are trivial, so is it OK if I leave it to you?

and disregard what I said about google news sitemaps, mine isnt 100% compliant so if it is, I will put it into another pull request.

[b-jam]

I added help text (with il8n) and marked the other messages for il8n. regarding tests:

So I wrote 3 test cases, but I can't figure out how to change the default site for client.get(). Any idea? The first case below fails, because its still passing in example.com when it should be example.org. The second one passes as it should. The other test case is fine so I didn't include it

self.site2, created2 = Site.objects.get_or_create(
    domain="example.org", name="example.org")
self.gallery3 = GalleryFactory(slug='canonical-multisite-gallery',
                               sites=[self.site1, self.site2],
                               canonical_site = self.site2)


def test_canonical_gallery(self):
    """Galleries on multiple sites, should only appear in the sitemap
    for their canonical site"""
    response = self.client.get('/sitemap.xml')
    response_site2 = self.client.get('/sitemap.xml', SERVER_NAME="example.org")
    self.assertContains(response_site2,
                        '<url><loc>http://example.org/ptests/gallery/canonical-multisite-gallery/</loc>'
                        '<lastmod>2011-12-23</lastmod></url>')

    self.assertNotContains(response,
                           '<url><loc>http://example.com/ptests/gallery/canonical-multisite-gallery/</loc>'
                           '<lastmod>2011-12-23</lastmod></url>')

[richardbarran]

I am very busy at the moment - hence why I have not reviewed your pull request yet! Please accept my apologies, and I will get back to you soon.

[b-jam]

Cool, no worries. Its all done except for the tests, I'm not sure if its possible to change the domain to example.org in tests? If not, it makes it kind of impossible to write proper tests

[richardbarran]

Just looked at your unit tests - the line response_site2 = self.client.get('/sitemap.xml', SERVER_NAME="example.org") I suggest would work better if you override the settings just for that one test, e.g.:

with self.settings(PHOTOLOGUE_MULTISITE=True, SITE_ID=self.site2.id):
    response_site2 = self.client.get('/sitemap.xml')