Skip to content

fix: [M3-7916] - Typo with toast success notification for updating Reverse DNS#10895

Merged
coliu-akamai merged 12 commits intolinode:developfrom
coliu-akamai:m3-7916
Sep 11, 2024
Merged

fix: [M3-7916] - Typo with toast success notification for updating Reverse DNS#10895
coliu-akamai merged 12 commits intolinode:developfrom
coliu-akamai:m3-7916

Conversation

@coliu-akamai
Copy link
Contributor

@coliu-akamai coliu-akamai commented Sep 5, 2024

Description 📝

  • fix typo with toast success notification when updating reverse DNS

Changes 🔄

  • fix typo
  • add e2e test to confirm typo is fixed
  • added some unit tests
  • other small cleanup

Target release date 🗓️

n/a

Preview 📷

Before After
image image

How to test 🧪

Verification steps

  • yarn cy:run -s cypress/e2e/core/linodes/linode-network.spec.ts
  • Confirm toast says RDNS when updating RDNS in a Linode's network tab
  • Confirm that RDNS drawer resets when opening/closing it

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

@coliu-akamai coliu-akamai added the Bug Fixes for regressions or bugs label Sep 5, 2024
@coliu-akamai coliu-akamai self-assigned this Sep 5, 2024
Comment on lines +49 to +52
const onExited = () => {
formik.resetForm();
reset();
};
Copy link
Contributor Author

Choose a reason for hiding this comment

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

see here for reasoning for this change

@coliu-akamai coliu-akamai marked this pull request as ready for review September 5, 2024 17:20
@coliu-akamai coliu-akamai requested review from a team as code owners September 5, 2024 17:20
@coliu-akamai coliu-akamai requested review from cliu-akamai, cpathipa, hkhalil-akamai and jdamore-linode and removed request for a team September 5, 2024 17:20
Comment on lines +78 to +81
cy.findByText('Save')
.should('be.visible')
.should('be.enabled')
.click();
Copy link
Contributor Author

@coliu-akamai coliu-akamai Sep 5, 2024

Choose a reason for hiding this comment

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

note - this test doesn't technically edit the RDNS, just clicks save to trigger and check the toast (I'm not too familiar with/sure how to edit the RDNS - would get an error each time)

Copy link
Contributor

Choose a reason for hiding this comment

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

That's actually useful context if we ever need to look back at this test after some time has passed. Maybe you can expand on that // click Save button comment to explain that we're only trying to trigger the toast and are intentionally not making any change to the form?

@github-actions
Copy link

github-actions bot commented Sep 5, 2024

Coverage Report:
Base Coverage: 86.37%
Current Coverage: 86.37%

@coliu-akamai coliu-akamai changed the title fix: [M3-7916] - Typo with Toast Success Notification For Updating Reverse DNS fix: [M3-7916] - Typo with toast success notification for updating Reverse DNS Sep 5, 2024
Copy link
Member

@bnussman-akamai bnussman-akamai left a comment

Choose a reason for hiding this comment

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

Looks good! Lots of nice clean up and testing 🧼 🧪

Copy link
Contributor

@jdamore-linode jdamore-linode left a comment

Choose a reason for hiding this comment

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

Nice work @coliu-akamai, thanks! The new test looks great!

Comment on lines +485 to +486
/**
* Intercepts POST request to get a Linode Resize.
Copy link
Contributor

Choose a reason for hiding this comment

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

Nice catch, thanks for the fix! 😅

Comment on lines +78 to +81
cy.findByText('Save')
.should('be.visible')
.should('be.enabled')
.click();
Copy link
Contributor

Choose a reason for hiding this comment

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

That's actually useful context if we ever need to look back at this test after some time has passed. Maybe you can expand on that // click Save button comment to explain that we're only trying to trigger the toast and are intentionally not making any change to the form?

@coliu-akamai
Copy link
Contributor Author

ty for the feedback!! Just pushed up changes to use mocks + add context and cleanup

@coliu-akamai coliu-akamai added the e2e Indicates that a PR touches Cypress tests in some way label Sep 5, 2024
@coliu-akamai coliu-akamai removed the e2e Indicates that a PR touches Cypress tests in some way label Sep 5, 2024
Copy link
Contributor

@hkhalil-akamai hkhalil-akamai left a comment

Choose a reason for hiding this comment

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

Great catch 👀 and thanks for adding unit and cypress tests!

loading: isPending,
type: 'submit',
}}
secondaryButtonProps={{ label: 'cancel', onClick: onClose }}
Copy link
Contributor

Choose a reason for hiding this comment

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

Nice catch!

@mjac0bs mjac0bs added Approved Multiple approvals and ready to merge! and removed Ready for Review labels Sep 10, 2024
Copy link
Contributor

@cpathipa cpathipa left a comment

Choose a reason for hiding this comment

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

Nice work! confirming on the RDNS drawers functionality and tests.

Copy link
Contributor

@jdamore-linode jdamore-linode left a comment

Choose a reason for hiding this comment

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

Nice work @coliu-akamai! The new test is really solid -- I ran it on repeat 15 times and it passed each time. Thanks again!

coliu-akamai and others added 2 commits September 11, 2024 10:42
Co-authored-by: jdamore-linode <97627410+jdamore-linode@users.noreply.github.com>
@coliu-akamai coliu-akamai merged commit 51b1efa into linode:develop Sep 11, 2024
@coliu-akamai coliu-akamai deleted the m3-7916 branch September 11, 2024 15:11
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! Bug Fixes for regressions or bugs

Projects

Archived in project

Development

Successfully merging this pull request may close these issues.

6 participants