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

Move Bulkrax field mappings to Hyku #2389

Conversation

bkiahstroud
Copy link
Collaborator

Follow up to:

A recent Hyku feature introduced the ability to configure Bulkrax field mappings on a per-tenant basis. It also introduced a bug that impacted existing Hyku applications who had custom field mappings. Since the feature fell back on Bulkrax's default field mappings if an Account hadn't explicitly set their own, the existing field mapping customizations in the Bulkrax initializer were ignored.

To solve that issue, as well as the question of "what if I want all my tenants to have the same field mappings, but don't want to customize them all by hand?", this commit introduces the idea of a set of default field mappings at the Hyku application level.

Caution

This is a breaking change

Existing Hyku applications with custom Bulkrax field mappings will need to move them from their config/initializers/bulkrax.rb to Hyku#default_bulkrax_field_mappings in config/application.rb

Now, when Bulkrax looks for field mappings, it will discover them in this order (depending on presence):

Account setting? -> Hyku defaults? -> Bulkrax defaults

There are a couple reasons why I decided to put this in the Hyku module instead of just using Hyku's Bulkrax initializer:

  1. Since they're Hyku's defaults, it makes sense to denote them as such semantically and conceptually
  2. Since the ultimate fallback is Bulkrax's default field mappings, it makes sense not to alter the field mappings that Bulkrax "manages" with this feature
  3. When adding hyku_knapsack into the mix, their often ends up being three separate Bulkrax config files (the knapsack's config/initializers/bulkrax.rb, Hyku's config/initializers/bulkrax.rb, and Bulkrax's lib/bulkrax.rb). This gets very muddy very quickly with how knapsack works technically

@samvera/hyku-code-reviewers

A recent Hyku feature introduced the ability to configure Bulkrax field
mappings on a per-tenant basis. It also introduced a bug that impacted
existing Hyku applications who had custom field mappings. Since the
feature fell back on Bulkrax's default field mappings if an Account
hadn't explicitly set their own, the existing field mapping
customizations in the Bulkrax initializer were ignored.

To solve that issue, as well as the question of "what if I want all my
tenants to have the same field mappings, but don't want to customize
them all by hand?", this commit introduces the idea of a set of default
field mappings at the Hyku application level.

This means that when Bulkrax looks for field mappings, it will discover
them in this order (depending on presence):

Account setting? -> Hyku defaults? -> Bulkrax defaults

There are a couple reasons why I decided to put this in the Hyku module
instead of just using Hyku's Bulkrax initializer:
1. Since they're Hyku's defaults, it makes sense to denote them as such
   semantically and conceptually
2. Since the ultimate fallback is Bulkrax's default field mappings, it
   makes sense not to alter the field mappings that Bulkrax "manages"
   with this feature
3. When adding hyku_knapsack into the mix, their often ends up being
   three separate Bulkrax config files (the knapsack's
   config/initializers/bulkrax.rb, Hyku's
   config/initializers/bulkrax.rb, and Bulkrax's lib/bulkrax.rb). This
   gets very muddy very quickly with how knapsack works technically

Ref:
- #2384
@bkiahstroud bkiahstroud added the major-ver for release notes label Nov 23, 2024
Copy link

github-actions bot commented Nov 23, 2024

Test Results

    3 files  ±0      3 suites  ±0   18m 3s ⏱️ -20s
2 053 tests +6  2 003 ✅ +7  50 💤 ±0  0 ❌  - 1 
2 080 runs  +6  2 028 ✅ +7  52 💤 ±0  0 ❌  - 1 

Results for commit 2051fdf. ± Comparison against base commit 571c2de.

This pull request removes 44 and adds 50 tests. Note that renamed tests count towards both.
spec.abilities.work_ability_spec ‑ Hyrax::Ability::WorkAbility when work depositor Etd permissions is expected not to be able to destroy 1585adcb-6113-45fd-b7ed-ea947c73a71d
spec.abilities.work_ability_spec ‑ Hyrax::Ability::WorkAbility when work depositor Etd permissions is expected not to be able to edit 035d082f-9152-4b56-8241-701432d748f4
spec.abilities.work_ability_spec ‑ Hyrax::Ability::WorkAbility when work depositor Etd permissions is expected not to be able to read e2565459-4945-48d8-a282-df2b26200af2
spec.abilities.work_ability_spec ‑ Hyrax::Ability::WorkAbility when work depositor Etd permissions is expected not to be able to update a8e985cd-3e60-486e-8821-8b6c7e9a531d
spec.abilities.work_ability_spec ‑ Hyrax::Ability::WorkAbility when work depositor FileSet permissions is expected not to be able to destroy 506d33f5-d061-4fd0-b076-128d76126a5b
spec.abilities.work_ability_spec ‑ Hyrax::Ability::WorkAbility when work depositor FileSet permissions is expected not to be able to edit 759c8043-8ac3-4aa8-b21d-45f23c40ecf0
spec.abilities.work_ability_spec ‑ Hyrax::Ability::WorkAbility when work depositor FileSet permissions is expected not to be able to read faf44d9b-7ac9-43e4-9bbe-399c739b6944
spec.abilities.work_ability_spec ‑ Hyrax::Ability::WorkAbility when work depositor FileSet permissions is expected not to be able to update e3045cf0-3795-4c9d-83c3-d39e6f5fcc2a
spec.abilities.work_ability_spec ‑ Hyrax::Ability::WorkAbility when work depositor GenericWork permissions is expected not to be able to destroy 4bbf76e6-e710-4075-bedf-10ce38bed194
spec.abilities.work_ability_spec ‑ Hyrax::Ability::WorkAbility when work depositor GenericWork permissions is expected not to be able to edit 51795746-9b9a-466c-83f0-439d63704ceb
…
spec.abilities.work_ability_spec ‑ Hyrax::Ability::WorkAbility when work depositor Etd permissions is expected not to be able to destroy a3388a52-91b3-47f3-a7e0-62d5fefb5809
spec.abilities.work_ability_spec ‑ Hyrax::Ability::WorkAbility when work depositor Etd permissions is expected not to be able to edit f9279256-6d1f-4d01-885c-77343d4316ee
spec.abilities.work_ability_spec ‑ Hyrax::Ability::WorkAbility when work depositor Etd permissions is expected not to be able to read 5c593c5c-8e85-433e-b69c-2710b5e90126
spec.abilities.work_ability_spec ‑ Hyrax::Ability::WorkAbility when work depositor Etd permissions is expected not to be able to update 1d6292ba-da67-40bd-be62-f66da8d87331
spec.abilities.work_ability_spec ‑ Hyrax::Ability::WorkAbility when work depositor FileSet permissions is expected not to be able to destroy 01a3f6b2-4701-40c4-9d01-d5de79a3f32e
spec.abilities.work_ability_spec ‑ Hyrax::Ability::WorkAbility when work depositor FileSet permissions is expected not to be able to edit 7b54c1e6-0b7e-4f4a-850d-926623fb5213
spec.abilities.work_ability_spec ‑ Hyrax::Ability::WorkAbility when work depositor FileSet permissions is expected not to be able to read da3238c4-782b-477f-b10e-e1c1932014aa
spec.abilities.work_ability_spec ‑ Hyrax::Ability::WorkAbility when work depositor FileSet permissions is expected not to be able to update 9d813bfe-d5f7-4a34-972d-b0813eedb0e1
spec.abilities.work_ability_spec ‑ Hyrax::Ability::WorkAbility when work depositor GenericWork permissions is expected not to be able to destroy badf163d-220f-4e7b-913d-02c70c7cdf41
spec.abilities.work_ability_spec ‑ Hyrax::Ability::WorkAbility when work depositor GenericWork permissions is expected not to be able to edit 06d0d2a6-e4a8-4e48-b534-150b10617ea0
…

♻️ This comment has been updated with latest results.

This can be used by a hyku_knapsack app initializer to override the
default Hyku application field mappings
laritakr
laritakr previously approved these changes Nov 25, 2024
Copy link
Collaborator

@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.

Tested this in a knapsack app and it is working as expected.

Copy link
Collaborator

@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.

Looks great!

@laritakr laritakr merged commit 36c6aab into main Nov 26, 2024
8 checks passed
@laritakr laritakr deleted the i286-move-responsibility-of-default-field-mappings-from-bulkrax-to-hyku branch November 26, 2024 15:56
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
major-ver for release notes
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants