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-7038] - Add Firewall and Linode Configuration success toast notifications #9725

Merged
merged 18 commits into from
Oct 11, 2023
Merged

fix: [M3-7038] - Add Firewall and Linode Configuration success toast notifications #9725

merged 18 commits into from
Oct 11, 2023

Conversation

tyler-akamai
Copy link
Contributor

@tyler-akamai tyler-akamai commented Sep 27, 2023

Description 📝

The Firewall and Linode Configuration toast notifications did not have a success message, causing no toast notifications when an action was successful.

Major Changes 🔄

  • Added success notifications for
    • Firewall notifications (enable, disable, create, delete, device add, device remove)
    • Linode configurations (create, delete, update)
  • Updated toast notification messages for consistency

Preview 📷

Before
M3-7038.mov
After

Screenshot 2023-09-27 at 10 30 38 AM

Screenshot 2023-09-27 at 10 29 51 AM

Screenshot 2023-09-27 at 10 30 05 AM

Screenshot 2023-09-27 at 10 31 52 AM

Screenshot 2023-09-27 at 10 32 09 AM

Screenshot 2023-09-27 at 10 32 41 AM

Screenshot 2023-09-27 at 10 30 22 AM

Screenshot 2023-09-27 at 10 36 05 AM

How to test 🧪

*** Note: The toast notifications take a couple seconds to pop up.

Navigate to the firewall landing page (/firewalls) and ensure:

  • Creating a Firewall triggers a successful toast notification
  • Disabling a Firewall triggers a successful toast notification
  • Enabling a Firewall triggers a successful toast notification
  • Deleting a Firewall triggers a successful toast notification

Navigate to the linodes tab within any firewall details page (/firewalls/[firewall_ID]/linodes) and ensure:

  • Adding a Linode triggers a successful toast notification
  • Deleting a Linode triggers a successful toast notification

Navigate to the config tab within any linode details page (/linodes/[linode_id]/configurations) and ensure:

  • Adding a Linode configuration triggers a successful toast notification
  • Deleting a Linode configuration triggers a successful toast notification
  • Editing a Linode configuration triggers a successful toast notification

@tyler-akamai tyler-akamai requested a review from a team as a code owner September 27, 2023 14:23
@tyler-akamai tyler-akamai requested review from bnussman-akamai and jaalah-akamai and removed request for a team September 27, 2023 14:23
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.

Thanks for working on this!

Do you know why we're getting those delays (VS creating or deleting a Linode for instance)? Pretty important IMO because the toast ends up being a bit distracting. It happens a few seconds afterwards (that was my experience at least, sometimes close to 5/6 seconds) and it's a bit odd since we can clearly see the UI update (ex row gets deleted), then the notification happens much later.

Considering the UI updates + the notification menu event notification, I wonder about the worth of this update if the delay is that long

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.

I feel like linode_config*, firewall_enable, firewall_disable, firewall_create, firewall_device_add, firewall_device_remove toasts should just be done on the initial request, not based on events polling. I'd have to look at the API code and see what actually takes place when some of these events are created but I feel like most of the events I listed above don't need to wait on an event to show, This is mostly my opinion. Anyone else have thoughts?

@abailly-akamai
Copy link
Contributor

@bnussman-akamai ah yeah that makes sense, they're based on event polling. Agree 💯

@tyler-akamai
Copy link
Contributor Author

Awesome, will change that over now.

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.

Thanks for making those changes! Feels much better.

for consistency, in the firewall flows, do we also want to add toasts when adding inbound/outbound rules? /firewalls/{id}/rules

@@ -191,6 +195,9 @@ const FirewallRulesLanding = (props: Props) => {
// Reset editor state.
inboundDispatch({ rules: _rules.inbound ?? [], type: 'RESET' });
outboundDispatch({ rules: _rules.outbound ?? [], type: 'RESET' });
enqueueSnackbar('Successfully updated Firewall rules', {
Copy link
Contributor Author

@tyler-akamai tyler-akamai Sep 29, 2023

Choose a reason for hiding this comment

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

The firewalls inbound/outbound details section is a little odd. Instead of having an api call for each action (edit, delete, add, clone), the user first executes an action and then when they click 'Save Changes', the api is called. This makes my job a little easier with the Toast Notifications since they are all routed through the function applyChanges

Copy link
Contributor Author

Choose a reason for hiding this comment

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


onClose();
onClose();
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@bnussman-akamai I got rid of the error toast notification and made it so the dialog only closed if the mutateasync does not return an error.

Copy link
Member

Choose a reason for hiding this comment

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

Could we clean this up still? Should we put queryClient.invalidateQueries in the then? Should we put onClose in the then too? Should we revert to the await syntax?

Right now, it's unclear to me why enqueueSnackbar is in the then but queryClient.invalidateQueries and onClose are not

Copy link
Contributor Author

@tyler-akamai tyler-akamai Oct 4, 2023

Choose a reason for hiding this comment

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

@bnussman-akamai queryClient.invalidateQueries and onClose are in the then statement.

I can go back to the await syntax though to make it more clear.

@@ -218,6 +225,9 @@ const FirewallRulesLanding = (props: Props) => {
});
}
}
enqueueSnackbar('Failed to update Firewall rules', {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Kept this error because there is no "Save Changes" dialog

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.

Creating a Firewall triggers a successful toast notification 🚫
Disabling a Firewall triggers a successful toast notification ✅
Enabling a Firewall triggers a successful toast notification ✅
Deleting a Firewall triggers a successful toast notification ✅

Adding a Linode triggers a successful toast notification 🚫
Deleting a Linode triggers a successful toast notification ✅

Adding a Linode configuration triggers a successful toast notification ✅
Deleting a Linode configuration triggers a successful toast notification ✅
Editing a Linode configuration triggers a successful toast notification ✅


onClose();
onClose();
Copy link
Member

Choose a reason for hiding this comment

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

Could we clean this up still? Should we put queryClient.invalidateQueries in the then? Should we put onClose in the then too? Should we revert to the await syntax?

Right now, it's unclear to me why enqueueSnackbar is in the then but queryClient.invalidateQueries and onClose are not

@tyler-akamai
Copy link
Contributor Author

Creating a Firewall triggers a successful toast notification 🚫 Disabling a Firewall triggers a successful toast notification ✅ Enabling a Firewall triggers a successful toast notification ✅ Deleting a Firewall triggers a successful toast notification ✅

Adding a Linode triggers a successful toast notification 🚫 Deleting a Linode triggers a successful toast notification ✅

Adding a Linode configuration triggers a successful toast notification ✅ Deleting a Linode configuration triggers a successful toast notification ✅ Editing a Linode configuration triggers a successful toast notification ✅

The two tests that failed are corrected in the nodebalancer-firewall feature branch. Banks left a comment pointing out that the changes conflict with the feature branch.

So the functionality will be added shortly once the feature branch is merged.

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.

Approving based on latest comments about expected cases to be handled in a different PR

Copy link
Member

Choose a reason for hiding this comment

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

Looks like we might need to update an e2e test

Screenshot 2023-10-06 at 1 43 22 PM

@@ -116,7 +116,7 @@ describe('clone linode', () => {

ui.toast.assertMessage(`Your Linode ${newLinodeLabel} is being created.`);
ui.toast.assertMessage(
`Linode ${linode.label} has been cloned successfully to ${newLinodeLabel}.`
`Linode ${linode.label} successfully cloned to ${newLinodeLabel}.`
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I had to update the E2E tests to match the toast notification messages.

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! Thanks for resolving the e2e failures! 🎉

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants