Merge branch 'main' into mccalluc-no-partial-match-ark-doi

app/models/s3_file.rb

@@ -12,7 +12,7 @@ def initialize(filename:, last_modified:, size:, checksum:, query_service: nil)
   end
 
   def globus_url
-    encoded_filename = filename.split("/").map { |name| CGI.escape(name) }.join("/")
+    encoded_filename = filename.split("/").map { |name| ERB::Util.url_encode(name) }.join("/")
     File.join(Rails.configuration.globus["post_curation_base_url"], encoded_filename)
   end
 

app/models/work.rb

@@ -303,7 +303,7 @@ def add_message(message, current_user_id)
   end
 
   def add_provenance_note(date, note, current_user_id)
-    WorkActivity.add_work_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, created_at: date)
   end
 
   def log_changes(resource_compare, current_user_id)
@@ -317,7 +317,7 @@ def log_file_changes(changes, current_user_id)
   end
 
   def activities
-    WorkActivity.activities_for_work(id)
+    WorkActivity.activities_for_work(id, WorkActivity::MESSAGE_ACTIVITY_TYPES + WorkActivity::CHANGE_LOG_ACTIVITY_TYPES)
   end
 
   def new_notification_count_for_user(user_id)

app/models/work_activity.rb

@@ -22,25 +22,21 @@ class WorkActivity < ApplicationRecord
   belongs_to :work
   has_many :work_activity_notifications, dependent: :destroy
 
-  def self.add_work_activity(work_id, message, user_id, activity_type:, date: nil)
+  def self.add_work_activity(work_id, message, user_id, activity_type:, created_at: nil)
     activity = WorkActivity.new(
       work_id: work_id,
       activity_type: activity_type,
       message: message,
       created_by_user_id: user_id,
-      created_at: date # If nil, will be set by activerecord at save.
+      created_at: created_at # If nil, will be set by activerecord at save.
     )
     activity.save!
     activity.notify_users
     activity
   end
 
-  def self.activities_for_work(work_id, types = [])
-    context = where(work_id: work_id).order(updated_at: :desc)
-    if types.count > 0
-      context = context.where(activity_type: types)
-    end
-    context
+  def self.activities_for_work(work_id, activity_types)
+    where(work_id: work_id, activity_type: activity_types)
   end
 
   def self.messages_for_work(work_id)
@@ -102,6 +98,7 @@ def initialize(work_activity)
     end
 
     UNKNOWN_USER = "Unknown user outside the system"
+    DATE_TIME_FORMAT = "%B %d, %Y %H:%M"
 
     def to_html
       title_html + "<span class='message-html'>#{body_html.chomp}</span>"
@@ -113,12 +110,18 @@ def created_by_user_html
       @work_activity.created_by_user.display_name_safe
     end
 
-    def created_at_html
-      @work_activity.created_at.time.strftime("%B %d, %Y %H:%M")
+    def created_updated_html
+      created = @work_activity.created_at.time.strftime(DATE_TIME_FORMAT)
+      updated = @work_activity.updated_at.time.strftime(DATE_TIME_FORMAT)
+      if created == updated
+        created
+      else
+        "#{created} (backdated event created #{updated})"
+      end
     end
 
     def title_html
-      "<span class='activity-history-title'>#{created_at_html} by #{created_by_user_html}</span>"
+      "<span class='activity-history-title'>#{created_updated_html} by #{created_by_user_html}</span>"
     end
   end
 
@@ -191,8 +194,15 @@ class OtherLogEvent < BaseMessage
 
   class Message < BaseMessage
     # Override the default:
+    def created_by_user_html
+      return UNKNOWN_USER unless @work_activity.created_by_user
+
+      user = @work_activity.created_by_user
+      "#{user.display_name_safe} (@#{user.uid})"
+    end
+
     def title_html
-      "<span class='activity-history-title'>#{created_by_user_html} at #{created_at_html}</span>"
+      "<span class='activity-history-title'>#{created_by_user_html} at #{created_updated_html}</span>"
     end
   end
 end

app/views/works/_work_activity_messages.html.erb

