AO3-6810 Don't send "Automatically flagged as spam" email when hiding works manually

[EchoEkhi]

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-6810

Purpose

When manually hiding a work that has been marked as spam by Akismet or otherwise, don't send the "Automatically flagged as spam" email, send the "Your work has been hidden by the Policy & Abuse team" email.

Testing Instructions

See JIRA ticket.

Credit

EchoEkhi (He/him)

[Bilka2]

Please action the comments by reviewdog, thank you!

Edit: I removed the rest of my review comments - my alternative approach didn't work, but I think the current approach is also problematic because it's prone to someone forgetting to set the attribute.

[sarken]

Would it help if we put the hide_spam and bulk_review(ids) methods in charge of sending the admin_spam_work_notification email instead?

I thought notify_of_hiding would also need return if spam? && saved_change_to_spam? to go with that change, but the test seems to pass without it, so I'm not sure.

Speaking of tests, though, I think we should probably also test which emails are sent when using the bulk hiding option on the Moderated Works page. It looks like the current tests only cover the option on the work itself.

[EchoEkhi]

I don't think the return if spam? && saved_change_to_spam? works, since it's effectively a rising edge detector, and the decision to send which email is not dependent on whether if there was a rising edge (spam changed from false to true), instead it only depends on which button the admin clicks.

I initially chose the approach of a virtual attribute because the alternative of changing the notify_of_hiding function to not be a after_save hook and instead have individual controllers call either notify_of_hiding or notify_of_hiding_for_spam could result in a situation where if a coder makes a mistake and forgets to call either of these functions when hiding a work, no emails would be sent at all and the user would not be aware that anything had happened. Whereas if the coder makes the mistake of forgetting to set the virtual attribute, the wrong email would be sent, but at least there would be an email.

I have changed the virtual attribute to be internal to the model. Instead of setting the attribute, the coder calls the notify_of_hiding_for_spam to send the hidden-for-spam emails, and that function would also set the attribute to prevent the normal hidden-by-admin emails from being sent. This way it's not possible for a coder to make a mistake in the future and cause a situation where no emails are sent when the work is hidden since the hidden-by-admin email is sent by default.

[EchoEkhi]

ps. I think that test might be flaky

[EchoEkhi]

nevermind it was my fault, fixed it now

[brianjaustin]

A couple suggestions in addition to the Rubocop

[brianjaustin]

Thanks!