resolve merge conflicts

.circleci/config.yml

@@ -5,7 +5,7 @@ commands:
       - run: gem install bundler -v '2.3.11'
       - run: cp Gemfile.lock Gemfile.lock.bak
       - restore_cache:
-          keys: &gem_key rails_template-cimg-{{ checksum "Gemfile.lock.bak" }}
+          key: &gem_key rails_template-cimg-{{ checksum "Gemfile.lock.bak" }}
       - run: bundle install --path ./vendor/bundle
       - save_cache:
           key: *gem_key

app/assets/stylesheets/application.scss

@@ -232,4 +232,8 @@ ul.no-beads {
     margin-bottom: 5px;
     padding: 5px;
   }
+}
+
+input:invalid {
+  box-shadow: 0 0 10px red;
 }

app/controllers/users_controller.rb

@@ -59,7 +59,7 @@ def set_user
 
     # Only allow a list of trusted parameters through.
     def user_params
-      @user_params ||= params.require(:user).permit([:display_name, :full_name, :family_name, :orcid, :email_messages_enabled, { collections_with_messaging: {} }])
+      @user_params ||= params.require(:user).permit([:display_name, :full_name, :family_name, :orcid, :email_messages_enabled, collections_with_messaging: {}])
     end
 
     def can_edit?

app/models/user.rb

@@ -209,13 +209,18 @@ def admin_collections
                            end
   end
 
+  def submitter_or_admin_collections
+    submitter_collections | admin_collections
+  end
+
   def pending_notifications_count
     WorkActivityNotification.where(user_id: id, read_at: nil).count
   end
 
   def assign_default_role
     @just_created = true
     add_role(:submitter, default_collection) unless has_role?(:submitter, default_collection)
+    enable_messages_from(collection: default_collection)
   end
 
   # Returns true if the user has notification e-mails enabled
@@ -227,14 +232,14 @@ def email_messages_enabled?
   # Permit this user to receive notification messages for members of a given Collection
   # @param collection [Collection]
   def enable_messages_from(collection:)
-    raise(ArgumentError, "User #{uid} is not an administrator for the collection #{collection.title}") unless can_admin?(collection)
+    raise(ArgumentError, "User #{uid} is not an administrator or depositor for the collection #{collection.title}") unless can_admin?(collection) || can_submit?(collection)
     collection_messaging_options << CollectionOption.new(option_type: CollectionOption::EMAIL_MESSAGES, user: self, collection: collection)
   end
 
   # Disable this user from receiving notification messages for members of a given Collection
   # @param collection [Collection]
   def disable_messages_from(collection:)
-    raise(ArgumentError, "User #{uid} is not an administrator for the collection #{collection.title}") unless can_admin?(collection)
+    raise(ArgumentError, "User #{uid} is not an administrator or depositor for the collection #{collection.title}") unless can_admin?(collection) || can_submit?(collection)
     collections_with_messaging.destroy(collection)
   end
 

app/models/work.rb

@@ -177,7 +177,6 @@ def valid_to_draft
   def valid_to_submit
     valid_to_draft
     validate_metadata
-    validate_uploads
     errors.count == 0
   end
 
@@ -276,7 +275,7 @@ def clear_curator(current_user)
     save!
 
     # ...and log the activity
-    WorkActivity.add_system_activity(id, "Unassigned existing curator", current_user.id)
+    WorkActivity.add_work_activity(id, "Unassigned existing curator", current_user.id, activity_type: WorkActivity::SYSTEM)
   end
 
   def update_curator(curator_user_id, current_user)
@@ -291,7 +290,7 @@ def update_curator(curator_user_id, current_user)
               else
                 "Set curator to @#{new_curator.uid}"
               end
-    WorkActivity.add_system_activity(id, message, current_user.id)
+    WorkActivity.add_work_activity(id, message, current_user.id, activity_type: WorkActivity::SYSTEM)
   end
 
   def curator_or_current_uid(user)
@@ -304,21 +303,21 @@ def curator_or_current_uid(user)
   end
 
   def add_message(message, current_user_id)
-    WorkActivity.add_system_activity(id, message, current_user_id, activity_type: WorkActivity::MESSAGE)
+    WorkActivity.add_work_activity(id, message, current_user_id, activity_type: WorkActivity::MESSAGE)
   end
 
   def add_provenance_note(date, note, current_user_id)
