fix: add stricter checks in link for `profile` #422

tenshiAMD · 2022-09-23T21:59:02Z

What:

add stricter checks in link for profile

CC @Roshanjossey

Why:

due to lack of GH parsing blog links

How:

update code for profile link

Checklist:

Documentation
Ready to be merged
Added myself to contributors table.
Bot Usage

vercel · 2022-09-23T21:59:04Z

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name	Status	Preview	Updated
app	✅ Ready (Inspect)	Visit Preview	Sep 25, 2022 at 0:29AM (UTC)

codecov · 2022-09-23T22:01:14Z

Codecov Report

Base: 100.00% // Head: 100.00% // No change to project coverage 👍

Coverage data is based on head (ae7601b) compared to base (deef897).
Patch coverage: 100.00% of modified lines in pull request are covered.

Additional details and impacted files

@@            Coverage Diff            @@
##            master      #422   +/-   ##
=========================================
  Coverage   100.00%   100.00%           
=========================================
  Files           16        16           
  Lines          391       390    -1     
  Branches        45        44    -1     
=========================================
- Hits           391       390    -1

Impacted Files	Coverage Δ
lib/get-user-details.js	`100.00% <100.00%> (ø)`
lib/modules/helpers.js	`100.00% <100.00%> (ø)`

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report at Codecov.
📢 Do you have feedback about the report comment? Let us know in this issue.

test/unit/helpers.test.js

Roshanjossey · 2022-09-24T16:07:01Z

lib/modules/helpers.js

+  const validRegexWithoutProtocol = /^[a-z0-9]+([\-\.]{1}[a-z0-9]+)*\.[a-z]{2,5}(:[0-9]{1,5})?(\/.*)?$/
+
+  if (validLink.match(validRegexWithoutProtocol)) validLink = `http://${url}/`
+  if (!validLink.match(validRegex)) validLink = `https://github.com/${validUsername}/`


One suggestion here, this is basically creating html_url that was already in the calling function. If how we call this function is changed like

getUserLink(blog, html_url)

This function can be changed into

function getUserLink(blog, githubProfileURL) { const validRegexWithScheme = /same as now/ const validRegexWithoutScheme = /same as now/ if (validRegexWithProtocol.test(blog)) return blog; if (validRegexWithoutProtocol.test(blog)) return `http://${blog}`; return githubProfileURL }

This way, someone reading the code in the future only have to think about which return would be triggered.
What do you think?

@Roshanjossey I somehow agree however we dp have tests that somehow act as the documentation for that method. I do not think this is necessary to add more lines. I like to simplify things rather than make them complicated.

I'll rephrase what I commented earlier. I'm suggesting to change

function generateValidLink(url, username) { let validLink = url let validUsername = username || 'all-contributors' const validRegex = /^(http:\/\/www\.|https:\/\/www\.|http:\/\/|https:\/\/)?[a-z0-9]+([\-\.]{1}[a-z0-9]+)*\.[a-z]{2,5}(:[0-9]{1,5})?(\/.*)?$/ const validRegexWithoutProtocol = /^[a-z0-9]+([\-\.]{1}[a-z0-9]+)*\.[a-z]{2,5}(:[0-9]{1,5})?(\/.*)?$/ if (validLink.match(validRegexWithoutProtocol)) validLink = `http://${url}/` if (!validLink.match(validRegex)) validLink = `https://github.com/${validUsername}/` return validLink }

to

function getUserLink(blog, githubProfileURL) { const validRegexWithScheme = /same as now/ const validRegexWithoutScheme = /same as now/ if (validRegexWithSchema.test(blog)) return blog; if (validRegexWithoutSchema.test(blog)) return `http://${blog}`; return githubProfileURL }

Isn't this simpler?

I'll rephrase what I commented earlier. I'm suggesting to change

function generateValidLink(url, username) { let validLink = url let validUsername = username || 'all-contributors' const validRegex = /^(http:\/\/www\.|https:\/\/www\.|http:\/\/|https:\/\/)?[a-z0-9]+([\-\.]{1}[a-z0-9]+)*\.[a-z]{2,5}(:[0-9]{1,5})?(\/.*)?$/ const validRegexWithoutProtocol = /^[a-z0-9]+([\-\.]{1}[a-z0-9]+)*\.[a-z]{2,5}(:[0-9]{1,5})?(\/.*)?$/ if (validLink.match(validRegexWithoutProtocol)) validLink = `http://${url}/` if (!validLink.match(validRegex)) validLink = `https://github.com/${validUsername}/` return validLink }

to

function getUserLink(blog, githubProfileURL) { const validRegexWithScheme = /same as now/ const validRegexWithoutScheme = /same as now/ if (validRegexWithSchema.test(blog)) return blog; if (validRegexWithoutSchema.test(blog)) return `http://${blog}`; return githubProfileURL }

Isn't this simpler?

@Roshanjossey I tried this earlier and 7 tests I made are failing.

Did you change the params for calling the function?

FAIL test/unit/helpers.test.js
● generateValidLink › returns valid link - valid URL format with http in between

expect(received).toEqual(expected) // deep equality Expected: "http://tenshhttpiamd.com/" Received: "tenshhttpiamd.com" 29 | let validUrl = generateValidLink(url, username); 30 | > 31 | expect(validUrl).toEqual(`http://${url}/`); | ^ 32 | }); 33 | 34 | test('returns valid link - valid URL format with `https` in between', async () => { at Object.<anonymous> (test/unit/helpers.test.js:31:22)

● generateValidLink › returns valid link - valid URL format with https in between

expect(received).toEqual(expected) // deep equality Expected: "http://tenshhttpsiamd.com/" Received: "tenshhttpsiamd.com" 36 | let validUrl = generateValidLink(url, username); 37 | > 38 | expect(validUrl).toEqual(`http://${url}/`); | ^ 39 | }); 40 | 41 | test('returns valid link - no protocol', async () => { at Object.<anonymous> (test/unit/helpers.test.js:38:22)

● generateValidLink › returns valid link - no protocol

expect(received).toEqual(expected) // deep equality Expected: "http://tenshiamd.com/" Received: "tenshiamd.com" 43 | let validUrl = generateValidLink(url, username); 44 | > 45 | expect(validUrl).toEqual(`http://${url}/`); | ^ 46 | }); 47 | 48 | test('returns valid link - no protocol and starting with `http`', async () => { at Object.<anonymous> (test/unit/helpers.test.js:45:22)

● generateValidLink › returns valid link - no protocol and starting with http

expect(received).toEqual(expected) // deep equality Expected: "http://httptenshiamd.com/" Received: "httptenshiamd.com" 50 | let validUrl = generateValidLink(url, username); 51 | > 52 | expect(validUrl).toEqual(`http://${url}/`); | ^ 53 | }); 54 | 55 | test('returns valid link - no protocol and starting with `https`', async () => { at Object.<anonymous> (test/unit/helpers.test.js:52:22)

● generateValidLink › returns valid link - no protocol and starting with https

expect(received).toEqual(expected) // deep equality Expected: "http://httpstenshiamd.com/" Received: "httpstenshiamd.com" 57 | let validUrl = generateValidLink(url, username); 58 | > 59 | expect(validUrl).toEqual(`http://${url}/`); | ^ 60 | }); 61 | 62 | test('returns valid link - incomplete URL format', async () => { at Object.<anonymous> (test/unit/helpers.test.js:59:22)

● generateValidLink › returns valid link - incomplete URL format

expect(received).toEqual(expected) // deep equality Expected: "https://github.com/tenshiAMD/" Received: "tenshiAMD" 64 | let validUrl = generateValidLink(url, username); 65 | > 66 | expect(validUrl).toEqual(`https://github.com/${username}/`); | ^ 67 | }); 68 | 69 | test('returns valid link - incomplete URL format with `null` username', async () => { at Object.<anonymous> (test/unit/helpers.test.js:66:22)

