Skip to content
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

change: [M3-8415] - Hide SMTP warning for Linodes and accounts that have SMTP enabled #10991

Merged

Conversation

hkhalil-akamai
Copy link
Contributor

@hkhalil-akamai hkhalil-akamai commented Sep 23, 2024

Description 📝

Leverages the new "SMTP Enabled" account & Linode capabilities to hide the SMTP warning when it doesn't apply.

The SMTP warning appears in two places:

  • At the bottom of the Linode create flow. This warning is now hidden for accounts with the new "SMTP Enabled" account capability.
  • In the Linode details page as a dismissible banner. This warning is now hidden when the "SMTP Enabled" Linode capability is present on the selected Linode.

How to test 🧪

Prerequisites

Use the alpha API environment.

You will need an account with at least one Linode.

In Admin: remove the SMTP_ENABLED customer tag from the account or ensure that it's not present. For the test Linode, check that the Net Filter Exceptions field appears as below or change it to match:

Screenshot 2024-09-23 at 4 56 53 PM

Testing steps

  1. Verify the SMTP banner appears in both locations. If it's not appearing for the Linode, you may have dismissed it. Reset your preferences using the Preference Editor: http://localhost:3000/profile/settings?preferenceEditor=true.
  2. Enable SMTP for a particular Linode by removing the SMTP Net Filter Exceptions.
  3. Verify the SMTP warning banner no longer appears for that particular Linode. However, it should still appear for other Linodes and at the bottom of the Create Linode flow
  4. Add the SMTP_ENABLED customer tag to your account
  5. Verify the SMTP warning no longer appears in the Linode Create flow

As an Author I have considered 🤔

Check all that apply

  • 👀 Doing a self review
  • ❔ Our contribution guidelines
  • 🤏 Splitting feature into small PRs
  • ➕ Adding a changeset
  • 🧪 Providing/Improving test coverage
  • 🔐 Removing all sensitive information from the code and PR description
  • 🚩 Using a feature flag to protect the release
  • 👣 Providing comprehensive reproduction steps
  • 📑 Providing or updating our documentation
  • 🕛 Scheduling a pair reviewing session
  • 📱 Providing mobile support
  • ♿ Providing accessibility support

@hkhalil-akamai hkhalil-akamai self-assigned this Sep 23, 2024
@hkhalil-akamai hkhalil-akamai requested a review from a team as a code owner September 23, 2024 21:03
@hkhalil-akamai hkhalil-akamai requested review from bnussman-akamai and hana-akamai and removed request for a team September 23, 2024 21:03
@hkhalil-akamai hkhalil-akamai marked this pull request as draft September 23, 2024 21:03
Copy link

github-actions bot commented Sep 23, 2024

Coverage Report:
Base Coverage: 87.13%
Current Coverage: 87.13%

Comment on lines -236 to -240
// "In an effort to fight spam, Linode restricts outbound connections on ports 25, 465, and 587 on all Linodes for new accounts created after November 5th, 2019."
// https://www.linode.com/docs/email/best-practices/running-a-mail-server/
export const MAGIC_DATE_THAT_EMAIL_RESTRICTIONS_WERE_IMPLEMENTED =
'2022-11-30T00:00:00.000Z'; // Date of release for Manager v1.81.0.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Since we are directly using the SMTP status of accounts/Linodes, we no longer need this magic date.

Copy link
Member

Choose a reason for hiding this comment

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

This is great! 🚀

@hkhalil-akamai hkhalil-akamai requested a review from mjac0bs October 7, 2024 18:08
@hkhalil-akamai hkhalil-akamai marked this pull request as ready for review October 7, 2024 18:08
@hkhalil-akamai hkhalil-akamai requested a review from a team as a code owner October 7, 2024 18:08
@hkhalil-akamai hkhalil-akamai requested review from AzureLatte and removed request for a team, mjac0bs and AzureLatte October 7, 2024 18:08
@hkhalil-akamai hkhalil-akamai marked this pull request as draft October 8, 2024 13:54
@hkhalil-akamai hkhalil-akamai marked this pull request as ready for review October 8, 2024 13:55
Copy link
Contributor

@mjac0bs mjac0bs left a comment

Choose a reason for hiding this comment

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

Thanks for taking this on, Hussain! Exciting to move away from the magic constant and have backend support for this.

