From 3aaee32966b3f0bd0b85ef46db0c3e2e6d5d572a Mon Sep 17 00:00:00 2001 From: aidewoode Date: Thu, 4 Jan 2024 13:20:25 +0800 Subject: [PATCH 1/3] Add default name to albums and artists --- app/helpers/song_helper.rb | 4 ++-- app/models/album.rb | 15 ++++++++---- app/models/artist.rb | 24 ++++++++++++------- app/models/concerns/imageable_concern.rb | 2 +- app/models/media.rb | 13 +++++----- app/views/albums/_album.html.erb | 4 ++-- app/views/albums/show.html.erb | 10 ++++---- app/views/artists/_album.html.erb | 2 +- app/views/artists/_artist.html.erb | 2 +- app/views/artists/show.html.erb | 4 ++-- .../current_playlist/songs/_song.html.erb | 2 +- app/views/playlists/songs/_song.html.erb | 2 +- app/views/songs/_song.html.erb | 4 ++-- ...2942_add_default_name_to_unknown_albums.rb | 9 +++++++ ...240103081109_change_name_null_on_albums.rb | 5 ++++ ...40103085953_add_default_name_to_artists.rb | 10 ++++++++ ...40103090230_change_name_null_on_artists.rb | 5 ++++ ...0104023417_rename_is_various_in_artists.rb | 5 ++++ db/schema.rb | 8 +++---- test/controllers/albums_controller_test.rb | 2 +- test/controllers/artists_controller_test.rb | 2 +- test/fixtures/artists.yml | 2 +- test/models/album_test.rb | 4 ++-- test/models/artist_test.rb | 8 +++---- test/models/media_test.rb | 4 ++-- 25 files changed, 99 insertions(+), 53 deletions(-) create mode 100644 db/migrate/20240103072942_add_default_name_to_unknown_albums.rb create mode 100644 db/migrate/20240103081109_change_name_null_on_albums.rb create mode 100644 db/migrate/20240103085953_add_default_name_to_artists.rb create mode 100644 db/migrate/20240103090230_change_name_null_on_artists.rb create mode 100644 db/migrate/20240104023417_rename_is_various_in_artists.rb diff --git a/app/helpers/song_helper.rb b/app/helpers/song_helper.rb index 775b3ac4..83826634 100644 --- a/app/helpers/song_helper.rb +++ b/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 diff --git a/app/models/album.rb b/app/models/album.rb index 139b4596..4dcc7108 100644 --- a/app/models/album.rb +++ b/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 diff --git a/app/models/artist.rb b/app/models/artist.rb index a049a087..51d0cfa4 100644 --- a/app/models/artist.rb +++ b/app/models/artist.rb @@ -1,10 +1,17 @@ # 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 @@ -12,15 +19,8 @@ class Artist < ApplicationRecord 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 diff --git a/app/models/concerns/imageable_concern.rb b/app/models/concerns/imageable_concern.rb index 97e9ddbc..f33e9d92 100644 --- a/app/models/concerns/imageable_concern.rb +++ b/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 diff --git a/app/models/media.rb b/app/models/media.rb index e690faee..0137b80a 100644 --- a/app/models/media.rb +++ b/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? diff --git a/app/views/albums/_album.html.erb b/app/views/albums/_album.html.erb index 357a25e0..ddbf5503 100644 --- a/app/views/albums/_album.html.erb +++ b/app/views/albums/_album.html.erb @@ -3,7 +3,7 @@ <%= image_tag image_url_for(album), class: "u-image-fluid" %> <% end %>
-

<%= link_to album.title, album_path(album) %>

-

<%= album.artist.title %>

+

<%= link_to album.name, album_path(album) %>

+

<%= album.artist.name %>

