AO3-6779 AO3-7250 Log work tag and language edits by admins correctly

[slavalamp]

Pull Request Checklist

• Have you read "How to write the perfect pull request"?
• Have you read the contributing guidelines?
• Have you added tests for any changed functionality?
• Have you added the Jira issue number
  as the first thing in your pull request title (e.g. AO3-1234 Fix thing)
• Do you have fewer than 5 pull requests already open? If not, please wait
  until they are reviewed and merged before creating new pull requests.

Issue

https://otwarchive.atlassian.net/browse/AO3-6779
https://otwarchive.atlassian.net/browse/AO3-7250

Purpose

Improvements for logging of tag and language edits by admins (superadmins/PAC):

• When a work language is edited, it gets logged as an "edit language" action with old and new languages in the summary (instead of an "update tags" one with no language info)
• When an admin clicks the "Update" button without changing anything (AO3-6779), only previews the edits or tries to make an invalid edit (AO3-7250), no log entries get created

Credit

slavalamp

caitlin (she/her) for the work in #5665 (AO3-7250)

[slavalamp]

The ticket doesn't mention the scenario of both the work language and some tags being changed, even though that's technically possible, and I'm not sure what a good way to account for that would be, so I haven't tried doing that here

[lydia-theda]

I think the easiest/best solution would be to create two Activity entries (one for Update Tags, one for Edit Language).

However, there is a bug in which update_tag entries are created if an admin Previews then Cancels a tag edit or if they click Update without changing anything [neither action should log anything in Activities]. I don't know if trying to implement the simultaneous entries for "changing tags plus language" check here would also require you to fix that too.

[slavalamp]

However, there is a bug in which update_tag entries are created if an admin Previews then Cancels a tag edit or if they click Update without changing anything [neither action should log anything in Activities]. I don't know if trying to implement the simultaneous entries for "changing tags plus language" check here would also require you to fix that too.

It would require fixing the updating-without-changing bug but not the preview one (they're actually separate issues), but it should be easy to fix the preview one too. I don't mind doing both, I feel like it would make sense to fix these all together... Neither of these two bugs you mentioned are logged on Jira right now though

[slavalamp]

Checking if tags got changed would also make it easy to log which exactly tag types got changed btw, I'm guessing that's an improvement that might be wanted

[slavalamp]

Right, it also logs it if an admin tries to make an invalid edit that doesn't actually get saved (e.g. removing all fandoms from a work)

That's caused by the same thing as the preview one, but fixing just the preview logging would be a one-line fix, this is a bit more complicated than that

[slavalamp]

i changed the big if clause for a return unless because it was annoying me and now the diff looks weird

not adding more tests yet in case i need to revert anything + i need to take a break now anyway

edit: oops also just saw that broke other things, gonna just revert that commit completely then

[sarken]

Since failing to include address edits to both tags and languages at once was my oversight when creating AO3-6779, I've updated it to cover:

• tags and languages are both edited
• blank edits
• invalid edits (although imo if we don't have to fix this like we do with blank edits, we can leave it for a separate issue)

We just need to make a separate issue for preview/cancel, since it's unrelated. (I'd leave that one for a separate pull request to avoid scope creep, especially if we need to address invalid edits.)

[slavalamp]

invalid edits (although imo if we don't have to fix this like we do with blank edits, we can leave it for a separate issue)

confirming we don't

invalid edits + previewing make sense to be one issue yeah, since the problem is that admin logs are created before the code checks if the edit is not a preview and valid

i was thinking of preventing just the previews from getting logged here because it should be just one line and probably creates annoyances for admins, then it could be changed to a nicer fix with the later issue, but yeah scope creep fair enough

---

(marking as draft since this is definitely incomplete now)

[sarken]

Got it -- I've taken the invalid edit bit out of AO3-6779 and made AO3-7250!

[marcus8448]

I'd prefer to not have manual comparison against the request parameters to check if the tags were changed. Could the activity logging be moved to after the work is updated? Then the saved_change_to_* methods could be used instead.

This approach might also happen to fix AO3-7250. If it does, you're welcome to claim it and just add some tests here.

[slavalamp]

yeah this does also solve AO3-7250, i'll ask caitlin if she's ok with me also taking the rest of her code here so there's only one pr to review

also gonna completely remove that if params[:action] == "update_tags" clause actually, it's only run when admins update tags anyway

[slavalamp]

i'm not loving this and i think the update_tags action in admin logs should just be renamed to either "edit tags" or "update tags", but this way it should mirror the changes in AO3-7336 (#5679)

[not-varram]

@slavalamp I don't understand what you're trying to say fully. so sorry if I'm misinterpreting- But If you think my PR Will be duplicate/your PR accomplishes everything mine will. Then I'm down to just pull my PR down and let this go through instead :)

Don't know exactly how ao3 staff would do this in Jira though xp

[slavalamp]

@not-varram no, your pr is fine and different, i just had to keep in mind that you're changing the update_tags action to update because i'm also changing some of the same code, to make merging easier

[caitlinkwan]

yeah this does also solve AO3-7250, i'll ask caitlin if she's ok with me also taking the rest of her code here so there's only one pr to review

Yes, works for me! I'll close my PR then - the combined changes look good to me.

[marcus8448]

Could you add a test to cover 7250's "cancelled preview of a valid edit" case? Everything else looks good.

[marcus8448]

Thank you slavalamp and caitlin!