Skip to content
This repository has been archived by the owner on Oct 19, 2024. It is now read-only.

Search test #731

Merged
merged 38 commits into from
Nov 12, 2023
Merged

Search test #731

merged 38 commits into from
Nov 12, 2023

Conversation

thesuzerain
Copy link
Contributor

No description provided.

migrations/20231005230721_dynamic-fields.sql Outdated Show resolved Hide resolved
migrations/20231005230721_dynamic-fields.sql Outdated Show resolved Hide resolved
src/routes/v2/admin.rs Outdated Show resolved Hide resolved
src/routes/v2/projects.rs Outdated Show resolved Hide resolved
src/routes/v2_reroute.rs Outdated Show resolved Hide resolved
}
}

pub async fn alter_actix_multipart(
Copy link
Contributor

Choose a reason for hiding this comment

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

Worth considering, not sure if it's worth doing - the methods in this file seem like great candidates for more detailed unit-level tests just around them, no integration. Seems like there are probably a lot of edge cases to be tested here, which can be done faster and more precisely than at the integration level

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah definitely. I think I want to come back to write a lot of tests around this area as I refactor it a little bit. Tbh Im not sure how I feel about the different ways to handle these rerouting functions so far, none of the ideas Ive run through have really felt 100% right

migrations/20231005230721_dynamic-fields.sql Outdated Show resolved Hide resolved
src/database/models/categories.rs Show resolved Hide resolved
sqlx-data.json Outdated Show resolved Hide resolved
migrations/20231005230721_dynamic-fields.sql Show resolved Hide resolved
src/database/models/project_item.rs Outdated Show resolved Hide resolved
src/models/v2/mod.rs Show resolved Hide resolved
src/routes/v2/admin.rs Show resolved Hide resolved
src/routes/updates.rs Outdated Show resolved Hide resolved
migrations/20231005230721_dynamic-fields.sql Outdated Show resolved Hide resolved
migrations/20231005230721_dynamic-fields.sql Show resolved Hide resolved

ALTER TABLE loaders ADD CONSTRAINT unique_loader_name UNIQUE (loader);

ALTER TABLE mods ADD COLUMN game_id integer REFERENCES games NOT NULL DEFAULT 1; -- all past ones are minecraft-java
Copy link
Member

Choose a reason for hiding this comment

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

this wasn't how I imagined this system would work- mainly bc the games a project supports is determined by the project types, which are determined by the loaders. So if a project had fabric and forge loaders, it would be a mod project type, and that wld be minecraft.

IMO we should probs get this merged so if possible we might just want to get rid of the game specific logic and add it to a later PR. LMK

ALTER TABLE loaders ADD CONSTRAINT unique_loader_name UNIQUE (loader);

ALTER TABLE mods ADD COLUMN game_id integer REFERENCES games NOT NULL DEFAULT 1; -- all past ones are minecraft-java
ALTER TABLE loaders ADD COLUMN game_id integer REFERENCES games NOT NULL DEFAULT 1; -- all past ones are minecraft-java
Copy link
Member

Choose a reason for hiding this comment

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

See above. Loaders technically aren't game specific, games are loader/project type specific

@@ -141,6 +142,7 @@ impl ModCategory {
#[derive(Clone)]
pub struct ProjectBuilder {
pub project_id: ProjectId,
pub game: Game,
Copy link
Member

Choose a reason for hiding this comment

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

see above note, don't think hame is actually needed here

@@ -141,6 +142,7 @@ impl ModCategory {
#[derive(Clone)]
pub struct ProjectBuilder {
pub project_id: ProjectId,
pub game: Game,
pub project_type_id: ProjectTypeId,
Copy link
Member

Choose a reason for hiding this comment

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

this column should also be able to be dropped, since project types will be an array inferred from loaders

@@ -14,3 +14,5 @@ pub mod sessions;
pub mod teams;
Copy link
Member

Choose a reason for hiding this comment

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

imo there should be a module for each api version, like v2 and v3. for ones unchanged in v2 -> v3, the v2 should just reexport the v3 file

Copy link
Member

Choose a reason for hiding this comment

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

see above comment, no need to reroute here

Copy link
Member

Choose a reason for hiding this comment

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

this is going to be a massive scope creep but bc we are drastically simplifying the project create route in v3 (basically making it only a shell of itself, this should stay as is and the v3 route should be simplified). ping me and I can explain more

Comment on lines +42 to +43
#[derive(Error, Debug)]
pub enum CreateError {
Copy link
Member

Choose a reason for hiding this comment

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

when you convert this you should just make this use the ApiError type, you could also do this for the v2 route if u want

"fabric_client_side": ["required"]
"fabric_server_side": ["optional"]
*/
pub loader_fields: HashMap<String, Vec<String>>,
Copy link
Member

Choose a reason for hiding this comment

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

since loader field names are unique I don't think you need to prefix anything here. Also don't see point of flatten

Copy link
Member

Choose a reason for hiding this comment

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

honestly I think you can just get rid of validators for now and just check for file extensions

@thesuzerain thesuzerain marked this pull request as ready for review November 7, 2023 01:51
Comment on lines 13 to 29
-- We create 'modpack' categories for every loader
-- That way we keep information like "this modpack is a fabric modpack"
INSERT INTO categories (category, project_type)
SELECT DISTINCT l.loader, pt.id FROM loaders l CROSS JOIN project_types pt WHERE pt.name = 'modpack' AND l.loader != 'mrpack';

-- insert the loader of every modpack mod as a category
INSERT INTO mods_categories (joining_mod_id, joining_category_id)
SELECT DISTINCT m.id, c.id
FROM mods m
LEFT JOIN versions v ON m.id = v.mod_id
LEFT JOIN loaders_versions lv ON v.id = lv.version_id
LEFT JOIN loaders l ON lv.loader_id = l.id
CROSS JOIN categories c
WHERE m.project_type = (SELECT id FROM project_types WHERE name = 'modpack') AND c.category = l.loader;

-- Non mrpack loaders no longer support modpacks
DELETE FROM loaders_project_types WHERE joining_loader_id != (SELECT id FROM loaders WHERE loader = 'mrpack') AND joining_project_type_id = (SELECT id FROM project_types WHERE name = 'modpack');
Copy link
Member

Choose a reason for hiding this comment

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

How come the loaders were made into categories? shouldn't they just be per-version dynamic fields?

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 this an exclusive v3 route? since it was added post v2-prod

Copy link
Member

Choose a reason for hiding this comment

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

same here- this file should be deleted and moved entirely to v3

Copy link
Member

Choose a reason for hiding this comment

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

orgs related routes in here should also be rerouted

* computer switch

* some fixes; github action

* added pr to master

* sqlx database setup

* switched intial GHA test db

* removed sqlx database setup

* unfinished patch route

* bug fixes + tests

* more tests, more fixes, cargo fmt

* merge fixes

* more tests, full reorganization

* fmt, clippy

* sqlx-data

* revs

* removed comments

* delete revs
@thesuzerain thesuzerain merged commit ae1c534 into master Nov 12, 2023
6 checks passed
@thesuzerain thesuzerain deleted the search-test branch November 13, 2023 18:25
thesuzerain added a commit that referenced this pull request Dec 5, 2023
* search patch for accurate loader/gv filtering

* backup

* basic search test

* finished test

* incomplete commit; backing up

* Working multipat reroute backup

* working rough draft v3

* most tests passing

* works

* search v2 conversion

* added some tags.rs v2 conversions

* Worked through warnings, unwraps, prints

* refactors

* new search test

* version files changes fixes

* redesign to revs

* removed old caches

* removed games

* fmt clippy

* merge conflicts

* fmt, prepare

* moved v2 routes over to v3

* fixes; tests passing

* project type changes

* moved files over

* fmt, clippy, prepare, etc

* loaders to loader_fields, added tests

* fmt, clippy, prepare

* fixed sorting bug

* reversed back- wrong order for consistency

* fmt; clippy; prepare

---------

Co-authored-by: Jai A <[email protected]>
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Development

Successfully merging this pull request may close these issues.

4 participants