@@ -4,8 +4,10 @@
     No messages
   <% end %>
   <ul class="no-beads work-messages">
-    <% @messages.each do |activity| %>
-      <%= render partial: 'work_activity', locals: { activity: activity } %>
+    <% @messages.sort_by(&:created_at).reverse.each do |activity| %>
+      <li class="activity-history-item">
+        <%= activity.to_html.html_safe %>
+      </li>
     <% end %>
     <% if @work.submission_notes.present? %>
       <li class="activity-history-item">

app/views/works/_work_activity_provenance.html.erb

@@ -4,12 +4,14 @@
     No activity
   <% end %>
   <ul class="beads">
-    <% @changes.sort_by(&:created_at).reverse.each do |activity| %>
-      <%= render partial: 'work_activity', locals: {activity: activity} %>
+    <% @changes.sort_by(&:created_at).each do |activity| %>
+      <li class="activity-history-item">
+        <%= activity.to_html.html_safe %>
+      </li>
     <% end %>
   </ul>
 
-  <% if ["cm7575"].include? current_user.uid %>
+  <% if can_add_provenance_note %>
     <%= form_with url: add_provenance_note_path(@work) do |f| %>
       <div class="field">
         <details>

app/views/works/show.html.erb

@@ -246,7 +246,7 @@
 <% end %>
 
 <%= render 'work_activity_messages' %>
-<%= render 'work_activity_provenance' %>
+<%= render(partial: 'work_activity_provenance', locals: {can_add_provenance_note: ["cm7575"].include?(current_user.uid)}) %>
 
 <%= javascript_include_tag 'show_work_utils' %>
 

spec/models/s3_file_spec.rb

