Merge branch 'master' into collections-sprint

# Conflicts: # app/actors/hyrax/actors/collections_membership_actor.rb # app/controllers/concerns/hyrax/collections_controller_behavior.rb # app/controllers/hyrax/dashboard/collections_controller.rb # app/helpers/hyrax/collections_helper.rb # app/views/hyrax/dashboard/collections/_button_remove_from_collection.html.erb # app/views/hyrax/dashboard/collections/_work_action_menu.html.erb # hyrax.gemspec # lib/tasks/default_admin_set.rake # spec/actors/hyrax/actors/collections_membership_actor_spec.rb # spec/factories/permission_templates.rb # spec/features/admin_admin_set_spec.rb # spec/features/browse_catalog_spec.rb
samvera · Dec 11, 2017 · 7158076 · 7158076
2 parents 3fe6c5f + 2b55172
commit 7158076
Show file tree

Hide file tree

Showing 195 changed files with 1,899 additions and 1,218 deletions.
diff --git a/.eslintrc b/.eslintrc
@@ -70,7 +70,6 @@ rules:
   no-implied-eval: 2
   no-invalid-this: 0
   no-iterator: 2
-  no-labels: 0
   no-lone-blocks: 2
   no-loop-func: 2
   no-magic-number: 0

diff --git a/.rubocop.yml b/.rubocop.yml
@@ -49,6 +49,10 @@ Metrics/BlockLength:
 Metrics/MethodLength:
   Max: 13
 
+Naming/FileName: # https://github.com/bbatsov/rubocop/issues/2973
+  Exclude:
+    - 'Gemfile'
+
 Style/AsciiComments:
   Enabled: false
 
@@ -60,10 +64,6 @@ Style/CollectionMethods:
     detect: 'find'
     find_all: 'select'
 
-Style/FileName: # https://github.com/bbatsov/rubocop/issues/2973
-  Exclude:
-    - 'Gemfile'
-
 Style/MethodMissing:
   Exclude:
     - 'app/models/concerns/hyrax/file_set/characterization.rb'
@@ -140,6 +140,9 @@ RSpec/DescribeClass:
     - 'spec/tasks/rake_spec.rb'
     - 'spec/views/**/*'
 
+RSpec/ContextWording:
+  Enabled: false
+
 # By default RSpec/MessageSpies has the following:
 #   Prefer have_received for setting message expectations. Setup form as a spy using allow or instance_spy.
 # The default assumes EnforcedStyle is 'have_received'. Most of our specs are 'receive'

diff --git a/.rubocop_fixme.yml b/.rubocop_fixme.yml
@@ -45,14 +45,24 @@ Metrics/MethodLength:
     - 'lib/hyrax/rails/routes.rb'
     - 'spec/support/**/*'
 
+
+Naming/PredicateName:
+  Exclude:
+    - 'app/helpers/hyrax/collections_helper.rb'
+
+
 # TODO: remove when https://github.com/bbatsov/rubocop/issues/4539 is fixed
 Style/FormatStringToken:
   Exclude:
     - 'config/routes.rb'
 
-Style/PredicateName:
+Style/DateTime:
+  Enabled: false
+
+# TODO: remove when https://github.com/bbatsov/rubocop/issues/5161 is fixed
+Style/CommentedKeyword:
   Exclude:
-    - 'app/helpers/hyrax/collections_helper.rb'
+    - 'app/models/vocab/fedora_resource_status.rb'
 
 Style/SpecialGlobalVars:
   Exclude:

