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.

Sign up for GitHub

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

Jump to bottom

Logging bug in HttpRequesterMiddleware #1953

Merged
merged 4 commits into from
Feb 27, 2024
Merged

Logging bug in HttpRequesterMiddleware #1953

merged 4 commits into from
Feb 27, 2024

Conversation

masoud-bahrami
Copy link
Contributor

@masoud-bahrami masoud-bahrami commented Jan 30, 2024 •
edited by raman-m
Loading

BadRequest should be logged as debug!

Update by Maintainer on Feb 27, 2024

BadRequest should be logged as warning via LogWarning method.

@raman-m
Copy link
Member

raman-m commented Jan 30, 2024

@masoud-bahrami
Now Masoud, please explain what did you do in this PR?
I see a small refactoring here... Nothing more!

Have you read our development process docs?

@masoud-bahrami
Copy link
Contributor Author

masoud-bahrami commented Jan 31, 2024 •
edited
Loading

@raman-m In the CreateLogBasedOnResponse method due to if (response.Data?.StatusCode <= HttpStatusCode.BadRequest) logic, BadRequest will be logged as Information.

As you can see, In the next branch of if, else if (response.Data?.StatusCode >= HttpStatusCode.BadRequest) again HttpStatusCode.BadRequest is checked.

It's like
If (a <= 400) do something else If (a >= 400) do something else

@raman-m
Copy link
Member

raman-m commented Feb 16, 2024

@masoud-bahrami

BadRequest should be logged as debug!

Did you mean as warning? 😉 to be logged by LogWarning method?
OK... Now I see!..
It is a small comparison typo <= in the code but resulting to real bug...

@raman-m raman-m added bug Identified as a potential bug Jan'24 January 2024 release labels Feb 16, 2024
@raman-m raman-m added this to the January'24 milestone Feb 16, 2024
@raman-m
Copy link
Member

raman-m commented Feb 16, 2024 •
edited
Loading

@ggnaegi Tom is the author of this tiny ugly bug in commit ebe662a 🤣 added in version 14.0.3
Please review!
I will ask unit & acceptance tests...

raman-m
raman-m requested changes Feb 16, 2024
View reviewed changes
Copy link
Member

@raman-m raman-m left a comment

Choose a reason for hiding this comment

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

Please cover this scenario by unit test(s)!
Will be good to write acceptance tests too

@raman-m raman-m changed the title Fix a bug in HttpRequesterMiddleware.cs, Logging bug in HttpRequesterMiddleware Feb 16, 2024
@masoud-bahrami @raman-m
Fix a bug in HttpRequesterMiddleware.cs,
6449783 
BadRequest should be logged as debug!
@raman-m raman-m merged commit 171e3a7 into ThreeMammals:develop Feb 27, 2024
2 checks passed
@raman-m raman-m mentioned this pull request Mar 1, 2024
Merged
@raman-m raman-m mentioned this pull request May 16, 2024
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@raman-m raman-m raman-m approved these changes

Assignees

@masoud-bahrami masoud-bahrami

Labels
bug Identified as a potential bug Jan'24 January 2024 release
Projects
None yet
Milestone
January'24
Development

Successfully merging this pull request may close these issues.
2 participants
@masoud-bahrami @raman-m