Add admin tool to handle duplicate artist names #480
Conversation
lib/Artist.php
Outdated
| @@ -151,6 +187,28 @@ public static function GetByAlternateUrlName(?string $urlName): Artist{ | |||
| ', [$urlName], Artist::class)[0] ?? throw new Exceptions\ArtistNotFoundException(); | |||
| } | |||
|
|
|||
| public function AddAlternateName(string $name): void{ | |||
| Db::Query(' | |||
There was a problem hiding this comment.
This can throw a duplicate key exception which we should have logic for - in case the user is accidentally adding an alias that already exists.
There was a problem hiding this comment.
I added logic to catch the duplicate key exception and show a useful error message to the user.
lib/Artist.php
Outdated
| SELECT * | ||
| from Artists | ||
| where Name = ? | ||
| ', [$name], Artist::class)[0] ?? throw new Exceptions\ArtistNotFoundException('Name: ' . $name); |
There was a problem hiding this comment.
I don't think we should override the default error message for this exception
lib/Artist.php
Outdated
| SELECT * | ||
| from Artists | ||
| where UrlName = ? | ||
| ', [$urlName], Artist::class)[0] ?? throw new Exceptions\ArtistNotFoundException('URL name: ' . $urlName); |
There was a problem hiding this comment.
Same. URL name is meaningless to anyone but us who are familiar with the system internals.
lib/Artist.php
Outdated
| public function Delete(): void{ | ||
| $artworkCount = count(Artwork::GetAllByArtist($this->UrlName, Enums\ArtworkFilterType::Admin, null /* submitterUserId */)); |
There was a problem hiding this comment.
We should not count like this because we're potentially instantiating a huge amount of objects just to get a count. Instead do a raw Db::QueryBool('select exists (select ...))') which will just return true on the first record match and then stop
|
|
||
| class ArtistHasArtworkException extends AppException{ | ||
| /** @var string $message */ | ||
| protected $message = 'Artist has artwork assoicated with it. Can’t delete.'; |
There was a problem hiding this comment.
Spelling error, also let's make this a complete sentence
There was a problem hiding this comment.
I took another whack at rewriting the error message.
www/artists/delete.php
Outdated
|
|
||
| $exception = HttpInput::SessionObject('exception', Exceptions\AppException::class); | ||
|
|
||
| $artist = Artist::GetByUrlName(HttpInput::Str(GET, 'artist-url-name') ?? ''); |
There was a problem hiding this comment.
I don't think we need ?? here because passing null will still throw an exception, but will also avoid a DB call
www/artists/delete.php
Outdated
| $exception = HttpInput::SessionObject('exception', Exceptions\AppException::class); | ||
|
|
||
| $artist = Artist::GetByUrlName(HttpInput::Str(GET, 'artist-url-name') ?? ''); | ||
| $artworkCount = count(Artwork::GetAllByArtist($artist->UrlName, Enums\ArtworkFilterType::Admin, null /* submitterUserId */)); |
There was a problem hiding this comment.
I don't think we need to calculate $artworkCount, it's a DB call and the number is not especially relevant to anything
www/artists/delete.php
Outdated
| <span>Reassign artwork by <?= Formatter::EscapeHtml($artist->Name) ?> to this artist. Total artworks to reassign: <?= $artworkCount ?></span> | ||
| <datalist id="artist-names-except-this-artist"> | ||
| <? foreach(Artist::GetAll() as $a){ ?> | ||
| <? if($a->ArtistId == $artist->ArtistId){ continue; } ?> |
There was a problem hiding this comment.
Instead of continue, test if $a->ArtistId != $artist->ArtistId so it's a more natural brace structure instead of a short circuit
www/artists/get.php
Outdated
|
|
||
| <? if($isAdminView){ ?> | ||
| <h2>Admin</h2> | ||
| <p><a href="<?= $artist->DeleteUrl ?>">Delete artist and reassign artwork</a></p> |
www/artists/post.php
Outdated
| if($httpMethod == Enums\HttpMethod::Delete){ | ||
| $artistToDelete = Artist::GetByUrlName(HttpInput::Str(GET, 'artist-url-name') ?? ''); | ||
| $exceptionRedirectUrl = $artistToDelete->DeleteUrl; | ||
| $artworkReassignedCount = count(Artwork::GetAllByArtist($artistToDelete->UrlName, Enums\ArtworkFilterType::Admin, null /* submitterUserId */)); |
|
Thanks for the quick review. I pushed the fixes as a separate commit so that they are easier to review. You can squash and merge them when we're done. |
|
Great work, thanks! |
Sorry for the delay.
Here's a video showing how it works:
simplescreenrecorder-2025-03-05_18.14.13.mp4
It works like we described in #419