more forgiving call for label from QaSelectService

[pgwillia]

more forgiving call for label from QaSelectService returns nil instead of throwing exception

What I did: Migrated (customized) metadata from Hydranorth application based on Sufia 6.2 to Sufia 7.2 application. All catalog calls failed in the new application.

Rendered /usr/lib64/ruby/gems/2.3.0/gems/sufia-7.2.0/app/views/catalog/_docume
nt.html.erb (53.2ms)
  Rendered curation_concerns/generic_works/_generic_work.html.erb (618.9ms)
  Rendered /usr/lib64/ruby/gems/2.3.0/gems/blacklight-6.7.1/app/views/catalog/_s
earch_results.html.erb (746.1ms)
  Rendered /usr/lib64/ruby/gems/2.3.0/gems/sufia-7.2.0/app/views/catalog/index.h
tml.erb within layouts/curation_concerns/1_column (897.9ms)
Completed 500 Internal Server Error in 1395ms (ActiveRecord: 29.6ms)

KeyError - key not found: "term":
  curation_concerns (1.6.2) app/services/curation_concerns/qa_select_service.rb:
27:in `label'
  sufia (7.2.0) app/helpers/sufia/sufia_helper_behavior.rb:175:in `block in lice
nse_links'
  sufia (7.2.0) app/helpers/sufia/sufia_helper_behavior.rb:175:in `license_links
'

Discovered that fetch will throw the KeyError exception while the [] syntax will return nil and carry on when the key value is not present.

@projecthydra/sufia-code-reviewers

[jcoyne]

I think we intended this to be an error. There's a problem in the data that drives the interface. Returning nil is not going to be an acceptable resolution.

[pgwillia]

I recognize that the data we've put in the rights field doesn't jive with the LicenseService. Is our better workaround to override the sufia_helper? Would a contribution to that codebase to make more sense?

#`bundle show sufia`/app/helpers/sufia/sufia_helper_behavior.rb
   # A Blacklight helper_method
    # @param [Hash] options from blacklight helper_method invocation. Maps rights URIs to links with labels.
    # @return [ActiveSupport::SafeBuffer] rights statement links, html_safe
    def license_links(options)
      service = CurationConcerns::LicenseService.new
      options[:value].map { |right| link_to service.label(right), right }.to_sentence.html_safe
    end

[jcoyne]

@pgwillia can you show the data you have in the yaml file you are loading? I would expect it to have term keys like https://github.com/projecthydra/curation_concerns/blob/master/spec/fixtures/authorities/licenses.yml#L3

[pgwillia]

@jcoyne here's the yaml file for licenses: https://github.com/ualbertalib/Hydranorth2/blob/master/config/authorities/licenses.yml

Problem is that we allowed free text in our licence field. Then we can have rights like:

We need this so that we can maintain the rights agreements that our users selected when they first deposited to our IR (pre Hydra).

[jeremyf]

@pgwillia There are some ways to break this apart:

1. In your Rails application that includes the CurationConcern gem, you may be able to re-open the CurationConcerns::LicenseService class to make your changes
2. We can look at registering different license services as a config option and allow you to specify a different one (e.g. one that does not re-open LicenseService class), then change the CurationConcerns application to use CurationConcerns.config.default_license_service_class instead of CurationConcerns::LicenseService

CurationConcerns.config.default_license_service_class = MyCustomLicenseService

[pgwillia]

I'm going to close this PR based on @jcoyne's response that returning nil is not acceptable.

@jeremyf I do have a workaround in place for our stakeholders to try-out Sufia 7 which involved opening gems and modifying in place. In Sufia::SufiaHelperBehavior I am currently catching the KeyError and returning the rights value without using the LincenseService. Editing gems in place cannot be a permanent solution a) because it would make our automated deployment to production environments complicated and b) hard to maintain.

The second idea does interest me. Being able to configure LicenseService would mean I wouldn't have to override Sufia::SufiaHelperBehavior#license_links. I'd still have to do some work to implement MyCustomLicenseService but that might make more sense.

Thanks for your input. I appreciate it!

[jeremyf]

@pgwillia I submitted a PR to codify the expectations via a spec.

[jeremyf]

@pgwillia The pattern for configuration is similar to what is found in CurationConcerns::Configuration. If you are interested, I can work on a patch later this week. I believe your particular use case may be something that others may be interested in.

[pgwillia]

@jeremyf that would be great! I took a quick look at CurationConcerns::Configuration and don't completely grok it. I'd learn a lot by helping you work on the patch. Let me know what I can do.