diff --git a/README.md b/README.md
@@ -249,12 +249,20 @@ You may wish to [customize your work type](https://github.com/samvera/hyrax/wiki
 
 Hyrax 2 uses a WebSocket-based user notifications system, which requires Redis. To enable user notifications, make sure that you have configured ActionCable to use Redis as the adapter in your application's `config/cable.yml`. E.g., for the `development` Rails environment:
 
-``` yaml
+```yaml
 development:
   adapter: redis
   url: redis://localhost:6379
 ```
 
+Using Rails up to version 5.1.4, ActionCable will not work with the 4.x series of the `redis` gem, so you will also need to pin your application to a 3.x release by adding this to your `Gemfile`:
+
+```ruby
+gem 'redis', '~> 3.0'
+```
+
+And then run `bundle update redis`.
+
 Note that the Hyrax Management Guide contains additional information on [how to configure ActionCable in production environments](https://github.com/samvera/hyrax/wiki/Hyrax-Management-Guide#notifications).
 
 # Managing a Hyrax-based app

diff --git a/app/actors/hyrax/actors/abstract_actor.rb b/app/actors/hyrax/actors/abstract_actor.rb
@@ -1,7 +1,7 @@
 module Hyrax
   module Actors
     ##
-    # `Hyrax::AbstractActor` implements the base (no-op) case for Hyrax Actor
+    # `Hyrax::Actors::AbstractActor` implements the base (no-op) case for Hyrax Actor
     # middleware. Concrete implementations may override any or all of the three
     # primary actions:
     #

diff --git a/app/actors/hyrax/actors/attach_members_actor.rb b/app/actors/hyrax/actors/attach_members_actor.rb
@@ -56,11 +56,11 @@ def remove(curation_concern, id)
         end
 
         # Determines if a hash contains a truthy _destroy key.
-        # rubocop:disable Style/PredicateName
+        # rubocop:disable Naming/PredicateName
         def has_destroy_flag?(hash)
           ActiveFedora::Type::Boolean.new.cast(hash['_destroy'])
         end
-      # rubocop:enable Style/PredicateName
+      # rubocop:enable Naming/PredicateName
     end
   end
 end
diff --git a/app/actors/hyrax/actors/collections_membership_actor.rb b/app/actors/hyrax/actors/collections_membership_actor.rb
@@ -1,53 +1,97 @@
 module Hyrax
   module Actors
-    # Adds membership to and removes membership from collections
+    # Adds membership to and removes membership from collections.
+    # This decodes parameters that follow the rails nested parameters conventions:
+    # e.g.
+    #   'member_of_collections_attributes' => {
+    #     '0' => { 'id' = '12312412'},
+    #     '1' => { 'id' = '99981228', '_destroy' => 'true' }
+    #   }
+    #
     class CollectionsMembershipActor < AbstractActor
       # @param [Hyrax::Actors::Environment] env
       # @return [Boolean] true if create was successful
       def create(env)
-        collection_ids = env.attributes.delete(:member_of_collection_ids)
-        extract_collection_id(env, collection_ids)
-        assign_collections(env, collection_ids) && next_actor.create(env)
+        attributes_collection = env.attributes.delete(:member_of_collections_attributes)
+        extract_collection_id(env, attributes_collection)
+        assign_nested_attributes_for_collection(env, attributes_collection) &&
+          next_actor.create(env)
       end
 
       # @param [Hyrax::Actors::Environment] env
       # @return [Boolean] true if update was successful
       def update(env)
-        collection_ids = env.attributes.delete(:member_of_collection_ids)
-        assign_collections(env, collection_ids) && next_actor.update(env)
+        attributes_collection = env.attributes.delete(:member_of_collections_attributes)
+        assign_nested_attributes_for_collection(env, attributes_collection) &&
+          next_actor.update(env)
       end
 
       private
 
-        # Maps from collection ids to collection objects
-        def assign_collections(env, collection_ids)
-          return true unless collection_ids
-          multiple_memberships = Hyrax::MultipleMembershipChecker.new(item: env.curation_concern).check(collection_ids: collection_ids)
-          if multiple_memberships
-            env.curation_concern.errors.add(:collections, multiple_memberships)
-            return false
+        # Attaches any unattached members.  Deletes those that are marked _delete
+        # @param [Hash<Hash>] a collection of members
+        def assign_nested_attributes_for_collection(env, attributes_collection)
+          return true unless attributes_collection
+          return false unless valid_membership? attributes_collection
+
+          attributes_collection = attributes_collection.sort_by { |i, _| i.to_i }.map { |_, attributes| attributes }
+          # checking for existing works to avoid rewriting/loading works that are
+          # already attached
+          existing_collections = env.curation_concern.member_of_collection_ids
+          attributes_collection.each do |attributes|
+            next if attributes['id'].blank?
+            if existing_collections.include?(attributes['id'])
+              remove(env.curation_concern, attributes['id']) if has_destroy_flag?(attributes)
+            else
+              add(env, attributes['id'])
+            end
           end
-          # grab/save collections this user has no edit access to
-          other_collections = collections_without_edit_access(env)
-          env.curation_concern.member_of_collections = ::Collection.find(collection_ids)
-          env.curation_concern.member_of_collections.concat other_collections
         end
 
-        def collections_without_edit_access(env)
-          env.curation_concern.member_of_collections.select { |coll| env.current_ability.cannot?(:edit, coll) }
+        # Adds the item to the ordered members so that it displays in the items
+        # along side the FileSets on the show page
+        def add(env, id)
+          member = Collection.find(id)
+          return unless env.current_ability.can?(:edit, member)
+          env.curation_concern.member_of_collections << member
+        end
+
+        # Remove the object from the members set and the ordered members list
+        def remove(curation_concern, id)
+          member = Collection.find(id)
+          curation_concern.member_of_collections.delete(member)
+        end
+
+        # Determines if a hash contains a truthy _destroy key.
+        # rubocop:disable Naming/PredicateName
+        def has_destroy_flag?(hash)
+          ActiveFedora::Type::Boolean.new.cast(hash['_destroy'])
         end
+        # rubocop:enable Naming/PredicateName
 
-        # Given an array of collection_ids when it is size:
+        # Given an array of collection_attributes when it is size:
         # * 0 do not set `env.attributes[:collection_id]`
         # * 1 set `env.attributes[:collection_id]` to the one and only one collection
         # * 2 do not set `env.attributes[:collection_id]`
         #
         # Later on in apply_permission_template_actor.rb, `env.attributes[:collection_id]` will be used to apply the
         # permissions of the collection to the created work.  With one and only one collection, the work is seen as
         # being created directly in that collection.
-        def extract_collection_id(env, collection_ids)
-          return unless collection_ids && collection_ids.size == 1
-          env.attributes[:collection_id] = collection_ids.first
+        #
+        # NOTE: Only called from create.  All collections are being added as parents.  None are being removed.
+        def extract_collection_id(env, collection_attributes)
+          return unless collection_attributes && collection_attributes.size == 1
+          env.attributes[:collection_id] = collection_attributes.first['id']
+        end
+
+        def valid_membership? attributes_collection
+          collection_ids = attributes_collection.map { |_, attributes| attributes['id'] }
+          multiple_memberships = Hyrax::MultipleMembershipChecker.new(item: env.curation_concern).check(collection_ids: collection_ids)
+          if multiple_memberships
+            env.curation_concern.errors.add(:collections, multiple_memberships)
+            return false
+          end
+          true
         end
     end
   end

diff --git a/app/actors/hyrax/actors/create_with_remote_files_actor.rb b/app/actors/hyrax/actors/create_with_remote_files_actor.rb
@@ -5,6 +5,9 @@ module Actors
     #     attributes[:remote_files] = filenames.map do |name|
     #       { url: "https://example.com/file/#{name}", file_name: name }
     #     end
+    #
+    # Browse everything may also return a local file. And although it's in the
+    # url property, it may have spaces, and not be a valid URI.
     class CreateWithRemoteFilesActor < Hyrax::Actors::AbstractActor
       # @param [Hyrax::Actors::Environment] env
       # @return [Boolean] true if create was successful
@@ -26,10 +29,10 @@ def whitelisted_ingest_dirs
           Hyrax.config.whitelisted_ingest_dirs
         end
 
-        def validate_remote_url(url)
-          uri = URI.parse(URI.encode(url))
+        # @param uri [URI] the uri fo the resource to import
+        def validate_remote_url(uri)
           if uri.scheme == 'file'
-            path = File.absolute_path(URI.decode(uri.path))
+            path = File.absolute_path(CGI.unescape(uri.path))
             whitelisted_ingest_dirs.any? do |dir|
               path.start_with?(dir) && path.length > dir.length
             end
@@ -46,26 +49,29 @@ def attach_files(env, remote_files)
           return true unless remote_files
           remote_files.each do |file_info|
             next if file_info.blank? || file_info[:url].blank?
-            unless validate_remote_url(file_info[:url])
+            # Escape any space characters, so that this is a legal URI
+            uri = URI.parse(Addressable::URI.escape(file_info[:url]))
+            unless validate_remote_url(uri)
               Rails.logger.error "User #{env.user.user_key} attempted to ingest file from url #{file_info[:url]}, which doesn't pass validation"
               return false
             end
-            create_file_from_url(env, file_info[:url], file_info[:file_name])
+            create_file_from_url(env, uri, file_info[:file_name])
           end
           true
         end
 
         # Generic utility for creating FileSet from a URL
         # Used in to import files using URLs from a file picker like browse_everything
-        def create_file_from_url(env, url, file_name)
-          ::FileSet.new(import_url: url, label: file_name) do |fs|
+        def create_file_from_url(env, uri, file_name)
+          ::FileSet.new(import_url: uri.to_s, label: file_name) do |fs|
             actor = Hyrax::Actors::FileSetActor.new(fs, env.user)
             actor.create_metadata(visibility: env.curation_concern.visibility)
             actor.attach_to_work(env.curation_concern)
             fs.save!
-            uri = URI.parse(URI.encode(url))
             if uri.scheme == 'file'
-              IngestLocalFileJob.perform_later(fs, URI.decode(uri.path), env.user)
+              # Turn any %20 into spaces.
+              file_path = CGI.unescape(uri.path)
+              IngestLocalFileJob.perform_later(fs, file_path, env.user)
             else
               ImportUrlJob.perform_later(fs, operation_for(user: actor.user))
             end

diff --git a/app/actors/hyrax/actors/file_set_actor.rb b/app/actors/hyrax/actors/file_set_actor.rb
@@ -14,7 +14,7 @@ def initialize(file_set, user)
 
       # Spawns asynchronous IngestJob unless ingesting from URL
       # Called from FileSetsController, AttachFilesToWorkJob, IngestLocalFileJob, ImportUrlJob
-      # @param [Hyrax::UploadedFile, File, ActionDigest::HTTP::UploadedFile] file the file uploaded by the user
+      # @param [Hyrax::UploadedFile, File] file the file uploaded by the user
       # @param [Symbol, #to_s] relation
       # @return [IngestJob, FalseClass] false on failure, otherwise the queued job
       def create_content(file, relation = :original_file, from_url: false)
@@ -130,8 +130,6 @@ def wrapper!(file:, relation:)
         def label_for(file)
           if file.is_a?(Hyrax::UploadedFile) # filename not present for uncached remote file!
             file.uploader.filename.present? ? file.uploader.filename : File.basename(Addressable::URI.parse(file.file_url).path)
-          elsif file.respond_to?(:original_filename) # e.g. ActionDispatch::Http::UploadedFile, CarrierWave::SanitizedFile
-            file.original_filename
           elsif file.respond_to?(:original_name) # e.g. Hydra::Derivatives::IoDecorator
             file.original_name
           elsif file_set.import_url.present?

diff --git a/app/assets/javascripts/hyrax.js b/app/assets/javascripts/hyrax.js
@@ -80,8 +80,8 @@
 //= require hyrax/permissions
 //= require hyrax/autocomplete
 //= require hyrax/autocomplete/default
-//= require hyrax/autocomplete/work
 //= require hyrax/autocomplete/linked_data
+//= require hyrax/autocomplete/resource
 //= require hyrax/relationships
 //= require hyrax/select_work_type
 //= require hyrax/collections
@@ -94,7 +94,6 @@
 //= require hyrax/file_manager/save_manager
 //= require hyrax/file_manager/member
 //= require hyrax/file_manager
-//= require hyrax/workflow_actions_affix
 //= require hyrax/authority_select
 //= require hyrax/sort_and_per_page
 //= require hyrax/thumbnail_select

diff --git a/app/assets/javascripts/hyrax/app.js.erb b/app/assets/javascripts/hyrax/app.js.erb
@@ -75,7 +75,7 @@ Hyrax = {
         var element = $("[data-behavior='work-form']")
         if (element.length > 0) {
           var Editor = require('hyrax/editor');
-          new Editor(element)
+          new Editor(element).init();
         }
     },
 

diff --git a/app/assets/javascripts/hyrax/autocomplete.es6 b/app/assets/javascripts/hyrax/autocomplete.es6
@@ -1,23 +1,28 @@
 import Default from './autocomplete/default'
-import Work from './autocomplete/work'
+import Resource from './autocomplete/resource'
 import LinkedData from './autocomplete/linked_data'
 
 export default class Autocomplete {
   /**
    * Setup for the autocomplete field.
    * @param {jQuery} element - The input field to add autocompete to
-   # @param {string} fieldName - The name of the field (e.g. 'based_near')
-   # @param {string} url - The url for the autocompete search endpoint
+   * @param {string} fieldName - The name of the field (e.g. 'based_near')
+   * @param {string} url - The url for the autocompete search endpoint
    */
   setup (element, fieldName, url) {
     switch (fieldName) {
       case 'work':
-        new Work(
+        new Resource(
           element,
           url,
-          element.data('exclude-work')
+          { excluding: element.data('exclude-work') }
         )
         break
+      case 'collection':
+        new Resource(
+          element,
+          url)
+        break
       case 'based_near':
         new LinkedData(element, url)
       default:

diff --git a/app/assets/javascripts/hyrax/autocomplete/default.es6 b/app/assets/javascripts/hyrax/autocomplete/default.es6
@@ -1,3 +1,4 @@
+// This script initializes a jquery-ui autocomplete widget
 export default class Default {
   constructor(element, url) {
     this.url = url;
@@ -10,7 +11,6 @@ export default class Default {
       minLength: 2,
 
       source: (request, response) => {
-        console.log("Requesting " + this.url)
         $.getJSON(this.url, {
           q: request.term
         }, response );

diff --git a/app/assets/javascripts/hyrax/autocomplete/linked_data.es6 b/app/assets/javascripts/hyrax/autocomplete/linked_data.es6
@@ -1,4 +1,4 @@
-// Autocomplete for linked data elements
+// Autocomplete for linked data elements using a select2 autocomplete widget
 // After selecting something, the seleted item is immutable
 export default class LinkedData {
   constructor(element, url) {