diff --git a/app/views/albums/show.html.erb b/app/views/albums/show.html.erb index c5948fdb..45a3059f 100644 --- a/app/views/albums/show.html.erb +++ b/app/views/albums/show.html.erb @@ -1,11 +1,11 @@ -<% page_title_tag @album.title %> +<% page_title_tag @album.name %>
<%= image_tag image_url_for(@album), class: "c-card__image u-image-large", data: {test_id: "album_image"} %>
-

<%= @album.title %>

-

<%= @album.artist.title %>

+

<%= @album.name %>

+

<%= @album.artist.name %>

<%= @album.songs.count %> <%= t("label.tracks") %> , @@ -58,8 +58,8 @@
<%= song.name %> - <% if @album.artist.is_various? %> - <%= song.artist.title %> + <% if @album.artist.various? %> + <%= song.artist.name %> <% end %>
<%= format_duration(song.duration) %>
diff --git a/app/views/artists/_album.html.erb b/app/views/artists/_album.html.erb index 211d0268..0d92d19c 100644 --- a/app/views/artists/_album.html.erb +++ b/app/views/artists/_album.html.erb @@ -3,6 +3,6 @@ <%= image_tag image_url_for(album), class: "u-image-fluid" %> <% end %>
-

<%= link_to album.title, album_path(album) %>

+

<%= link_to album.name, album_path(album) %>

diff --git a/app/views/artists/_artist.html.erb b/app/views/artists/_artist.html.erb index dfa57df0..ca64684c 100644 --- a/app/views/artists/_artist.html.erb +++ b/app/views/artists/_artist.html.erb @@ -3,6 +3,6 @@ <%= image_tag image_url_for(artist), class: "u-image-fluid" %> <% end %>
-

<%= link_to artist.title, artist_path(artist) %>

+

<%= link_to artist.name, artist_path(artist) %>

diff --git a/app/views/artists/show.html.erb b/app/views/artists/show.html.erb index 56f5dc9f..c22148ad 100644 --- a/app/views/artists/show.html.erb +++ b/app/views/artists/show.html.erb @@ -1,10 +1,10 @@ -<% page_title_tag @artist.title %> +<% page_title_tag @artist.name %>
<%= image_tag image_url_for(@artist), class: "c-card__image u-image-large", data: {test_id: "artist_image"} %>
-

<%= @artist.title %>

+

<%= @artist.name %>

