BUG Fix issue with page search ignoring filters #992

Merged
merged 1 commit into from Apr 28, 2014

Projects

None yet

5 participants

@tractorcow
Collaborator

Reworked solution to #969

@phptek do you mind to please review this?

I've changed the search filter titles to:

  • All pages
  • All pages, including deleted
  • Changed pages
  • Deleted pages
  • Draft unpublished pages
  • Live but removed from draft
@tractorcow tractorcow changed the title from Pulls/966 fix searchfilters to BUG Fix issue with page search ignoring filters Apr 10, 2014
@phptek
phptek commented Apr 22, 2014

Search titles fine with me. Personally I liked the <label>: <full-title> format, but as long as the functionality's there, I'm happy. Thanks for taking an interest!

@tractorcow
Collaborator

I figured it would be best to stick to a single format for the whole list, and try to make it as close to the terminology used in the rest of the page as possible.

Now we just need someone to merge this. @chillu ? :)

@mateusz mateusz commented on the diff Apr 27, 2014
code/controllers/CMSSiteTreeFilter.php
@@ -125,7 +129,68 @@ public function isPageIncluded($page) {
return (isset($this->_cache_ids[$page->ID]) && $this->_cache_ids[$page->ID]);
}
+
+ /**
+ * Applies the default filters to a specified DataList of pages
+ *
+ * @param DataList $query Unfiltered query
+ * @return DataList Filtered query
+ */
+ protected function applyDefaultFilters($query) {
@mateusz
mateusz Apr 27, 2014 SilverStripe Ltd. member

With that change, 'CMSSiteTreeFilter_Search' doesn't make sense anymore. API change though, so probably not appropriate until 3.2 :-|

@mateusz mateusz commented on an outdated diff Apr 27, 2014
code/controllers/CMSSiteTreeFilter.php
// remove abstract CMSSiteTreeFilter class
array_shift($filters);
// add filters to map
$filterMap = array();
+ // Ensure that 'all pages' filter is on top position and everything else is sorted alphabetically
@mateusz
mateusz Apr 27, 2014 SilverStripe Ltd. member

That's a bit unreadable :-) called is misleading, could change to filterTitle or something like that. In any case that seems to be a roundabout way of pulling that specific filter to the top, why not something more explicit?

// ... at the end of the function:
// Pull out the "All pages" filter to the top.
unset($allFiltersArray['CMSSiteTreeFilter_Search']);
return array('CMSSiteTreeFilter_Search' => CMSSiteTreeFilter_Search::title()) + $allFiltersArray;
@mateusz
Member
mateusz commented Apr 27, 2014

Squash please, too.

@mateusz mateusz and 1 other commented on an outdated diff Apr 27, 2014
tests/controller/CMSSiteTreeFilterTest.php
+ $removedDraftPage->delete();
+ $this->assertEmpty($f->isPageIncluded($removedDraftPage));
+ }
+
+ public function testStatusDeletedFilter() {
+ $deletedPage = $this->objFromFixture('Page', 'page7');
+ $deletedPage->publish('Stage', 'Live');
+ $deletedPageID = $deletedPage->ID;
+
+ // Can't use straight $blah->delete() as that blows it away completely and test fails
+ $deletedPage->deleteFromStage('Live');
+ $deletedPage->deleteFromStage('Draft');
+
+ /*
+ * Pretty funky way to get the data out. But none of the Versioned::get_xx() methods
+ * worked out of the box, not even get_including_deleted() which the logic under test actually uses!
@mateusz
mateusz Apr 27, 2014 SilverStripe Ltd. member

Curious what didn't work - do you mean get_including_deleted('SiteTree', 'ID={$deletedPageID}')?

@tractorcow
tractorcow Apr 28, 2014 collaborator

Have to as @phptek that, since this is under his authorship. I'll see if I can rewrite this.

@tractorcow
tractorcow Apr 28, 2014 collaborator

I've discovered a bug in Versioned::get_including_deleted and documented it at silverstripe/silverstripe-framework#3075

@tractorcow
tractorcow Apr 28, 2014 collaborator

We can use Versioned::get_latest_version in this case to get the correct record, but the bug above is still an issue in framework.

@mateusz
Member
mateusz commented Apr 27, 2014

Also, consider updating search-for-a-page.feature behat test to test for the new functionality.

Russell Michell NEW Fixes #966. Ability to filter pages on page status.
- New filters for statuses normally found through SiteTree::getStatusFlags().
- Refactored menu sorting. Now alphabetical, as it wasn't previously.
a502c9d
@tractorcow
Collaborator

Hi @mateusz , I've updated the above with your feedback:

  • CMSSiteTreeFilter::get_all_filters now uses a fixed version of the original code instead of the rewrite from the old PR.
  • Squashed and rebased.
  • Added behat tests
  • CMSSiteTreeFilterTest now uses Versioned::get_latest_version since it doesn't have the same bug with requesting versioned records by id. silverstripe/silverstripe-framework#3075 will still need to be addressed at some point.
@tractorcow
Collaborator

@phptek seems to have credit for the commit at least. Stealing my thunder I guess eh?

@mateusz
Member
mateusz commented Apr 28, 2014

One last comment: the CMSSiteTreeFilterTest seems redundant now that we have the behat test. This is supposed to be a unit test, but we are testing all sorts of stuff, like the cache (isPageIncluded instead of pagesIncluded) and so on. I guess can wait for 3.2

@mateusz mateusz merged commit 072e0d9 into silverstripe:3.1 Apr 28, 2014
@mateusz
Member
mateusz commented Apr 28, 2014

Thanks @tractorcow and @phptek .

@tractorcow tractorcow deleted the tractorcow:pulls/966-fix-searchfilters branch Apr 28, 2014
@stojg
Collaborator
stojg commented Jul 14, 2014

FYI: the Versioned::get_latest_version doesn't work correctly. I'm currently looking into changing the labels for the filters, the values and fixing any bugs around this.

Basically the current "Pages filters" are nonsensical, the labels/badges are confusing and the filters not working as expected.

@phptek
phptek commented Jul 14, 2014

@stojg if you can get this through I will be your friend forever:

Basically the current "Pages filters" are nonsensical, the labels/badges are confusing and the filters not working as expected.

Amen.

@phptek
phptek commented Jul 14, 2014

My 2c originally was to go with established concepts via the "lozenges" shown in SiteTree, using the same text in the dropdowns, as per SiteTree::getStatusFlags()

@stojg
Collaborator
stojg commented Jul 14, 2014

Current wip is https://github.com/stojg/silverstripe-cms/compare/pull/change-filter-labels

@phptek I'll try, I've had chats with @clarkepaul and @adrexia, I'm sure I will find more people with passion for UX to provide the answers.

@tractorcow
Collaborator

So are we adding the "archived" terminology to the CMS? What is the distinction between delete and archive? I'm not sure how adding another term makes what a deleted page is more clear.

@stojg
Collaborator
stojg commented Jul 14, 2014

the reason is that it's never deleted. Content editors believes that the page is lost forever, where as a fact it's just .. archived. The terminology 'delete' is a technical term. I've had numerous conversations with content editor where we totally misunderstood each other.

@adrexia
adrexia commented Jul 14, 2014

Agree, "Archived" is a more correct term for what is happening. No page added is ever deleted, and that has confused quite a few CMS users in the past.

@tractorcow
Collaborator

Ok, that makes sense, well as long as we are consistent across the CMS. :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment