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 IWebhook.GetUdi() and IEntity.GetUdi() extension methods #15288

Merged
merged 4 commits into from
Nov 23, 2023

Conversation

ronaldbarendse
Copy link
Contributor

Prerequisites

  • I have added steps to test this contribution in the description below

Description

Although PR #15267 includes most of the boilerplate code to make Webhook implement IEntity, the GetUdi() extension method wasn't updated to use the interface and the IEntity.GetUdi() extension method didn't support the webhook entity type. This PR fixes both of these issues in the first commit.

I've also taken the opportunity to refactor the code in the second commit, so it's much more compact and readable by using:

  • ArgumentNullException.ThrowIfNull() for all null checks (although we're already in a nullable context);
  • switch expressions, which also takes care of detecting unreachable code, in case multiple types would match (e.g. IContent and IContentBase).

Finally, I've also added a quick unit test to verify the GetUdi() extension methods do what they should 😄

Copy link
Contributor

@AndyButland AndyButland 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 to me. I just followed the "be consistent trail" a little bit more and added a "webhook builder" to use in the added unit test.

@Zeegaan
Copy link
Member

Zeegaan commented Nov 23, 2023

Lgtm 👍

@Zeegaan Zeegaan merged commit 5ad4001 into release/13.0 Nov 23, 2023
13 checks passed
@Zeegaan Zeegaan deleted the v13/bugfix/iwebhook-getudi branch November 23, 2023 07:02
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.

3 participants