samvera / hyrax

Hyrax is a Ruby on Rails Engine built by the Samvera community. Hyrax provides a foundation for creating many different digital repository applications.
http://hyrax.samvera.org/
Apache License 2.0
184 stars 124 forks source link

:bug:Account for Valkyrie ID in location indexer #6790

Open ShanaLMoore opened 5 months ago

ShanaLMoore commented 5 months ago

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

github-actions[bot] commented 5 months ago

Test Results

    9 files  ±0      9 suites  ±0   16m 55s :stopwatch: -22s 4 762 tests ±0  4 699 :white_check_mark: +1  63 :zzz: ±0  0 :x:  - 1  6 488 runs  ±0  6 425 :white_check_mark: +1  63 :zzz: ±0  0 :x:  - 1 

Results for commit c3386ff3. ± Comparison against base commit 42eccf44.

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 # spec.abilities.ability_spec ‑ Hyrax::Ability AdminSets and PermissionTemplates a user without edit access is expected not to be able to create # 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 # 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 # 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 # spec.abilities.ability_spec ‑ Hyrax::Ability AdminSets and PermissionTemplates a user without edit access is expected not to be able to create # 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 # 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 # 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 commented 5 months ago

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