Fix #985, typos in comments #991

Closed
wants to merge 1 commit into
from

Projects

None yet

2 participants

@brente
Contributor
brente commented Sep 3, 2011

Fixed #985 and added a test

I think it would be safer to also add a filter on the site, at least for root nodes (non-root nodes rely on the parent).

Also, I think there's something wrong with this test (in Page._publisher_save_public):

            not (self.level > 0 and self.parent.publisher_public == self.old_public.parent)

This is always true for root nodes, so the branch for moved nodes is always taken. I didn't touch this, though, because the final result seems to be the same if a root node is modified but not moved.

Finally, why not include filters on node status (and site) as defaults in get_*_filtered_sibling?
After all we're looking for the next/previous sibling among nodes of the same kind (draft or public).
I can't think of a case where we might need to find a sibling for a public node including drafts (or nodes from a different site).

@ojii
Collaborator
ojii commented Sep 3, 2011

thanks for the pull request. For the future please do not mix cleanup (comments/codestyle) and actual fixes in a single pull request, since that just makes it a lot harder for us to review. Instead, please send two pull requests (one with the fix, one with cleanup).

@ojii
Collaborator
ojii commented Sep 3, 2011

Didn't you say in #985 that get_previous_filtered_siblings should also filter by site?

@brente
Contributor
brente commented Sep 3, 2011

I did, but I couldn't write a failing test, so I just submitted this.
I'm still working on the site part.

@ojii
Collaborator
ojii commented Sep 3, 2011

If you hop on IRC (#django-cms on freenode) I might be able to help you with that test

@ojii
Collaborator
ojii commented Sep 5, 2011

the test you added doesn't actually seem to test what this issue is about...

@brente
Contributor
brente commented Sep 5, 2011

Why not?
I opened #985 because the home page was losing its status when the user updated it, which is exactly what PagesTestCase.test_public_home_page_replaced tests.
Probably it's too specific, and I agree that other test cases are missing (e.g. multi-site, non-root nodes with unpublished siblings, ...). I hope I'll be able to include them in the next submission.

@ojii
Collaborator
ojii commented Sep 5, 2011

I've written a test and fix for it now, nvm

@brente
Contributor
brente commented Sep 5, 2011

OK.
Thank you.

@ojii ojii pushed a commit that closed this pull request Sep 6, 2011
Jonas Obrist Added a fix for #985
Fixes #985
Fixes #991
f573a24
@ojii ojii closed this in f573a24 Sep 6, 2011
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment