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.

Sign up for GitHub

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

Jump to bottom

Enable editing Valkyrie resources with children #5516

Merged
merged 2 commits into from
Mar 16, 2022
Merged

Enable editing Valkyrie resources with children #5516

merged 2 commits into from
Mar 16, 2022

Conversation

eliotjordan
Copy link
Contributor

@eliotjordan eliotjordan commented Mar 9, 2022 •
edited
Loading

Closes #5462
Closes #5447

Adds work-around for two issues:

  1. Named routes for url_for are not available for Valkyrie resources.
  • Solution is to use the main_app helper.
  1. PcdmMemberPresenterFactory does not work with ActiveFedora adapter
  • Solr queries against the Valkyrie and ActiveFedora cores are subtly different, and we have to bypass the qt: standard param.
  • Another complication is that FileSets are indexed with has_model_ssim: FileSet in ActiveFedora and with has_model_ssim: Hyrax::FileSet with the Valkyrie solr adapter.

@eliotjordan eliotjordan force-pushed the 5461-parent-editing branch 2 times, most recently from d71a84b to c30b4d3 Compare March 11, 2022 18:22
@eliotjordan eliotjordan changed the title Include Rails url_helpers Use ActionView url_helpers in MembershipHelper Mar 11, 2022
@eliotjordan eliotjordan marked this pull request as draft March 11, 2022 18:53
@eliotjordan eliotjordan force-pushed the 5461-parent-editing branch 3 times, most recently from b2ba588 to 152c094 Compare March 14, 2022 21:44
@eliotjordan eliotjordan marked this pull request as ready for review March 14, 2022 21:44
@eliotjordan eliotjordan changed the title Use ActionView url_helpers in MembershipHelper Enable editing Valkyrie resources with children Mar 14, 2022
elrayle
elrayle requested changes Mar 15, 2022
View reviewed changes
Copy link
Contributor

@elrayle elrayle left a comment

Choose a reason for hiding this comment

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

This is looking really good. I just have one question.

spec/presenters/hyrax/pcdm_member_presenter_factory_spec.rb
describe '#file_set_presenters' do
it 'is empty' do
expect(factory.file_set_presenters.to_a).to be_empty
context 'with ActiveFedora index adapter' do
Copy link
Contributor

Choose a reason for hiding this comment

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

Good to see this tested with both AF and Valkyrie.

@eliotjordan eliotjordan force-pushed the 5461-parent-editing branch from 152c094 to 00700c6 Compare March 15, 2022 17:21
elrayle
elrayle previously approved these changes Mar 15, 2022
View reviewed changes
Copy link
Contributor

@elrayle elrayle left a comment

Choose a reason for hiding this comment

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

This looks good to me.

cjcolvar
cjcolvar requested changes Mar 15, 2022
View reviewed changes
@eliotjordan eliotjordan force-pushed the 5461-parent-editing branch from 00700c6 to 4758791 Compare March 15, 2022 19:22
@eliotjordan eliotjordan requested a review from cjcolvar March 15, 2022 19:46
@eliotjordan eliotjordan force-pushed the 5461-parent-editing branch from 4758791 to d754171 Compare March 15, 2022 19:48
cjcolvar
cjcolvar previously approved these changes Mar 15, 2022
View reviewed changes
elrayle
elrayle requested changes Mar 15, 2022
View reviewed changes
Copy link
Contributor

@elrayle elrayle left a comment

Choose a reason for hiding this comment

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

AF works need to switch back to using generic_type_sim to avoid backward incompatibility issues.

@eliotjordan eliotjordan force-pushed the 5461-parent-editing branch from d754171 to 38ff5ac Compare March 15, 2022 21:07
elrayle
elrayle reviewed Mar 15, 2022
View reviewed changes
app/indexers/hyrax/file_set_indexer.rb
@@ -30,6 +30,7 @@ def generate_solr_document # rubocop:disable Metrics/AbcSize, Metrics/MethodLeng
solr_doc['original_checksum_tesim'] = object.original_checksum
solr_doc['alpha_channels_ssi'] = object.alpha_channels
solr_doc['original_file_id_ssi'] = original_file_id
solr_doc['generic_type_si'] = 'FileSet'
Copy link
Contributor

Choose a reason for hiding this comment

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

The generic_type field is used in PcdmMemberPresenterFactory. This is called from:

  • Hyrax::WorkShowPresenter #member_presenter_factory which only calls it for Valkyrie::Resource
  • Hyrax::WorkFormHelper #form_file_set_select_for(parent:) which is called from several views.
    • select representative_id in _form_representative.html.erb
    • select rendering_ids in _form_rendering.html.erb
    • select thumbnail_id in _form_thumbnail.html.erb

Copy link
Contributor

Choose a reason for hiding this comment

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

I tested with Main and with this PR's branch. The behavior for GenericWork is the same for both branches. GenericWork uses a different member_presenter_factory that does not depend on generic_type_si, so the addition of this index field has no impact on non-valkyrie works.

@elrayle elrayle merged commit fcb3bda into main Mar 16, 2022
@elrayle elrayle deleted the 5461-parent-editing branch March 16, 2022 15:29
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@elrayle elrayle elrayle approved these changes

@cjcolvar cjcolvar cjcolvar left review comments

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
3 participants
@eliotjordan @cjcolvar @elrayle