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

[ROMM-994] Autogenerated collections #1562

Open
wants to merge 22 commits into
base: master
Choose a base branch
from
Open

[ROMM-994] Autogenerated collections #1562

wants to merge 22 commits into from

Conversation

gantoine
Copy link
Member

@gantoine gantoine commented Jan 29, 2025

Description

Very excited about this one! This PR creates a virtual database table that collects and groups games based on certain IGDB metadata fields: collections, franchises, genres, companies and modes. The list of virtual VirtualCollections can be fetched and filtered by type, and displayed in the home screen and collections sidebar. Editing these virtual collections is not possible, so controls like the collection editor window are hidden.

As we add more metadata providers, we can update the virtual table to pull properties outside igdb_metadata.

TODO

  • Test migration with postgresql

Related Issues

Closes #994

Checklist

Please check all that apply.

  • I've tested the changes locally
  • I've updated the wiki accordingly
  • I've have updated relevant comments
  • I've assigned reviewers for this PR
  • I've added unit tests that cover the changes
  • All existing tests are passing

Screenshots

Screenshot 2025-01-28 at 7 00 51 PMScreenshot 2025-01-28 at 7 04 04 PM Screenshot 2025-01-28 at 9 52 22 PM

Copy link

github-actions bot commented Jan 29, 2025

Test Results

93 tests  ±0   93 ✅ ±0   27s ⏱️ -1s
 1 suites ±0    0 💤 ±0 
 1 files   ±0    0 ❌ ±0 

Results for commit 75eb037. ± Comparison against base commit b70887c.

♻️ This comment has been updated with latest results.

@zurdi15 zurdi15 closed this Jan 30, 2025
@zurdi15 zurdi15 deleted the romm-994 branch January 30, 2025 22:47
@zurdi15 zurdi15 restored the romm-994 branch January 30, 2025 22:49
@zurdi15 zurdi15 reopened this Jan 30, 2025
@gantoine gantoine marked this pull request as ready for review February 9, 2025 20:21
SELECT * FROM companies_collection
) combined
GROUP BY collection_type, collection_name
HAVING COUNT(DISTINCT rom_id) > 2
Copy link
Collaborator

Choose a reason for hiding this comment

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

Should this be > 1 or >= 2 instead?

Copy link
Member Author

Choose a reason for hiding this comment

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

imo 2 games don't make a collection, so i set the minimum to 3 games

SELECT * FROM companies
) combined
GROUP BY collection_type, collection_name
HAVING COUNT(DISTINCT rom_id) > 2
Copy link
Collaborator

Choose a reason for hiding this comment

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

Same here.

Copy link
Member Author

Choose a reason for hiding this comment

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

same answer!

backend/endpoints/collections.py Outdated Show resolved Hide resolved
backend/endpoints/collections.py Outdated Show resolved Hide resolved
Comment on lines +239 to +242
JSON_ARRAYAGG(DISTINCT rom_id) as rom_ids,
JSON_ARRAYAGG(DISTINCT path_cover_s) as path_covers_s,
JSON_ARRAYAGG(DISTINCT path_cover_l) as path_covers_l
FROM
Copy link
Collaborator

Choose a reason for hiding this comment

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

Should these three fields always match their orderings? e.g. Second rom_id corresponds with second element for the cover paths in the other arrays.

If that's the case, I think we should enforce sorting using something like:

                    JSON_ARRAYAGG(DISTINCT rom_id ORDER BY rom_id) as rom_ids,
                    JSON_ARRAYAGG(DISTINCT path_cover_s ORDER BY rom_id) as path_covers_s,
                    JSON_ARRAYAGG(DISTINCT path_cover_l ORDER BY rom_id) as path_covers_l

Copy link
Member Author

@gantoine gantoine Feb 10, 2025

Choose a reason for hiding this comment

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

the magic here is that it'll always be in order, even without ORDER BY!
the duplicates here are a postgres thing, but don't affect anything
Screenshot 2025-02-09 at 8 36 17 PM

Comment on lines +118 to +120
array_to_json(array_agg(rom_id)) as rom_ids,
array_to_json(array_agg(path_cover_s)) as path_covers_s,
array_to_json(array_agg(path_cover_l)) as path_covers_l
Copy link
Collaborator

Choose a reason for hiding this comment

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

This differs from MySQL, where we use DISTINCT within the aggregation. I don't think that's needed as I don't see a way for CTEs to repeat rom_id values. We should have the same logic for both engines.

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 can't seem to figure out how to DISTINCT in postgres like i did in mysql 🤷🏼

@gantoine gantoine requested a review from adamantike February 10, 2025 01:39
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants