diff --git a/.hound.yml b/.hound.yml new file mode 100644 index 0000000..a648009 --- /dev/null +++ b/.hound.yml @@ -0,0 +1,11 @@ +MethodLength: + Max: 10 + CountComments: false + +LineLength: + Max: 80 + CountComments: false + +ClassLength: + Max: 100 + CountComments: false \ No newline at end of file diff --git a/app/controllers/orcid/application_controller.rb b/app/controllers/orcid/application_controller.rb index cc9b0d6..e38649a 100644 --- a/app/controllers/orcid/application_controller.rb +++ b/app/controllers/orcid/application_controller.rb @@ -1,14 +1,23 @@ module Orcid + # The foundation for Orcid controllers. A few helpful accessors. class ApplicationController < Orcid.parent_controller.constantize private - def redirecting_because_user_already_has_a_connected_orcid_profile - if orcid_profile = Orcid.profile_for(current_user) - flash[:notice] = I18n.t("orcid.requests.messages.previously_connected_profile", orcid_profile_id: orcid_profile.orcid_profile_id) + + def redirecting_because_user_has_connected_orcid_profile + if orcid_profile + flash[:notice] = I18n.t( + 'orcid.requests.messages.previously_connected_profile', + orcid_profile_id: orcid_profile.orcid_profile_id + ) redirect_to main_app.root_path return true else return false end end + + def orcid_profile + @orcid_profile ||= Orcid.profile_for(current_user) + end end end diff --git a/app/controllers/orcid/profile_connections_controller.rb b/app/controllers/orcid/profile_connections_controller.rb index 93d172a..6535112 100644 --- a/app/controllers/orcid/profile_connections_controller.rb +++ b/app/controllers/orcid/profile_connections_controller.rb @@ -1,4 +1,5 @@ module Orcid + # Responsible for negotiating a user request Profile Creation. class ProfileConnectionsController < Orcid::ApplicationController respond_to :html before_filter :authenticate_user! @@ -10,16 +11,16 @@ def index end def new - return false if redirecting_because_user_already_has_a_connected_orcid_profile + return false if redirecting_because_user_has_connected_orcid_profile assign_attributes(new_profile_connection) - respond_with(orcid,new_profile_connection) + respond_with(orcid, new_profile_connection) end def create - return false if redirecting_because_user_already_has_a_connected_orcid_profile + return false if redirecting_because_user_has_connected_orcid_profile assign_attributes(new_profile_connection) create_profile_connection(new_profile_connection) - respond_with(orcid,new_profile_connection) + respond_with(orcid, new_profile_connection) end protected @@ -41,32 +42,33 @@ def create_profile_connection(profile_connection) end def new_profile_connection - @profile_connection ||= Orcid::ProfileConnection.new(params[:profile_connection]) + @profile_connection ||= begin + Orcid::ProfileConnection.new(params[:profile_connection]) + end end def redirecting_because_user_does_not_have_a_connected_orcid_profile - return false if Orcid.profile_for(current_user) - flash[:notice] = I18n.t("orcid.connections.messages.profile_connection_not_found") + return false if orcid_profile + flash[:notice] = I18n.t( + 'orcid.connections.messages.profile_connection_not_found' + ) redirect_to orcid.new_profile_connection_path - return true end - protected :redirecting_because_user_does_not_have_a_connected_orcid_profile def redirecting_because_user_must_verify_their_connected_profile - return false unless profile = Orcid.profile_for(current_user) - return false if profile.verified_authentication? + return false unless orcid_profile + return false if orcid_profile.verified_authentication? - redirect_to user_omniauth_authorize_url("orcid") - return true + redirect_to user_omniauth_authorize_url('orcid') end - protected :redirecting_because_user_must_verify_their_connected_profile def redirecting_because_user_has_verified_their_connected_profile orcid_profile = Orcid.profile_for(current_user) - redirect_to '/', flash: {notice: I18n.t("orcid.connections.messages.verified_profile_connection_exists", orcid_profile_id: orcid_profile.orcid_profile_id)} - return true + notice = I18n.t( + 'orcid.connections.messages.verified_profile_connection_exists', + orcid_profile_id: orcid_profile.orcid_profile_id + ) + redirect_to('/', flash: { notice: notice }) end - protected :redirecting_because_user_has_verified_their_connected_profile - end end diff --git a/app/controllers/orcid/profile_requests_controller.rb b/app/controllers/orcid/profile_requests_controller.rb index c8c33db..7b12230 100644 --- a/app/controllers/orcid/profile_requests_controller.rb +++ b/app/controllers/orcid/profile_requests_controller.rb @@ -1,4 +1,5 @@ module Orcid + # Responsible for helping a user request a new Orcid Profile. class ProfileRequestsController < Orcid::ApplicationController respond_to :html before_filter :authenticate_user! @@ -7,20 +8,20 @@ class ProfileRequestsController < Orcid::ApplicationController helper_method :profile_request def show - return false if redirecting_because_user_already_has_a_connected_orcid_profile + return false if redirecting_because_user_has_connected_orcid_profile return false if redirecting_because_no_profile_request_was_found respond_with(orcid, existing_profile_request) end def new - return false if redirecting_because_user_already_has_a_connected_orcid_profile + return false if redirecting_because_user_has_connected_orcid_profile return false if redirecting_because_user_has_existing_profile_request assign_attributes(new_profile_request) respond_with(orcid, new_profile_request) end def create - return false if redirecting_because_user_already_has_a_connected_orcid_profile + return false if redirecting_because_user_has_connected_orcid_profile return false if redirecting_because_user_has_existing_profile_request assign_attributes(new_profile_request) create_profile_request(new_profile_request) @@ -31,16 +32,16 @@ def create def redirecting_because_no_profile_request_was_found return false if existing_profile_request - flash[:notice] = I18n.t("orcid.requests.messages.existing_request_not_found") + flash[:notice] = I18n.t( + 'orcid.requests.messages.existing_request_not_found' + ) redirect_to orcid.new_profile_request_path - true end def redirecting_because_user_has_existing_profile_request - return false if ! existing_profile_request - flash[:notice] = I18n.t("orcid.requests.messages.existing_request") + return false unless existing_profile_request + flash[:notice] = I18n.t('orcid.requests.messages.existing_request') redirect_to orcid.profile_request_path - true end def existing_profile_request @@ -60,8 +61,10 @@ def create_profile_request(profile_request) end def profile_request_params - return {} unless params.has_key?(:profile_request) - params[:profile_request].permit(:given_names, :family_name, :primary_email, :primary_email_confirmation) + return {} unless params.key?(:profile_request) + params[:profile_request].permit( + :given_names, :family_name, :primary_email, :primary_email_confirmation + ) end end end diff --git a/app/models/orcid/profile.rb b/app/models/orcid/profile.rb index 4145020..9c815bf 100644 --- a/app/models/orcid/profile.rb +++ b/app/models/orcid/profile.rb @@ -1,17 +1,23 @@ module Orcid + # Provides a container around an Orcid Profile and its relation to the Orcid + # Works. class Profile - - attr_reader :orcid_profile_id, :mapper, :remote_service, :xml_renderer, :xml_parser + attr_reader( + :orcid_profile_id, :mapper, :remote_service, :xml_renderer, :xml_parser + ) private :mapper def initialize(orcid_profile_id, config = {}) @orcid_profile_id = orcid_profile_id @mapper = config.fetch(:mapper) { Orcid.mapper } - @remote_service = config.fetch(:remote_service) { Orcid::Remote::WorkService } + @remote_service = config.fetch(:remote_service) do + Orcid::Remote::WorkService + end @xml_renderer = config.fetch(:xml_renderer) { Orcid::Work::XmlRenderer } @xml_parser = config.fetch(:xml_parser) { Orcid::Work::XmlParser } end - # Answers the question: Has the user been authenticated via the ORCID system. + # Answers the question: Has the user been authenticated via the ORCID + # system. def verified_authentication? Orcid.authenticated_orcid?(orcid_profile_id) end @@ -40,10 +46,9 @@ def replace_works_with(*works) # Note: We can handle def normalize_work(*works) - Array.wrap(works).collect do |work| + Array.wrap(works).map do |work| mapper.map(work, target: 'orcid/work') end end - end end diff --git a/app/models/orcid/profile_connection.rb b/app/models/orcid/profile_connection.rb index 4db6196..f14c82b 100644 --- a/app/models/orcid/profile_connection.rb +++ b/app/models/orcid/profile_connection.rb @@ -6,7 +6,7 @@ class ProfileConnection include ActiveModel::Validations extend ActiveModel::Naming - self.class_attribute :available_query_attribute_names + class_attribute :available_query_attribute_names self.available_query_attribute_names = [:email, :text] available_query_attribute_names.each do |attribute_name| @@ -19,13 +19,16 @@ class ProfileConnection validates :user, presence: true validates :orcid_profile_id, presence: true - - def save(config = {}) - persister = config.fetch(:persister) { Orcid.method(:connect_user_and_orcid_profile) } + def save(collaborators = {}) + persister = collaborators.fetch(:persister) do + Orcid.method(:connect_user_and_orcid_profile) + end valid? ? persister.call(user, orcid_profile_id) : false end - def persisted?; false; end + def persisted? + false + end attr_writer :profile_query_service def profile_query_service @@ -34,10 +37,10 @@ def profile_query_service private :profile_query_service def default_profile_query_service - Remote::ProfileQueryService.new {|on| - on.found {|results| self.orcid_profile_candidates = results } + Remote::ProfileQueryService.new do |on| + on.found { |results| self.orcid_profile_candidates = results } on.not_found { self.orcid_profile_candidates = [] } - } + end end private :default_profile_query_service @@ -52,22 +55,19 @@ def orcid_profile_candidates end def lookup_profile_candidates - if query_requested? - profile_query_service.call(query_attributes) - end + profile_query_service.call(query_attributes) if query_requested? end private :lookup_profile_candidates def query_requested? - !!available_query_attribute_names.detect { |attribute_name| + available_query_attribute_names.any? do |attribute_name| attributes[attribute_name].present? - } + end end private :query_requested? def query_attributes attributes.slice(*available_query_attribute_names) end - end end diff --git a/app/models/orcid/profile_request.rb b/app/models/orcid/profile_request.rb index b0743e8..f272563 100644 --- a/app/models/orcid/profile_request.rb +++ b/app/models/orcid/profile_request.rb @@ -20,19 +20,28 @@ def self.find_by_user(user) belongs_to :user def run(options = {}) - # Why dependency injection? Because this is going to be a plugin, and things - # can't possibly be simple. + # Why dependency injection? Because this is going to be a plugin, and + # things can't possibly be simple. I also found it easier to test the + # #run method with these injected dependencies validator = options.fetch(:validator) { method(:validate_before_run) } return false unless validator.call(self) - payload_xml_builder = options.fetch(:payload_xml_builder) { method(:xml_payload) } - profile_creation_service = options.fetch(:profile_creation_service) { default_profile_creation_service } + payload_xml_builder = options.fetch(:payload_xml_builder) do + method(:xml_payload) + end + profile_creation_service = options.fetch(:profile_creation_service) do + default_profile_creation_service + end profile_creation_service.call(payload_xml_builder.call(attributes)) end def default_profile_creation_service - @default_profile_creation_service ||= Orcid::Remote::ProfileCreationService.new do |on| - on.success {|orcid_profile_id| handle_profile_creation_response(orcid_profile_id) } + @default_profile_creation_service ||= begin + Orcid::Remote::ProfileCreationService.new do |on| + on.success do |orcid_profile_id| + handle_profile_creation_response(orcid_profile_id) + end + end end end @@ -43,7 +52,8 @@ def validate_before_run(context = self) return false end - if user_orcid_profile = Orcid.profile_for(context.user) + user_orcid_profile = Orcid.profile_for(context.user) + if user_orcid_profile context.errors.add(:base, "#{context.class} ID=#{context.to_param}'s associated user #{context.user.to_param} already has an assigned :orcid_profile_id #{user_orcid_profile.to_param}") return false end @@ -83,6 +93,5 @@ def handle_profile_creation_response(orcid_profile_id) Orcid.connect_user_and_orcid_profile(user, orcid_profile_id) end end - end end diff --git a/app/models/orcid/work.rb b/app/models/orcid/work.rb index e0db9af..f205709 100644 --- a/app/models/orcid/work.rb +++ b/app/models/orcid/work.rb @@ -2,10 +2,21 @@ module Orcid # A well-defined data structure that coordinates with its :template in order # to generate XML that can be POSTed/PUT as an Orcid Work. class Work - VALID_WORK_TYPES = [ - "artistic-performance","book-chapter","book-review","book","conference-abstract","conference-paper","conference-poster","data-set","dictionary-entry","disclosure","dissertation","edited-book","encyclopedia-entry","invention","journal-article","journal-issue","lecture-speech","license","magazine-article","manual","newsletter-article","newspaper-article","online-resource","other","patent","registered-copyright","report","research-technique","research-tool","spin-off-company","standards-and-policy","supervised-student-publication","technical-standard","test","translation","trademark","website","working-paper", - ].freeze + VALID_WORK_TYPES = + %w(artistic-performance book-chapter book-review book + conference-abstract conference-paper conference-poster + data-set dictionary-entry disclosure dissertation + edited-book encyclopedia-entry invention journal-article + journal-issue lecture-speech license magazine-article + manual newsletter-article newspaper-article online-resource + other patent registered-copyright report research-technique + research-tool spin-off-company standards-and-policy + supervised-student-publication technical-standard test + translation trademark website working-paper + ).freeze + # An Orcid Work's external identifier is not represented in a single + # attribute. class ExternalIdentifier include Virtus.value_object values do @@ -49,11 +60,11 @@ def to_xml XmlRenderer.call(self) end - def ==(comparison_object) + def ==(other) super || - comparison_object.instance_of?(self.class) && + other.instance_of?(self.class) && id.present? && - comparison_object.id == id + other.id == id end def id diff --git a/app/models/orcid/work/xml_parser.rb b/app/models/orcid/work/xml_parser.rb index f9cd132..9ffcf8b 100644 --- a/app/models/orcid/work/xml_parser.rb +++ b/app/models/orcid/work/xml_parser.rb @@ -1,5 +1,7 @@ module Orcid class Work + # Responsible for taking an Orcid Work and extracting the value/text from + # the document and reifying an Orcid::Work object. class XmlParser def self.call(xml) new(xml).call @@ -11,19 +13,20 @@ def initialize(xml) end def call - document.css('orcid-works orcid-work').collect do |node| + document.css('orcid-works orcid-work').map do |node| transform(node) end end private + def document @document ||= Nokogiri::XML.parse(xml) end def transform(node) Work.new.tap do |work| - work.put_code = node.attributes.fetch("put-code").value + work.put_code = node.attributes.fetch('put-code').value work.title = node.css('work-title title').text work.work_type = node.css('work-type').text work.journal_title = node.css('journal-title').text diff --git a/app/models/orcid/work/xml_renderer.rb b/app/models/orcid/work/xml_renderer.rb index ec21c80..0dd5032 100644 --- a/app/models/orcid/work/xml_renderer.rb +++ b/app/models/orcid/work/xml_renderer.rb @@ -1,5 +1,6 @@ module Orcid class Work + # Responsible for transforming a Work into an Orcid Work XML document class XmlRenderer def self.call(works, options = {}) new(works, options).call @@ -8,7 +9,10 @@ def self.call(works, options = {}) attr_reader :works, :template def initialize(works, options = {}) self.works = works - @template = options.fetch(:template_path) { Orcid::Engine.root.join('app/templates/orcid/work.template.v1.1.xml.erb').read } + @template = options.fetch(:template_path) do + template_name = 'app/templates/orcid/work.template.v1.1.xml.erb' + Orcid::Engine.root.join(template_name).read + end end def call @@ -16,10 +20,10 @@ def call end protected + def works=(thing) @works = Array.wrap(thing) end - end end end diff --git a/app/services/orcid/remote/profile_creation_service.rb b/app/services/orcid/remote/profile_creation_service.rb index 6b37ec9..24904b6 100644 --- a/app/services/orcid/remote/profile_creation_service.rb +++ b/app/services/orcid/remote/profile_creation_service.rb @@ -1,43 +1,51 @@ require 'orcid/remote/service' -module Orcid::Remote - # Responsible for minting a new ORCID for the given payload. - class ProfileCreationService < Orcid::Remote::Service +module Orcid + module Remote + # Responsible for minting a new ORCID for the given payload. + class ProfileCreationService < Orcid::Remote::Service + def self.call(payload, config = {}, &callback_config) + new(config, &callback_config).call(payload) + end - def self.call(payload, config = {}, &callback_config) - new(config, &callback_config).call(payload) - end + attr_reader :token, :path, :headers + def initialize(config = {}, &callback_config) + super(&callback_config) + @token = config.fetch(:token) do + Orcid.client_credentials_token('/orcid-profile/create') + end + @path = config.fetch(:path) { 'v1.1/orcid-profile' } + @headers = config.fetch(:headers) { default_headers } + end - attr_reader :token, :path, :headers - def initialize(config = {}, &callback_config) - super(&callback_config) - @token = config.fetch(:token) { Orcid.client_credentials_token('/orcid-profile/create') } - @path = config.fetch(:path) { "v1.1/orcid-profile" } - @headers = config.fetch(:headers) { default_headers } - end + def call(payload) + response = deliver(payload) + parse(response) + end - def call(payload) - response = deliver(payload) - parse(response) - end + protected - protected - def deliver(body) - token.post(path, body: body, headers: headers) - end + def deliver(body) + token.post(path, body: body, headers: headers) + end - def parse(response) - uri = URI.parse(response.headers.fetch(:location)) - if orcid_profile_id = uri.path.sub(/\A\//, "").split("/").first - callback(:success, orcid_profile_id) - orcid_profile_id - else - callback(:failure) - false + def parse(response) + uri = URI.parse(response.headers.fetch(:location)) + orcid_profile_id = uri.path.sub(/\A\//, '').split('/').first + if orcid_profile_id + callback(:success, orcid_profile_id) + orcid_profile_id + else + callback(:failure) + false + end end - end - def default_headers - { "Accept" => 'application/xml', 'Content-Type'=>'application/vdn.orcid+xml' } + def default_headers + { + 'Accept' => 'application/xml', + 'Content-Type' => 'application/vdn.orcid+xml' + } + end end end end diff --git a/app/services/orcid/remote/profile_query_service/search_response.rb b/app/services/orcid/remote/profile_query_service/search_response.rb index dfc5cb2..a22d9e0 100644 --- a/app/services/orcid/remote/profile_query_service/search_response.rb +++ b/app/services/orcid/remote/profile_query_service/search_response.rb @@ -1,26 +1,30 @@ require_dependency 'orcid/remote/profile_query_service' -module Orcid::Remote - class ProfileQueryService - class SearchResponse - delegate :[], :has_key?, :fetch, to: :@records - def initialize(attributes = {}) - @attributes = attributes.with_indifferent_access - end +module Orcid + module Remote + class ProfileQueryService + # A search response against Orcid. This should implement the Questioning + # Authority query response object interface. + class SearchResponse + delegate :[], :has_key?, :key?, :fetch, to: :@records + def initialize(attributes = {}) + @attributes = attributes.with_indifferent_access + end - def id - @attributes.fetch(:id) - end + def id + @attributes.fetch(:id) + end - def orcid_profile_id - @attributes.fetch(:id) - end + def orcid_profile_id + @attributes.fetch(:id) + end - def label - @attributes.fetch(:label) - end + def label + @attributes.fetch(:label) + end - def biography - @attributes.fetch(:biography) + def biography + @attributes.fetch(:biography) + end end end end diff --git a/spec/views/orcid/profile_connections/new.html.erb_spec.rb b/spec/views/orcid/profile_connections/new.html.erb_spec.rb index ed83c6e..f53d093 100644 --- a/spec/views/orcid/profile_connections/new.html.erb_spec.rb +++ b/spec/views/orcid/profile_connections/new.html.erb_spec.rb @@ -5,13 +5,21 @@ it 'renders a form' do view.stub(:profile_connection).and_return(profile_connection) render - expect(rendered).to have_tag('form.search-form', with: {action: orcid.new_profile_connection_path, method: :get}) do + expect(rendered).to( + have_tag( + 'form.search-form', + with: { action: orcid.new_profile_connection_path, method: :get } + ) + ) do with_tag('fieldset') do profile_connection.available_query_attribute_names.each do |field_name| - with_tag('input', with: {name: "profile_connection[#{field_name}]", type: 'search'}) + with_tag( + 'input', + with: { name: "profile_connection[#{field_name}]", type: 'search' } + ) end end - with_tag('button', with: {type: 'submit'}) + with_tag('button', with: { type: 'submit' }) end end end