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-6430] - Event entities should only be linked for true labels #9003

Conversation

abailly-akamai
Copy link
Contributor

@abailly-akamai abailly-akamai commented Apr 14, 2023

Description 📝

Issue
Our applyLinking script was attempting to replace entity labels within backticks, resulting in plaintext HTML in our event list. It was also not linking the true entity because the replace instance wasn't using a global modifier in case two instances of the label were matching.

Solution
Modified the replace to only target true labels and leave alone strings surrounded by backticks. Also added a test for the applyLinking function

Preview 📷

Before
Screenshot 2023-04-14 at 2 27 36 PM

After
Screenshot 2023-04-14 at 2 27 23 PM

How to test 🧪

  1. add a new domain
  2. add a new ttl record with a different name
  3. add a new ttl record with same name as domain
  4. navigate to /events

@abailly-akamai abailly-akamai self-assigned this Apr 14, 2023
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! @abailly-akamai

@@ -87,4 +88,52 @@ describe('Event message generation', () => {
).not.toMatch('booted with');
});
});

describe('apply linking to labels', () => {
Copy link
Contributor

Choose a reason for hiding this comment

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

Thank you for adding test coverage!

@cpathipa cpathipa added the Approved Multiple approvals and ready to merge! label Apr 17, 2023
Comment on lines 94 to 98
const mockEvent = {
action: 'create',
entity: null,
secondary_entity: null,
};
Copy link
Member

Choose a reason for hiding this comment

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

Can we use the eventFactory from packages/manager/src/factories/events.ts to create the mockEvents so we don't have to type cast?

What can we do to avoid message as any?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

message is a required prop, which can't be null which is why it's casted. not particularly important to be very strict with the type here. That being said I refactored some of the unit to use the factory for events 👍

@abailly-akamai abailly-akamai merged commit 31f9dec into linode:develop Apr 17, 2023
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.

4 participants