Allow facet level sorting #300

whikloj · 2016-12-14T22:14:45Z

JIRA Ticket: https://jira.duraspace.org/browse/ISLANDORA-1337

https://groups.google.com/forum/#!topic/islandora/-sO_VrHBIQg

What does this Pull Request do?

Add a configuration option to facets to allow you to change the sort order from by count to by labels.

What's new?

New solr_field_settings parameter sort_by with either count or index (count is default).

How should this be tested?

Apply PR.
Make a facet with some values for testing. Leave Sort By as Counts.
Do search, see normal facet ordering.
Edit the facet and change Sort By to Labels. Save settings.
Re-run search and see facets ordered by labels.

Example:

Does this change require documentation to be updated? There is not much documentation about Facets, so I guess no.
Does this change add any new dependencies? No
Does this change require any other modifications to be made to the repository (ie. Regeneration activity, etc.)? No
Could this change impact execution of existing code? No

Interested parties

@DiegoPino (as he was working Islandora-1337)

DiegoPino

@whikloj as always, thanks for this. You are way more concrete and faster than me. I would just ask for some minor corrections:
Travis is failing because of some minor coding standard issues
See: https://travis-ci.org/Islandora/islandora_solr_search/jobs/184075282#L455-L457

It passes other tests because, as we all already known, in many modules and versions codesniffer is simply not running.

DiegoPino · 2016-12-14T23:03:57Z

includes/results.inc

@@ -854,6 +854,17 @@ class IslandoraSolrFacets {
  }

  /**


Missing short description of the function here

DiegoPino · 2016-12-14T23:05:25Z

includes/results.inc

@@ -854,6 +854,17 @@ class IslandoraSolrFacets {
  }

  /**
+   *
+   * @return mixed


seems like codeSniffer is expecting a comment after/partof @return statement? Kinda new for me, or i'm just getting old.

whikloj · 2016-12-15T14:15:00Z

@DiegoPino Oops, I made that function and then never used it. I have removed it, that should remove those errors.

DiegoPino · 2016-12-15T16:31:28Z

@whikloj thanks. Will test and should be ready to merge by today. =)

DiegoPino · 2016-12-15T19:00:33Z

@whikloj will wait until we reach the 24hr mark. I'm fine with this by the way 😃

adam-vessey · 2016-12-15T20:25:47Z

includes/results.inc

+    }
+
+    if (isset($this->settings['solr_field_settings']['sort_by']) && $this->settings['solr_field_settings']['sort_by'] == 'index') {
+      ksort($buckets);


Running this way... this would return the top number of results by count, ordered by value; while, it may be expected that it gives the top values (how I first read it, anyway). For example, say you're limiting the number of values returned, and by numbers, would only get things near the bottom of things lexicographically... Could end up with strange results.

May have to go jam things into the $params_array... Untested, but thinking something like:

$facet_array = islandora_solr_get_fields('facet_fields', TRUE, FALSE); [...] $params_array = array( 'facet' => 'true', 'facet.mincount' => variable_get('islandora_solr_facet_min_limit', '2'), 'facet.limit' => variable_get('islandora_solr_facet_max_limit', '20'), 'facet.field' => explode(',', $facet_fields), ); foreach ($facet_array as $field => $info) { $settings = $info['solr_field_settings']; if (($settings['sort_by'] == 'count' && $params_array['facet.limit'] < 0) || ($settings['sort_by'] == 'index' && $params_array['facet.limit'] > 0)) { // Avoid specifying default behaviour: // @see https://wiki.apache.org/solr/SimpleFacetParameters#facet.sort // "The default is count if facet.limit is greater than 0, index otherwise." $params_array["facet.{$field}.sort"] = $settings['sort_by']; } }

@adam-vessey I did that, but the problem was that you then still have to sort it this way as (for example) sorting by content model in solr you are sorting

islandora:compoundCModel islandora:bookCModel islandora:pageCModel

but then in Islandora you see the transformed text but still sorted based on the old text values.

I can go do what you are suggesting, but we still need to sort here to account for the later transformations.

@adam-vessey Just re-reading your comments

For example, say you're limiting the number of values returned, and by numbers, would only get things near the bottom of things lexicographically... Could end up with strange results.

This is totally true, but all I am providing is a way to re-order the facets you have from by count to by value. I was trying not to sell this as anything more than adjusting the sort order of your existing facets.

@whikloj

Just trying to avoid confusion... The JIRA ticket indicates:

This would implement facet.sort ...

... which is not happening here... Should this be a distinct ticket?

Also, I feel that there may be mixups between the two... Could expand on the description beside the option, to be more explicit in the difference.

... Dealing with the transformations... I'm not sure there's any clean way to do it generally... Almost feeling like it may be more reliably done with something concatenating things together into a purpose-built field in the index... something like the text to display/the:pid, and having something strip off the /the:pid bit when displaying (some arbitrary separator could be used instead of a forward-slash... just using it here as an example)... Dunno...

@adam-vessey Oops, that is my mistake. I did try that until I ran in issue of sorting by one value then displaying a different one (ie. replace with object label). I'll update the ticket with the issue I had with that method.

I'll also work on some more descriptive wording for the options (maybe switch to radio buttons).

As for transformations, I see what you are suggesting. Which isn't a bad idea, but would require a wholesale change to the way facets are created...I think. If not, I'm open to it. But I might need a little direction.

DiegoPino · 2017-02-21T17:28:18Z

@adam-vessey @whikloj are we OK with this one? Would love to see this in the new release (as a new feature). Thanks

adam-vessey · 2017-02-21T17:48:09Z

@DiegoPino, @whikloj: Were it described as a feature separate from ISLANDORA-1337 (as in, a separate ticket entirely)... I think it would be fine? The original discussion (linked from ISLANDORA-1337) regarded ordering dates/years, which could end up with holes as described above with "strange results".

... Also, thinking we should be more explicit about what this new ticket does... As if/when ISLANDORA-1337 is actually implemented using facet.sort, I imagine there being confusion about how the two options may interact.

whikloj · 2017-02-21T17:58:13Z

@adam-vessey Those are fair points... I can see a clear path to using facet.sort with dates and numbers. Its just handling the renderText which changes the text value we sorted on to the object label. That could cause you to get odd results.

But I'll move back to that attempt and perhaps you can help me solve any issues that arise.

@DiegoPino I'm not sure this will be complete for code freeze, but I'll see what I can do.

DiegoPino · 2017-02-21T18:09:02Z

@whikloj no worries. Thanks for taking care of this. @adam-vessey thanks for the reviews. Even if not ready for code freeze, i really appreciate the work implied 😄

DiegoPino · 2017-02-21T20:20:35Z

includes/query_processor.inc

@@ -236,6 +236,19 @@ class IslandoraSolrQueryProcessor {
      $params_array = array_merge($params_array, $params_date_facets);
    }

+    // Determine the default facet sort order.
+    // https://cwiki.apache.org/confluence/display/solr/Faceting#Faceting-Thefacet.sortParameter


Line 240 is giving "Line exceeds 80 characters; contains 96 characters". Since It's a link, so not sure how you want to split that into 2 lines.

DiegoPino · 2017-02-27T14:26:10Z

@whikloj give me ping when you feel you are ready for another review. Thanks!

whikloj · 2017-04-17T18:53:27Z

Sorry for the delay, so this seems to work.

But again if you add (for example) the content model and replace the entry with the object label, then the order will be wrong again.

Not sure there is a simple solution to that problem...so I think this is a handy feature so long as you are aware of that.

whikloj · 2017-04-24T15:01:47Z

bump

DiegoPino · 2017-04-24T15:12:55Z

@whikloj i got confused by your comment, sorry. how do we make people aware of the limitations? is that another ticket?

whikloj · 2017-04-24T16:03:05Z

@DiegoPino I'm not sure what the best way to handle that would be.

Perhaps just a warning that if you use this sort on a field that you have already enabled "Replace PID with Object Label" then the sort order will not work. But I'm not really sure.

DiegoPino · 2017-04-24T18:22:21Z

@whikloj I feel that what you suggest there is fine, a warning/little comment on the UI side should be enough. No rush on this. I'm not the best at wording stuff, so feel free to suggest/commit and we can ask @manez or @adam-vessey to review?

DiegoPino · 2017-05-02T20:29:49Z

@whikloj this just needs that last piece of wording and we should be ready to merge. Thanks

whikloj · 2017-05-03T15:03:46Z

@DiegoPino I stuck that under the radio buttons on the facet configuration... I think that is probably the best place for it.
752ebdf

whikloj · 2017-07-11T15:15:15Z

bump

DiegoPino · 2017-07-11T15:17:04Z

@whikloj oh... i had forgotten about this. I will merge now. This was checked and tested, and wording was Ok for me.

Allow facet level sorting

1f95a10

DiegoPino requested changes Dec 14, 2016

View reviewed changes

Remove unused function

1ae3c7c

DiegoPino approved these changes Dec 15, 2016

View reviewed changes

adam-vessey reviewed Dec 15, 2016

View reviewed changes

Update text for the admin page around the new sort options.

4d9a5db

whikloj added 2 commits February 21, 2017 13:21

Use f.foo.facet.sort instead

0d65ee8

Update admin form description.

752ebdf

DiegoPino reviewed Feb 21, 2017

View reviewed changes

Remove link comment

df25927

DiegoPino merged commit c836f6a into Islandora:7.x Jul 11, 2017

whikloj mentioned this pull request Jul 16, 2018

Make facets work with Solr 7 #342

Merged

whikloj deleted the ISLANDORA-1337 branch February 7, 2019 00:25

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Allow facet level sorting #300

Allow facet level sorting #300

whikloj commented Dec 14, 2016

DiegoPino left a comment •

edited

Loading

DiegoPino Dec 14, 2016

DiegoPino Dec 14, 2016

whikloj commented Dec 15, 2016

DiegoPino commented Dec 15, 2016

DiegoPino commented Dec 15, 2016

adam-vessey Dec 15, 2016 •

edited

Loading

whikloj Dec 16, 2016

whikloj Jan 11, 2017

adam-vessey Jan 11, 2017 •

edited

Loading

whikloj Jan 12, 2017

DiegoPino commented Feb 21, 2017

adam-vessey commented Feb 21, 2017

whikloj commented Feb 21, 2017

DiegoPino commented Feb 21, 2017

DiegoPino Feb 21, 2017

DiegoPino commented Feb 27, 2017

whikloj commented Apr 17, 2017

whikloj commented Apr 24, 2017

DiegoPino commented Apr 24, 2017

whikloj commented Apr 24, 2017

DiegoPino commented Apr 24, 2017

DiegoPino commented May 2, 2017

whikloj commented May 3, 2017

whikloj commented Jul 11, 2017

DiegoPino commented Jul 11, 2017

Allow facet level sorting #300

Allow facet level sorting #300

Conversation

whikloj commented Dec 14, 2016

What does this Pull Request do?

What's new?

How should this be tested?

Interested parties

DiegoPino left a comment • edited Loading

Choose a reason for hiding this comment

DiegoPino Dec 14, 2016

Choose a reason for hiding this comment

DiegoPino Dec 14, 2016

Choose a reason for hiding this comment

whikloj commented Dec 15, 2016

DiegoPino commented Dec 15, 2016

DiegoPino commented Dec 15, 2016

adam-vessey Dec 15, 2016 • edited Loading

Choose a reason for hiding this comment

whikloj Dec 16, 2016

Choose a reason for hiding this comment

whikloj Jan 11, 2017

Choose a reason for hiding this comment

adam-vessey Jan 11, 2017 • edited Loading

Choose a reason for hiding this comment

whikloj Jan 12, 2017

Choose a reason for hiding this comment

DiegoPino commented Feb 21, 2017

adam-vessey commented Feb 21, 2017

whikloj commented Feb 21, 2017

DiegoPino commented Feb 21, 2017

DiegoPino Feb 21, 2017

Choose a reason for hiding this comment

DiegoPino commented Feb 27, 2017

whikloj commented Apr 17, 2017

whikloj commented Apr 24, 2017

DiegoPino commented Apr 24, 2017

whikloj commented Apr 24, 2017

DiegoPino commented Apr 24, 2017

DiegoPino commented May 2, 2017

whikloj commented May 3, 2017

whikloj commented Jul 11, 2017

DiegoPino commented Jul 11, 2017

DiegoPino left a comment •

edited

Loading

adam-vessey Dec 15, 2016 •

edited

Loading

adam-vessey Jan 11, 2017 •

edited

Loading