@@ -35,7 +35,7 @@
     it "builds the URL for the S3 endpoint" do
       expect(s3_file.globus_url).to match(%r{^https://example.data.globus.org/10-34770/pe9w-x904/filename})
       url_file = s3_file.globus_url.split("/").last
-      expect(url_file).to eq("filename+%5Bwith+spaces%5D+w%C3%A9%C3%AE%C2%AE%E2%88%82+chars.txt")
+      expect(url_file).to eq("filename%20%5Bwith%20spaces%5D%20w%C3%A9%C3%AE%C2%AE%E2%88%82%20chars.txt")
       expect(CGI.unescape(url_file)).to eq(filename.split("/").last)
     end
   end

spec/models/work_activity_spec.rb

@@ -56,29 +56,25 @@
     end
 
     describe "#activities_for_work" do
-      it "finds all the activities for the work" do
-        expect(described_class.activities_for_work(work)).to eq([notification, work_activity])
-      end
-
       it "finds all the activities for the work and type" do
         expect(described_class.activities_for_work(work, [WorkActivity::SYSTEM])).to eq([work_activity])
         expect(described_class.activities_for_work(work, [WorkActivity::NOTIFICATION])).to eq([notification])
-        expect(described_class.activities_for_work(work, [WorkActivity::SYSTEM, WorkActivity::NOTIFICATION])).to eq([notification, work_activity])
+        expect(described_class.activities_for_work(work, [WorkActivity::SYSTEM, WorkActivity::NOTIFICATION])).to eq([work_activity, notification])
       end
     end
 
     describe "#messages_for_work" do
       it "finds all the messages for the work" do
         activity_message = described_class.add_work_activity(work.id, message, user.id, activity_type: WorkActivity::MESSAGE)
-        expect(described_class.messages_for_work(work.id)).to eq([activity_message, notification])
+        expect(described_class.messages_for_work(work.id)).to eq([notification, activity_message])
       end
     end
 
     describe "#changes_for_work" do
       it "finds all the changes for the work" do
         change_file = described_class.add_work_activity(work.id, message, user.id, activity_type: WorkActivity::FILE_CHANGES)
         changes = described_class.add_work_activity(work.id, message, user.id, activity_type: WorkActivity::CHANGES)
-        expect(described_class.changes_for_work(work.id)).to eq([changes, change_file, work_activity])
+        expect(described_class.changes_for_work(work.id)).to eq([work_activity, change_file, changes])
       end
     end
   end

spec/models/work_spec.rb

@@ -383,19 +383,19 @@
       expect(work.curator).to be nil
 
       work.change_curator(curator_user.id, user)
-      activity = WorkActivity.changes_for_work(work.id).first
+      activity = WorkActivity.changes_for_work(work.id).last
       expect(activity.message).to eq("Set curator to @#{curator_user.uid}")
       expect(work.curator.id).to be curator_user.id
       expect(activity.created_by_user.id).to eq user.id
 
       work.change_curator(user.id, user)
-      activity = WorkActivity.changes_for_work(work.id).first
+      activity = WorkActivity.changes_for_work(work.id).last
       expect(activity.message).to eq("Self-assigned as curator")
       expect(work.curator.id).to be user.id
       expect(activity.created_by_user.id).to eq user.id
 
       work.clear_curator(user)
-      activity = WorkActivity.changes_for_work(work.id).first
+      activity = WorkActivity.changes_for_work(work.id).last
       expect(activity.message).to eq("Unassigned existing curator")
       expect(work.curator).to be nil
       expect(activity.created_by_user.id).to eq user.id
@@ -857,12 +857,12 @@
         expect(work.pre_curation_uploads.first.key).to eq("#{work.doi}/#{work.id}/SCoData_combined_v1_2020-07_README.txt")
         expect(work.pre_curation_uploads.last).to be_a(ActiveStorage::Attachment)
         expect(work.pre_curation_uploads.last.key).to eq("#{work.doi}/#{work.id}/SCoData_combined_v1_2020-07_datapackage.json")
-        expect(WorkActivity.activities_for_work(work.id, ["FILE-CHANGES"]).count).to eq(1)
+        expect(WorkActivity.activities_for_work(work.id, [WorkActivity::FILE_CHANGES]).count).to eq(1)
 
         # call the s3 reload and make sure no more files get added to the model
         work.attach_s3_resources
         expect(work.pre_curation_uploads.length).to eq(2)
-        expect(WorkActivity.activities_for_work(work.id, ["FILE-CHANGES"]).count).to eq(1)
+        expect(WorkActivity.activities_for_work(work.id, [WorkActivity::FILE_CHANGES]).count).to eq(1)
 
         # Make sure pre-curation files do not show up in JSON:
         expect(work.as_json["files"]).to eq([])

spec/views/works/work_activity_messages.html.erb_spec.rb

@@ -0,0 +1,72 @@
+# frozen_string_literal: true
+require "rails_helper"
+
+describe "Messages" do
+  let(:user) { FactoryBot.create :user }
+  let(:work) { FactoryBot.create :draft_work }
+  let(:partial) { "works/work_activity_messages" }
+  let(:older) do
+    WorkActivity.add_work_activity(work.id, "older", user.id,
+      activity_type: WorkActivity::MESSAGE, created_at: "2021-01-01")
+  end
+  let(:newer) do
+    WorkActivity.add_work_activity(work.id, "newer", user.id,
+      activity_type: WorkActivity::MESSAGE, created_at: "2022-01-01")
+  end
+
+  it "handles no messages" do
+    assign(:work, work)
+    assign(:messages, [])
+    render(partial: partial)
+    expect(rendered).to include("No messages")
+  end
+
+  it "handles unknown user" do
+    assign(:work, work)
+    assign(:messages, [WorkActivity.add_work_activity(work.id, "message!", nil,
+      activity_type: WorkActivity::MESSAGE)])
+    render(partial: partial)
+    expect(rendered).to include("Unknown user outside the system")
+  end
+
+  it "handles submission notes" do
+    work.submission_notes = "submission note!"
+    assign(:work, work)
+    assign(:messages, [WorkActivity.add_work_activity(work.id, "message!", user.id,
+      activity_type: WorkActivity::MESSAGE)])
+    render(partial: partial)
+    expect(rendered).to include("submission note!")
+  end
+
+  it "handles message" do
+    assign(:work, work)
+    assign(:messages, [WorkActivity.add_work_activity(work.id, "message!", user.id,
+      activity_type: WorkActivity::MESSAGE)])
+    render(partial: partial)
+    expect(rendered).to include("message!")
+    expect(rendered).to include("(@#{user.uid})")
+  end
+
+  it "handles notification" do
+    assign(:work, work)
+    assign(:messages, [WorkActivity.add_work_activity(work.id, "notification!", user.id,
+      activity_type: WorkActivity::NOTIFICATION)])
+    render(partial: partial)
+    expect(rendered).to include("notification!")
+    expect(rendered).to include("(@#{user.uid})")
+  end
+
+  it "shows newest message first, when array is in the same order" do
+    assign(:work, work)
+    assign(:messages, [newer, older])
+    render(partial: partial)
+    expect(rendered).to match(/newer.*older/m)
+  end
+
+  it "still shows newest message first, when array is in the reverse order" do
+    assign(:work, work)
+    assign(:messages, [older, newer])
+    render(partial: partial)
+    expect(rendered).to match(/newer.*older/m)
+  end
+end

spec/views/works/work_activity_provenance.html.erb_spec.rb

@@ -0,0 +1,68 @@
+# frozen_string_literal: true
+require "rails_helper"
+
+describe "Change History, AKA Provenance" do
+  let(:user) { FactoryBot.create :user }
+  let(:work) { FactoryBot.create :draft_work }
+  let(:partial) { "works/work_activity_provenance" }
+  let(:older) do
+    WorkActivity.add_work_activity(work.id, "older", user.id,
+      activity_type: WorkActivity::SYSTEM, created_at: "2021-01-01")
+  end
+  let(:newer) do
+    WorkActivity.add_work_activity(work.id, "newer", user.id,
+      activity_type: WorkActivity::SYSTEM, created_at: "2022-01-01")
+  end
+
+  it "handles no activity" do
+    assign(:changes, [])
+    render(partial: partial, locals: { can_add_provenance_note: false })
+    expect(rendered).to include("No activity")
+  end
+
+  it "handles metadata changes" do
+    assign(:changes, [WorkActivity.add_work_activity(work.id, JSON.dump({ a_field: [{ action: "changed", from: "old", to: "new" }] }), user.id,
+      activity_type: WorkActivity::CHANGES)])
+    render(partial: partial, locals: { can_add_provenance_note: false })
+    expect(rendered).to include("<del>old</del><ins>new</ins>")
+  end
+
+  it "handles file changes" do
+    assign(:changes, [WorkActivity.add_work_activity(work.id, "{}", user.id,
+      activity_type: WorkActivity::FILE_CHANGES)])
+    render(partial: partial, locals: { can_add_provenance_note: false })
+    expect(rendered).to include("Files updated:")
+  end
+
+  it "handles prov note" do
+    assign(:changes, [WorkActivity.add_work_activity(work.id, "note!", user.id,
+      activity_type: WorkActivity::PROVENANCE_NOTES)])
+    render(partial: partial, locals: { can_add_provenance_note: false })
+    expect(rendered).to include("note!")
+  end
+
+  it "handles error" do
+    assign(:changes, [WorkActivity.add_work_activity(work.id, "error!", user.id,
+      activity_type: WorkActivity::DATACITE_ERROR)])
+    render(partial: partial, locals: { can_add_provenance_note: false })
+    expect(rendered).to include("error!")
+  end
+
+  it "handles backdated prov note" do
+    assign(:changes, [older])
+    render(partial: partial, locals: { can_add_provenance_note: false })
+    expect(rendered).to include("January 01, 2021 00:00 (backdated event created")
+  end
+
+  it "shows oldest change first, when array is in the same order" do
+    assign(:changes, [older, newer])
+    render(partial: partial, locals: { can_add_provenance_note: false })
+    expect(rendered).to match(/older.*newer/m)
+  end
+
+  it "still shows oldest change first, when array is in the reverse order" do
+    assign(:changes, [newer, older])
+    render(partial: partial, locals: { can_add_provenance_note: false })
+    expect(rendered).to match(/older.*newer/m)
+  end
+end