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

🐛 monitor middleware - fix ignore custom settings #2024

Merged

Conversation

wangjq4214
Copy link
Member

Please provide enough information so that others can review your pull request:

Fix monitor middleware ignores custom settings if default title is used.

Explain the details for making this change. What existing problem does the pull request solve?

close #2019

@wangjq4214 wangjq4214 marked this pull request as ready for review August 19, 2022 08:35
Copy link
Member

@efectn efectn left a comment

Choose a reason for hiding this comment

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

Can you add some test cases

@ReneWerner87 ReneWerner87 changed the title 🐛 fix ignore custom settings 🐛 [monitor] fix ignore custom settings Aug 19, 2022
@ReneWerner87 ReneWerner87 changed the title 🐛 [monitor] fix ignore custom settings 🐛 monitor middleware - fix ignore custom settings Aug 19, 2022
@ReneWerner87
Copy link
Member

Can you add some test cases

would be nice
then we probably would have noticed it in the beginning and no one can break it without us noticing it

middleware/monitor/config.go Outdated Show resolved Hide resolved
Copy link
Member

@gaby gaby left a comment

Choose a reason for hiding this comment

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

These changes are not fixing the problem. The else statement never runs if the default Title and Refresh are used which is the original issue

@wangjq4214
Copy link
Member Author

These changes are not fixing the problem. The else statement never runs if the default Title and Refresh are used which is the original issue

To be honest I don't really understand what the point of this judgment is, it seems to me to be unnecessary.

@gaby
Copy link
Member

gaby commented Aug 19, 2022

These changes are not fixing the problem. The else statement never runs if the default Title and Refresh are used which is the original issue

To be honest I don't really understand what the point of this judgment is, it seems to me to be unnecessary.

No judging, just wanted to point the issue with the URL's. The original commit fixed the issue for the "refresh" field. Looks better now! 👍🏻

@wangjq4214
Copy link
Member Author

@ReneWerner87 @efectn @gaby Please check out the new changes when you have time.

Copy link
Member

@gaby gaby 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! Thanks! 🚀

@ReneWerner87 ReneWerner87 merged commit 32d311c into gofiber:master Aug 19, 2022
@wangjq4214 wangjq4214 deleted the fix-monitor-ignore-custom-settings branch August 27, 2022 01:56
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.

🐛 [Bug]: Monitor middleware ignores custom settings if default title is used.
4 participants