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

fix: Decode spaces in factory url #976

Merged
merged 4 commits into from
Nov 9, 2023
Merged

fix: Decode spaces in factory url #976

merged 4 commits into from
Nov 9, 2023

Conversation

tolusha
Copy link
Contributor

@tolusha tolusha commented Nov 6, 2023

What does this PR do?

Decode spaces in factory url

What issues does this PR fix or reference?

eclipse-che/che#22458

Is it tested? How?

N/A

Release Notes

N/A

Docs PR

@che-bot
Copy link
Contributor

che-bot commented Nov 6, 2023

Click here to review and test in web IDE: Contribute

olexii4
olexii4 approved these changes Nov 6, 2023
View reviewed changes
Copy link
Contributor

@olexii4 olexii4 left a comment

Choose a reason for hiding this comment

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

LGTM

@openshift-ci openshift-ci bot added the lgtm label Nov 6, 2023
@codecov Codecov
Copy link

codecov bot commented Nov 6, 2023 •
edited
Loading

Codecov Report

Merging #976 (6fdb8f8) into main (3ca3f0b) will decrease coverage by 0.02%.
Report is 3 commits behind head on main.
The diff coverage is 0.00%.

@@            Coverage Diff             @@
##             main     #976      +/-   ##
==========================================
- Coverage   85.15%   85.13%   -0.02%     
==========================================
  Files         380      380              
  Lines       39186    39198      +12     
  Branches     2523     2523              
==========================================
+ Hits        33370    33373       +3     
- Misses       5789     5798       +9     
  Partials       27       27
Flag Coverage Δ
unittests 85.13% <0.00%> (-0.02%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

Files Coverage Δ
...frontend/src/services/backend-client/factoryApi.ts 91.89% <0.00%> (-8.11%) ⬇️
...hboard-frontend/src/store/FactoryResolver/index.ts 85.30% <0.00%> (-1.88%) ⬇️

... and 1 file with indirect coverage changes

@github-actions GitHub Actions
Copy link

github-actions bot commented Nov 6, 2023

Docker image build succeeded: quay.io/eclipse/che-dashboard:pr-976

ibuziuk
ibuziuk reviewed Nov 7, 2023
View reviewed changes
packages/dashboard-backend/src/routes/factoryAcceptanceRedirect.ts Outdated
@@ -37,6 +37,10 @@ export function registerFactoryAcceptanceRedirect(instance: FastifyInstance): vo
params.append(ERROR_CODE_ATTR, querystring.unescape(query[ERROR_CODE_ATTR] as string));
}

const url = params.get('url');
if (url) {
params.set('url', url.replaceAll(' ', '%20'));
Copy link
Member

Choose a reason for hiding this comment

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

can we use encodeURIComponent to cover the general case? https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/encodeURIComponent

@openshift-ci openshift-ci bot removed the lgtm label Nov 7, 2023
@github-actions GitHub Actions
Copy link

github-actions bot commented Nov 7, 2023

Docker image build succeeded: quay.io/eclipse/che-dashboard:pr-976

akurinnoy
akurinnoy approved these changes Nov 8, 2023
View reviewed changes
Copy link
Contributor

@akurinnoy akurinnoy left a comment

Choose a reason for hiding this comment

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

LGTM

@openshift-ci openshift-ci bot added the lgtm label Nov 8, 2023
@tolusha
Copy link
Contributor Author

tolusha commented Nov 8, 2023

/retest

ibuziuk
ibuziuk approved these changes Nov 8, 2023
View reviewed changes
Copy link
Member

@ibuziuk ibuziuk left a comment

Choose a reason for hiding this comment

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

I'm ok to merge as a hot fix for 3.10, but not really understand why do we decoding twice in general🤷

@tolusha tolusha marked this pull request as draft November 8, 2023 14:11
@openshift-ci openshift-ci bot removed the lgtm label Nov 8, 2023
@tolusha
fixup
e4c1756 
Signed-off-by: Anatolii Bazko <[email protected]>
@tolusha
fixes
9598e8e 
Signed-off-by: Anatolii Bazko <[email protected]>
@tolusha tolusha requested review from olexii4 and akurinnoy November 8, 2023 16:06
@github-actions GitHub Actions
Copy link

github-actions bot commented Nov 8, 2023

Docker image build succeeded: quay.io/eclipse/che-dashboard:pr-976

@tolusha tolusha marked this pull request as ready for review November 9, 2023 07:52
@tolusha
fix
6fdb8f8 
Signed-off-by: Anatolii Bazko <[email protected]>
@github-actions GitHub Actions
Copy link

github-actions bot commented Nov 9, 2023

Docker image build succeeded: quay.io/eclipse/che-dashboard:pr-976

@openshift-ci openshift-ci bot added the lgtm label Nov 9, 2023
@openshift-ci OpenShift CI
Copy link

openshift-ci bot commented Nov 9, 2023

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: akurinnoy, ibuziuk, olexii4, tolusha

The full list of commands accepted by this bot can be found here.

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@tolusha tolusha merged commit 5fb409d into main Nov 9, 2023
11 of 13 checks passed
@tolusha tolusha deleted the 22458 branch November 9, 2023 14:39
@devstudio-release
Copy link

Build 3.10 :: dashboard_3.10/19: Console, Changes, Git Data

@devstudio-release
Copy link

Build 3.11 :: dashboard_3.x/391: Console, Changes, Git Data

@devstudio-release
Copy link

Build 3.10 :: sync-to-downstream_3.10/123: Console, Changes, Git Data

@devstudio-release
Copy link

Build 3.11 :: sync-to-downstream_3.x/5216: Console, Changes, Git Data

@devstudio-release
Copy link

Build 3.10 :: get-sources-rhpkg-container-build_3.10/123: Console, Changes, Git Data

@devstudio-release
Copy link

Build 3.11 :: get-sources-rhpkg-container-build_3.x/5095: Console, Changes, Git Data

@devstudio-release
Copy link

Build 3.10 :: get-sources-rhpkg-container-build_3.10/123: SUCCESS

dashboard : 3.10 :: Build 56864823 : dashboard-rhel8-3.10-53
SUCCESS; copied to quay

@devstudio-release
Copy link

Build 3.10 :: dashboard_3.10/19: SUCCESS

Upstream sync done; /DS_CI/sync-to-downstream_3.10/123 triggered

@devstudio-release
Copy link

Build 3.11 :: get-sources-rhpkg-container-build_3.x/5095: SUCCESS

dashboard : 3.x :: Build 56864929 : dashboard-rhel8-3.11-19
SUCCESS; copied to quay

@devstudio-release
Copy link

Build 3.11 :: dashboard_3.x/391: SUCCESS

Upstream sync done; /DS_CI/sync-to-downstream_3.x/5216 triggered

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

@akurinnoy akurinnoy akurinnoy approved these changes

@ibuziuk ibuziuk ibuziuk approved these changes

@olexii4 olexii4 Awaiting requested review from olexii4 olexii4 is a code owner

Assignees

@ibuziuk ibuziuk

@olexii4 olexii4

@akurinnoy akurinnoy

Labels
lgtm
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@tolusha @che-bot @devstudio-release @ibuziuk @olexii4 @akurinnoy