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

feat: club and event search #873

Merged
merged 38 commits into from
May 28, 2024
Merged

feat: club and event search #873

merged 38 commits into from
May 28, 2024

Conversation

michael-brennan2005
Copy link
Contributor

@michael-brennan2005 michael-brennan2005 commented May 21, 2024

Club and event search.

Considerations before approval

  • How should error handling work with search? If upserting/deleting a club or event fails, should we rollback in the outer club/event transaction that it was called in? Obv answer is no, but we should return an error at least. Is it bad go practice to both return a value and an error? I think that would get gross very quickly but idk. Key thing is its ok if something is in the DB but not in search.
  • Right now there's a lot of repetition in codebase, its very obvious that clubs and events should be behind some "SearchObject" interface or something. What's tricky is that the interfacing/abstraction isn't just methods but also types - each object has a results type (what is returned from search endpoint, right now they are same and this is probably just a Results sort of type), document type (what acc gets uploaded to opensearch index), and query type (what filters/queries we can search type with). Im very tired so this may just be something I need to think about.
  • Security for opensearch is striaght up disabled, but this is something I was gonna work on w deployment so

@garrettladley garrettladley requested review from a team, DOOduneye and garrettladley and removed request for a team May 21, 2024 03:03
Copy link
Member

@garrettladley garrettladley left a comment

Choose a reason for hiding this comment

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

some nits then LGTM
great work on this MB @michael-brennan2005 ⚽🐐

backend/entities/clubs/base/transactions.go Outdated Show resolved Hide resolved
backend/migrations/000001_init.up.sql Outdated Show resolved Hide resolved
backend/search/base/controller.go Show resolved Hide resolved
backend/search/base/controller.go Show resolved Hide resolved
backend/search/base/transactions.go Outdated Show resolved Hide resolved
backend/search/base/transactions.go Show resolved Hide resolved
backend/search/base/transactions.go Show resolved Hide resolved
backend/search/seed.go Outdated Show resolved Hide resolved
backend/search/types/request.go Outdated Show resolved Hide resolved
docker-compose.yml Show resolved Hide resolved
Copy link
Member

@garrettladley garrettladley left a comment

Choose a reason for hiding this comment

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

LGTM good stuff @michael-brennan2005

backend/search/base/controller.go Show resolved Hide resolved
@garrettladley garrettladley enabled auto-merge (squash) May 28, 2024 17:16
@garrettladley garrettladley changed the title Club and event search feat: club and event search May 28, 2024
@garrettladley garrettladley merged commit 67d0b1a into main May 28, 2024
5 checks passed
@garrettladley garrettladley deleted the club-search branch May 28, 2024 17:18
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.

2 participants