Skip to content
This repository has been archived by the owner on Aug 26, 2022. It is now read-only.

transfers: delete ACH file when a transfer is deleted #39

Merged
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion database.go
Original file line number Diff line number Diff line change
Expand Up @@ -36,7 +36,7 @@ var (
`create table if not exists originators(originator_id primary key, user_id, default_depository, identification, metadata, created_at datetime, last_updated_at datetime, deleted_at datetime);`,

// Transfers
`create table if not exists transfers(transfer_id, user_id, type, amount, originator_id, originator_depository, customer, customer_depository, description, standard_entry_class_code, status, same_day, created_at datetime, last_updated_at datetime, deleted_at datetime);`,
`create table if not exists transfers(transfer_id, user_id, type, amount, originator_id, originator_depository, customer, customer_depository, description, standard_entry_class_code, status, same_day, file_id, created_at datetime, last_updated_at datetime, deleted_at datetime);`,
}

// Metrics
Expand Down
14 changes: 14 additions & 0 deletions pkg/achclient/client.go
Original file line number Diff line number Diff line change
Expand Up @@ -146,6 +146,20 @@ func (a *ACH) GET(relPath string) (*http.Response, error) {
return resp, nil
}

func (a *ACH) DELETE(relPath string) (*http.Response, error) {
req, err := http.NewRequest("DELETE", a.buildAddress(relPath), nil)
if err != nil {
return nil, err
}
requestId := createRequestId()
a.addRequestHeaders("", requestId, req)
resp, err := a.client.Do(req)
if err != nil {
return resp, fmt.Errorf("ACH DELETE requestId=%s : %v", requestId, err)
}
return resp, nil
}

// POST performs a HTTP POST request against a.endpoint and relPath.
// Retries are supported only if idempotencyKey is non-empty, otherwise only one attempt is made.
//
Expand Down
27 changes: 27 additions & 0 deletions pkg/achclient/client_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -35,6 +35,13 @@ var (
w.WriteHeader(http.StatusOK)
})
}
addDeleteRoute = func(r *mux.Router) {
r.Methods("DELETE").Path("/test").HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
w.Header().Set("Content-Type", "text/plain")
w.Write([]byte("delete"))
w.WriteHeader(http.StatusOK)
})
}
)

func newACHWithClientServer(name string, routes ...func(*mux.Router)) (*ACH, *http.Client, *httptest.Server) {
Expand Down Expand Up @@ -76,6 +83,26 @@ func TestACH__pingRoute(t *testing.T) {
}
}

func TestACH__delete(t *testing.T) {
achClient, _, server := newACHWithClientServer("delete", addDeleteRoute)
defer server.Close()

resp, err := achClient.DELETE("/test")
if err != nil {
t.Fatal(err)
}
defer resp.Body.Close()

// verify we hit the 'DELETE /test' route
bs, err := ioutil.ReadAll(resp.Body)
if err != nil {
t.Error(err)
}
if v := string(bs); v != "delete" {
t.Error(v)
}
}

func TestACH__post(t *testing.T) {
achClient, _, server := newACHWithClientServer("post", addCreateRoute)
defer server.Close()
Expand Down
10 changes: 10 additions & 0 deletions pkg/achclient/files.go
Original file line number Diff line number Diff line change
Expand Up @@ -92,3 +92,13 @@ func (a *ACH) GetFileContents(fileId string) (*bytes.Buffer, error) {
}
return &buf, nil
}

// DeleteFile makes an HTTP request to our ACH service to delete a file.
func (a *ACH) DeleteFile(fileId string) error {
resp, err := a.DELETE(fmt.Sprintf("/files/%s", fileId))
if err != nil {
return fmt.Errorf("DeleteFile: problem with HTTP request: %v", err)
}
resp.Body.Close()
return nil
}
36 changes: 36 additions & 0 deletions pkg/achclient/files_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -30,6 +30,14 @@ func addFileCreateRoute(ww *httptest.ResponseRecorder, r *mux.Router) {
})
}

func addFileDeleteRoute(ww *httptest.ResponseRecorder, r *mux.Router) {
r.Methods("DELETE").Path("/files/delete").HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
w.Header().Set("Content-Type", "application/json; charset=utf-8")
w.WriteHeader(http.StatusOK)
w.Write([]byte("{}"))
})
}

func TestFiles__CreateFile(t *testing.T) {
w := httptest.NewRecorder()

Expand Down Expand Up @@ -101,3 +109,31 @@ func TestFiles__CreateFile(t *testing.T) {
t.Errorf("batch Control ID=%v", batch.GetControl().ID)
}
}

func TestFiles__DeleteFile(t *testing.T) {
w := httptest.NewRecorder()

achClient, _, server := newACHWithClientServer("fileDelete", func(r *mux.Router) {
addFileCreateRoute(w, r)
addFileDeleteRoute(w, r)
})
defer server.Close()

// Create file
bs, err := ioutil.ReadFile(filepath.Join("..", "..", "testdata", "ppd-valid.json"))
if err != nil {
t.Fatal(err)
}
file, err := ach.FileFromJSON(bs)
if err != nil {
t.Fatal(err)
}
if _, err := achClient.CreateFile("create", file); err != nil {
t.Fatal(err)
}

// Delete File
if err := achClient.DeleteFile("delete"); err != nil {
t.Fatal(err)
}
}
58 changes: 46 additions & 12 deletions transfers.go
Original file line number Diff line number Diff line change
Expand Up @@ -270,14 +270,14 @@ func getUserTransfer(transferRepo transferRepository) http.HandlerFunc {

// readTransferRequests will attempt to parse the incoming body as either a transferRequest or []transferRequest.
// If no requests were read a non-nil error is returned.
func readTransferRequests(r *http.Request) ([]transferRequest, error) {
func readTransferRequests(r *http.Request) ([]*transferRequest, error) {
bs, err := read(r.Body)
if err != nil {
return nil, err
}

var req transferRequest
var requests []transferRequest
var requests []*transferRequest
if err := json.Unmarshal(bs, &req); err != nil {
// failed, but try []transferRequest
if err := json.Unmarshal(bs, &requests); err != nil {
Expand All @@ -287,7 +287,7 @@ func readTransferRequests(r *http.Request) ([]transferRequest, error) {
if err := req.missingFields(); err != nil {
return nil, err
}
requests = append(requests, req)
requests = append(requests, &req)
}
if len(requests) == 0 {
return nil, errors.New("no Transfer request objects found")
Expand All @@ -314,10 +314,11 @@ func createUserTransfers(custRepo customerRepository, depRepo depositoryReposito
return
}

id, userId, requestId := nextID(), moovhttp.GetUserId(r), moovhttp.GetRequestId(r)
userId, requestId := moovhttp.GetUserId(r), moovhttp.GetRequestId(r)
ach := achclient.New(userId, logger)

for i := range requests {
id := nextID()
Copy link
Member Author

Choose a reason for hiding this comment

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

I think this fixes a bug where multiple transfers would have been created with the same ID.

req := requests[i]

if err := req.missingFields(); err != nil {
Expand Down Expand Up @@ -368,7 +369,7 @@ func createUserTransfers(custRepo customerRepository, depRepo depositoryReposito
return
}

req.fileId = fileId
req.fileId = fileId // add fileId onto our request

err = eventRepo.writeEvent(userId, &Event{
ID: EventID(nextID()),
Expand Down Expand Up @@ -416,7 +417,24 @@ func deleteUserTransfer(transferRepo transferRepository) http.HandlerFunc {
return
}

// TODO(adam): Check status? Only allow Pending transfers to be deleted?

id, userId := getTransferId(r), moovhttp.GetUserId(r)

// Delete from our ACH service
fileId, err := transferRepo.getFileIdForTransfer(id, userId)
if err != nil {
internalError(w, err)
return
}

ach := achclient.New(userId, logger)
if err := ach.DeleteFile(fileId); err != nil { // TODO(adam): ignore 404's
internalError(w, err)
return
}

// Delete from our database
if err := transferRepo.deleteUserTransfer(id, userId); err != nil {
internalError(w, err)
return
Expand All @@ -431,7 +449,7 @@ func deleteUserTransfer(transferRepo transferRepository) http.HandlerFunc {
// 400 - errors, check json
func validateUserTransfer(transferRepo transferRepository) http.HandlerFunc {
return func(w http.ResponseWriter, r *http.Request) {
w, err := wrapResponseWriter(w, r, "deleteUserTransfer")
w, err := wrapResponseWriter(w, r, "validateUserTransfer")
if err != nil {
return
}
Expand All @@ -442,7 +460,7 @@ func validateUserTransfer(transferRepo transferRepository) http.HandlerFunc {

func getUserTransferFiles(transferRepo transferRepository) http.HandlerFunc {
return func(w http.ResponseWriter, r *http.Request) {
w, err := wrapResponseWriter(w, r, "deleteUserTransfer")
w, err := wrapResponseWriter(w, r, "getUserTransferFiles")
if err != nil {
return
}
Expand Down Expand Up @@ -489,8 +507,9 @@ func getUserTransferEvents(eventRepo eventRepository, transferRepo transferRepos
type transferRepository interface {
getUserTransfers(userId string) ([]*Transfer, error)
getUserTransfer(id TransferID, userId string) (*Transfer, error)
getFileIdForTransfer(id TransferID, userId string) (string, error)

createUserTransfers(userId string, requests []transferRequest) ([]*Transfer, error)
createUserTransfers(userId string, requests []*transferRequest) ([]*Transfer, error)
deleteUserTransfer(id TransferID, userId string) error
}

Expand Down Expand Up @@ -561,8 +580,23 @@ limit 1`
return transfer, nil
}

func (r *sqliteTransferRepo) createUserTransfers(userId string, requests []transferRequest) ([]*Transfer, error) {
query := `insert into transfers (transfer_id, user_id, type, amount, originator_id, originator_depository, customer, customer_depository, description, standard_entry_class_code, status, same_day, created_at) values (?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?)`
func (r *sqliteTransferRepo) getFileIdForTransfer(id TransferID, userId string) (string, error) {
query := `select file_id from transfers where transfer_id = ? and user_id = ? and deleted_at is null limit 1;`
stmt, err := r.db.Prepare(query)
if err != nil {
return "", err
}
row := stmt.QueryRow(id, userId)

var fileId string
if err := row.Scan(&fileId); err != nil {
return "", err
}
return fileId, nil
}

func (r *sqliteTransferRepo) createUserTransfers(userId string, requests []*transferRequest) ([]*Transfer, error) {
query := `insert into transfers (transfer_id, user_id, type, amount, originator_id, originator_depository, customer, customer_depository, description, standard_entry_class_code, status, same_day, file_id, created_at) values (?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?)`
stmt, err := r.db.Prepare(query)
if err != nil {
return nil, err
Expand Down Expand Up @@ -592,7 +626,7 @@ func (r *sqliteTransferRepo) createUserTransfers(userId string, requests []trans
}

// write transfer
_, err := stmt.Exec(transferId, userId, req.Type, req.Amount.String(), req.Originator, req.OriginatorDepository, req.Customer, req.CustomerDepository, req.Description, req.StandardEntryClassCode, status, req.SameDay, now)
_, err := stmt.Exec(transferId, userId, req.Type, req.Amount.String(), req.Originator, req.OriginatorDepository, req.Customer, req.CustomerDepository, req.Description, req.StandardEntryClassCode, status, req.SameDay, req.fileId, now)
if err != nil {
return nil, err
}
Expand Down Expand Up @@ -634,7 +668,7 @@ func abaCheckDigit(rtn string) string {
// This method also verifies the status of the Customer, Customer Depository and Originator Repository
//
// All return values are either nil or non-nil and the error will be the opposite.
func getTransferObjects(req transferRequest, userId string, custRepo customerRepository, depRepo depositoryRepository, origRepo originatorRepository) (*Customer, *Depository, *Originator, *Depository, error) {
func getTransferObjects(req *transferRequest, userId string, custRepo customerRepository, depRepo depositoryRepository, origRepo originatorRepository) (*Customer, *Depository, *Originator, *Depository, error) {
// Customer
cust, err := custRepo.getUserCustomer(req.Customer, userId)
if err != nil {
Expand Down
10 changes: 8 additions & 2 deletions transfers_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -164,7 +164,7 @@ func TestTransfers__getUserTransfers(t *testing.T) {

amt, _ := NewAmount("USD", "12.42")
userId := nextID()
req := transferRequest{
req := &transferRequest{
Type: PushTransfer,
Amount: *amt,
Originator: OriginatorID("originator"),
Expand All @@ -173,9 +173,10 @@ func TestTransfers__getUserTransfers(t *testing.T) {
CustomerDepository: DepositoryID("customer"),
Description: "money",
StandardEntryClassCode: "PPD",
fileId: "test-file",
}

if _, err := repo.createUserTransfers(userId, []transferRequest{req}); err != nil {
if _, err := repo.createUserTransfers(userId, []*transferRequest{req}); err != nil {
t.Fatal(err)
}

Expand Down Expand Up @@ -203,6 +204,11 @@ func TestTransfers__getUserTransfers(t *testing.T) {
if v := transfers[0].Amount.String(); v != "USD 12.42" {
t.Errorf("got %q", v)
}

fileId, _ := repo.getFileIdForTransfer(transfers[0].ID, userId)
if fileId != "test-file" {
t.Error("no fileId found in transfers table")
}
}

func TestTransfers__ABA(t *testing.T) {
Expand Down