-    WorkActivity.add_system_activity(id, note, current_user_id, activity_type: WorkActivity::PROVENANCE_NOTES, date: date)
+    WorkActivity.add_work_activity(id, note, current_user_id, activity_type: WorkActivity::PROVENANCE_NOTES, date: date)
   end
 
   def log_changes(resource_compare, current_user_id)
     return if resource_compare.identical?
-    WorkActivity.add_system_activity(id, resource_compare.differences.to_json, current_user_id, activity_type: WorkActivity::CHANGES)
+    WorkActivity.add_work_activity(id, resource_compare.differences.to_json, current_user_id, activity_type: WorkActivity::CHANGES)
   end
 
   def log_file_changes(changes, current_user_id)
     return if changes.count == 0
-    WorkActivity.add_system_activity(id, changes.to_json, current_user_id, activity_type: WorkActivity::FILE_CHANGES)
+    WorkActivity.add_work_activity(id, changes.to_json, current_user_id, activity_type: WorkActivity::FILE_CHANGES)
   end
 
   def activities
@@ -524,7 +523,7 @@ def generate_attachment_key(attachment)
     def track_state_change(user, state = aasm.to_state)
       uw = UserWork.new(user_id: user.id, work_id: id, state: state)
       uw.save!
-      WorkActivity.add_system_activity(id, "marked as #{state.to_s.titleize}", user.id)
+      WorkActivity.add_work_activity(id, "marked as #{state.to_s.titleize}", user.id, activity_type: WorkActivity::SYSTEM)
       WorkStateTransitionNotification.new(self, user.id).send
     end
 
@@ -580,7 +579,7 @@ def publish_doi(user)
         if result.failure?
           resolved_user = curator_or_current_uid(user)
           message = "@#{resolved_user} Error publishing DOI. #{result.failure.status} / #{result.failure.reason_phrase}"
-          WorkActivity.add_system_activity(id, message, user.id, activity_type: "DATACITE_ERROR")
+          WorkActivity.add_work_activity(id, message, user.id, activity_type: WorkActivity::DATACITE_ERROR)
         end
       elsif ark.blank? # we can not update the url anywhere
         Honeybadger.notify("Publishing for a DOI we do not own and no ARK is present: #{doi}")

app/models/work_activity.rb

@@ -12,7 +12,8 @@ class WorkActivity < ApplicationRecord
   FILE_CHANGES = "FILE-CHANGES"
   PROVENANCE_NOTES = "PROVENANCE-NOTES"
   SYSTEM = "SYSTEM"
-  CHANGE_LOG_ACTIVITY_TYPES = [CHANGES, FILE_CHANGES, PROVENANCE_NOTES, SYSTEM].freeze
+  DATACITE_ERROR = "DATACITE-ERROR"
+  CHANGE_LOG_ACTIVITY_TYPES = [CHANGES, FILE_CHANGES, PROVENANCE_NOTES, SYSTEM, DATACITE_ERROR].freeze
 
   USER_REFERENCE = /@[\w]*/.freeze # e.g. @xy123
 
@@ -21,7 +22,7 @@ class WorkActivity < ApplicationRecord
   belongs_to :work
   has_many :work_activity_notifications, dependent: :destroy
 
-  def self.add_system_activity(work_id, message, user_id, activity_type: SYSTEM, date: nil)
+  def self.add_work_activity(work_id, message, user_id, activity_type:, date: nil)
     activity = WorkActivity.new(
       work_id: work_id,
       activity_type: activity_type,
@@ -77,32 +78,16 @@ def message_event_type?
     MESSAGE_ACTIVITY_TYPES.include? activity_type
   end
 
-  def changes_event_type?
-    activity_type == CHANGES
-  end
-
-  def file_changes_event_type?
-    activity_type == FILE_CHANGES
-  end
-
-  def system_event_type?
-    activity_type == SYSTEM
-  end
-
-  def provenance_notes_type?
-    activity_type == PROVENANCE_NOTES
-  end
-
   def log_event_type?
     CHANGE_LOG_ACTIVITY_TYPES.include? activity_type
   end
 
   def to_html
-    klass = if changes_event_type?
+    klass = if activity_type == CHANGES
        MetadataChanges
-     elsif file_changes_event_type?
+     elsif activity_type == FILE_CHANGES
        FileChanges
-     elsif log_event_type?
+     elsif CHANGE_LOG_ACTIVITY_TYPES.include?(activity_type)
        OtherLogEvent
      else
        Message

app/models/work_state_transition_notification.rb

@@ -22,7 +22,7 @@ def initialize(work, current_user_id)
   def send
     return if notification.nil?
 
-    WorkActivity.add_system_activity(id, notification, current_user_id, activity_type: WorkActivity::NOTIFICATION)
+    WorkActivity.add_work_activity(id, notification, current_user_id, activity_type: WorkActivity::NOTIFICATION)
   end
 
     private

app/views/users/_form.html.erb

@@ -9,20 +9,18 @@
         </div>
       </fieldset>
 
-      <% if @user.moderator? %>
-        <section class="card container">
-          <h2>Collections</h2>
-          <fieldset>
-            <% @user.admin_collections.each do |admin_collection| %>
-              <legend><%= admin_collection.title %></legend>
-              <div class="form-check">
-                <%= check_box_tag("user[collections_with_messaging][#{admin_collection.id}]", "1", user.messages_enabled_from?(collection: admin_collection), { class: ["form-check-input"] }) %>
-                <%= label_tag("user[collections_with_messaging][#{admin_collection.id}]", t("users.form.email_messages_enabled"), class: ["form-check-label"]) %>
-              </div>
-            <% end %>
-          </fieldset>
-        </section>
-      <% end %>
+      <section class="card container collections"> 
+        <h2>Collections</h2>
+        <fieldset>
+          <% @user.submitter_or_admin_collections.each do |collection| %>
+            <legend><%= collection.title %></legend>
+            <div class="form-check">
+              <%= form.check_box("[collections_with_messaging][#{collection.id}]", { id: "collection_messaging_#{collection.id}", checked: user.messages_enabled_from?(collection: collection), class: ["form-check-input"] }) %>
+              <%= form.label("[collections_with_messaging][#{collection.id}]", t("users.form.email_messages_enabled"), class: ["form-check-label"]) %>
+            </div>
+          <% end %>
+        </fieldset>
+      </section>
 
       <button type="submit" class="btn btn-primary"><%= t("users.form.email_messages_submit") %></button>
     <% end %>

app/views/works/_work_activity_history.html.erb

@@ -50,15 +50,17 @@
           The date in the change history your note should be given. Format as "YYY-MM-DD".
           Leave blank to use the current date and time.
         </details>
-        <input id="new-provenance-date" name="new-provenance-date" ></textarea>
+        <input id="new-provenance-date" name="new-provenance-date"
+          placeholder="YYYY-MM-DD"
+          pattern="\d{4}-\d{2}-\d{2}">
       </div>
 
       <div class="field">
         <details>
           <summary>Note</summary>
           The note to add to the change history. Markdown can be used.
         </details>
-        <input id="new-provenance-note" name="new-provenance-note" ></textarea>
+        <input id="new-provenance-note" name="new-provenance-note">
       </div>
 
       <%= f.submit("Add Provenance Note", class: "btn btn-secondary") %>

spec/controllers/works_controller_spec.rb

@@ -789,7 +789,7 @@
 
       context "when the work has changes and messages" do
         before do
-          WorkActivity.add_system_activity(work.id, "Hello System", user.id)
+          WorkActivity.add_work_activity(work.id, "Hello System", user.id, activity_type: WorkActivity::SYSTEM)
           work.add_message("Hello World", user.id)
         end
 
@@ -903,7 +903,7 @@
           expect(response.status).to be 302
           expect(response.location).to eq "http://test.host/works/#{work.id}"
           expect(work.reload).to be_approved
-          error = work.work_activity.find { |activity| activity.activity_type == "DATACITE_ERROR" }
+          error = work.work_activity.find { |activity| activity.activity_type == WorkActivity::DATACITE_ERROR }
           expect(error.message).to include("Error publishing DOI")
         end
       end
@@ -1055,6 +1055,10 @@
 
       it "posts a message with sanitized HTML" do
         sign_in user
+        # The ERB only shows the form to a subset of users,
+        # but the endpoint has no such restriction: Anyone can POST.
+        # In some contexts, a hole like this would be a security problem,
+        # but this is low-stakes.
         post :add_message, params: { id: work.id, "new-message" => "<div>hello world</div>" }
         expect(response.status).to be 302
         expect(response.location).to eq "http://test.host/works/#{work.id}"