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

🐛Account for Valkyrie ID in location indexer #6790

Draft
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

ShanaLMoore
Copy link
Contributor

@ShanaLMoore ShanaLMoore commented Apr 24, 2024

Without passing a string, we get an error when based_near is nil. This was discovered when testing Bulkrax in a valkyrized app. When we uploaded a CSV with no based_near defined, I was getting an error *** ArgumentError Exception: g410220 is not a valid type from the #extract_id method.

    def based_near_prepopulator
      self.based_near = based_near.map do |loc|
        uri = RDF::URI.parse(loc)
        if uri
          Hyrax::ControlledVocabularies::Location.new(uri)
        else
          loc
        end
      end

based_near => [#<Valkyrie::ID:0x0000ffff5f734970 @id="g410220">]

This is because at this point, obj is a neither a string or a URI. It's a Valkyrie::ID.

    def extract_id(obj)
      uri = case obj
            when String
              URI(obj)
            when URI
              obj
            else
              raise ArgumentError, "#{obj} is not a valid type"
            end
      uri.path.split('/').last
    end

@samvera/hyrax-code-reviewers

@ShanaLMoore ShanaLMoore changed the title 🐛 pass loc as a string to location service 🐛Account for Valkyrie ID in location indexer Apr 24, 2024
In #extract_id we were receiving an error because
it didn't account for a Valkyrie::ID in its case
statement. This commit adds a case for Valkyrie::ID.
Copy link

Test Results

    9 files  ±0      9 suites  ±0   16m 55s ⏱️ -22s
4 762 tests ±0  4 699 ✅ +1  63 💤 ±0  0 ❌  - 1 
6 488 runs  ±0  6 425 ✅ +1  63 💤 ±0  0 ❌  - 1 

Results for commit c3386ff. ± Comparison against base commit 42eccf4.

This pull request removes 100 and adds 100 tests. Note that renamed tests count towards both.
spec.abilities.ability_spec ‑ Hyrax::Ability AdminSets and PermissionTemplates a user without edit access is expected not to be able to create #<Hyrax::PermissionTemplate:0x00007f1e87701e10>
spec.abilities.ability_spec ‑ Hyrax::Ability AdminSets and PermissionTemplates a user without edit access is expected not to be able to create #<Hyrax::PermissionTemplateAccess:0x00007f1e879a7818>
spec.abilities.ability_spec ‑ Hyrax::Ability AdminSets and PermissionTemplates a user without edit access is expected not to be able to destroy Hyrax::AdministrativeSet: 8bb0b55d-bca0-4243-af01-8c8ac4e095a3
spec.abilities.ability_spec ‑ Hyrax::Ability AdminSets and PermissionTemplates a user without edit access is expected not to be able to edit Hyrax::AdministrativeSet: bb3e79c0-dc50-4c27-923b-8154a0c5bed0
spec.abilities.ability_spec ‑ Hyrax::Ability AdminSets and PermissionTemplates a user without edit access is expected not to be able to update Hyrax::AdministrativeSet: 1fdd2150-05f4-49b5-947b-a6f51e490f7a
spec.abilities.ability_spec ‑ Hyrax::Ability AdminSets and PermissionTemplates as admin behaves like A user with additional access is expected to be able to create #<Hyrax::PermissionTemplate:0x00007f1e8765caa0>
spec.abilities.ability_spec ‑ Hyrax::Ability AdminSets and PermissionTemplates as admin behaves like A user with additional access is expected to be able to create #<Hyrax::PermissionTemplateAccess:0x00007f1e87533c00>
spec.abilities.ability_spec ‑ Hyrax::Ability AdminSets and PermissionTemplates as admin behaves like A user with additional access is expected to be able to destroy Hyrax::AdministrativeSet: 26561ab6-83c9-4af0-a645-b40628705c4d
spec.abilities.ability_spec ‑ Hyrax::Ability AdminSets and PermissionTemplates as admin behaves like A user with additional access is expected to be able to edit Hyrax::AdministrativeSet: 223a61da-469f-4860-9176-e860ad74f0d1
spec.abilities.ability_spec ‑ Hyrax::Ability AdminSets and PermissionTemplates as admin behaves like A user with additional access is expected to be able to update Hyrax::AdministrativeSet: e321ed3d-292d-4220-ab80-4c674d2c98bf
…
spec.abilities.ability_spec ‑ Hyrax::Ability AdminSets and PermissionTemplates a user without edit access is expected not to be able to create #<Hyrax::PermissionTemplate:0x00007f92143a5018>
spec.abilities.ability_spec ‑ Hyrax::Ability AdminSets and PermissionTemplates a user without edit access is expected not to be able to create #<Hyrax::PermissionTemplateAccess:0x00007f92154d3498>
spec.abilities.ability_spec ‑ Hyrax::Ability AdminSets and PermissionTemplates a user without edit access is expected not to be able to destroy Hyrax::AdministrativeSet: 81ff6a93-e9c9-446d-8478-1007dde90c46
spec.abilities.ability_spec ‑ Hyrax::Ability AdminSets and PermissionTemplates a user without edit access is expected not to be able to edit Hyrax::AdministrativeSet: 91139008-12d5-4808-871d-f72c9ae6dd28
spec.abilities.ability_spec ‑ Hyrax::Ability AdminSets and PermissionTemplates a user without edit access is expected not to be able to update Hyrax::AdministrativeSet: 0b7e1789-c121-4f34-a365-e2cb1fc6c03b
spec.abilities.ability_spec ‑ Hyrax::Ability AdminSets and PermissionTemplates as admin behaves like A user with additional access is expected to be able to create #<Hyrax::PermissionTemplate:0x00007f9217823858>
spec.abilities.ability_spec ‑ Hyrax::Ability AdminSets and PermissionTemplates as admin behaves like A user with additional access is expected to be able to create #<Hyrax::PermissionTemplateAccess:0x00007f9217aef000>
spec.abilities.ability_spec ‑ Hyrax::Ability AdminSets and PermissionTemplates as admin behaves like A user with additional access is expected to be able to destroy Hyrax::AdministrativeSet: 5ede7f56-5136-4fb1-819c-fa543f1e8ba2
spec.abilities.ability_spec ‑ Hyrax::Ability AdminSets and PermissionTemplates as admin behaves like A user with additional access is expected to be able to edit Hyrax::AdministrativeSet: 39d6bdff-3548-4ebf-94f6-dabcbfe298fb
spec.abilities.ability_spec ‑ Hyrax::Ability AdminSets and PermissionTemplates as admin behaves like A user with additional access is expected to be able to update Hyrax::AdministrativeSet: 850cd483-63a0-4ebd-83ca-b0328181c7fb
…

@ShanaLMoore ShanaLMoore marked this pull request as ready for review April 24, 2024 23:10
Copy link
Contributor

@no-reply no-reply left a comment

Choose a reason for hiding this comment

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

seems good, approving.

i think a unit test would improve things---it would be really easy to regress this in a refactor.

i also wonder whether a refactor like uri = URI(obj) wouldn't do the trick? URI() will already raise an informative error message if you try to cast a type it can't handle:

`URI': bad argument (expected URI object or URI string) (ArgumentError)

this seems better than the ambiguous "not a valid type" message you get in our code.


looking at this closer, i definitely think we should do this refactor. it opens the whole class to extension in a way the type check strictly forecloses. if i can pass a type that ruby URI() will cast, i should be able to use it here.

@ShanaLMoore ShanaLMoore marked this pull request as draft April 25, 2024 19:11
@ShanaLMoore
Copy link
Contributor Author

ShanaLMoore commented May 2, 2024

looking at this closer, i definitely think we should do this refactor. it opens the whole class to extension in a way the type check strictly forecloses. if i can pass a type that ruby URI() will cast, i should be able to use it here.

I put this back in draft.

@no-reply Actually, I think the problem is here. It's forcing based_near to have a value even though we aren't declaring one. If we don't do this, based_near remains an empty [ ], and I no longer need this (or a similar) PR to avoid the error. Is there an issue with removing this line?

image

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

Successfully merging this pull request may close these issues.

2 participants