Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat(game): sync tags to the taggables table #2963

Merged

Conversation

wescopeland
Copy link
Member

@wescopeland wescopeland commented Dec 21, 2024

This PR begins the initial work of migrating tags (~Hack~, ~Homebrew~, ~Prototype~, etc) out of game titles and into structured data powered by spatie/laravel-tags.

There are two new behaviors included in this PR:

  • A new command has been added to perform the initial sync of structured tags from GameData.
  • Game title changes now double-write to structured tags.

How to test this PR

sail artisan ra:sync:legacy-game-tags 

After running the command, you should observe in your database that the tags and taggables tables are populated.

SELECT * FROM taggables WHERE taggable_type = "game" AND taggable_id = 1; -- returns 0 results

This should return 0 results.

Now, go change game ID 1's title to something like "~Hack~ Sonic the Hedgehog" and run the query again. You should get 1 result.

Later down the road, having tags as structured data will unlock several behaviors:

  • Improved sorting and filtering by game tags.
  • Internationalization of game tags.

This is also a precursor to migrating the beaten games leaderboard to React.

NOTE: A single game in your local snapshot probably has a tag of ~Multi~. This game does not have a set. In the production database, I have removed this tag. It is not handled by the command or action.

Another thing to keep in mind is spatie/laravel-tags localizes tags, and there is no way to disable this behavior:
Screenshot 2024-12-21 at 1 39 42 PM

We should probably only store "en" values in the database, and let the React UI do the presentation/localization work.

@wescopeland wescopeland added the deployment/command Includes an Artisan command that needs to be run before/during deployment label Dec 21, 2024
@wescopeland wescopeland requested a review from a team December 21, 2024 18:37
@Jamiras
Copy link
Member

Jamiras commented Dec 27, 2024

This should return 0 results.

What does the sail artisan ra:sync:legacy-game-tags command do if not populate the tags?

@wescopeland
Copy link
Member Author

taggable_id in that query is 1. Game ID 1 from any prod dump I'm aware of should not have any tags by default.

@Jamiras
Copy link
Member

Jamiras commented Dec 27, 2024

Yea, I figured that out. Couldn't get that query to return any results no matter how much I changed the name of a different game.

@@ -157,6 +160,17 @@ public static function boot()
}
}
}

// Double write to taggables for both new and updated games.
if ($originalTitle !== $freshGame->title || $game->wasRecentlyCreated) {
Copy link
Member

Choose a reason for hiding this comment

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

Why isn't the wasRecentlyCreated logic required for the WriteGameSortTitleFromGameTitleAction call? Shouldn't both dependencies always be updated at the same time?

Copy link
Member Author

Choose a reason for hiding this comment

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

In createNewGame(), we have this line:

$game->sort_title = (new ComputeGameSortTitleAction())->execute($title);

It's being optimistically written on create rather than written as the product of an event.

Copy link
Member Author

Choose a reason for hiding this comment

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

This does seem like a bit of an inconsistency. It seems like we have three possible paths to choose from:

  • Do nothing, keep things as-is.
  • Remove that line from createNewGame(), handle it in boot().
  • Keep that line in createNewGame(), and also handle tag syncing in that function too.

After weighing the pros & cons of each, I lean towards moving everything into boot(). What do you think?

Copy link
Member Author

Choose a reason for hiding this comment

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

I went with option 2. I've removed the optimistic write in createNewGame() so everything is now being handled in boot().

@wescopeland wescopeland merged commit b516c74 into RetroAchievements:master Dec 30, 2024
8 checks passed
@wescopeland wescopeland deleted the sync-legacy-game-tags branch December 30, 2024 15:50
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
deployment/command Includes an Artisan command that needs to be run before/during deployment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants