Alerting: Fix incorrect decoding for alert rules with % characters #76148
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
gillesdemey
requested review from
VikaCep,
konrad147 and
soniaAguilarPeiron
and removed request for
a team
|
Tests are currently failing because they rely pretty heavily on mocked location data passed in to the ruler viewer – I'll need to find a way around that to unblock this PR. |
konrad147
changed the title
…ecode-alert-rule-name
|
/deploy-to-hg |
|
|
soniaAguilarPeiron
approved these changes
Oct 11, 2023
There was a problem hiding this comment.
- are we confident this patch to
historywon't cause unintended behaviour elsewhere in the app? - if so, can we please create an issue to upgrade
react-router+history, including the context from https://github.com/grafana/grafana/pull/76148/files#diff-d39aa8bb5f5c32177ba0cef60b1277b58288013d5d2ac2daaa68930771e4c350R251-R256, and also reminding us to remove the patch. i don't know where that issue should live - tbh it probably ends up on frontend platform's board 😬
gillesdemey
commented
Oct 13, 2023
| // It was probably fixed in React-Router v6 | ||
| type PathWithOptionalID = { id?: string }; | ||
| export function getRuleIdFromPathname(params: PathWithOptionalID): string | undefined { | ||
| const { pathname = '' } = locationService.getLocation(); |
There was a problem hiding this comment.
Now that we're patching the router, do we still need this workaround? Can we go back to using useParams()?
There was a problem hiding this comment.
I need to verify. IIRC react-router-dom has the same bug. In this PR we only patch the history package so it doesn't break outgoing links, but it shouldn't affect parsing path params
I created this issue @ashharrison90 , should I assign to the Grafana Frontend platform project? |
|
@soniaAguilarPeiron sure 👍 |
@ashharrison90 This patch does the same as this fix , that has been merged to main. So, we believe it should not break anything. We tested locally and on the ephemeral instance. We cannot be 100% confident about not breaking anything, but we believe it should not. |
|
/deploy-to-hg |
|
|
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
What is this feature?
This PR fixes a crash in the front-end when trying to view the details for an alert rule that has a "%" character encoded in the alert rule name.
This can be traced to a bug in react-router (remix-run/history#505).
the rule editor was also affected, it didn't crash but also did not properly decode the rule identifier.
Special notes for your reviewer:
Some additional manual testing would be appreciated since I failed to successfully write a test to mock
window.location