Skip to content

fixes issue 239: classes derived from EnableQueryAttribute unable to unit test OnActionExecuted in isolation - #720

Closed
howcheng wants to merge 2 commits into
OData:mainfrom
howcheng:main
Closed

fixes issue 239: classes derived from EnableQueryAttribute unable to unit test OnActionExecuted in isolation#720
howcheng wants to merge 2 commits into
OData:mainfrom
howcheng:main

Conversation

@howcheng

Copy link
Copy Markdown

see #239

RequestQueryData requestQueryData = request.HttpContext.Items[nameof(RequestQueryData)] as RequestQueryData;

if (requestQueryData.QueryValidationRunBeforeActionExecution)
if (requestQueryData?.QueryValidationRunBeforeActionExecution ?? false)

@gathogojr gathogojr Dec 1, 2022

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

It took me a bit of time to understand this change. An easier to understand fix would be:

Suggested change
if (requestQueryData?.QueryValidationRunBeforeActionExecution ?? false)
if (requestQueryData != null && requestQueryData.QueryValidationRunBeforeActionExecution)

KenitoInc
KenitoInc previously approved these changes Dec 2, 2022

@KenitoInc KenitoInc left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Fix the change requested by @gathogojr and the PR will be good to merge

@KenitoInc

Copy link
Copy Markdown
Contributor

@howcheng Kindly sign the CLA agreement

@habbes

habbes commented Dec 20, 2022

Copy link
Copy Markdown
Contributor

Replaced by #776

@habbes habbes closed this Dec 20, 2022
@howcheng

Copy link
Copy Markdown
Author

@KenitoInc Where can I find the CLA agreement? Thanks.

@KenitoInc

Copy link
Copy Markdown
Contributor

@KenitoInc Where can I find the CLA agreement? Thanks.

We copied your code to #776
That's why we closed this PR

@howcheng

Copy link
Copy Markdown
Author

@KenitoInc OK, so I assume I don't need to sign anything then. Thanks.

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants