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

Expose endpoint for category filtering #70

Merged
merged 8 commits into from
Feb 20, 2024
Merged

Expose endpoint for category filtering #70

merged 8 commits into from
Feb 20, 2024

Conversation

ZoopOTheGoop
Copy link
Contributor

@ZoopOTheGoop ZoopOTheGoop commented Feb 8, 2024

This relies on snapd-rs to work, and adds a couple dependencies to make life less miserable.

As requested, this just looks up the queries on every call to vote or get_vote in the hope that
eventually all the categories will be populated.

Closes #60

@ZoopOTheGoop
Copy link
Contributor Author

BTW, this deletes the repeated protobuf include values in test in favor of just making pb pub from the main crate. This is not just a random refactor, it's because implementing to_kebab_case on Categories was the best option, and it would have been bad to maintain the same function in two places.

@ZoopOTheGoop
Copy link
Contributor Author

I'm stuck on the sql queries, another pair of eyes would help.

@matthew-hagemann
Copy link
Collaborator

I'm stuck on the sql queries, another pair of eyes would help.

Hi Zoe, what part are you stuck on?

@ZoopOTheGoop
Copy link
Contributor Author

ZoopOTheGoop commented Feb 9, 2024

so the update_category in the infrastructure.rs according to my tests is not writing the categories properly.

there also might be a problem with the chart filtering but its hard to tease apart since theyre so correlated.

Edit: actually, this is a snapd query issue, it appears to be very finnicky to get snaps by ID. The find endpoint is not working properly and the assertions doesn't work for every snap.

@ZoopOTheGoop ZoopOTheGoop force-pushed the category-expose branch 2 times, most recently from c9c052d to 5fdc03a Compare February 9, 2024 12:42
Copy link
Contributor

@tim-hm tim-hm left a comment

Choose a reason for hiding this comment

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

A few changes please. Can you also squash your commits so atomic features ... also try and keep tests as their own commit.

example.env Outdated Show resolved Hide resolved
sql/migrations/20240208071037_categories.up.sql Outdated Show resolved Hide resolved
src/features/user/infrastructure.rs Outdated Show resolved Hide resolved
sql/migrations/20240208071037_categories.down.sql Outdated Show resolved Hide resolved
proto/ratings_features_chart.proto Outdated Show resolved Hide resolved
src/features/chart/infrastructure.rs Outdated Show resolved Hide resolved
src/features/pb.rs Outdated Show resolved Hide resolved
src/features/pb.rs Outdated Show resolved Hide resolved
src/features/user/infrastructure.rs Outdated Show resolved Hide resolved
src/features/user/infrastructure.rs Outdated Show resolved Hide resolved
@ZoopOTheGoop ZoopOTheGoop force-pushed the category-expose branch 7 times, most recently from efcc672 to e99a8ed Compare February 15, 2024 08:45
@ZoopOTheGoop
Copy link
Contributor Author

ZoopOTheGoop commented Feb 15, 2024

Hmm, GHA is failing because Category is marked optional in the protobuf. This works locally, I wonder if we need to bump protoc in GHA?

E: Yup, The snap hasn't been updated since 2021, need to move to the deb.

@ZoopOTheGoop
Copy link
Contributor Author

Blocked on #77 because it's needed to get github actions to pass.

Zoe Spellman added 3 commits February 20, 2024 09:37
This foregos a SQL enum in favor of constraining
the input to the range of the protobuf enum,
this allows the same idea as an enum but
works around some limitations with `prost`
using `#[repr(i32)]` on its enums which messes
with `sqlx`'s derives.
@ZoopOTheGoop ZoopOTheGoop force-pushed the category-expose branch 2 times, most recently from f60de84 to 739f664 Compare February 20, 2024 17:44
@ZoopOTheGoop ZoopOTheGoop merged commit d47dd21 into dev Feb 20, 2024
3 of 5 checks passed
@ZoopOTheGoop ZoopOTheGoop deleted the category-expose branch February 20, 2024 17:52
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.

Extend topN/chart API to filter by category
3 participants