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

fix: [M3-7977] - Surface interface error in Linode Config dialog #10429

Merged

Conversation

mjac0bs
Copy link
Contributor

@mjac0bs mjac0bs commented May 1, 2024

Description 📝

There were a couple of problems in the Add/Edit Configuration dialog:

  • Interface select errors were not always being surfaced.
  • This was fixed in PR feat: [M3-8069] - scrollErrorIntoViewV2 - POC #10459: General errors were not reliably being focused by scrollErrorIntoView because the scrolling can happen before the general error is updated in form state (via formik.setStatus), and therefore there is no notice to scroll to in the DOM, since generalError being defined is a condition for rendering the notice.

Note

This PR's scope is small. There is more going on with the Linode Config Dialog because it's too large of a component handling both Create and Edit state and should be refactored. This is resulting in:

  • A bug related to form state for the interfaces array causing it to sometimes display incorrect form values for interfaces in Create (or Edit) mode, depending on the actions the user has taken, e.g. previously opening the Edit (or Create).
  • An issue permitting the user to Edit an existing config and add a VLAN in eth0, because primary (interface) is set to false. Instead of seeing the same error as in the Add Config dialog detailed beflow, the request submits successfully but the interfaces no longer have a primary interface.
    Creating a follow up ticket with subtasks to refactor this component and address its issues: M3-8165.

Changes 🔄

  • Surfaced a previously unsurfaced API error on config interfaces by adding an error notice in the Networking section of the Config dialog.

Target release date 🗓️

6/10

Preview 📷

Before After
Screenshot 2024-05-28 at 11 33 16 AM Screenshot 2024-05-28 at 11 31 48 AM

How to test 🧪

Reproduction steps

(How to reproduce the issue, if applicable)

  • Test the following in production:

Error: assigning a VLAN to eth0 is not a valid configuration, but does not surface an error
Repro:

  1. Add a Config.
  2. Primary interface should be eth0.
  3. Set eth0 to VLAN and give it an IP a name and optionally IP address.
  4. Set eth1 to None.
  5. Set eth2 to None.
  6. Click Add Configuration.
  7. Observe nothing happens.
  8. Open the browser tools and note the failed 400 POST request with the error:
{reason: "Field is not allowed for this interface type", field: "interfaces[0].primary"}

Verification steps

(How to verify changes)

  • Check out this branch and repeat the above steps to trigger errors.
  • Confirm that for each error, it is scrolled into view.
  • Confirm that assigning a VLAN to eth0 does show a surfaced error.

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

@mjac0bs mjac0bs self-assigned this May 1, 2024
@abailly-akamai
Copy link
Contributor

@mjac0bs see #10459 for POC for improved util

@abailly-akamai
Copy link
Contributor

@mjac0bs #10459 has been merge so this PR is unblocked

please let me know if you need any help moving forward. Sorry it took a while!

@mjac0bs mjac0bs force-pushed the M3-7977-improve-edit-config-error-visibility branch from aa1c0d7 to f4db602 Compare May 24, 2024 20:26
@mjac0bs mjac0bs changed the title fix: [M3-7977] - Improve visibility of errors in Linode Config dialog fix: [M3-7977] - Surface interface error in Linode Config dialog May 28, 2024
@mjac0bs mjac0bs marked this pull request as ready for review May 28, 2024 21:00
@mjac0bs mjac0bs requested a review from a team as a code owner May 28, 2024 21:00
@mjac0bs mjac0bs requested review from jdamore-linode and abailly-akamai and removed request for a team May 28, 2024 21:00
Copy link

Coverage Report:
Base Coverage: 82.29%
Current Coverage: 82.29%

Copy link
Contributor

@abailly-akamai abailly-akamai left a comment

Choose a reason for hiding this comment

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

Confirming error is now visible and scrolled into view ✅

@mjac0bs mjac0bs added Add'tl Approval Needed Waiting on another approval! and removed Ready for Review labels May 29, 2024
@bnussman-akamai bnussman-akamai added Approved Multiple approvals and ready to merge! and removed Add'tl Approval Needed Waiting on another approval! labels May 29, 2024
@mjac0bs mjac0bs merged commit c4b60ae into linode:develop May 29, 2024
18 checks passed
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!
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants