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

Re-implement util functions using URL API #2954

Merged
merged 15 commits into from
Aug 10, 2021
Merged

Conversation

TimonDB
Copy link
Contributor

@TimonDB TimonDB commented Aug 3, 2021

This pull request re-implements the helper URL functions by using the URL API. Also, a test file for these functions is added.

Closes #2614 .

@chvp chvp added chore Repository/build/dependency maintenance enhancement A change that isn't substantial enough to be called a feature and removed chore Repository/build/dependency maintenance labels Aug 3, 2021
@TimonDB TimonDB marked this pull request as ready for review August 4, 2021 12:43
@TimonDB TimonDB requested a review from a team as a code owner August 4, 2021 12:43
@TimonDB TimonDB requested review from bmesuere and chvp and removed request for a team August 4, 2021 12:43
app/assets/javascripts/util.js Outdated Show resolved Hide resolved
app/assets/javascripts/util.js Outdated Show resolved Hide resolved
app/assets/javascripts/util.js Outdated Show resolved Hide resolved
app/assets/javascripts/util.js Outdated Show resolved Hide resolved
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.

The issue also mentios the updateURL... functions. Is there a reason these weren't changed?

app/assets/javascripts/util.js Outdated Show resolved Hide resolved
app/assets/javascripts/util.js Outdated Show resolved Hide resolved
@TimonDB
Copy link
Contributor Author

TimonDB commented Aug 4, 2021

The issue also mentios the updateURL... functions. Is there a reason these weren't changed?

@chvp The updateURL... functions sometimes have only a pathname as URL argument (e.g. nl/submissions/53 was one I encountered), this resulted in an error (Error: Invalid URL) when trying to create the URL const url = new URL(_url)

@niknetniko
Copy link
Member

The updateURL... functions sometimes have only a pathname as URL argument (e.g. nl/submissions/53 was one I encountered), this resulted in an error (Error: Invalid URL) when trying to create the URL const url = new URL(_url)

Is this used this way in a lot of places? Because if not, it might be worth to update the caller to provide the full URL, allowing us to also update the updateURL... functions, which would be nice.

Copy link
Member

@bmesuere bmesuere left a comment

Choose a reason for hiding this comment

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

I also agree with @niknetniko's comment

app/assets/javascripts/util.js Outdated Show resolved Hide resolved
app/assets/javascripts/util.js Outdated Show resolved Hide resolved
@TimonDB
Copy link
Contributor Author

TimonDB commented Aug 4, 2021

The updateURL... functions sometimes have only a pathname as URL argument (e.g. nl/submissions/53 was one I encountered), this resulted in an error (Error: Invalid URL) when trying to create the URL const url = new URL(_url)

Is this used this way in a lot of places? Because if not, it might be worth to update the caller to provide the full URL, allowing us to also update the updateURL... functions, which would be nice.

So far, I only found this to be true in the function loadfeedback in exercise.js. The other callers seem to provide a full URL

@chvp
Copy link
Member

chvp commented Aug 5, 2021

The easiest way to fix that there would probably be to add a base_submissions_url data attribute in the HTML. This can then be read in the setup function and used in place of the /submissions/ strings in the javascript file.

@dodona-edu dodona-edu deleted a comment from lgtm-com bot Aug 5, 2021
@dodona-edu dodona-edu deleted a comment from lgtm-com bot Aug 5, 2021
Copy link
Member

@bmesuere bmesuere left a comment

Choose a reason for hiding this comment

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

It would be reassuring if some jest tests were added covering those new functions. In addition, it would make sense to "port" these functions to typescript.

@chvp do we already have a file where we can place such helper functions in ts?

app/assets/javascripts/exercise.js Outdated Show resolved Hide resolved
@chvp
Copy link
Member

chvp commented Aug 9, 2021

@chvp do we already have a file where we can place such helper functions in ts?

We don't, no.

test/javascript/util.test.js Outdated Show resolved Hide resolved
@dodona-edu dodona-edu deleted a comment from lgtm-com bot Aug 9, 2021
@dodona-edu dodona-edu deleted a comment from lgtm-com bot Aug 9, 2021
@dodona-edu dodona-edu deleted a comment from lgtm-com bot Aug 9, 2021
@dodona-edu dodona-edu deleted a comment from lgtm-com bot Aug 9, 2021
@dodona-edu dodona-edu deleted a comment from lgtm-com bot Aug 9, 2021
@dodona-edu dodona-edu deleted a comment from lgtm-com bot Aug 9, 2021
@dodona-edu dodona-edu deleted a comment from lgtm-com bot Aug 9, 2021
@dodona-edu dodona-edu deleted a comment from lgtm-com bot Aug 9, 2021
@dodona-edu dodona-edu deleted a comment from lgtm-com bot Aug 9, 2021
app/assets/javascripts/exercise.js Outdated Show resolved Hide resolved
app/assets/javascripts/exercise.js Outdated Show resolved Hide resolved
app/assets/javascripts/exercise.js Outdated Show resolved Hide resolved
Co-authored-by: Charlotte Van Petegem <[email protected]>
@lgtm-com
Copy link

lgtm-com bot commented Aug 10, 2021

This pull request fixes 1 alert when merging cdafacc into b300479 - view on LGTM.com

fixed alerts:

  • 1 for Incomplete string escaping or encoding

@dodona-edu dodona-edu deleted a comment from lgtm-com bot Aug 10, 2021
@bmesuere bmesuere merged commit 620a0b0 into develop Aug 10, 2021
@bmesuere bmesuere deleted the util-functions-URL-API branch August 10, 2021 08:55
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement A change that isn't substantial enough to be called a feature
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Use URL API in util.js
4 participants