Skip to content
This repository was archived by the owner on Sep 30, 2024. It is now read-only.

feat/graphql/access_tokens: send access token creation/deletion emails in background and create special message for dial errors#63760

Merged
ggilmore merged 1 commit into
mainfrom
graphite-ggilmoreenhancement_graphql_access_tokens_send_access_token_creation_deletion_emails_in_background_and_create_special_message_for_dial_errors
Jul 10, 2024
Merged

feat/graphql/access_tokens: send access token creation/deletion emails in background and create special message for dial errors#63760
ggilmore merged 1 commit into
mainfrom
graphite-ggilmoreenhancement_graphql_access_tokens_send_access_token_creation_deletion_emails_in_background_and_create_special_message_for_dial_errors

Conversation

@ggilmore

@ggilmore ggilmore commented Jul 10, 2024

Copy link
Copy Markdown
Contributor

Closes https://linear.app/sourcegraph/issue/SRC-463/error-handling-for-smtp-config-issues

Before, when someone created or deleted an access token, we'd block on sending the email before we'd return to the user.

However, if an error occurred when sending an email - the error was only logged - not returned to the caller.

This PR makes three changes:

  1. We now sending the email in a background goroutine. This makes it so that the caller doesn't have to wait for the email to be sent before getting the access token back. This isn't a behavior change since we only logged any SMTP errors and deliberatly avoided forwarding that error to the caller. That behavior doesn't change here.

  2. If an error occurs while sending the access token email, we now have a specialized log message indicating that the user's SMTP configuration might be incorrect.

  3. The log error that's printed is now an ERROR instead of a WARN level.

Test plan

  1. On local dev, I edited the credentials in the dev-private site configuration file to point to an invalid port.

  2. I attempted to create an access token, and after only 30 seconds I saw this error print in the terminal:

[       frontend] WARN schemaResolver.CreateAccessToken graphqlbackend/access_tokens.go:150 Failed to send email to inform user of access token creation. (This error might indicate that your SMTP connection settings are incorrect. Please check your site configuration.) {"userID": 1, "error": "establishing TCP connection to \"smtp.gmail.com:57\": dial tcp 142.250.101.109:57: i/o timeout"}
  1. I fixed the broken SMTP configuration, and deleted the access token. I saw it suscessfully send an email:

Screenshot 2024-07-10 at 11.44.07 AM.png

Changelog

  • When creating or deleting an access token, we no longer wait for the email to be sent before returning to the caller. Instead, we now send it in the background.

@cla-bot cla-bot Bot added the cla-signed label Jul 10, 2024

ggilmore commented Jul 10, 2024

Copy link
Copy Markdown
Contributor Author

@github-actions github-actions Bot added team/product-platform team/source Tickets under the purview of Source - the one Source to graph it all labels Jul 10, 2024
@ggilmore ggilmore force-pushed the graphite-ggilmoreenhancement_graphql_access_tokens_send_access_token_creation_deletion_emails_in_background_and_create_special_message_for_dial_errors branch from 6eb016d to f590c83 Compare July 10, 2024 18:42
@ggilmore ggilmore changed the title enhancement/graphql/access_tokens: send access token creation/deletion emails in background and create special message for dial errors feat/graphql/access_tokens: send access token creation/deletion emails in background and create special message for dial errors Jul 10, 2024
@ggilmore ggilmore marked this pull request as ready for review July 10, 2024 18:46
@ggilmore ggilmore requested review from a team July 10, 2024 18:46

@bobheadxi bobheadxi left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Nice, looks like it might be worth making a helper function for this? The two new blocks of code looks the same, docstrings and all

Comment thread cmd/frontend/graphqlbackend/access_tokens.go Outdated

ggilmore commented Jul 10, 2024

Copy link
Copy Markdown
Contributor Author

@bobheadxi

I think it's worth keeping this separte for a few reasons:

Say that we have a siguratute that looks liek:

func sendAccessTokenCreationEmail(ctx context.Context, db database.DB, logger log15.Logger, userID int32, note string, isDeleted bool
  1. I think it's strange for a function to create it's own goroutine, so we'll still need that set up logic outside the function
  2. I also think it'd be strange for this function to create its own separate context object that only cancels after two minutes (the function would be making assumptions about the circumstances underwhich it's called - which breaks encapsulation)
  3. The additional logic this function could solely handle is the custom error message, but I think for most people they'd have to jump to the function definition anyway to see what the contents of the log are - which I don't think adds a ton of value on its (since you'd have to hop between the two function to get a picture of what's going on).

For these reasons, I don't think it's worth it to DRY this functionality yet.

@ggilmore ggilmore changed the base branch from graphite-ggilmoreenhancement_internal_txemail_add_30_second_timeout_to_connect_to_smtp_server to graphite-base/63760 July 10, 2024 19:34
@BolajiOlajide BolajiOlajide changed the base branch from graphite-base/63760 to main July 10, 2024 19:34
…n emails in background and create special message for dial errors
@ggilmore ggilmore force-pushed the graphite-ggilmoreenhancement_graphql_access_tokens_send_access_token_creation_deletion_emails_in_background_and_create_special_message_for_dial_errors branch from f590c83 to fbb493c Compare July 10, 2024 19:34
@ggilmore ggilmore merged commit 9063f05 into main Jul 10, 2024

Copy link
Copy Markdown
Contributor Author

Merge activity

@ggilmore ggilmore deleted the graphite-ggilmoreenhancement_graphql_access_tokens_send_access_token_creation_deletion_emails_in_background_and_create_special_message_for_dial_errors branch July 10, 2024 19:43
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

cla-signed team/product-platform team/source Tickets under the purview of Source - the one Source to graph it all

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants