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

Add saved searches APIs. #121

Merged
merged 2 commits into from
Jun 27, 2024
Merged

Add saved searches APIs. #121

merged 2 commits into from
Jun 27, 2024

Conversation

richardxia
Copy link
Member

This implements the list, get, post, and delete routes. It omits the put route for now, since we might not actually need it yet, and it will probably require some significant refactoring.

This one's a bit more complicated than the others, since we have a JSONB column for the search query information. On the API side, we want the frontend to be able to send eligibilities and categories by their names, but on the DB side, we want to save them as their integers IDs. In hindsight, maybe we should've just modeled the stuff in JSONB column in a normal, relational way, where the saved search API could return the entire eligibility object instead of its name.

We don't have time to change this before launch, but it might be a good post-launch tech debt payoff project.

internal/savedsearches/manager.go Dismissed Show dismissed Hide dismissed
internal/savedsearches/manager.go Dismissed Show dismissed Hide dismissed
@@ -6,7 +6,7 @@ import (
"fmt"
"log"

_ "github.com/lib/pq"
Copy link
Contributor

Choose a reason for hiding this comment

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

is this bc we didn't want to connect things before?

Copy link
Member Author

Choose a reason for hiding this comment

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

To be honest, I don't actually know what this syntax means in Go. I'm calling some methods from the pq package now, since that appeared to be necessary in order to put an array value in a prepared SQL statement, and when I attempted to call that method, my editor provided an autofix suggestion to import this package under its default name of pq.

Copy link
Member

Choose a reason for hiding this comment

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

If you don't need to explicitly interact with anything from the package you can alias it as _, if you are calling methods there then you can alias it or just remove that _

Copy link
Contributor

@katerina-kossler katerina-kossler left a comment

Choose a reason for hiding this comment

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

didn't see anything that screamed wrong to me

WHERE e.id = ANY ($1)
`

const eligibilitiesByNamesSql = `
Copy link
Contributor

Choose a reason for hiding this comment

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

ya definitely should move away from this later

}

// Return the elements of a that are not in b
func diffStringSlices(a []string, b []string) []string {
Copy link
Contributor

Choose a reason for hiding this comment

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

maybe not-"go like" but feels like we would want to eventually make a shared_logic with a manager for something like this?? or like this would be built-in with soemthing?

Copy link
Member

Choose a reason for hiding this comment

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

We can move this to a common utility file, theres also a slice.Contains function that might make this easier https://pkg.go.dev/slices

@lgarofalo
Copy link
Member

For correctness of code, I'd suggesting adding to the integration tests but otherwise seems ok

This implements the list, get, post, and delete routes. It omits the put
route for now, since we might not actually need it yet, and it will
probably require some significant refactoring.
This both performs an actual early exit as well as correctly reports a
4xx error rather than a 5xx error if the client submits data in the
wrong format.
@richardxia
Copy link
Member Author

Improved some minor error handling in 04d080f, since it was helpful as I was debugging the integration with the frontend in ShelterTechSF/askdarcel-web#1368.

I think I'm going to merge both of these PRs as soon as they pass CI. Thanks for the review, and sorry to not add the integration tests, but it's something I can do after the initial launch!

@richardxia richardxia merged commit 2262874 into main Jun 27, 2024
5 checks passed
@richardxia richardxia deleted the 58-saved-searches branch June 27, 2024 16:17
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