KSP-SpaceDock / SpaceDock

Current Codebase (Python /Flask)
https://spacedock.info
Other
73 stars 33 forks source link

Add game ID filter #466

Closed RedstoneWizard08 closed 1 year ago

RedstoneWizard08 commented 1 year ago

Fixes #465

V1TA5 commented 1 year ago

This should be available in all sub paths of browse like "New"

RedstoneWizard08 commented 1 year ago

Okay, I'll fix that too.

V1TA5 commented 1 year ago

Thank you :)

HebaruSan commented 1 year ago

Now that we're looking at this closely, I strongly suspect these old lines don't do anything (and therefore the order and orderby params are broken):

https://github.com/KSP-SpaceDock/SpaceDock/blob/dbd67221961376193983028c8977c51b5296204d/KerbalStuff/blueprints/api.py#L325-L328

Just calling order_by doesn't change the query, we would need to overwrite the mods variable like:

    if order == "desc":
        mods = mods.order_by(desc(orderby))
    else:
        mods = mods.order_by(asc(orderby))

Maybe you could confirm this with the client that you're testing. We might as well fix that while we're touching this code.

RedstoneWizard08 commented 1 year ago

@HebaruSan I think that /api/browse/new is the only one that I can implement that for. The other ones don't use the same API that it does, which is directly querying the database.

RedstoneWizard08 commented 1 year ago

Now that we're looking at this closely, I strongly suspect these old lines don't do anything (and therefore the order and orderby params are broken):

https://github.com/KSP-SpaceDock/SpaceDock/blob/dbd67221961376193983028c8977c51b5296204d/KerbalStuff/blueprints/api.py#L325-L328

Just calling order_by doesn't change the query, we would need to overwrite the mods variable like:

    if order == "desc":
        mods = mods.order_by(desc(orderby))
    else:
        mods = mods.order_by(asc(orderby))

Maybe you could confirm this with the client that you're testing. We might as well fix that while we're touching this code.

Yeah it doesn't change anything rn. I'll fix that too.

V1TA5 commented 1 year ago

Your on fire. Tell me when youre satisfied with the state and i should merge it.

RedstoneWizard08 commented 1 year ago

I'm satisfied rn lol

RedstoneWizard08 commented 1 year ago

This is good enough for what I need atm, so I think it's good!

HebaruSan commented 1 year ago

The one remaining comment that I had was that the game_version parameter might be clearer as game_version_id so the user of the API knows it's not something like "0.1.0.0" but a database number.

Also we forgot to update the docs:

https://github.com/KSP-SpaceDock/SpaceDock/blob/master/api.md#browse

HebaruSan commented 1 year ago

@RedstoneWizard08, @V1TA5, I'm going to address the concerns from my previous comment in a new PR (game_version_id as the param name, updating the API docs).

Confirmed the sorting fix is working:

And so is the new filter:

HebaruSan commented 1 year ago

Actually, the API currently doesn't expose the internal game version id at all, so it would not be possible for API users to use that parameter, so we should probably just switch game_version to matching GameVersion.friendly_version.

RedstoneWizard08 commented 1 year ago

The version should be, if you query /api/[game_id]/versions

HebaruSan commented 1 year ago

Ahh, that's fair. I was looking at /api/browse itself, where we just get versions[].game_version as GameVersion.friendly_version:

{"friendly_version":"1.7","game_version":"1.10.0","id":24,"created":"2020-07-28T18:03:39.568092+00:00","download_path":"/mod/5/AwesomeMod/download/1.7","changelog":"","downloads":47}

In that case, maybe we should support both.