-
Notifications
You must be signed in to change notification settings - Fork 8
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
Make user-friendly status filter multi-search #43
Make user-friendly status filter multi-search #43
Conversation
err = database.Migrate(cfg.DbConfig) | ||
if err != nil { | ||
panic(err) | ||
} | ||
db.LogMode(false) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is now added to management-service/database/database.go
, as a part of the Gorm config, when opening the DB connection. Ref: https://gorm.io/docs/logger.html
@@ -49,12 +49,15 @@ func NewServer(configFiles []string) (*Server, error) { | |||
if err != nil { | |||
panic(err) | |||
} | |||
sqlDB, err := db.DB() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Gorm doesn't expose a close method anymore on its DB. We have to grab the underlying SQL db and close it.
OR tstzrange(start_time, end_time, '()') @> tstzrange(?, ?, '[]') | ||
OR tstzrange(?, ?, '[]') @> tstzrange(start_time, end_time, '[)')`, | ||
params.StartTime, params.StartTime, params.EndTime, params.EndTime, params.StartTime, params.EndTime, | ||
svc.query(). |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Refactoring this OR clause to use the APIs rather than raw query string, since the new version of the Gorm API supports it correctly.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM! 🚀 Thanks a lot for updating all the instances where we're using the gorm library!
} | ||
} | ||
if len(params.StatusFriendly) > 0 { | ||
query = svc.filterExperimentStatusFriendly(query, params.StatusFriendly) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for refactoring this into a separate method!
What this PR does / why we need it: This is a follow up from the previous PR #42 which introduced a search filter for the experiments using user-friendly status names. However, in that PR, the API was only designed to take in a single value. This PR makes it possible to search using multiple values of the status, to match any of them. This required the underlying
gorm
library to be upgraded, as the OR clauses were not gracefully handled in the older version.Illustration
multiSelectStatus.mov
Search Filter Changes
api/experiments.yaml
- Accept multiple values forstatus_friendly
, in List Experiments filtersmanagement-service/controller/experiment_controller.go
- Handle multiple values forstatus_friendly
in List API search filtersmanagement-service/services/experiment_service.go
- AddfilterExperimentStatusFriendly
to handle matching the status values.ui/src/experiments/list/search/components/ExperimentStatusFilter.js
- Make the status filter multiselectGORM upgrade
github.com/jinzhu/gorm
->gorm.io/gorm
@ latestgithub.com/jinzhu/gorm/dialects/postgres
->gorm.io/driver/postgres
@ latestmanagement-service/database/database.go
,management-service/server/server.go
,management-service/internal/testutils/database.go
- DB init / close related API changesmanagement-service/models/experiment.go
,management-service/models/experiment_history.go
- AddAfterFind
hook as the new ORM version doesn't respect the timezone info in the connection string.int
/int32
/int64
conversions in OFFSET / LIMIT / COUNT queries to match the new APIssave
function in every service that saves the data to the DB is refactored to use Upsert instead of Create / Update. This is needed because theNewRecord
function from Gorm v1 no longer exists. Ref: https://gorm.io/docs/create.html#Upsert-x2F-On-Conflict