Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[i387] - add default value to is_child property #237

Merged
merged 2 commits into from
May 11, 2023
Merged

Conversation

ShanaLMoore
Copy link
Contributor

@ShanaLMoore ShanaLMoore commented May 11, 2023

Story

This PR is to address the feedback from the following ticket. The user was still able to see the child works in the catalog and recent works section. To resolve this, we are setting the child work is_child attribute to be true on default.

Refs scientist-softserv/britishlibrary#387

Expected Behavior Before Changes

ref: scientist-softserv/britishlibrary#387 (comment)

image

Expected Behavior After Changes

As a result, you cannot see the child records while it's still processing. The user should and does not see child works in the following scenarios:

EMPTY CATALOG SEARCH :

image

RECENT WORKS SECTION:

image

VIEW RECENT ADDITIONS CATALOG PG:

image

Sidekiq

Notice is_child property is being set to true in the CreateWorkJob:

image

sample work: service-rbc-rbc0001-2015-2015gen56010-2015gen56010.pdf

Notes

@ShanaLMoore ShanaLMoore self-assigned this May 11, 2023
Copy link
Contributor

@laritakr laritakr left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This makes a lot of sense, since we never want these to appear, and it should fix it for everyone!

I'm still curious if we know why the searches don't exclude the work types that are configured to be excluded? I thought that was the purpose of that config option. If it isn't then what does that configuration do?

@ShanaLMoore
Copy link
Contributor Author

ShanaLMoore commented May 11, 2023

This makes a lot of sense, since we never want these to appear, and it should fix it for everyone!

I'm still curious if we know why the searches don't exclude the work types that are configured to be excluded? I thought that was the purpose of that config option. If it isn't then what does that configuration do?

The catalog search is fine, however the work index page is not because that code lives in UTK only. I started to bring it over to iiif_print but given the discussion we had in today's retro, maybe making a ticket for it to be picked up later is better? I can lay out the details of what needs to be done. @laritakr

ref:
image

UTK's implementation: https://github.com/scientist-softserv/utk-hyku/blob/main/app/search_builders/hyrax/filter_by_type_decorator.rb

@laritakr
Copy link
Contributor

laritakr commented May 12, 2023

The catalog search is fine, however the work index page is not because that code lives in UTK only.

We don't want to eliminate these from the dashboard views (that is the part that is only in UTK, correct?).

I thought the config option removed them from the catalog search and recent items searches, but perhaps I misunderstand that config?

Regardless, this is an ideal situation for these generated child works. I'm only concerned in case we ever DO want to eliminate a work type from the catalog search results, as we tried to do before this change.

@ShanaLMoore
Copy link
Contributor Author

ShanaLMoore commented May 12, 2023

Ah ok. It's possible I misunderstood or am confused by all the hiding of work types across the various apps too 🙃 Yes, that feature was requested by UTK to hide Attachments. I created a ticket with the assumption that it should be hidden everywhere, like UTK's but i'll add a discuss label to it.

What you're saying makes sense because that's how it was implemented, to exclude the work types from catalog search. Ref the readme. Unfortunately the readme just says search and doesn't specify.

Perhaps this spec can offer us some clarity:

# specs for IiifPrint::ExcludeModels
describe 'exclude_models' do
let(:solr_parameters) { { all_fields: 'prohibition' } }
subject { described_class.new(solr_parameters) }
it 'is included in the default_processor_chain' do
expect(described_class.default_processor_chain).to include(:exclude_models)
end
context 'with configured model name solr field values' do
before do
config = IiifPrint::Configuration.new.tap { |c| c.excluded_model_name_solr_field_values = ['Excluded Model', 'Another Excluded Model'] }
subject.exclude_models(solr_parameters, config: config)
end
it 'adds the facet fields to solr_parameters with default key' do
expect(solr_parameters[:fq]).to be_truthy
expect(solr_parameters[:fq]).to(
include("-human_readable_type_sim:\"Excluded Model\"", "-human_readable_type_sim:\"Another Excluded Model\"")
)
end
context 'with configured model name solr field key' do
before do
config = IiifPrint::Configuration.new.tap do |c|
c.excluded_model_name_solr_field_values = ['ExcludedModel', 'AnotherExcludedModel']
c.excluded_model_name_solr_field_key = 'has_model_ssim'
end
subject.exclude_models(solr_parameters, config: config)
end
it 'adds the facet fields to solr_parameters with configured key' do
expect(solr_parameters[:fq]).to be_truthy
expect(solr_parameters[:fq]).to(
include("-has_model_ssim:\"ExcludedModel\"", "-has_model_ssim:\"AnotherExcludedModel\"")
)
end
end
end
end
I observed that this didn't behave that way for the recent works catalog search too. Maybe we can see if @kirkkwang remembers if it should.

@kirkkwang
Copy link
Contributor

From what I understand, it doesn't remove from recent works, only the catalog search.

@laritakr
Copy link
Contributor

I'm pretty sure I made a later change to remove it from recent works as well. I will search for that.

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

Successfully merging this pull request may close these issues.

3 participants