Merge pull request #4003 from evazion/fix-3946

Artist versions: improve diffing of urls and other names (#3946)

app/helpers/application_helper.rb

@@ -1,6 +1,11 @@
 require 'dtext'
 
 module ApplicationHelper
+  def diff_list_html(new, old, latest)
+    diff = SetDiff.new(new, old, latest)
+    render "diff_list", diff: diff
+  end
+
   def wordbreakify(string)
     lines = string.scan(/.{1,10}/)
     wordbreaked_string = lines.map{|str| h(str)}.join("<wbr>")

app/helpers/artist_versions_helper.rb

@@ -1,42 +1,17 @@
 module ArtistVersionsHelper
   def artist_version_other_names_diff(artist_version)
-    diff = artist_version.other_names_diff(artist_version.previous)
-    html = '<span class="diff-list">'
+    new_names = artist_version.other_names
+    old_names = artist_version.previous.try(:other_names)
+    latest_names = artist_version.artist.other_names
 
-    diff[:added_names].each do |name|
-      prefix = diff[:obsolete_added_names].include?(name) ? '<ins class="obsolete">' : '<ins>'
-      html << prefix + h(name) + '</ins>'
-    end
-    diff[:removed_names].each do |name|
-      prefix = diff[:obsolete_removed_names].include?(name) ? '<del class="obsolete">' : '<del>'
-      html << prefix + h(name) + '</del>'
-    end
-    diff[:unchanged_names].each do |name|
-      html << '<span>' + h(name) + '</span>'
-      html << " "
-    end
-
-    html << "</span>"
-    return html.html_safe
+    diff_list_html(new_names, old_names, latest_names)
   end
 
   def artist_version_urls_diff(artist_version)
-    diff = artist_version.urls_diff(artist_version.previous)
-    html = '<ul class="diff-list">'
+    new_urls = artist_version.urls
+    old_urls = artist_version.previous.try(:urls)
+    latest_urls = artist_version.artist.urls.map(&:to_s)
 
-    diff[:added_urls].each do |url|
-      prefix = diff[:obsolete_added_urls].include?(url) ? '<ins class="obsolete">' : '<ins>'
-      html << '<li>' + prefix + h(url) + '</ins></li>'
-    end
-    diff[:removed_urls].each do |url|
-      prefix = diff[:obsolete_removed_urls].include?(url) ? '<del class="obsolete">' : '<del>'
-      html << '<li>' + prefix + h(url) + '</del></li>'
-    end
-    diff[:unchanged_urls].each do |url|
-      html << '<li><span>' + h(url) + '</span></li>'
-    end
-
-    html << "</ul>"
-    html.html_safe
+    diff_list_html(new_urls, old_urls, latest_urls)
   end
 end

app/javascript/src/styles/common/diffs.scss

@@ -1,21 +1,21 @@
 .diff-list {
-  ins, ins a {
+  .added, .added a {
     color: green;
     text-decoration: none;
     margin-right: 0.5em;
   }
 
-  ins.obsolete, ins.obsolete a {
+  .added.obsolete, .added.obsolete a {
     color: darkGreen;
   }
 
-  del, del a {
+  .removed, .removed a {
     color: red;
     text-decoration: line-through;
     margin-right: 0.5em;
   }
 
-  del.obsolete, del.obsolete a {
+  .removed.obsolete, .removed.obsolete a {
     color: darkRed;
   }
 }

app/logical/set_diff.rb

@@ -0,0 +1,35 @@
+class SetDiff
+  attr_reader :added, :removed, :obsolete_added, :obsolete_removed, :changed, :unchanged
+
+  def initialize(new, old, latest)
+    new, old, latest = new.to_a, old.to_a, latest.to_a
+
+    @added, @removed, @changed = changes(new, old)
+    @unchanged = new & old
+    @obsolete_added = added - latest
+    @obsolete_removed = removed & latest
+  end
+
+  def changes(new, old)
+    added = new - old
+    removed = old - new
+    changed = []
+
+    removed.each do |removal|
+      if addition = find_similar(removal, added)
+        changed << [removal, addition]
+        added -= [addition]
+        removed -= [removal]
+      end
+    end
+
+    [added, removed, changed]
+  end
+
+  def find_similar(string, candidates, max_dissimilarity: 0.75)
+    distance = ->(other) { DidYouMean::Levenshtein.distance(string, other) }
+    max_distance = string.size * max_dissimilarity
+
+    candidates.select { |candidate| distance[candidate] <= max_distance }.sort_by(&distance).first
+  end
+end

app/models/artist_version.rb

@@ -50,40 +50,6 @@ class ArtistVersion < ApplicationRecord
 
   extend SearchMethods
 
-  def urls_diff(version)
-    latest_urls = artist.url_array || []
-    new_urls = urls
-    old_urls = version.present? ? version.urls : []
-
-    added_urls = new_urls - old_urls
-    removed_urls = old_urls - new_urls
-
-    return {
-      :added_urls => added_urls,
-      :removed_urls => removed_urls,
-      :obsolete_added_urls => added_urls - latest_urls,
-      :obsolete_removed_urls => removed_urls & latest_urls,
-      :unchanged_urls => new_urls & old_urls,
-    }
-  end
-
-  def other_names_diff(version)
-    latest_names = artist.other_names || []
-    new_names = other_names
-    old_names = version.present? ? version.other_names : []
-
-    added_names = new_names - old_names
-    removed_names = old_names - new_names
-
-    return {
-      :added_names => added_names,
-      :removed_names => removed_names,
-      :obsolete_added_names => added_names - latest_names,
-      :obsolete_removed_names => removed_names & latest_names,
-      :unchanged_names => new_names & old_names,
-    }
-  end
-
   def previous
     ArtistVersion.where("artist_id = ? and created_at < ?", artist_id, created_at).order("created_at desc").first
   end

app/views/application/_diff_list.html.erb

@@ -0,0 +1,21 @@
+<%# diff %>
+
+<ul class="diff-list">
+  <% diff.added.each do |item| %>
+    <%= tag.li item, class: (item.in?(diff.obsolete_added) ? "obsolete added" : "added") %>
+  <% end %>
+
+  <% diff.removed.each do |item| %>
+    <%= tag.li item, class: (item.in?(diff.obsolete_removed) ? "obsolete removed" : "removed") %>
+  <% end %>
+
+  <% diff.changed.each do |old, new| %>
+    <%= tag.li class: "changed" do %>
+      <%= tag.span old, class: "removed" %>→ <%= tag.span new, class: "added" %>
+    <% end %>
+  <% end %>
+
+  <% diff.unchanged.each do |item| %>
+    <%= tag.li item, class: "unchanged" %>
+  <% end %>
+</ul>