● generateValidLink › returns valid link - incomplete URL format with null username

expect(received).toEqual(expected) // deep equality Expected: "https://github.com/all-contributors/" Received: null 71 | let validUrl = generateValidLink(url, null); 72 | > 73 | expect(validUrl).toEqual(`https://github.com/all-contributors/`); | ^ 74 | }); 75 | }); 76 | at Object.<anonymous> (test/unit/helpers.test.js:73:22)

let validUrl = generateValidLink(url, username);

This is still the same parameters, the function I suggested takes in html_url as second param.

I think there's a communication problem here. Do you wanna get on a call and sort this out?
https://meet.jit.si/open

@Roshanjossey hmm I see I still need to change my tests 😕 though it's the same implementation, this just takes my bandwidth. :(

I could create a PR if you want

I could create a PR if you want

@Roshanjossey No need 😄 it will just take your time and I added some tweaks. LGTM now. I will just add you as a co-author.

github-actions · 2022-09-25T00:33:03Z

🎉 This PR is included in version 1.16.3 🎉

The release is available on GitHub release

Your semantic-release bot 📦🚀

tenshiAMD requested a review from a team as a code owner September 23, 2022 21:59

tenshiAMD force-pushed the tenshiamd/fix-profile-link branch from 2f5ef0b to a1e728c Compare September 23, 2022 22:00

vercel bot deployed to Preview September 23, 2022 22:01 View deployment

tenshiAMD mentioned this pull request Sep 23, 2022

fix: resolve invalid link for profile #421

Merged

3 tasks

tenshiAMD force-pushed the tenshiamd/fix-profile-link branch from a1e728c to c1201a6 Compare September 23, 2022 22:05

vercel bot deployed to Preview September 23, 2022 22:05 View deployment

fix: add stricter checks in link for profile

08c21b8

tenshiAMD force-pushed the tenshiamd/fix-profile-link branch from c1201a6 to 08c21b8 Compare September 23, 2022 22:07

vercel bot deployed to Preview September 23, 2022 22:08 View deployment

Roshanjossey reviewed Sep 24, 2022

View reviewed changes

tenshiAMD self-assigned this Sep 24, 2022

test

ae7601b

vercel bot deployed to Preview September 25, 2022 00:29 View deployment

tenshiAMD merged commit 629c5b8 into master Sep 25, 2022

tenshiAMD deleted the tenshiamd/fix-profile-link branch September 25, 2022 00:32

github-actions bot added the released label Sep 25, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix: add stricter checks in link for `profile` #422

fix: add stricter checks in link for `profile` #422

tenshiAMD commented Sep 23, 2022

vercel bot commented Sep 23, 2022 •

edited

Loading

codecov bot commented Sep 23, 2022 •

edited

Loading

Roshanjossey Sep 24, 2022

tenshiAMD Sep 24, 2022

Roshanjossey Sep 24, 2022

tenshiAMD Sep 25, 2022

Roshanjossey Sep 25, 2022

tenshiAMD Sep 25, 2022

Roshanjossey Sep 25, 2022

tenshiAMD Sep 25, 2022

Roshanjossey Sep 25, 2022

tenshiAMD Sep 25, 2022 •

edited

Loading

github-actions bot commented Sep 25, 2022

fix: add stricter checks in link for profile #422

fix: add stricter checks in link for profile #422

Conversation

tenshiAMD commented Sep 23, 2022

vercel bot commented Sep 23, 2022 • edited Loading

codecov bot commented Sep 23, 2022 • edited Loading

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tenshiAMD Sep 25, 2022 • edited Loading

Choose a reason for hiding this comment

github-actions bot commented Sep 25, 2022

fix: add stricter checks in link for `profile` #422

fix: add stricter checks in link for `profile` #422

vercel bot commented Sep 23, 2022 •

edited

Loading

codecov bot commented Sep 23, 2022 •

edited

Loading

tenshiAMD Sep 25, 2022 •

edited

Loading