This looks good in dev. From what I can tell based on the API PRs merge dates, this has not yet been release to prod yet. Can we update the testing instructions to specify testing in the dev environment?

const { data: account } = useAccount();

const displayRestrictionText =
Copy link
Contributor

Choose a reason for hiding this comment

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

More of a backend question: Do we know if it's possible for an account to have SMTP_ENABLED, but an individual Linode is still disabled (with net exceptions on its ports)? If that state were possible, we're currently showing the notice for the Linode, which seems like what we'd want for transparency... unless, somehow, the individual net exceptions don't always get removed when SMTP_ENABLED is added account-wide, which could be confusing for customers.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Do we know if it's possible for an account to have SMTP_ENABLED, but an individual Linode is still disabled

Yes, it's possible.

I was surprised to learn that there is no logic in the API that links the customer tag with the Net Filter Exceptions of a particular Linode. Adding the tag is a manual process used by support representatives when a customer requests SMTP abilities on their Linode and it is only used to keep track of which customers have had this request granted.

The availability of SMTP for a Linode is determined solely by the presence (or lack thereof) of the SMTP Net Filter Exceptions. There is also a field on the customer called 'Default Net Filter Exceptions' that is applied by default to new Linodes.

So the choice to only use the capability from the Linode object was intentional, since that is the source of truth of whether a particular Linode is SMTP-capable.

For a detailed summary of this topic, I suggest the KB article SMTP Port Restrictions.

Copy link
Contributor

Choose a reason for hiding this comment

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

Adding the tag is a manual process used by support representatives when a customer requests SMTP abilities on their Linode and it is only used to keep track of which customers have had this request granted.

Super helpful context and surprises me also.

So the choice to only use the capability from the Linode object was intentional, since that is the source of truth of whether a particular Linode is SMTP-capable.

Got it; agree with this.

Copy link
Contributor

@mjac0bs mjac0bs left a comment

Choose a reason for hiding this comment

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

Once you merge, can you update Diana and Avi in the internal ticket, just to let them know when we can expect this to be released? I have a feeling they'll be interested.

Thank you! 🙌🏼

const { data: account } = useAccount();

const displayRestrictionText =
Copy link
Contributor

Choose a reason for hiding this comment

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

Adding the tag is a manual process used by support representatives when a customer requests SMTP abilities on their Linode and it is only used to keep track of which customers have had this request granted.

Super helpful context and surprises me also.

So the choice to only use the capability from the Linode object was intentional, since that is the source of truth of whether a particular Linode is SMTP-capable.

Got it; agree with this.

@mjac0bs mjac0bs added the Add'tl Approval Needed Waiting on another approval! label Oct 8, 2024
@bnussman-akamai bnussman-akamai added Approved Multiple approvals and ready to merge! and removed Add'tl Approval Needed Waiting on another approval! labels Oct 8, 2024
@hkhalil-akamai hkhalil-akamai merged commit 2667f23 into linode:develop Oct 8, 2024
20 checks passed
@hkhalil-akamai hkhalil-akamai deleted the M3-8415-hide-SMTP-warning branch October 8, 2024 20:16
Copy link

cypress bot commented Oct 8, 2024

Cloud Manager E2E    Run #6643

Run Properties:  status check passed Passed #6643  •  git commit 2667f23d59: change: [M3-8415] - Hide SMTP warning for Linodes and accounts that have SMTP en...
Project Cloud Manager E2E
Run status status check passed Passed #6643
Run duration 27m 01s
Commit git commit 2667f23d59: change: [M3-8415] - Hide SMTP warning for Linodes and accounts that have SMTP en...
Committer Hussain Khalil
View all properties for this run ↗︎

Test results
Tests that failed  Failures 0
Tests that were flaky  Flaky 3
Tests that did not run due to a developer annotating a test with .skip  Pending 2
Tests that did not run due to a failure in a mocha hook  Skipped 0
Tests that passed  Passing 429

hasyed-akamai pushed a commit to hasyed-akamai/manager that referenced this pull request Oct 9, 2024
…ave SMTP enabled (linode#10991)

* Hide SMTP warning for Linodes and accounts that have SMTP enabled

* Update unit tests

* Added changeset: Hide SMTP warning for Linodes and accounts that have SMTP enabled

* Added changeset: 'SMTP Enabled' account & Linode capabilities

* Remove magic date
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Approved Multiple approvals and ready to merge! Ready for Review
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants