Merge pull request #13 from jeremyf/updating-to-appease-the-hound

Working to appease the hound

I'm exploring Hound interaction, I'm using the pull request model.

.hound.yml

@@ -0,0 +1,11 @@
+MethodLength:
+  Max: 10
+  CountComments: false
+
+LineLength:
+  Max: 80
+  CountComments: false
+
+ClassLength:
+  Max: 100
+  CountComments: false

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

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

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

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

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

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

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