Skip to content

poc: [M3-9996] - Assigning alert definitions to a Linode during creation #12248

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

Draft
wants to merge 14 commits into
base: develop
Choose a base branch
from

Conversation

pmakode-akamai
Copy link
Contributor

@pmakode-akamai pmakode-akamai commented May 20, 2025

Description 📝

An alternative way to ensure alerts are assigned only during the Linode creation process, with RHF integrated to support Create Linode form flow

Note

Changes 🔄

  • Added a support to assign alert definitions to a Linode during creation

Target release date 🗓️

N/A

Preview 📷

Alerts Mode (Create Linode Flow) Preview
Legacy Alerts Screenshot 2025-05-20 at 8 18 16 PM
Beta Alerts Screenshot 2025-05-20 at 8 18 52 PM

How to test 🧪

Prerequisites

  • Enable MSW and the ACLP Integration feature flag

Verification steps

  • Verify the request payload for both legacy and beta alerts in the Create Linode flow:
    • Ensure the alerts object is included only when the ACLP beta mode preference toggle is selected (and that it contains the correct enabled alert Ids for both system and user alerts)
    • Ensure the alerts object is undefined or not present or excluded in legacy mode
  • Confirm that the Alerts behavior in the Create Linode flow meets the product requirements
Author Checklists

As an Author, to speed up the review process, I considered 🤔

👀 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

  • I have read and considered all applicable items listed above.

As an Author, before moving this PR from Draft to Open, I confirmed ✅

  • All unit tests are passing
  • TypeScript compilation succeeded without errors
  • Code passes all linting rules

@pmakode-akamai pmakode-akamai self-assigned this May 20, 2025
@pmakode-akamai pmakode-akamai added the ACLP Integration CI (Cloud Interfaces) Support for CC (Core Compute) CloudPulse Integration label May 20, 2025
@pmakode-akamai pmakode-akamai added the Linodes Dealing with the Linodes section of the app label May 20, 2025
Comment on lines 85 to 101
const handleToggleCreateFlow = (alert: Alert) => {
const alerts = field.value;
const currentAlertIds = alerts?.[alert.type] || [];
const updatedAlerts = { ...alerts };

if (currentAlertIds?.includes(alert.id)) {
// Disable the alert (remove from the list)
updatedAlerts[alert.type] = currentAlertIds.filter(
(id) => id !== alert.id
);
} else {
// Enable the alert (add to the list)
updatedAlerts[alert.type] = [...currentAlertIds, alert.id];
}

field.onChange(updatedAlerts);
};
Copy link
Contributor

@nikhagra-akamai nikhagra-akamai May 20, 2025

Choose a reason for hiding this comment

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

great work @pmakode-akamai

@jaalah-akamai this type of similar approach we've in our mind. Only difference is instead of using complete form context, we'll introduce an additional prop i.e. onChange handler function to reusable component that will update the latest value on every toggle click on which @ankita-akamai is working on.

In this way, we can avoid tight coupling of reusable component with any service specific types/forms as you can see the snippet at line 78 and can keep it completely flexible and easy to integrate.

May be, we can discuss any doubts or suggestions regarding this approach in detail either tomorrow or in weekly syncup

Copy link
Contributor Author

@pmakode-akamai pmakode-akamai May 21, 2025

Choose a reason for hiding this comment

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

@nikhagra-akamai @ankita-akamai

Although the PR implementation works, if we want to keep the ACLP related reusable component decoupled, then your suggestion makes sense -- avoiding the use of react-hook-form context in these components helps maintain that separation and move this RHF logic two levels up into the Create Linode-specific Alerts logic.

I’ve just updated the PR -- I think this should be the approach we’re looking for?

@linode-gh-bot
Copy link
Collaborator

Cloud Manager UI test results

🎉 599 passing tests on test run #7 ↗︎

❌ Failing✅ Passing↪️ Skipped🕐 Duration
0 Failing599 Passing4 Skipped114m 30s

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
ACLP Integration CI (Cloud Interfaces) Support for CC (Core Compute) CloudPulse Integration Linodes Dealing with the Linodes section of the app
Projects
Status: Review
Development

Successfully merging this pull request may close these issues.

4 participants