Fixing Works Created Count Discrepency - #3478

[jeremyf]

Adjusting logic to ensure use of local variable

b0b5dc4

Prior to this commit, we could be passing the application controller's
params instead of of the local variable params. This could result
in some unexpected behavior.

Note, in SHA @b04c41d2772505d8a548c3fd48774a5642d3ac90 we went from:

params = { search_field: name, q: "\"#{value}\"" }
state = search_state_with_facets(params, facet_hash)

to

params = { search_field: name, q: "\"#{value}\"" } if name != ''
state = search_state_with_facets(params, facet_hash)

The prior change meant that instead of always having the local variable
named params, we would sometimes fallback to the application's
params method.

I don't think this is a major issue, but one that could introduce some
unexpected results.

Related to #4322, #3478

Introducing where clause for number_of_works()

980dc92

Based on some investigation around #3478, I suspect that the
Hyrax::WorkRelation#where call that uses only a depositor field and
user key is finding more results than the query run on the catalog
search page.

By adding the where clause, I hope to align the query logic of the
number_of_works and the link_to_field on the _vitals.html.erb partial.

Related to #3478

Adding where clause to align counts on two pages

212cf16

On the "vitals" page, there is a count that we derive from
number_of_works. When you click on the link_to_field that new page
also as a count based on the provided facets. Prior to this commit, I
believe the underlying queries were not using the same logic.

With this change, I hope to bring this in alignment.

Please note, I don't have tests to confirm this, nor a data state that
would help confirm it. So, I'm pushing up a change that I believe
should work.

My plan is to write some helper tests at a later date, but for now am
drawing to the end of my work day.

Related to #3478

Moving where clause into helper

7abf757

Per discussion on #4494, a view
likely should not know about the Solr document field names. This is
better served in a helper.

[cjcolvar]

@jeremyf I didn't do a full dive on your code and I'm a bit fuzzy on this area of the codebase, but I would have thought that using Hyrax::WorkRelation would have scoped the results to only works such that you shouldn't need to send that extra where condition. Other parts of the code are using Hyrax::WorkRelation as well so I'm concerned that they're broken as well.

[jeremyf]

@cjcolvar That is definitely a possible concern. I'm not overly familiar with this as well. A real challenge is that to test "does this work" requires a complex state. And from that complex state, we could kick the tires of the other Hyrax::WorkRelation calls.

[no-reply]

see question about arguments passed from views.

[no-reply]

@jeremyf I didn't do a full dive on your code and I'm a bit fuzzy on this area of the codebase, but I would have thought that using Hyrax::WorkRelation would have scoped the results to only works such that you shouldn't need to send that extra where condition. Other parts of the code are using Hyrax::WorkRelation as well so I'm concerned that they're broken as well.

my understanding is that the count discrepancy is due to there being two different ways of scoping to "only works" in play: the generic_type_sim field vs. the model names + curation_concerns configuration.

WorkRelation does scope to works only, but it does so using a list of registered curation_concerns minus FileSet and Collection. the search results page limits based on generic_type. in theory, these should be equivalent in normal use, but i doubt there's a good reason not to just use generic_type in WorkRelation.

i'd like to be moving away from relying on curation_concerns registration for so many things---i think it makes sense for configuring routes, but there's a lot of complex code forcing this class list into a usable shape for differing purposes. generic_type seems better here, at least.

i don't think we should block this on a better resolution, though. i'd be in favor of approving this upon resolution of the discussion here: #4494 (comment)

I've made the suggested changes