Skip to content

AO3-6811 Fix tag nomination parented bug#5845

Open
pmonfort wants to merge 2 commits into
otwcode:masterfrom
pmonfort:AO3-6811
Open

AO3-6811 Fix tag nomination parented bug#5845
pmonfort wants to merge 2 commits into
otwcode:masterfrom
pmonfort:AO3-6811

Conversation

@pmonfort

Copy link
Copy Markdown
Contributor

Pull Request Checklist

Issue

https://otwarchive.atlassian.net/browse/AO3-6811

Purpose

When a canonical tag is saved, Tag#update_tag_nominations sets parented: true on matching TagNomination records.
However, it only checked for Fandom-type parents and only matched nominations with a blank parent_tagname (empty string).
This caused nominations to be incorrectly marked as parented: false when:

  • The tag's parent was a non-Fandom type (e.g., a Media parent on a Fandom tag).
  • The nomination had a nil parent_tagname instead of an empty string.

This PR fixes both issues by:

  1. Checking all parent types instead of only Fandoms.
  2. Including both "" and nil in the parent_names list so nominations with either blank value are matched.

Testing Instructions

  1. Create a canonical fandom tag with a Media parent (but no Fandom parent).
  2. Create a tag nomination for that fandom in a tag set.
  3. Save the fandom tag.
  4. Verify the nomination's parented field is true.

Credit

Pablo Monfort (he/him)

@sarken sarken self-requested a review June 2, 2026 03:04
@sarken

sarken commented Jun 2, 2026

Copy link
Copy Markdown
Collaborator

Assigning myself as reviewer since I saw some things I want to poke at, but I am beat so that's not gonna happen tonight

@sarken sarken left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for working on this! I've updated the Jira issue with new testing steps and just have one question about this code.

Comment thread app/models/tag.rb Outdated
# Calculate the parents associated with this tag, because we'll set any
# TagNominations with a matching parent_tagname to have parented: true.
parent_names = parents.where(type: "Fandom").pluck(:name)
parent_names = parents.pluck(:name)

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure this is necessary. A nomination's parent_tagname should always be the name of a fandom unless the nominated tag itself is a fandom. For fandom nominations, it seems like the parent_tagname is always nil, never the name of a media tag. I took a look at staging to confirm:

3.4.6 :001 > noms = FandomNomination.where("parent_tagname IS NOT NULL") ; nil
 => nil 
3.4.6 :002 > noms.size
 => 0 
3.4.6 :003 > noms = FandomNomination.where(parent_tagname: "") ; nil
 => nil 
3.4.6 :004 > noms.size
 => 0 
3.4.6 :005 > noms = FandomNomination.where(parent_tagname: nil) ; nil
 => nil 
3.4.6 :006 > noms.size
 => 21488 
3.4.6 :007 > FandomNomination.all.count
 => 21488

Can you think of a scenario in which we'd expect to find a tag nomination with a non-fandom, non-blank, and non-nil parent_tagname?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You're right that fandom nominations always have parent_tagname: nil and the media parent name itself won't directly match any nomination. But after digging deeper, I think removing the type filter is still needed, looking at https://github.com/otwcode/otwarchive/blob/master/app/models/fandom.rb#L44 a Fandom's parent_types are either Media or MetaTag which means parents.where(type: "Fandom") would always return [] for any fandom tag. So parent_names.present? would always be false, nil would never get added, and the update_all would match nothing, leaving the nominations as parented: false

I admit the fix is a bit indirect though, the media parent names in the list won't match anything, but they make parent_names non-empty so that nil gets added on line 1259, which is what actually matches the fandom nominations.

Maybe this would be a cleaner approach.

if canonical? && parents.exists?
  parent_names = parents.where(type: "Fandom").pluck(:name).push("", nil)
  TagNomination.where(tagname: name, parent_tagname: parent_names).update_all(parented: true)
end

@sarken thanks for you review! Let me know your thoughts

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, that's more like what I was thinking! Basically, I just wanted us to narrow down the list of parent_names to ones that were actually useful (so fandom names, nil, and blank) -- no point looking for nominations with certain media tags or character tags as parents, since we know we won't find them.

Just as an aside, it looks like parent_types and the corresponding child_types method are misleadingly named 😬 tag.parents and tag.children are about the association between two tags that's created through CommonTaggings. A tag's parents and children will only ever be tags, which can only have these types:

TYPES = ['Rating', 'ArchiveWarning', 'Category', 'Media', 'Fandom', 'Relationship', 'Character', 'Freeform', 'Banned' ]

The parent_types and child_types methods include things that aren't true tag types at all, like metatags, subtags, and mergers. We should probably discuss renaming them to something like parent_association_types/child_association_types at some point, especially since I see a few places in the code that assume it only means the types of tags that can be related to through CommonTaggings...

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks! I've updated the code to use the cleaner approach.
Agreed on the parent_types/child_types naming, I would have expected those to be related to parent and child tag types, thanks for the clarification!

@pmonfort pmonfort requested a review from sarken June 7, 2026 00:49
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants