sc-14491 Part 2: Updates GDS endpoints #1008
Conversation
| ctx, | ||
| ) | ||
| if err != nil { | ||
| log.Error().Err(err).Msg("could not create verify contact email") |
There was a problem hiding this comment.
Note that ideally we want to do the logging as close to the handlers as possible; I realize that the existing method also did logging here, so maybe we should just create a follow on story to remove the logging in this file and push it into the handlers.
pkg/gds/gds.go
Outdated
| @@ -470,50 +468,63 @@ func (s *GDS) VerifyContact(ctx context.Context, in *api.VerifyContactRequest) ( | |||
| return nil, status.Error(codes.NotFound, "could not find associated VASP record by ID") | |||
| } | |||
|
|
|||
| // Retrieve email address from one of the supplied contacts. | |||
| var email string | |||
| if email = GetContactEmail(vasp); email == "" { | |||
There was a problem hiding this comment.
Do we still need to iterate through the VASP's contacts to determine which contact(s) to mark verified? GetContactEmail only returns the first email in the contact order, but we still want to have a way to verify the other contacts right? For example, if the technical contact is verified then this endpoint would always return "contact already verified" which means that even with the right token we wouldn't be able to verify the other contacts.
There was a problem hiding this comment.
Structurally this looks good, I just had a lot of small comments relating to error messages and one slightly more important one relating to the found = true logic. Hopefully not too painful to apply those suggestions. I appreciate the code comments!
I think there is one additional change we need to make. In Register, we should set the VASP to pending review by copying the steps at the end of VerifyContact (it involves sending the review request to the admins). This will allow VASPs with an already verified email address to skip the VerifyContact process. I propose that we create a helper method that can be called by both Register and VerifyContact. Error handling might be tricky, so I think we should create a separate story for this.
pkg/gds/gds.go
Outdated
| log.Info().Int("sent", sent).Msg("contact email verifications sent") | ||
| // If there does not exist a model contact associated with the vasp contact's email then create one. | ||
| var contact *models.Contact | ||
| if contact, err = s.db.RetrieveContact(ctx, vaspContact.Email); err != nil { |
There was a problem hiding this comment.
Note: we probably want to distinguish between "not found" errors and all other database errors. In the "not found" case we want to do what you're doing here, create the contact since the store told us it doesn't exist. In the second case we actually aren't sure if the contact exists, it's possible that kubernetes bounced the trtl pod or something. In that case we just want to return a status error aborted to avoid overwriting an existing contact.
We should be able to do if errors.Is(err, storeerrors.ErrEntityNotFound) (may have to import the package above) to check for the not found error, and more generally we should be doing this for all the s.db.Retrieve error handling. However, it's possible that other places in the code don't have this handling because this code is old.
Co-authored-by: Patrick Deziel <[email protected]>
Co-authored-by: Patrick Deziel <[email protected]>
Co-authored-by: Patrick Deziel <[email protected]>
Co-authored-by: Patrick Deziel <[email protected]>
Co-authored-by: Patrick Deziel <[email protected]>
Co-authored-by: Patrick Deziel <[email protected]>
| if err != nil { | ||
| log.Error().Err(err).Msg("could not retrieve verification from contact extra data field") | ||
| return nil, status.Error(codes.Aborted, "could not verify contact") | ||
| } | ||
|
|
||
| // If the model contact is verified make sure the vasp contact is verified | ||
| if contact.Verified { | ||
| found = true |
There was a problem hiding this comment.
Can we remove the found = true or is that causing issues?
Scope of changes
Updates the GDS endpoints as part of the email verification epic.
Type of change
Acceptance criteria
Describe how reviewers can test this change to be sure that it works correctly. Add a checklist if possible
Author checklist
Reviewer(s) checklist