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

hotfix: Notification Menu crash due to null value #9331

Merged

Conversation

hkhalil-akamai
Copy link
Contributor

@hkhalil-akamai hkhalil-akamai commented Jun 27, 2023

Description 📝

Fixes a bug that causes the app to crash when opening the Notification Menu.

Major Changes 🔄

  • Updates Entity type in api-v4 package to reflect the possibility of a null label
  • Adds null check to applyLinking function

How to test 🧪

  1. Verify opening the Events Menu does not cause a crash when a user has an entity event with a null label (e.g., entity_transfer)

@hkhalil-akamai hkhalil-akamai added Bug Fixes for regressions or bugs @linode/api-v4 Changes are made to the @linode/api-v4 package Hotfix Hotfix: This is going to staging labels Jun 27, 2023
@hkhalil-akamai hkhalil-akamai self-assigned this Jun 27, 2023
@hkhalil-akamai hkhalil-akamai removed Release → Staging Pre-Release: Release → Staging Missing Changeset labels Jun 27, 2023
@bnussman-akamai bnussman-akamai added Release → Staging Pre-Release: Release → Staging labels Jun 27, 2023
mjac0bs
mjac0bs previously approved these changes Jun 27, 2023
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.

Confirmed that the notification menu and events landing pages no longer result in a crash with an entity_transfer event. Nice catch here. Can we also submit a request to get the API docs updated to show this field as nullable?

Screenshot 2023-06-27 at 2 37 19 PM

packages/manager/CHANGELOG.md Outdated Show resolved Hide resolved
@bnussman-akamai bnussman-akamai added Add'tl Approval Needed Waiting on another approval! and removed Ready for Review labels Jun 27, 2023
abailly-akamai
abailly-akamai previously approved these changes Jun 27, 2023
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! I'll make a ticket to refactor this file.

  1. It's another case of crashing the browser where we could error a bit more gracefully (try/catch)
  2. We've got 100s of instances of entity.label handled in various ways which we should try to consolidate

packages/manager/src/eventMessageGenerator.ts Show resolved Hide resolved
@bnussman-akamai bnussman-akamai added Add'tl Approval Needed Waiting on another approval! and removed Ready for Review labels Jun 27, 2023
@bnussman-akamai bnussman-akamai added Add'tl Approval Needed Waiting on another approval! and removed Ready for Review labels Jun 27, 2023
@bnussman-akamai bnussman-akamai added Approved Multiple approvals and ready to merge! and removed Add'tl Approval Needed Waiting on another approval! labels Jun 27, 2023
@cypress
Copy link

cypress bot commented Jun 28, 2023

5 failed tests on run #4577 ↗︎

5 162 3 0 Flakiness 0

Details:

Check all instances of entity.label in eventMesssageGenerator
Project: Cloud Manager E2E Commit: a676e3851a
Status: Failed Duration: 20:14 💡
Started: Jun 28, 2023 12:23 AM Ended: Jun 28, 2023 12:44 AM
Failed  firewalls/update-firewall.spec.ts • 1 failed test

View Output Video

Test Artifacts
update firewall > updates a firewall's linodes and rules Output Screenshots Video
Failed  images/create-linode-from-image.spec.ts • 2 failed tests

View Output Video

Test Artifacts
create linode from image, mocked data > creates linode from image on images tab Output Screenshots Video
create linode from image, mocked data > creates linode from preselected image on images tab Output Screenshots Video
Failed  firewalls/create-firewall.spec.ts • 1 failed test

View Output Video

Test Artifacts
create firewall > creates a firewall assigned to a linode Output Screenshots Video
Failed  images/smoke-create-image.spec.ts • 1 failed test

View Output Video

Test Artifacts
create image > captures image from Linode and mocks create image Output Screenshots Video

This comment has been generated by cypress-bot as a result of this project's GitHub integration settings.

@jaalah-akamai jaalah-akamai merged commit 16b3d79 into linode:staging Jun 28, 2023
@hkhalil-akamai hkhalil-akamai deleted the hotfix/fix-null-label-crash branch June 28, 2023 12:53
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 Hotfix Hotfix: This is going to staging @linode/api-v4 Changes are made to the @linode/api-v4 package Release → Staging Pre-Release: Release → Staging
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants