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 error when trying to create an URL if only path is given #2995

Merged
merged 3 commits into from
Aug 18, 2021
Merged

Fix error when trying to create an URL if only path is given #2995

merged 3 commits into from
Aug 18, 2021

Conversation

TimonDB
Copy link
Contributor

@TimonDB TimonDB commented Aug 17, 2021 •
edited
Loading

This pull request provides a baseURL in the URL creation. This is needed because it is possible only a path is provided instead of a full URL.

@TimonDB TimonDB requested a review from a team as a code owner August 17, 2021 13:42
@TimonDB TimonDB requested review from bmesuere and niknetniko and removed request for a team August 17, 2021 13:42
niknetniko
niknetniko requested changes Aug 17, 2021
View reviewed changes
app/assets/javascripts/util.js Outdated Show resolved Hide resolved
@TimonDB TimonDB requested a review from niknetniko August 17, 2021 13:55
niknetniko
niknetniko requested changes Aug 17, 2021
View reviewed changes
Copy link
Member

@niknetniko niknetniko left a comment

Choose a reason for hiding this comment

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

Code looks good to me 👍 , but I think it's a good idea to add one or more test cases where a partial URL is used.

@TimonDB
Copy link
Contributor Author

TimonDB commented Aug 17, 2021

Code looks good to me , but I think it's a good idea to add one or more test cases where a partial URL is used.

Ah yes, fair point. I will add them

chvp
chvp approved these changes Aug 17, 2021
View reviewed changes
Copy link
Member

@chvp chvp left a comment

Choose a reason for hiding this comment

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

Agree with adding some tests, looks fine otherwise.

@bmesuere bmesuere merged commit 3fd30f7 into develop Aug 18, 2021
@bmesuere bmesuere deleted the fix/url-creation branch August 18, 2021 15:55
@bmesuere bmesuere restored the fix/url-creation branch August 19, 2021 11:52
@bmesuere bmesuere added bug Something isn't working deploy mestra Request a deployment on mestra labels Aug 19, 2021
@bmesuere bmesuere temporarily deployed to mestra August 19, 2021 11:52 Inactive
@bmesuere bmesuere deleted the fix/url-creation branch August 19, 2021 11:54
@github-actions github-actions bot removed the deploy mestra Request a deployment on mestra label Aug 19, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@chvp chvp chvp approved these changes

@niknetniko niknetniko niknetniko approved these changes

@bmesuere bmesuere bmesuere approved these changes

Assignees
No one assigned
Labels
bug Something isn't working
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@TimonDB @bmesuere @niknetniko @chvp