Skip to content

Fix for REST query string URL encoding #7795

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

Merged
merged 2 commits into from
Sep 16, 2022
Merged

Fix for REST query string URL encoding #7795

merged 2 commits into from
Sep 16, 2022

Conversation

mike12345567
Copy link
Collaborator

@mike12345567 mike12345567 commented Sep 15, 2022

Description

Fixing issue introduced by fix for #7683 - encoding the query string caused handlebars statements to break, this rectifies that.

Also minor fix for z-index issue with a tooltip.

Addresses:

…caused handlebars statements to break, this rectifies that.
@codecov-commenter
Copy link

codecov-commenter commented Sep 15, 2022

Codecov Report

Merging #7795 (4265462) into master (9bf4ef0) will not change coverage.
The diff coverage is 79.41%.

@@           Coverage Diff           @@
##           master    #7795   +/-   ##
=======================================
  Coverage   67.77%   67.77%           
=======================================
  Files         125      125           
  Lines        4162     4162           
  Branches      655      655           
=======================================
  Hits         2821     2821           
  Misses        941      941           
  Partials      400      400           
Impacted Files Coverage Δ
...ackages/server/src/utilities/rowProcessor/utils.js 41.86% <0.00%> (ø)
packages/server/src/utilities/index.js 80.00% <75.00%> (ø)
packages/server/src/automations/triggers.js 85.24% <76.19%> (ø)
packages/server/src/api/controllers/backup.js 100.00% <100.00%> (ø)
packages/server/src/automations/index.js 87.50% <100.00%> (ø)
packages/server/src/utilities/usageQuota/rows.js 85.71% <100.00%> (ø)
... and 1 more

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

Copy link
Member

@shogunpurple shogunpurple left a comment

Choose a reason for hiding this comment

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

LGTM - just a few log lines left in - but the actual parsing logic with HBS would be well covered with a test just for safety.

@@ -46,7 +48,19 @@ export function buildQueryString(obj) {
if (str !== "") {
str += "&"
}
str += `${key}=${encodeURIComponent(value || "")}`
const bindings = findHBSBlocks(value)
Copy link
Member

Choose a reason for hiding this comment

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

It would be really good to have a test for this, if possible

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Did my best to get jest tests working in the builder, had to remove some of the old (disabled) jest tests as they just don't work anymore and probably need re-thought.

This is probably something we need to extend out further - cypress is great but jest tests are nice for little unit test type things. I've made the (limited) builder tests part of CI so that they hopefully offer some value from here forward.

Copy link
Member

@shogunpurple shogunpurple left a comment

Choose a reason for hiding this comment

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

LGTM! Thanks for putting together the jest setup 👌

@mike12345567 mike12345567 merged commit 57c486b into master Sep 16, 2022
@github-actions github-actions bot locked and limited conversation to collaborators Sep 16, 2022
@mike12345567 mike12345567 deleted the fix/7683 branch September 16, 2022 11:22
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants