BUG Fix orphaned pages reporting they can be viewed #991
Conversation
| $member = new Member(); | ||
| $member->Email = 'someguy@example.com'; | ||
| $member->write(); | ||
| $member->Groups()->add($group); |
There was a problem hiding this comment.
Should this level of setup be left to the fixtures file rather than test code?
There was a problem hiding this comment.
Good point, it would result in a cleaner test case.
|
Ran into this issue myself after a db migration. Changes would resolve the issues I had. I wonder if we need a report in the reports module as well for orphaned pages (just as an aside). |
| $this->assertTrue($staff->canView($member)); | ||
|
|
||
| // Cleanup | ||
| Versioned::set_reading_mode($origStage); |
There was a problem hiding this comment.
Not sure the best way to handle this (probably hard code the default the reading mode in setup?) but if the test itself fails the assertion then this code won't be called right? Might just be one of those things that one breakage, causes a chain of failures and not much we can do.
There was a problem hiding this comment.
Versioned is typically a problem across the test cases; I admit this isn't the best handling of versioned state. I'll fix it in this fixture at least.
|
What is the potential of the performance hit of doing this on canView() rather than an pre / post check operation (i.e on publish of the source page).. |
|
Hey @wilr thanks for the review. The problem is that there are plenty of valid use cases where a page should be orphaned; If a parent is temporarily unpublished, we don't want child pages showing up in fulltext search, for instance. It's not an "error" we can (or should) catch at publish or unpublish stage. |
BUG Fix orphaned pages reporting they can be viewed
|
True. Considering that a page without a parent may cause sites to break easily figure 3.1 is appropriate as well. |
|
Thanks @wilr :) |
This issue resolves problems with orphaned pages appearing in search results. This issue has actually appeared in several places in the past, and was typically patched where it was found (e.g. in google sitemaps). This fix resolves the problem closer to the root, ensuring that the default behaviour acts a little more consistently.