test AdminAdminSetMemberSearchBuilder for collection search #5229
Conversation
no-reply
force-pushed
the
col-admin-set-search
branch
from
435063c to
79f57c1
Compare
no-reply
force-pushed
the
col-admin-set-search
branch
from
79f57c1 to
4acebd9
Compare
update specs to ensure the correct models are included in the search: - always include `Collection`, - include configured collection model name, - include registered curationconcerns should the implementation be updated to search using the indexed generic type?
| @@ -13,7 +13,17 @@ | |||
|
|
|||
| it 'searches for valid work types' do | |||
| expect(builder.filter_models(solr_params)) | |||
| .to contain_exactly("{!terms f=has_model_ssim}Monograph,#{Hyrax.config.collection_class}") | |||
| .to contain_exactly(include("{!terms f=has_model_ssim}Monograph")) | |||
There was a problem hiding this comment.
It seems right to me that admin sets would only filter on work classes. But it seems that the search builder was adding in collection classes. I'm not seeing a change to the builder class, so I'm wondering how it is that this test passes. See question in PR #5207.
There was a problem hiding this comment.
this still tests that the collection classes are included, it just breaks those expectations out into separate specs: https://github.com/samvera/hyrax/pull/5229/files#diff-ab3a1a3b8fc88305f01fc237b7a07fc00abf0736666fa40d77baf46de41c7c65R19-R27
There was a problem hiding this comment.
I see what you are doing here. This won't fail if more collections are added to the list. Are you ok with that? Testing for them all in one test will let us know if another one gets added. This would most likely be ok, but we might want to know so we can evaluate the impact. Not a blocker for me if this is the way you want to go.
There was a problem hiding this comment.
This won't fail if more collections are added to the list. Are you ok with that?
i think so. this approach would have made this test less fragile to the changes that caused the issue here to begin with. i see that as an improvement
| it 'searches for collections indexed as ActiveFedora' do | ||
| expect(builder.filter_models(solr_params)) | ||
| .to contain_exactly(include("Collection")) | ||
| end | ||
|
|
||
| it 'searches for collections indexed as valkyrie' do | ||
| expect(builder.filter_models(solr_params)) | ||
| .to contain_exactly(include(Hyrax.config.collection_class.to_s)) | ||
| end |
There was a problem hiding this comment.
An alternative that searches for all known collections...
| it 'searches for collections indexed as ActiveFedora' do | |
| expect(builder.filter_models(solr_params)) | |
| .to contain_exactly(include("Collection")) | |
| end | |
| it 'searches for collections indexed as valkyrie' do | |
| expect(builder.filter_models(solr_params)) | |
| .to contain_exactly(include(Hyrax.config.collection_class.to_s)) | |
| end | |
| it 'searches for collections' do | |
| Hyrax.collection_classes do |klass| | |
| expect(builder.filter_models(solr_params)) | |
| .to contain_exactly(include(klass.to_s)) | |
| end | |
| end |
update specs to ensure the correct models are included in the search:
Collection,should the implementation be updated to search using the indexed generic type?
@samvera/hyrax-code-reviewers