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

Update archive.html #1232

Closed
wants to merge 1 commit into from
Closed

Update archive.html #1232

wants to merge 1 commit into from

Conversation

segun-codes
Copy link
Collaborator

Fixes #1187 (<=== Add issue number here)

Make sure these boxes are checked before your pull request (PR) is ready to be reviewed and merged. Thanks!

  • [X ] PR is descriptively titled 📑 and links the original issue above 🔗
  • tests pass -- look for a green checkbox ✔️ a few minutes after opening your PR -- or run tests locally with grunt test
  • code is in uniquely-named feature branch and has no merge conflicts 📁
  • screenshots/GIFs are attached 📎 in case of UI updates
  • @mention the original creator of the issue in a comment below for help or for a review

We're happy to help you get this ready -- don't be afraid to ask for help, and don't be discouraged if your tests fail at first!

If tests do fail, click on the red X to learn why by reading the logs.

Please be sure you've reviewed our contribution guidelines at https://publiclab.org/contributing-to-public-lab-software

Thanks!

@gitpod-io
Copy link

gitpod-io bot commented Oct 24, 2022

@vanithaak
Copy link
Contributor

Hi @segun-codes , I have worked on a similar issue , Where I made a more flexible function(It's yet to be merged, I hope @jywarren merges it soon, haha) . For example, when I enter a url without https://archive.org/, the function adds https://archive.org/ in prefix to the incomplete url. Similarly, when I enter only texas-barnraising, the rest of the url is added as prefix. Please have a look at it #1175

@segun-codes
Copy link
Collaborator Author

Hi @segun-codes , I have worked on a similar issue , Where I made a more flexible function(It's yet to be merged, I hope @jywarren merges it soon, haha) . For example, when I enter a url without https://archive.org/, the function adds https://archive.org/ in prefix to the incomplete url. Similarly, when I enter only texas-barnraising, the rest of the url is added as prefix. Please have a look at it #1175

Hi @vanithaak, okay but it appears the focus of the issue in #1187 slightly differs from yours, don't you think so? @jywarren, I will appreciate your thought on this too?

@vanithaak
Copy link
Contributor

I mean if the function already adds https://archive.org/.... how can we validate, as you mentioned in the issue ? 😅 Please correct me if I'm wrong! Thanks!

@segun-codes
Copy link
Collaborator Author

I mean if the function already adds https://archive.org/.... how can we validate, as you mentioned in the issue ? 😅 Please correct me if I'm wrong! Thanks!

Hi @vanithaak, I was just processing this same thought now. If @jywarren is okay with the flexibility your PR is proposing then the idea of validation immediately becomes tricky. So for instance, it means we will only validate user-supplied URLs and ignore any url generated with assistance from your function. @jywarren, will you like to have the validation given the aforementioned consideration?

@vanithaak
Copy link
Contributor

You mean if a complete url https://.... doesn't have archive.org, we give a message to user, else my proposed function will work??

@segun-codes
Copy link
Collaborator Author

segun-codes commented Oct 24, 2022

You mean if a complete url https://.... doesn't have archive.org, we give a message to user, else my proposed function will work??

If a user-supplied url is faulty in a way (e.g., arch.net instead of archive.org, http instead of https etc.) based on certain criteria set, then validation will set in. In my opinion, whether this is trivial or not is really a simple design decision.

@vanithaak
Copy link
Contributor

Great idea! Let's wait for Jeff's thoughts. What do you think?

@segun-codes
Copy link
Collaborator Author

Great idea! Let's wait for Jeff's thoughts. What do you think?

I agree with you.

@jywarren
Copy link
Member

Hi all, thank you for your careful consideration here.

I'm a bit confused about this question:

So for instance, it means we will only validate user-supplied URLs and ignore any url generated with assistance from your function. @jywarren, will you like to have the validation given the aforementioned consideration?

I think that within a certain range of irregularities, such as http vs. https, forgetting to include https://, or similar, we can correct it. But if the hostname archive.org is wrong, we should show an error message. Would you two be interested in working together on a PR which has this distinction? You could both cite the PR as your contribution as long as it's clearly labeled as a collaboration at the top. What do you think?

@segun-codes
Copy link
Collaborator Author

segun-codes commented Oct 25, 2022

Hi all, thank you for your careful consideration here.

I'm a bit confused about this question:

So for instance, it means we will only validate user-supplied URLs and ignore any url generated with assistance from your function. @jywarren, will you like to have the validation given the aforementioned consideration?

I think that within a certain range of irregularities, such as http vs. https, forgetting to include https://, or similar, we can correct it. But if the hostname archive.org is wrong, we should show an error message. Would you two be interested in working together on a PR which has this distinction? You could both cite the PR as your contribution as long as it's clearly labeled as a collaboration at the top. What do you think?

@jywarren, thank you for your thought and it shows you actually understand the conversation between @vanithaak and myself.

Hi @vanithaak, I am okay with with @jywarren's suggestion that we collaborate. Given what you have done so far, I think while I focus on simply validating hostname and generating a message to user, you could just add the functionality of correcting issues such as http vs. https, forgetting to include https://, Afterwards, we can integrate our codes into a single PR. What do you think?

@vanithaak
Copy link
Contributor

sure!

@segun-codes
Copy link
Collaborator Author

sure!

Hi @vanithaak, having gone through your code, I think there's no further need for URL validation. Your logic takes over, almost completely, the responsibility (of supplying correct url) from the user so that concern related to correctness of url form no longer exists. If @jywarren is satisfied with what you have done, then I haven't any need to contribute here. Many thanks.

@jywarren
Copy link
Member

Hi all, OK, thank you for carefully thinking through this. I did merge the URL correction code in #1175. But there was one more case -- where http instead of https is entered. @segun-codes would you be willing to update the code so it can automatically switch to https? Otherwise the request will fail, due to CORS restrictions, I believe. Thank you both! Shall we close this PR then?

@segun-codes
Copy link
Collaborator Author

Hi all, OK, thank you for carefully thinking through this. I did merge the URL correction code in #1175. But there was one more case -- where http instead of https is entered. @segun-codes would you be willing to update the code so it can automatically switch to https? Otherwise the request will fail, due to CORS restrictions, I believe. Thank you both! Shall we close this PR then?

Hi @jywarren, yes I would like to update the code so it can automatically switch http to https and by the way, this will be done through another PR. This way, I guess we can close this particular PR. Many thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Review expensive HTTP operation in archive.html
3 participants