Merge pull request #336 from blackcandy-org/default-name

Add default name to albums and artists

app/assets/stylesheets/components/_action_bar.scss

@@ -1,7 +1,7 @@
 @use "../tools/functions" as *;
 
 .c-action-bar {
-  padding: spacing("narrow");
+  padding: spacing("tiny") spacing("narrow");
   border-radius: border-radius("medium");
   background: var(--action-bar-bg-color);
 }

app/assets/stylesheets/components/_button.scss

@@ -8,6 +8,10 @@
   padding: spacing("tiny") spacing("narrow");
   cursor: pointer;
   font-size: font-size("medium");
+
+  &:hover {
+    text-decoration: none;
+  }
 }
 
 .c-button--primary {

app/assets/stylesheets/components/_list.scss

@@ -25,6 +25,12 @@
   text-transform: uppercase;
 }
 
+.c-list__item__subtext {
+  display: inline-block;
+  margin-top: spacing("tiny");
+  color: var(--list-subtext-color);
+}
+
 .c-list--border-none .c-list__item {
   border-bottom: none;
 }

app/assets/stylesheets/elements/_content.scss

@@ -37,7 +37,7 @@ a {
 }
 
 a:hover {
-  color: var(--link-active-color);
+  text-decoration: underline;
 }
 
 a.is-active {

app/assets/stylesheets/settings/_dark_theme.scss

@@ -54,6 +54,7 @@
   --list-border-color: #{$grey-800};
   --list-active-color: #{$primary-color};
   --list-grouped-bg-color: #{$grey-900};
+  --list-subtext-color: #{$grey-400};
 
   /* Table */
   --table-border-color: #{$grey-900};

app/assets/stylesheets/settings/_light_theme.scss

@@ -54,6 +54,7 @@
   --list-border-color: #{$grey-200};
   --list-active-color: #{$primary-color};
   --list-grouped-bg-color: #{$grey-100};
+  --list-subtext-color: #{$grey-500};
 
   /* Table */
   --table-border-color: #{$grey-200};

app/helpers/song_helper.rb

@@ -8,8 +8,8 @@ def song_json_builder(song, for_api: false)
     Jbuilder.new do |json|
       json.call(song, :id, :name, :duration)
       json.url need_transcode?(song) ? transcoded_stream_url : stream_url
-      json.album_name song.album.title
-      json.artist_name song.artist.title
+      json.album_name song.album.name
+      json.artist_name song.artist.name
       json.is_favorited song.is_favorited.nil? ? Current.user.favorited?(song) : song.is_favorited
       json.format need_transcode?(song) ? Stream::TRANSCODE_FORMAT : song.format
       json.album_image_url do

app/javascript/controllers/mixins/event_handler.js

@@ -22,6 +22,8 @@ class EventHandler {
     const handler = {
       listener (event) {
         if (targetMatching) {
+          if (event.target.dataset.preventDelegation) { return }
+
           const target = event.target.closest(targetMatching)
           if (!target) { return }
         }

app/models/album.rb

@@ -1,11 +1,16 @@
 # frozen_string_literal: true
 
 class Album < ApplicationRecord
+  UNKNOWN_NAME = "Unknown Album"
+
   include SearchableConcern
   include ImageableConcern
   include FilterableConcern
   include SortableConcern
 
+  after_initialize :set_default_name, if: :new_record?
+
+  validates :name, presence: true
   validates :name, uniqueness: {scope: :artist}
 
   has_many :songs, -> { order(:discnum, :tracknum) }, inverse_of: :album, dependent: :destroy
@@ -18,11 +23,13 @@ class Album < ApplicationRecord
   sort_by :name, :year, :created_at
   sort_by_associations artist: :name
 
-  def title
-    is_unknown? ? I18n.t("label.unknown_album") : name
+  def unknown?
+    name == UNKNOWN_NAME
   end
 
-  def is_unknown?
-    name.blank?
+  private
+
+  def set_default_name
+    self.name ||= UNKNOWN_NAME
   end
 end

app/models/artist.rb

@@ -1,26 +1,26 @@
 # frozen_string_literal: true
 
 class Artist < ApplicationRecord
+  UNKNOWN_NAME = "Unknown Artist"
+  VARIOUS_NAME = "Various Artists"
+
   include SearchableConcern
   include ImageableConcern
   include SortableConcern
 
+  after_initialize :set_default_name, if: :new_record?
+
+  validates :name, presence: true
+
   has_many :albums, dependent: :destroy
   has_many :songs
 
   search_by :name
 
   sort_by :name, :created_at
 
-  def title
-    return I18n.t("label.various_artists") if is_various?
-    return I18n.t("label.unknown_artist") if is_unknown?
-
-    name
-  end
-
-  def is_unknown?
-    name.blank?
+  def unknown?
+    name == UNKNOWN_NAME
   end
 
   def all_albums
@@ -30,4 +30,10 @@ def all_albums
   def appears_on_albums
     Album.joins(:songs).where("albums.artist_id != ? AND songs.artist_id = ?", id, id).distinct
   end
+
+  private
+
+  def set_default_name
+    self.name ||= various? ? VARIOUS_NAME : UNKNOWN_NAME
+  end
 end

app/models/concerns/imageable_concern.rb

@@ -16,6 +16,6 @@ def attach_image_from_discogs
   private
 
   def needs_image_from_discogs?
-    Setting.discogs_token.present? && !has_image? && !is_unknown?
+    Setting.discogs_token.present? && !has_image? && !unknown?
   end
 end

app/models/media.rb

@@ -61,14 +61,13 @@ def remove_files(file_paths)
     end
 
     def attach(file_info)
-      artist = Artist.find_or_create_by!(name: file_info[:artist_name])
+      artist = Artist.find_or_create_by!(name: file_info[:artist_name] || Artist::UNKNOWN_NAME)
+      various_artist = Artist.find_or_create_by!(various: true) if various_artist?(file_info)
 
-      album = if various_artist?(file_info)
-        various_artist = Artist.find_or_create_by!(is_various: true)
-        Album.find_or_initialize_by(artist: various_artist, name: file_info[:album_name])
-      else
-        Album.find_or_initialize_by(artist: artist, name: file_info[:album_name])
-      end
+      album = Album.find_or_initialize_by(
+        artist: various_artist || artist,
+        name: file_info[:album_name] || Album::UNKNOWN_NAME
+      )
 
       album.update!(album_info(file_info))
       album.update!(image: file_info[:image]) unless album.has_image?

app/views/albums/_album.html.erb

@@ -3,7 +3,7 @@
     <%= image_tag image_url_for(album), class: "u-image-fluid" %>
   <% end %>
   <div class='c-card__body'>
-    <p class='c-card__body__text u-text-weight-bold u-text-line-clamp-2'><%= link_to album.title, album_path(album) %></p>
-    <p class='c-card__body__text u-text-line-clamp-2'><%= album.artist.title %></p>
+    <p class='c-card__body__text u-text-weight-bold u-text-line-clamp-2'><%= link_to album.name, album_path(album) %></p>
+    <p class='c-card__body__text u-text-line-clamp-2'><%= album.artist.name %></p>
   </div>
 </div>

app/views/albums/show.html.erb

@@ -1,11 +1,11 @@
-<% page_title_tag @album.title %>
+<% page_title_tag @album.name %>
 
 <div class='o-container o-container--narrow' data-controller='playlist-songs playlist-songs-bridge'>
   <div class='c-card c-card--horizontal c-card--center@narrow u-my-large'>
     <%= image_tag image_url_for(@album), class: "c-card__image u-image-large", data: {test_id: "album_image"} %>
     <div class='c-card__body'>
-      <h1 class='c-card__body__title'><%= @album.title %></h1>
-      <p class='c-card__body__text'><%= @album.artist.title %></p>
+      <h1 class='c-card__body__title'><%= @album.name %></h1>
+      <p class='c-card__body__text'><%= @album.artist.name %></p>
       <div class='c-card__body__text'>
         <span><%= @album.songs.count %> <%= t("label.tracks") %></span>
         <span>,</span>
@@ -57,9 +57,9 @@
                 ) do %>
               <div class='o-flex o-flex--justify-between o-flex--align-center'>
                 <div>
-                  <span class='u-mb-tiny' data-test-id='album_song_name'><%= song.name %></span>
-                  <% if @album.artist.is_various? %>
-                    <%= song.artist.title %>
+                  <div data-test-id='album_song_name'><%= song.name %></div>
+                  <% if @album.artist.various? %>
+                    <%= link_to song.artist.name, artist_path(song.artist), class: "c-list__item__subtext" %>
                   <% end %>
                 </div>
                 <div class='u-text-monospace u-mr-narrow'><%= format_duration(song.duration) %></div>

app/views/artists/_album.html.erb

@@ -3,6 +3,6 @@
     <%= image_tag image_url_for(album), class: "u-image-fluid" %>
   <% end %>
   <div class='c-card__body'>
-    <p class='c-card__body__text u-text-weight-bold u-text-line-clamp-2'><%= link_to album.title, album_path(album) %></p>
+    <p class='c-card__body__text u-text-weight-bold u-text-line-clamp-2'><%= link_to album.name, album_path(album) %></p>
   </div>
 </div>

app/views/artists/_artist.html.erb

@@ -3,6 +3,6 @@
     <%= image_tag image_url_for(artist), class: "u-image-fluid" %>
   <% end %>
   <div class='c-card__body'>
-    <p class='c-card__body__text u-text-weight-bold u-text-line-clamp-2'><%= link_to artist.title, artist_path(artist) %></p>
+    <p class='c-card__body__text u-text-weight-bold u-text-line-clamp-2'><%= link_to artist.name, artist_path(artist) %></p>
   </div>
 </div>

app/views/artists/show.html.erb

@@ -1,10 +1,10 @@
-<% page_title_tag @artist.title %>
+<% page_title_tag @artist.name %>
 
 <div class='o-container o-container--wide'>
   <div class='c-card c-card--horizontal c-card--center@narrow u-my-large'>
     <%= image_tag image_url_for(@artist), class: "c-card__image u-image-large", data: {test_id: "artist_image"} %>
     <div class='c-card__body'>
-      <h1 class='c-card__body__title'><%= @artist.title %></h1>
+      <h1 class='c-card__body__title'><%= @artist.name %></h1>
       <div class='c-card__body__text'>
         <span><%= @artist.all_albums.size %><%= t("label.albums") %></span>
         <span>,</span>

app/views/current_playlist/songs/_song.html.erb

@@ -15,8 +15,8 @@
     <button class='c-button c-button--link o-flex__item--grow-1' data-delegated-action='current-playlist-songs#play'>
       <div class='o-flex o-flex--justify-between o-flex--align-center'>
         <div class='u-mr-narrow'>
-          <span class='u-mb-tiny u-text-line-clamp-2' data-test-id='current_playlist_song_name'><%= song.name %></span>
-          <%= song.artist.title %>
+          <span class='u-text-line-clamp-2' data-test-id='current_playlist_song_name'><%= song.name %></span>
+          <%= link_to song.artist.name, artist_path(song.artist), class: "c-list__item__subtext", data: {"turbo-frame" => "_top", "prevent-delegation" => true} %>
         </div>
         <div class='u-text-monospace u-mr-narrow'><%= format_duration(song.duration) %></div>
       </div>

app/views/playlists/songs/_song.html.erb

@@ -19,7 +19,7 @@
       <div class='o-flex o-flex--justify-between o-flex--align-center'>
         <div class='u-mr-narrow'>
           <span class='u-mb-tiny u-text-line-clamp-2' data-test-id='playlist_song_name'><%= song.name %></span>
-          <%= song.artist.title %>
+          <%= song.artist.name %>
         </div>
         <div class='u-text-monospace u-mr-narrow'><%= format_duration(song.duration) %></div>
       </div>

app/views/songs/_song.html.erb

@@ -16,10 +16,10 @@
     <% end %>
   </div>
   <div role='cell' class='u-display-none@medium'>
-    <%= link_to song.artist.title, artist_path(song.artist) %>
+    <%= link_to song.artist.name, artist_path(song.artist) %>
   </div>
   <div role='cell' class='u-display-none@medium'>
-    <%= link_to song.album.title, album_path(song.album) %>
+    <%= link_to song.album.name, album_path(song.album) %>
   </div>
   <div role='cell' class='u-display-none@small'><%= format_duration(song.duration) %></div>
   <div role='cell'>

config/locales/en.yml

@@ -74,9 +74,6 @@ en:
     add_user: 'Add user'
     create_user: 'Create User'
     media_path: 'Media path'
-    various_artists: 'Various Artists'
-    unknown_artist: 'Unknown Artist'
-    unknown_album: 'Unknown Album'
     no_repeat_mode: 'No repeat mode'
     repeat_mode: 'Repeat mode'
     single_mode: 'Single mode'

db/migrate/20240103072942_add_default_name_to_unknown_albums.rb

@@ -0,0 +1,9 @@
+class AddDefaultNameToUnknownAlbums < ActiveRecord::Migration[7.1]
+  def up
+    Album.where(name: [nil, ""]).update_all(name: Album::UNKNOWN_NAME)
+  end
+
+  def down
+    Album.where(name: Album::UNKNOWN_NAME).update_all(name: nil)
+  end
+end

db/migrate/20240103081109_change_name_null_on_albums.rb

@@ -0,0 +1,5 @@
+class ChangeNameNullOnAlbums < ActiveRecord::Migration[7.1]
+  def change
+    change_column_null :albums, :name, false
+  end
+end

db/migrate/20240103085953_add_default_name_to_artists.rb

@@ -0,0 +1,10 @@
+class AddDefaultNameToArtists < ActiveRecord::Migration[7.1]
+  def up
+    Artist.where(is_various: true).update_all(name: Artist::VARIOUS_NAME)
+    Artist.where(name: [nil, ""]).update_all(name: Artist::UNKNOWN_NAME)
+  end
+
+  def down
+    Artist.where(name: [Artist::UNKNOWN_NAME, Artist::VARIOUS_NAME]).update_all(name: nil)
+  end
+end

db/migrate/20240103090230_change_name_null_on_artists.rb

@@ -0,0 +1,5 @@
+class ChangeNameNullOnArtists < ActiveRecord::Migration[7.1]
+  def change
+    change_column_null :artists, :name, false
+  end
+end

db/migrate/20240104023417_rename_is_various_in_artists.rb

@@ -0,0 +1,5 @@
+class RenameIsVariousInArtists < ActiveRecord::Migration[7.1]
+  def change
+    rename_column :artists, :is_various, :various
+  end
+end

db/schema.rb

@@ -10,9 +10,9 @@
 #
 # It's strongly recommended that you check this file into your version control system.
 
-ActiveRecord::Schema[7.1].define(version: 2023_12_07_020650) do
+ActiveRecord::Schema[7.1].define(version: 2024_01_04_023417) do
   create_table "albums", force: :cascade do |t|
-    t.string "name"
+    t.string "name", null: false
     t.string "image"
     t.datetime "created_at", null: false
     t.datetime "updated_at", null: false
@@ -25,11 +25,11 @@
   end
 
   create_table "artists", force: :cascade do |t|
-    t.string "name"
+    t.string "name", null: false
     t.string "image"
     t.datetime "created_at", null: false
     t.datetime "updated_at", null: false
-    t.boolean "is_various", default: false
+    t.boolean "various", default: false
     t.index ["name"], name: "index_artists_on_name", unique: true
   end
 

test/controllers/albums_controller_test.rb

@@ -36,7 +36,7 @@ class AlbumsControllerTest < ActionDispatch::IntegrationTest
     album = albums(:album1)
 
     assert_not album.has_image?
-    assert_not album.is_unknown?
+    assert_not album.unknown?
 
     assert_enqueued_with(job: AttachImageFromDiscogsJob) do
       get album_url(album)

test/controllers/artists_controller_test.rb

@@ -43,7 +43,7 @@ class ArtistsControllerTest < ActionDispatch::IntegrationTest
     artist = artists(:artist1)
 
     assert_not artist.has_image?
-    assert_not artist.is_unknown?
+    assert_not artist.unknown?
 
     assert_enqueued_with(job: AttachImageFromDiscogsJob) do
       get artist_url(artist)

test/fixtures/artists.yml

@@ -8,5 +8,5 @@ artist2:
 
 various_artists:
   name: 'various_artists'
-  is_various: true
+  various: true
   created_at: 2023-01-03

test/models/album_test.rb

@@ -8,8 +8,8 @@ class AlbumTest < ActiveSupport::TestCase
     assert_not artists(:artist1).albums.build(name: "best").valid?
   end
 
-  test "should have default title when name is empty" do
-    assert_equal "Unknown Album", Album.create(name: nil).title
+  test "should have default name when name is empty" do
+    assert_equal "Unknown Album", Album.create(name: nil).name
   end
 
   test "should order by discnum and tracknum for associated songs" do