-
Notifications
You must be signed in to change notification settings - Fork 34
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
B 20493 int #13265
B 20493 int #13265
Conversation
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.
Need you to clarify some things with Dan before signing off or doing additional work.
sqlQuery := ` | ||
with names as (select office.id as transportation_office_id, office.name, similarity(office.name, $1) as sim | ||
from transportation_offices as office | ||
where name % $1 and provides_ppm_closeout is true | ||
where name % $1 | ||
order by sim desc | ||
limit 5) | ||
select office.* |
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.
So I know this works, but I have some reservations.
This is also used for customers when searching for transpo offices in /pkg/handlers/internalapi/transportation_offices.go:54
:
transportationOffices, err := h.TransportationOfficesFetcher.GetTransportationOffices(appCtx, params.Search)
if err != nil {
appCtx.Logger().Error("Error searching for Transportation Offices: ", zap.Error(err))
return transportationofficeop.NewGetTransportationOfficesInternalServerError(), err
}
and...
This is also used for customers when searching for transpo offices in /pkg/handlers/ghcapi/transportation_offices.go:23
:
transportationOffices, err := h.TransportationOfficesFetcher.GetTransportationOffices(appCtx, params.Search)
if err != nil {
appCtx.Logger().Error("Error searching for Transportation Offices: ", zap.Error(err))
return transportationofficeop.NewGetTransportationOfficesInternalServerError(), err
}
So I think that if that is fine for customers and office users inside of the app to see all transpo offices, then we can leave this.
If not, then we should probably create a new service object that only this open handler uses. Should prob ask Dan.
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.
good call
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.
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.
Can just create a new service object that the handler calls. Just copy over what's in pkg/services/transportation_office/transportation_office_fetcher.go
:
func (o transportationOfficesFetcher) GetTransportationOfficesForRequestedOfficeUsers(appCtx appcontext.AppContext, search string) (*models.TransportationOffices, error) {
officeList, err := FindTransportationOfficeForRequestedOfficeUsers(appCtx, search)
if err != nil {
switch err {
case sql.ErrNoRows:
return &officeList, apperror.NewNotFoundError(uuid.Nil, "Search string: "+search)
default:
return &officeList, err
}
}
return &officeList, nil
}
func FindTransportationOfficeForRequestedOfficeUsers(appCtx appcontext.AppContext, search string) (models.TransportationOffices, error) {
var officeList []models.TransportationOffice
// The % operator filters out strings that are below this similarity threshold
err := appCtx.DB().Q().RawQuery("SET pg_trgm.similarity_threshold = 0.03").Exec()
if err != nil {
return officeList, err
}
sqlQuery := `
with names as (select office.id as transportation_office_id, office.name, similarity(office.name, $1) as sim
from transportation_offices as office
where name % $1
order by sim desc
limit 5)
select office.*
from names n inner join transportation_offices office on n.transportation_office_id = office.id
group by office.id
order by max(n.sim) desc, office.name
limit 5`
query := appCtx.DB().Q().RawQuery(sqlQuery, search)
if err := query.All(&officeList); err != nil {
if errors.Cause(err).Error() != models.RecordNotFoundErrorString {
return officeList, err
}
}
for i := range officeList {
err := appCtx.DB().Load(&officeList[i], "Address")
if err != nil {
return officeList, err
}
}
return officeList, nil
}
Bing bang boom.
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.
I don't like copied code- I prefer to make the code more flexible for multiple use
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.
Fine by me! Your solution looks good!
B-20493 update query
You just not putting labels on your PRs now, boss? :) |
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.
Query results look good!
May need a make mocks_generate
on your branch
gen mocks
I never claimed to be perfect... |
already did |
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.
Nailed it!
B-20493
Summary
Verification Steps for the Author
These are to be checked by the author.
Verification Steps for Reviewers
These are to be checked by a reviewer.
Setup to Run the Code
How to test
Backend
Screenshots