Editing artwork in cover art database

[acabal]

Let's use this issue to coordinate editing existing cover art.

Both the submitter and anyone with the User::Benefits->CanReviewArtwork property should be able to edit artwork.

We can put a link to the edit form on the artwork's page.

The edit form will live at: /artworks/<ARTIST>/<TITLE>/edit

If the user isn't logged in, it should redirect to the login form. If the user doesn't have permission it should output HTTP 403.

It will have a form that issues a PUT request to /artworks/<ARTIST>/<TITLE>

Since the add and edit forms will be basically identical, maybe there's a way to combine the two using a template, so we don't have two identical forms in two different files.

Of course due to limitations of HTML forms, the update logic will live in /artworks/post.php.

The logic will be similar to the PATCH logic in there; but note how PATCH doesn't zero out missing fields, it merely sets them to the existing values. PUT should zero out nonexistant fields as we're replacing the object with a wholly new version, not editing a few specific fields.

Let me know if you need anything!

[colagrosso]

On it!

Thanks for the write-up, and thanks for all the other changes you've pushed in the last 2 days. /artworks/post.php is clearer now, plus there is no duplicate /admin/artworks/post.php, so it's clear where to put the PUT logic.

[acabal]

Thinking about the PUT logic, it's basically the same as POST except we save instead of create. Maybe there's a way to avoid duplicating all that code that sets the object properties for POST.

[colagrosso]

Thinking about the PUT logic, it's basically the same as POST except we save instead of create. Maybe there's a way to avoid duplicating all that code that sets the object properties for POST.

Yep, I'll find a way to not duplicate that code.

Another question:

As part of this issue, should I update this line:

https://github.com/standardebooks/web/blob/86af259f0bf3ca767ca9ff2dd02dc247304fae48/www/artworks/get.php#L14

to also allow the submitter to see their unverified submissions? Without a change there, they won't be able to get to the edit link.

(Even with a change there, the submitter will have to use their browser history to get back to their previous submissions. In a followup issue, we could discuss options for how to show a submitter all of their previous submissions.)

[acabal]

Yes, good idea. Make sure only the submitter can see, it can't be public yet.

[colagrosso]

Wait, the submitter won't have the full URL of the unverified artwork in their browser history. I'll have to think about this harder and try it out.

[acabal]

You can update Library::FilterArtwork() with a parameter to include the logged in user's submissions. We should actually update that function to be pure SQL and not do sorting/filtering in PHP. The Library::FilterEbooks() function has PHP-level sorting because it pulls from APCu, but the DB can do actual filtering and sorting itself which will be much faster and clearer.

[acabal]

FYI I made a small tweak to how tags are set on the Artwork object, make sure to pull from master before continuing.

[colagrosso]

Got it. I'll update Library::FilterArtwork().

Also got the changes to lib/Artwork.php and artworks/post.php. Nice use of the setter for tags!

[colagrosso]

Thanks for the feedback already on PR #316, and sorry it was in rough shape. Since users are eager for the feature and you've been active in the codebase, I thought it was worth sending it out early. I'll improve it tonight.

Two higher-level topics:

1. I have a new version of Library::FilterArtwork() as pure SQL in progress, but I'll send it out in a separate PR. It's pretty involved SQL because it joins Artworks, Artists, ArtistAlternateSpellings, and ArtworkTags. (The current code is no better, it just spreads out all the operations over multiple property lookups.)

Is it OK to delay the improvement to Library::FilterArtwork() until after editing is working? The improvement will be invisible to the user.

2. As you can see in #316, I haven't done any de-duplication work yet between artworks/new.php and artworks/edit.php. Similarly, I haven't yet done any de-duplication between the handling of HTTP POST and HTTP PUT in post.php. As I got into it, there were more differences than I expected. Here's the biggest one:

• When editing: As it stands now, we don't support submitting a new image to replace the existing one. We're not set up yet to generate thumbnails and do all the other file operations in Artwork::Save().

How strongly do you feel about letting users submit a new image when editing? It seems like to could be useful if the reviewer knows about a better copy of an image than the submitter. On the other hand, I don't think it's what users are asking for right now.

That one has a few consequences:

• We don't set MimeType when editing
• I decided to show the existing image on the edit page. This required a bit of CSS (that I messed up and will fix tonight).

There are a few other little differences, too. Overall, there will be code to share, but it won't be as much as I initially thought.

[acabal]

You can update FilterArtwork later, that's fine. It might make sense to create a summary column with a fulltext index for searching, so we don't have to do so many joins. Then again, not sure if that would be the best choice for searching by tag.

I don't think it would be too hard to pull out the image file operations into something like private Artwork::WriteThumbnails(string $path) and call that from both Create() and Save(). We can assume if the edit form does not submit an image, then we leave it unchanged. Therefore that form field should not be required, unlike the create form.

[acabal]

Also, I just added a special notes field to the artwork table, please pull from master and run:

ALTER TABLE `se`.`Artworks` 
ADD COLUMN `Notes` TEXT NULL DEFAULT NULL AFTER `Exception`;

[acabal]

FYI I just pushed an update that renames some constants (Removing COVER_ from artwork constants to be more consistent with actual naming) and converts the ARTWORK_STATUS_* constants an enum.

I'm trying out the new PHP8 enums to see. They feel a little unergonomic, and type hinting is a little tough with our getter/setter base class. I'm not sure yet if I want to continue converting other constants like SORT_* to enums. What do you think?

[acabal]

We've just had the case come up where someone wants to edit an image to add a different image file. So we definitely need that functionality for editing. The logic can be that if no image is present in the PUT operation, we assume the image is unchanged. If there is an image, we update it.

[colagrosso]

I'm trying out the new PHP8 enums to see. They feel a little unergonomic, and type hinting is a little tough with our getter/setter base class. I'm not sure yet if I want to continue converting other constants like SORT_* to enums. What do you think?

I like them in statements like:

if($artwork->Status == ArtworkStatus::Approved){ ... }

That reads better than the COVER_ constants. ArtworkStatus::Approved->value is a bit verbose, but OK.

We've just had the case come up where someone wants to edit an image to add a different image file. So we definitely need that functionality for editing. The logic can be that if no image is present in the PUT operation, we assume the image is unchanged. If there is an image, we update it.

Yep, done. I found a nice image of the ship of Theseus to test it with. I was able to submit the image and then edit all the fields, including the image itself, until none of the original fields were the same. One warning, though: My browser caches the artwork thumbnail and images such that I have to <shift>+refresh to see the updates. Without <shift>+refresh, it looks like the new image didn't work after editing. I don't think there's a way around this with our current naming system. Usually it's a good idea for the browser to cache images like:

/images/cover-uploads/16-thumb.jpg

Setting Cache-Control headers sounds like a pain. I'll look into if you test it and don't like <shift>+refresh after uploading a new image. I guess it could be confusing to other editors if they aren't expecting it.

316 is ready for review. I'll respond to the review comments so far over there.

[acabal]

When constructing the thumbnail path, append ?ts=<updated-timestamp> to the path. That way it the browser will fetch a new version when the updated timestamp changes.

[colagrosso]

Brilliant. I should have remembered that approach.

[colagrosso]

I think we're all set here, so I'm going to close this as completed. Here were my related PRs to allow editing and updating Library::FilterArtwork() to pure SQL:

• 316

• 321

• 323

• 324

• 325

• 329

plus many fixes Alex added. Thanks for all the help, Alex!