AO3-6714 Fix duplicate tag set ownership on create

[tommilligan]

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

Purpose

This PR is based on #4863

This PR implements point 1. of the "What should happen" section of AO3-6714.

This PR prevents a OwnedTagSet being created with duplicate TagSetOwnership relationships. This happens when the currently logged in user explicitly adds themselves as an owner via the UI.

Notes

As this PR blocks creating tag sets with duplicate owners in the first place, the test case added in #4863 cannot be maintained without directly constructing an invalid state, with which to continue testing. I didn't do this; please let me know if you think it's required.

Testing Instructions

Manually follow the recreation steps in the issue. Verify that when adding your username as an explicit owner, the created tag set only lists the currently logged in username as a maintainer once.

References

N/A

Credit

What name and pronouns should we use to credit you in the Archive of Our Own's Release Notes?

Tom Milligan (they/them)

[brianjaustin]

Thank you! A couple of comments (mostly about tests) for you, but overall your approach looks reasonable to me.

On a more general note: I talked over the owners field with some of the other devs, and at some point we'd like to rework it so it's more like the tags field (where you can click an "X" to remove things instead of, somewhat confusingly, adding the pseud to the list to mean it should be removed). If you'd like to tackle that now, great! But that definitely would add a lot more scope, so we are happy to put a Jira ticket in for that to be done later.

[brianjaustin]

I wasn't able to get the duplication to happen on edit, just on creation. I think it would be a good idea to replicate the steps to reproduce from the bug ticket, which also triggers the duplication behaviour while creating the tag set

[tommilligan]

I'm not sure what you mean here - the steps to reproduce the duplication are here, where we specify an additional owner on creation who is the same as the logged in user: https://github.com/otwcode/otwarchive/pull/4864/files#diff-7aa40c6f60753ed081b2a3c186f775734fc427fdc9adcb714b424ff5007e8e40R26

Possibly the step should be phrased "additional owners" to make this clear?

    And I set up the tag set "Duplicate Ownership" with additional owners "tagsetter" and the freeform tags "Clones"

So I think this test is already reproducing the bug ticket.

You can verify that these setup steps would have reproduced the failure, by looking at this test from the first PR I submitted: 64092e8#diff-7aa40c6f60753ed081b2a3c186f775734fc427fdc9adcb714b424ff5007e8e40R28

[brianjaustin]

Ah, got it. Yes, I like that proposed wording, I think it makes things a bit clearer (especially given the current UX is Weird ™️)

[brianjaustin]

I'm thinking I'd prefer this to live in the model somewhere, but bouncing some ideas around right now (with a few clarifications I think the Jira story could use) so feel free to wait on any other changes until I have an answer for you on that

[brianjaustin]

Ok, I think we can do this with a model validation in https://github.com/otwcode/otwarchive/blob/1bdb4441e193ca269bb31f8c4a78ab5db7d1b663/app/models/tagset_models/tag_set_ownership.rb:

class TagSetOwnership < ApplicationRecord
  belongs_to :pseud
  belongs_to :owned_tag_set

  validates :pseud, uniqueness: { scope: :owned_tag_set_id }
end

(credit to @Bilka2 for the above). This will validate that the pseud is unique and only appears for moderator or owner, not both. The message can be taken care of with Rails I18n, similar to

otwarchive/config/locales/models/en.yml

Line 109 in dd6ec55

taken: You have already blocked that user.

(docs: https://github.com/otwcode/otwarchive/wiki/Internationalization-%28i18n%29-Standards#overriding-a-default-error-message); please let me know if additional examples would be helpful there.

With that validation in place, we won't need to worry about removing owners multiple times. So, instead of special-casing in the model, could you write a one-time Rake task to find and fix any duplicate owners?

Sorry for the wall of text, please don't hesitate to let us know if you have any questions!