http: add endpoint to fetch a detailed list of customer and account info #211
Conversation
Codecov Report
@@ Coverage Diff @@
## master #211 +/- ##
==========================================
+ Coverage 58.72% 59.13% +0.40%
==========================================
Files 49 51 +2
Lines 2629 2692 +63
==========================================
+ Hits 1544 1592 +48
- Misses 786 796 +10
- Partials 299 304 +5 |
cmd/server/main.go
Outdated
| @@ -96,6 +97,7 @@ func main() { | |||
| disclaimerRepo := documents.NewDisclaimerRepo(logger, db) | |||
| documentRepo := documents.NewDocumentRepo(logger, db) | |||
| validationsRepo := validator.NewRepo(db) | |||
| aggregateRepo := reports.NewRepository(logger, db) | |||
There was a problem hiding this comment.
Would this make more sense as reportsRepo := ...?
There was a problem hiding this comment.
Oh yeah, aggregate was something I used before refactoring, thanks for the catch.
pkg/reports/repo.go
Outdated
| @@ -0,0 +1,80 @@ | |||
| package reports | |||
There was a problem hiding this comment.
Nit: let's call it repository.go
pkg/reports/repo.go
Outdated
|
|
||
| func (r *sqlRepository) FindByAccountIDs(accountIDs []string) ([]*CustomerAccount, error) { | ||
| query := fmt.Sprintf( | ||
| `select |
There was a problem hiding this comment.
Why would we duplicate the logic for reading a Customer? That's pretty involved logic to duplicate.
There was a problem hiding this comment.
We aren't reading out address, phone or metadata either.
There was a problem hiding this comment.
Agreed, I was debating on whether to implement this. Based on the issue description, it seemed like we wanted to minimize the # of db queries we were making for fetching n # of customers/accounts. So it wasn't too much code to duplicate.
I could rip it out and call getCustomer * n. I also looked into using searchCustomers but underneath it still calls getCustomer n times.
And I assumed address, phone or metadata of the customer wasn't relevant to this endpoint.
There was a problem hiding this comment.
I would always default to assuming we need to return the entire model. Otherwise the UI and folks could get confused.
It sounds like we need to reinvent the repository method for getting multiple customers then? I don't like this hack when we could have one method used in GET /customers/.... routes and this reporting endpoint.
There was a problem hiding this comment.
Good point.
I'll rework this, I'll use searchCustomers for now but will make sure to optimize it in another PR.
Related issue: #216
pkg/reports/report.go
Outdated
| @@ -0,0 +1,49 @@ | |||
| package reports | |||
There was a problem hiding this comment.
Nit: Let's call this router.go.
pkg/reports/router.go
Outdated
|
|
||
| allCustomers := make(map[string]*client.Customer) | ||
| for _, acc := range allAccounts { | ||
| cust, err := customerRepo.GetCustomer(acc.CustomerID) |
There was a problem hiding this comment.
In the future, need to update searchCustomers to accept a list of customerIDs to filter for.
New related issue: #220
api/admin.yaml
Outdated
| @@ -82,7 +82,7 @@ paths: | |||
| content: | |||
| application/json: | |||
| schema: | |||
| $ref: 'https://raw.githubusercontent.com/moov-io/api/master/openapi-common.yaml#/components/schemas/Error' | |||
| $ref: 'https://raw.githubusercontent.com/moov-io/base/master/api/common.yaml#/components/schemas/Error' | |||
There was a problem hiding this comment.
This should be fixed so we can remove this change.
There was a problem hiding this comment.
Sweet, will rebase and update.
Adds the endpoint
/reports/accounts?accountIDs={...}that returns a list of objects that includecustomerandaccountinformation.Fixes #196