IMG-84: Event Art Archive #3150

mwiencek · 2024-01-23T18:14:07Z

IMG-84 - Event Art Archive

Summary

This adds support for uploading evert artwork images, like posters, to the Event Art Archive through MusicBrainz Server. Event artwork images can be requested via eventartarchive.org in the same way as coverartarchive.org.

It copies the CAA implementation. I've done my best to factor out shared functionality into roles. There is a small amount of duplication remaining, particularly in the .tt files, but these use so many conflicting strings that parameterizing them would prove to be a pain (and is perhaps better left until after they are converted to React).

One thing that is left unimplemented is exposing event-art-archive metadata through the MusicBrainz web service; but I would prefer that that be done as a later project, and that we allow uploads first. Note that this is unrelated to the artwork-redirect API exposed through eventartarchive.org, which will work as expected.

Testing

I've copied the existing CAA tests (both Perl and Selenium) to new EAA variants. The Selenium test submits each of the four edit types (add, edit, remove, reorder) through the web interface.

Documenting

I haven't made any updates to documentation, but the following pages should likely be cloned or updated to mention event art:

https://wiki.musicbrainz.org/Event
https://wiki.musicbrainz.org/Cover_Art/Types
https://wiki.musicbrainz.org/How_to_Add_Cover_Art
https://wiki.musicbrainz.org/Cover_Art
https://wiki.musicbrainz.org/Cover_Art_Archive/API

Further action

Add display placeholder for EAA edits in production #3271 must be deployed to production first.
Any stats that look at the current CAA queue size must be updated to read this from PostgreSQL instead.

reosarevok

Did not check everything bit by bit but did look through all commits, leaving some comments. Is it possible to throw this up on test with a test EAA instance for manual testing?

root/components/ArtLinks.js

lib/MusicBrainz/Server/Data/CoverArtArchive.pm

lib/MusicBrainz/Server/Entity/Artwork.pm

reosarevok · 2024-01-24T09:37:11Z

lib/MusicBrainz/Server/Data/Statistics.pm

+    'count.event.eventart.eaa' => {
+        DESC => 'Count of events with artwork',
+        SQL => 'SELECT COUNT(distinct event) FROM event_art_archive.event_art ea
+                  JOIN event_art_archive.event_art_type eat ON ea.id = eat.id
+                WHERE eat.type_id = 1',
+    },


I understand this is named to match the current release stat but that name is only because we used to have several types - is eaa needed here? Especially if you're not adding a none alternative.

Right, I'll remove that. I'm also missing these keys from root/statistics/stats.js, so I'll add them there too. Other statistics-related things I haven't done, but maybe can wait until later (?):

Adding count.eventart as a default line to the timeline (not sure about this yet?).

Adding a page like root/statistics/CoverArt.js.

We could also rename the page Artwork or something and use it for both, if that seems better.
Similarly, I'd probably just rename "Cover art" to "Art", "Artwork", "images" or whatnot on the timeline and put "Pieces of event art" and "Events with art" under the same header, I think?

We could also rename the page Artwork or something and use it for both, if that seems better.

I renamed the page to "Images" and added two of the event art stats to the "Basics" section. I didn't add an "Events" section to the page (because we have nothing else to go there).

Similarly, I'd probably just rename "Cover art" to "Art", "Artwork", "images" or whatnot on the timeline and put "Pieces of event art" and "Events with art" under the same header, I think?

For this I added a separate "Event art" category, because I believe renaming the cover-art category would break existing links.

reosarevok · 2024-01-24T09:39:45Z

lib/MusicBrainz/Server/EditRegistry.pm

    MusicBrainz::Server::Edit::Event::DeleteAlias
+    MusicBrainz::Server::Edit::Event::EditEventArt


The alphabetical order here is dodgy AF :)

Looks like the entities are sorted alphabetically, but then it's more pseudo-alphabetical-with-categories-considered...e.g., AddAlias, DeleteAlias, and EditAlias for an entity might appear next to each other, which makes some sense at least. But there are exceptions to that, too. :)

root/components/EntityTabs.js

root/edit/components/EditArtwork.js

reosarevok · 2024-01-24T10:01:45Z

root/edit/details/ReorderArt.js

+  if (entityType === 'event') {
+    historyMessage = expand2html(
+      l(`We are unable to display history for this event
+         art. For a current listing of evebt art, please see the


evebt :)

More generally, I wonder if we could share the string somehow.

(Ignore my previous question then. :))

What about something like this?

We are unable to display history for this piece of artwork. For what's available, see the {artpage|current listing of artwork}.

Maybe? :) If @yvanzo and @Aerozol are also ok with it. I think specifying "the release" / "the event" doesn't help a lot here, anyway, but I guess just changing it with "the entity" might be too technical?

I'm assuming the context this is displayed in shows the event/release name somewhere on the page? In which case I'm fine with leaving it out.

Otherwise some tweaks could make it work:

We are unable to display the history for this artwork*. See the {artpage|current artwork} for this {entitytype}.

p.s. If I remember correctly we were trying to avoid using 'entity' in user-facing situations?
p.p.s. perhaps 'file' instead of artwork, but that might be opening a can of worms elsewhere

That tweak would probably translate very badly in languages like Estonian that require the entity type name to be inflexed / suffixed in some way.

True! In any case, I think it can probably be left simple. I can't think of a scenario where someone is lost and desperately needs to know if it's a event or event image without clicking the link or checking the URL or header or something (as I type this I slowly become more sure that this statement will bite me in the ass, but whatever)

Maybe it could still be shortened, but I don't think it's a big deal if not

We are unable to display the history for this artwork. See all {artpage|current artwork}.

I went with:

l(`We are unable to display history for this piece of artwork. See {artpage|all current artwork}.`),

So that the first sentence matches the existing string, l('We are unable to display history for this piece of artwork.') below. We could change both if you much prefer the shorter sentence, though.

reosarevok · 2024-01-24T10:15:48Z

root/layout/components/sidebar/EventSidebar.js

-        cachedImage={$c.stash.commons_image}
-        entity={event}
-      />
+      {(eventArtPresence === 'present' || !$c.stash.commons_image) ? (


Sigh. Is it time to drop the CommonsImage code?

Yes, I wondered the same, sadly (but not for this PR, so I kept it on life support here :)).

mwiencek · 2024-01-24T17:04:15Z

root/statistics/stats.js

+  'count.event.eventart': {
+    category: 'event-art',
+    color: '#dd2200',
+    label: l_statistics('Events with EAA event art'),


"Events with event art archive event art" is definitely a sentence.

reosarevok · 2024-01-26T09:33:14Z

I just noticed this doesn't seem to change the Cover art provided by the {caa|Cover Art Archive}. footer string. Should it?

reosarevok

Issues found when testing on test.mb:

Types section is wrongly formatted, see comment.
Edits do not appear on the edit history for the event.
Cancelling an add edit (either intentionally, or by entering a remove when your own add is pending) causes an ISE.
ApeKattQuest also noticed that using accept on a remove edit causes an ISE.

root/event/add_event_art.tt

mwiencek · 2024-02-01T05:45:43Z

Types section is wrongly formatted, see comment.

If it was just the missing CSS, then it should be fixed now (but I didn't confirm).

Edits do not appear on the edit history for the event.

Fixed.

Cancelling an add edit (either intentionally, or by entering a remove when your own add is pending) causes an ISE.
ApeKattQuest also noticed that using accept on a remove edit causes an ISE.

Should also be fixed: I was able to approve https://test.musicbrainz.org/edit/100290814. It was actually not an MBS issue, but due to outdated artwork-indexer triggers.

mwiencek · 2024-02-01T05:55:20Z

I just noticed this doesn't seem to change the Cover art provided by the {caa|Cover Art Archive}. footer string. Should it?

I suppose it should. Should we just remove the CAA reference, since it's already mentioned on the art pages and elsewhere?

reosarevok · 2024-02-01T09:43:24Z

I checked with @mayhem and he said it's fine to just remove it from that footer.

reosarevok

Left a couple comments on the new stuff but it seems generally fine, and probably ready to put into beta this week.

lib/MusicBrainz/Server/Data/Statistics.pm

docker/ssssss/DBDefs.pm

t/lib/t/MusicBrainz/Server/Edit/Event/AddEventArt.pm

reosarevok · 2024-04-02T09:07:51Z

Why the account admin restriction? I assume it's just for testing since it says temporarily, but I'm still not sure of the rationale :)

Did you figure out the OOM error? If so, maybe we can put this on the next beta after releasing the current one.

mwiencek · 2024-04-02T16:53:39Z

Why the account admin restriction? I assume it's just for testing since it says temporarily, but I'm still not sure of the rationale :)

Just being slightly paranoid: I'd just like to make sure it actually works and doesn't make a mess at the IA before we actually enable it. I wasn't intending for there to be a full beta cycle with admin restrictions enabled, though; just long enough to perform some basic testing.

Did you figure out the OOM error? If so, maybe we can put this on the next beta after releasing the current one.

Yes, the OOM issue was fixed.

It was decided on IRC (in #metabrainz on 2024-02-01) to prefer "image" to "art" or "artwork."

"Item" is the terminology used at the IA; we could also use "entity" to refer to the MB entity in a generic way, but I'm not sure if it's any clearer.

"[A]re part of the" may be clearer, as the images are originally provided by users. Co-Authored-By: yvanzo <yvanzo@metabrainz.org>

…er.py A subcommand was added for setting up the schema. Executing the SQL files directly is no longer guaranteed to work correctly.

The `convert` command is needed by ssssss.psgi.

This is a prerequisite step for IMG-84 / PR metabrainz#3150. If you were to submit any of the new EAA edits on the beta server once it's deployed there, and then try to load any of those edits in production (whether directly or via an edit listing), then it would cause an ISE in production. Pulling all of the display code for these new edit types would basically mean having to merge a very large chunk of the event-art-archive branch, as they depend heavily on changes to the `Data::` and `Entity::` layers. This patch just adds a placeholder component, `BetaOnlyEdit`, which links to the edit on the beta server (saying that it can only be viewed there temporarily).

This is a prerequisite step for IMG-84 / PR metabrainz#3150. If you were to submit any of the new EAA edits on the beta server once it's deployed there, and then try to load any of those edits in production (whether directly or via an edit listing), then it would cause an ISE in production. Pulling all of the display code for these new edit types would basically mean having to merge a very large chunk of the event-art-archive branch, as they depend heavily on changes to the `Data::` and `Entity::` layers. This patch adds a placeholder component, `BetaOnlyEdit`, which can be used to link these EAA edits to the beta server (saying that they can only be viewed there temporarily). In a subsequent commit I will add in the machinery to actually make use of this component for the EAA edits specifically. It may also be useful for other new edit types we introduce in the future.

This is a prerequisite step for IMG-84 / PR metabrainz#3150. It adds temporary (stub) edit classes that don't do anything except display the type of edit. The core display of the edits is delegated to the `BetaOnlyEdit` component added in a8e12fd.

yvanzo

💯 👍 🥇 ✔️ 🏁 ⛵ 🛥️ 🚤 🚢 🚀 🌔 🪐

mwiencek force-pushed the event-art-archive branch 5 times, most recently from 4c67d5e to ae9ccf2 Compare January 24, 2024 06:32

reosarevok reviewed Jan 24, 2024

View reviewed changes

mwiencek force-pushed the event-art-archive branch from ae9ccf2 to 7db9988 Compare January 24, 2024 17:02

mwiencek commented Jan 24, 2024

View reviewed changes

mwiencek force-pushed the event-art-archive branch 3 times, most recently from 6c26688 to 678f8ce Compare January 25, 2024 20:36

mwiencek force-pushed the event-art-archive branch 2 times, most recently from 5af90eb to 30d17c1 Compare January 26, 2024 23:21

reosarevok requested changes Jan 31, 2024

View reviewed changes

root/event/add_event_art.tt Show resolved Hide resolved

mwiencek force-pushed the event-art-archive branch from 30d17c1 to 0d37b96 Compare February 1, 2024 05:38

mwiencek force-pushed the event-art-archive branch 4 times, most recently from 87f255e to 6971c61 Compare February 1, 2024 20:50

reosarevok reviewed Feb 12, 2024

View reviewed changes

lib/MusicBrainz/Server/Data/Statistics.pm Show resolved Hide resolved

docker/ssssss/DBDefs.pm Show resolved Hide resolved

t/lib/t/MusicBrainz/Server/Edit/Event/AddEventArt.pm Outdated Show resolved Hide resolved

mwiencek force-pushed the event-art-archive branch 4 times, most recently from daa69cb to 51c6ee1 Compare March 28, 2024 19:21

mwiencek added 10 commits May 22, 2024 13:58

Replace "Comment:" with existing "Comment" string

f090a12

Merge "(no other $entity art)" strings

5014472

It was decided on IRC (in #metabrainz on 2024-02-01) to prefer "image" to "art" or "artwork."

Merge CAA/EAA "experiencing difficulties" strings

678ae82

Merge CAA/EAA "takedown request" strings

a429e09

"Item" is the terminology used at the IA; we could also use "entity" to refer to the MB entity in a generic way, but I'm not sure if it's any clearer.

Merge CAA/EAA "can’t be uploaded" strings

115a159

Merge CAA/EAA "has been hidden" strings

03ed9a1

Merge CAA/EAA "failed to load correctly" strings

763f660

Merge CAA/EAA "Log in to upload" strings

82dfc4d

Use "images" in "unable to display history" string

ecb2070

Merge CAA/EAA "No ... art available" strings

94113d8

mwiencek force-pushed the event-art-archive branch from b4cbdab to 16dfc91 Compare May 22, 2024 18:59

mwiencek and others added 2 commits May 22, 2024 15:40

Change "images provided by" string for the CAA/EAA

43a66c3

"[A]re part of the" may be clearer, as the images are originally provided by users. Co-Authored-By: yvanzo <yvanzo@metabrainz.org>

Merge CAA/EAA "wish to remove" strings

63bfb4c

mwiencek force-pushed the event-art-archive branch from 16dfc91 to 7649896 Compare May 22, 2024 20:44

mwiencek added 2 commits May 22, 2024 23:27

Dockerfile.tests.m4: Bump ARTWORK_REDIRECT_COMMIT

cd47ed1

run_selenium_tests.sh: Install the artwork_indexer schema via index…

9a66aca

…er.py A subcommand was added for setting up the schema. Executing the SQL files directly is no longer guaranteed to work correctly.

mwiencek force-pushed the event-art-archive branch 3 times, most recently from 9bef854 to ea44279 Compare May 23, 2024 04:32

mwiencek added 2 commits May 23, 2024 01:02

Dockerfile.tests.m4: Bump ARTWORK_INDEXER_COMMIT

d46b9f5

Add imagemagick to selenium_caa_deps

68adc18

The `convert` command is needed by ssssss.psgi.

mwiencek force-pushed the event-art-archive branch from ea44279 to 68adc18 Compare May 23, 2024 06:16

mwiencek mentioned this pull request May 23, 2024

Add display placeholder for EAA edits in production #3271

Merged

yvanzo approved these changes May 23, 2024

View reviewed changes

mwiencek merged commit d4956de into metabrainz:master May 23, 2024
2 checks passed

mwiencek deleted the event-art-archive branch May 23, 2024 15:53

mwiencek mentioned this pull request May 24, 2024

Support separate CAA and EAA access keys #3275

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

IMG-84: Event Art Archive #3150

IMG-84: Event Art Archive #3150

mwiencek commented Jan 23, 2024 •

edited

reosarevok left a comment

reosarevok Jan 24, 2024

mwiencek Jan 24, 2024

reosarevok Jan 25, 2024

mwiencek Feb 1, 2024 •

edited

reosarevok Jan 24, 2024

mwiencek Jan 24, 2024

reosarevok Jan 24, 2024

mwiencek Jan 24, 2024

reosarevok Jan 25, 2024

Aerozol Jan 25, 2024 •

edited

reosarevok Jan 26, 2024

Aerozol Jan 26, 2024

mwiencek Feb 1, 2024

reosarevok Jan 24, 2024

mwiencek Jan 24, 2024

mwiencek Jan 24, 2024

reosarevok commented Jan 26, 2024

reosarevok left a comment •

edited

mwiencek commented Feb 1, 2024

mwiencek commented Feb 1, 2024

reosarevok commented Feb 1, 2024

reosarevok left a comment

reosarevok commented Apr 2, 2024

mwiencek commented Apr 2, 2024

yvanzo left a comment

		MusicBrainz::Server::Edit::Event::DeleteAlias
		MusicBrainz::Server::Edit::Event::EditEventArt

IMG-84: Event Art Archive #3150

IMG-84: Event Art Archive #3150

Conversation

mwiencek commented Jan 23, 2024 • edited

IMG-84 - Event Art Archive

Summary

Testing

Documenting

Further action

reosarevok left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mwiencek Feb 1, 2024 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Aerozol Jan 25, 2024 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

reosarevok commented Jan 26, 2024

reosarevok left a comment • edited

Choose a reason for hiding this comment

mwiencek commented Feb 1, 2024

mwiencek commented Feb 1, 2024

reosarevok commented Feb 1, 2024

reosarevok left a comment

Choose a reason for hiding this comment

reosarevok commented Apr 2, 2024

mwiencek commented Apr 2, 2024

yvanzo left a comment

Choose a reason for hiding this comment

mwiencek commented Jan 23, 2024 •

edited

mwiencek Feb 1, 2024 •

edited

Aerozol Jan 25, 2024 •

edited

reosarevok left a comment •

edited