<%= @artist.all_albums.size %><%= t("label.albums") %> , diff --git a/app/views/current_playlist/songs/_song.html.erb b/app/views/current_playlist/songs/_song.html.erb index c2312183..8594d2c7 100644 --- a/app/views/current_playlist/songs/_song.html.erb +++ b/app/views/current_playlist/songs/_song.html.erb @@ -16,7 +16,7 @@
<%= song.name %> - <%= song.artist.title %> + <%= song.artist.name %>
<%= format_duration(song.duration) %>
diff --git a/app/views/playlists/songs/_song.html.erb b/app/views/playlists/songs/_song.html.erb index 5505b4f2..2c6b74a1 100644 --- a/app/views/playlists/songs/_song.html.erb +++ b/app/views/playlists/songs/_song.html.erb @@ -19,7 +19,7 @@
<%= song.name %> - <%= song.artist.title %> + <%= song.artist.name %>
<%= format_duration(song.duration) %>
diff --git a/app/views/songs/_song.html.erb b/app/views/songs/_song.html.erb index 44dbf54c..59856143 100644 --- a/app/views/songs/_song.html.erb +++ b/app/views/songs/_song.html.erb @@ -16,10 +16,10 @@ <% end %>
- <%= link_to song.artist.title, artist_path(song.artist) %> + <%= link_to song.artist.name, artist_path(song.artist) %>
- <%= link_to song.album.title, album_path(song.album) %> + <%= link_to song.album.name, album_path(song.album) %>
<%= format_duration(song.duration) %>
diff --git a/db/migrate/20240103072942_add_default_name_to_unknown_albums.rb b/db/migrate/20240103072942_add_default_name_to_unknown_albums.rb new file mode 100644 index 00000000..fc3df65b --- /dev/null +++ b/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 diff --git a/db/migrate/20240103081109_change_name_null_on_albums.rb b/db/migrate/20240103081109_change_name_null_on_albums.rb new file mode 100644 index 00000000..7b597e5c --- /dev/null +++ b/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 diff --git a/db/migrate/20240103085953_add_default_name_to_artists.rb b/db/migrate/20240103085953_add_default_name_to_artists.rb new file mode 100644 index 00000000..bbdd1630 --- /dev/null +++ b/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 diff --git a/db/migrate/20240103090230_change_name_null_on_artists.rb b/db/migrate/20240103090230_change_name_null_on_artists.rb new file mode 100644 index 00000000..6678c224 --- /dev/null +++ b/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 diff --git a/db/migrate/20240104023417_rename_is_various_in_artists.rb b/db/migrate/20240104023417_rename_is_various_in_artists.rb new file mode 100644 index 00000000..6ce51a11 --- /dev/null +++ b/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 diff --git a/db/schema.rb b/db/schema.rb index 5032edd1..0e222b50 100644 --- a/db/schema.rb +++ b/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 diff --git a/test/controllers/albums_controller_test.rb b/test/controllers/albums_controller_test.rb index 86058ea3..3856c5df 100644 --- a/test/controllers/albums_controller_test.rb +++ b/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) diff --git a/test/controllers/artists_controller_test.rb b/test/controllers/artists_controller_test.rb index 1b2d97ae..2ca16703 100644 --- a/test/controllers/artists_controller_test.rb +++ b/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) diff --git a/test/fixtures/artists.yml b/test/fixtures/artists.yml index 817f3e38..b14202ff 100644 --- a/test/fixtures/artists.yml +++ b/test/fixtures/artists.yml @@ -8,5 +8,5 @@ artist2: various_artists: name: 'various_artists' - is_various: true + various: true created_at: 2023-01-03 diff --git a/test/models/album_test.rb b/test/models/album_test.rb index e5d125c1..e22f3e46 100644 --- a/test/models/album_test.rb +++ b/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 diff --git a/test/models/artist_test.rb b/test/models/artist_test.rb index 81709ebc..7d4c7453 100644 --- a/test/models/artist_test.rb +++ b/test/models/artist_test.rb @@ -3,12 +3,12 @@ require "test_helper" class ArtistTest < ActiveSupport::TestCase - test "should have default title when name is empty" do - assert_equal "Unknown Artist", Artist.create(name: nil).title + test "should have default name when name is empty" do + assert_equal "Unknown Artist", Artist.create(name: nil).name end - test "should have default title when is various artist" do - assert_equal "Various Artists", Artist.create(is_various: true).title + test "should have default name when is various artist" do + assert_equal "Various Artists", Artist.create(various: true).name end test "should get all albums" do diff --git a/test/models/media_test.rb b/test/models/media_test.rb index 571d164c..38fd0bb8 100644 --- a/test/models/media_test.rb +++ b/test/models/media_test.rb @@ -19,7 +19,7 @@ class MediaTest < ActiveSupport::TestCase test "should create associations between artists and albums" do assert_equal Album.where(name: %w[album1 album2]).ids.sort, Artist.find_by(name: "artist1").albums.ids.sort assert_equal Album.where(name: "album3").ids.sort, Artist.find_by(name: "artist2").albums.ids.sort - assert_equal Album.where(name: "album4").ids.sort, Artist.find_by(is_various: true).albums.ids.sort + assert_equal Album.where(name: "album4").ids.sort, Artist.find_by(various: true).albums.ids.sort end test "should create associations between albums and songs" do @@ -40,7 +40,7 @@ class MediaTest < ActiveSupport::TestCase assert_equal artist1_songs_ids, Artist.find_by(name: "artist1").songs.ids.sort assert_equal artist2_songs_ids, Artist.find_by(name: "artist2").songs.ids.sort - assert_equal [], Artist.find_by(is_various: true).songs.ids.sort + assert_equal [], Artist.find_by(various: true).songs.ids.sort end test "should change associations when modify album info on file" do From 1ca6d07dd6d34af3f003aa32816c86fcf23a8c4c Mon Sep 17 00:00:00 2001 From: aidewoode Date: Thu, 4 Jan 2024 16:40:33 +0800 Subject: [PATCH 2/3] Fix style issue of song list in album of various artist page --- app/assets/stylesheets/components/_action_bar.scss | 2 +- app/assets/stylesheets/components/_button.scss | 4 ++++ app/assets/stylesheets/components/_list.scss | 6 ++++++ app/assets/stylesheets/elements/_content.scss | 2 +- app/assets/stylesheets/settings/_dark_theme.scss | 1 + app/assets/stylesheets/settings/_light_theme.scss | 1 + app/javascript/controllers/mixins/event_handler.js | 2 ++ app/views/albums/show.html.erb | 4 ++-- app/views/current_playlist/songs/_song.html.erb | 4 ++-- 9 files changed, 20 insertions(+), 6 deletions(-) diff --git a/app/assets/stylesheets/components/_action_bar.scss b/app/assets/stylesheets/components/_action_bar.scss index 8ab99e05..bb2950c5 100644 --- a/app/assets/stylesheets/components/_action_bar.scss +++ b/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); } diff --git a/app/assets/stylesheets/components/_button.scss b/app/assets/stylesheets/components/_button.scss index 0f061a45..01a441a4 100644 --- a/app/assets/stylesheets/components/_button.scss +++ b/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 { diff --git a/app/assets/stylesheets/components/_list.scss b/app/assets/stylesheets/components/_list.scss index c4b386ce..19f3227f 100644 --- a/app/assets/stylesheets/components/_list.scss +++ b/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; } diff --git a/app/assets/stylesheets/elements/_content.scss b/app/assets/stylesheets/elements/_content.scss index d6cd65ed..72d63ea9 100644 --- a/app/assets/stylesheets/elements/_content.scss +++ b/app/assets/stylesheets/elements/_content.scss @@ -37,7 +37,7 @@ a { } a:hover { - color: var(--link-active-color); + text-decoration: underline; } a.is-active { diff --git a/app/assets/stylesheets/settings/_dark_theme.scss b/app/assets/stylesheets/settings/_dark_theme.scss index ae0c85e6..dc157258 100644 --- a/app/assets/stylesheets/settings/_dark_theme.scss +++ b/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}; diff --git a/app/assets/stylesheets/settings/_light_theme.scss b/app/assets/stylesheets/settings/_light_theme.scss index 37263f2e..0e30a2bc 100644 --- a/app/assets/stylesheets/settings/_light_theme.scss +++ b/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}; diff --git a/app/javascript/controllers/mixins/event_handler.js b/app/javascript/controllers/mixins/event_handler.js index e3205889..0cacf2f4 100644 --- a/app/javascript/controllers/mixins/event_handler.js +++ b/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 } } diff --git a/app/views/albums/show.html.erb b/app/views/albums/show.html.erb index 45a3059f..9fde0849 100644 --- a/app/views/albums/show.html.erb +++ b/app/views/albums/show.html.erb @@ -57,9 +57,9 @@ ) do %>
- <%= song.name %> +
<%= song.name %>
<% if @album.artist.various? %> - <%= song.artist.name %> + <%= link_to song.artist.name, artist_path(song.artist), class: "c-list__item__subtext" %> <% end %>
<%= format_duration(song.duration) %>
diff --git a/app/views/current_playlist/songs/_song.html.erb b/app/views/current_playlist/songs/_song.html.erb index 8594d2c7..13b83652 100644 --- a/app/views/current_playlist/songs/_song.html.erb +++ b/app/views/current_playlist/songs/_song.html.erb @@ -15,8 +15,8 @@