Skip to content
This repository has been archived by the owner on May 25, 2023. It is now read-only.

Bug: When adding brob/plug11ty.com "not valid" error #1451

Closed
2 tasks done
brob opened this issue Jun 10, 2022 · 14 comments · Fixed by #1453
Closed
2 tasks done

Bug: When adding brob/plug11ty.com "not valid" error #1451

brob opened this issue Jun 10, 2022 · 14 comments · Fixed by #1453

Comments

@brob
Copy link

brob commented Jun 10, 2022

Describe the bug

When adding a repository from my synced GitHub account, I get the message "Invalid GitHub repository"

Steps to reproduce

  1. Go to https://app.opensauced.pizza/
  2. In owner/repo field type brob/plug11ty.com (corresponds to this repo)
  3. Click "Add"
  4. Error message "Invalid GitHub repository" appears

Affected services

opensauced.pizza

Platforms

Desktop

Browsers

Chrome

Environment

Production

Additional context

No response

Code of Conduct

  • I agree to follow this project's Code of Conduct

Contributing Docs

  • I agree to follow this project's Contribution Docs
@mtfoley
Copy link
Contributor

mtfoley commented Jun 10, 2022

I haven't reproduced yet, but here is the code path I suspect is responsible:

Repo Form checking validity:

const [isValid, repoUrl] = isValidRepoUrl(urlRef.current.value.replace(/\s+/g, ""));

Validity Checker catching .com and assuming this is an absolute URL:

const isRelativeUrl = !(url.substr(0, 4) === "http" || url.includes(".com") || url.includes("www."));

From there, I haven't validated, but I believe the intention would be for us to consider this a "relative URL".

@0-vortex
Copy link
Contributor

I haven't reproduced yet, but here is the code path I suspect is responsible:

Repo Form checking validity:

const [isValid, repoUrl] = isValidRepoUrl(urlRef.current.value.replace(/\s+/g, ""));

Validity Checker catching .com and assuming this is an absolute URL:

const isRelativeUrl = !(url.substr(0, 4) === "http" || url.includes(".com") || url.includes("www."));

From there, I haven't validated, but I believe the intention would be for us to consider this a "relative URL".

Screenshot 2022-06-10 at 16 48 10

Needs a fix for sure :D

@mtfoley
Copy link
Contributor

mtfoley commented Jun 10, 2022

my gut tells me that this would be a sufficient check:

const isRelativeUrl = !(url.substr(0,7) === 'http://' || url.substr(0,8) === 'https://');

The upsides to this would be:

  • we don't try to get too clever with regex
  • we don't try to anticipate all of the TLDs

Test suite would need adjustment too.
https://github.com/open-sauced/open-sauced/blob/4a26b94eaa1a1094ee0dd0d7ca76ead50da2acc4/src/tests/util.test.js

@mtfoley
Copy link
Contributor

mtfoley commented Jun 10, 2022

Marking this as a good first issue - I think there's enough context to work with.

@0-vortex
Copy link
Contributor

the RFC itself is terrible, the function could use more regex to properly check for owner/repo and not complicate so much with URL handling - there are node classes now handling that including https://developer.mozilla.org/en-US/docs/Web/API/URL

@mtfoley
Copy link
Contributor

mtfoley commented Jun 10, 2022

For what it's worth, this repo is flagged as invalid, for similar reasons:
https://github.com/httpie/httpie

@us3r64bit
Copy link
Contributor

I want to work on this issue, please assign me

@bdougie
Copy link
Member

bdougie commented Jun 18, 2022

Please review the CONTIBUTING.md for info on how to assign yourself to the issue.

@us3r64bit
Copy link
Contributor

.take

@github-actions
Copy link
Contributor

Thanks for taking this on! If you have not already, join the conversation in our Discord

@us3r64bit
Copy link
Contributor

us3r64bit commented Jun 19, 2022

@mtfoley my test cases are not getting cleared. I am not getting what changes I need to do in https://github.com/open-sauced/open-sauced/blob/4a26b94eaa1a1094ee0dd0d7ca76ead50da2acc4/src/tests/util.test.js
although brob/plug11ty.com is valid to add but test cases are getting failed.
so can you please me through this process?
I am a newbie in this open-source contributions

@0-vortex
Copy link
Contributor

@mtfoley my test cases are not getting cleared. I am not getting what changes I need to do in https://github.com/open-sauced/open-sauced/blob/4a26b94eaa1a1094ee0dd0d7ca76ead50da2acc4/src/tests/util.test.js
although brob/plug11ty.com is valid to add but test cases are getting failed.
so can you please me through this process?
I am a newbie in this open-source contributions

Hey, I don't see you having forked the open-sauced repo, once you do, you can apply the fixes discussed above. There is nothing you need to do to the unit test, it's the code that this issue requires fixing.

If that is not the case IE you are not looking to contribute, let us know and we will assign to someone else :D

@us3r64bit
Copy link
Contributor

I made a PR
Please check it out

github-actions bot pushed a commit that referenced this issue Jun 25, 2022
### [0.52.1](v0.52.0...v0.52.1) (2022-06-25)

### 🐛 Bug Fixes

* able to add repo name with "."(dot) in it issue[#1451](#1451) ([#1453](#1453)) ([fcca807](fcca807))
@github-actions
Copy link
Contributor

🎉 This issue has been resolved in version 0.52.1 🎉

The release is available on:

Your semantic-release bot 📦🚀

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
5 participants