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

Fix #74: Validate blog URLs. #123

Merged
merged 3 commits into from Nov 14, 2019
Merged

Fix #74: Validate blog URLs. #123

merged 3 commits into from Nov 14, 2019

Conversation

@Cadesh
Copy link
Collaborator

Cadesh commented Nov 9, 2019

I tested some ways to validate the URLs, the one that worked better was with valid-url (https://www.npmjs.com/package/valid-url)

These are the changes to the code:

  1. Added a new file utils.js with a function to validade URLs.
  2. Added a file utils.test.js with a test for the new function.
@Reza-Rajabi Reza-Rajabi added the on-board label Nov 9, 2019
@Reza-Rajabi Reza-Rajabi added this to In progress/Review in Main via automation Nov 9, 2019
@kartik-budhiraja

This comment has been minimized.

Copy link
Contributor

kartik-budhiraja commented Nov 10, 2019

@Cadesh can you please look into the eslint errors, the travis pipeline logs can help you there.

@humphd humphd changed the title Fix #74: Validade blog URLs. Fix #74: Validate blog URLs. Nov 10, 2019
}
}

module.exports = {isValidUrl}

This comment has been minimized.

Copy link
@manekenpix

manekenpix Nov 10, 2019

Collaborator

Missing newline at the end of the file

This comment has been minimized.

Copy link
@Cadesh

Cadesh Nov 10, 2019

Author Collaborator

newline added.

expect (utils.isValidUrl('https://github.com/Seneca-CDOT/telescope/issues/74')).toBe(true);
expect (utils.isValidUrl('github.com/Seneca-CDOT/telescope/issues/74')).toBe(false);
expect (utils.isValidUrl('https//github.com/Seneca-CDOT/telescope/issus/74')).toBe(false);
})

This comment has been minimized.

Copy link
@manekenpix

manekenpix Nov 10, 2019

Collaborator

Missing newline at the end of the file

This comment has been minimized.

Copy link
@Cadesh

Cadesh Nov 10, 2019

Author Collaborator

newline added

@manekenpix

This comment has been minimized.

Copy link
Collaborator

manekenpix commented Nov 11, 2019

@Cadesh There's still a conflict with package-lock.json.

@humphd

This comment has been minimized.

Copy link
Contributor

humphd commented Nov 11, 2019

git checkout master
git pull upstream master
git checkout issue-74
git rebase master
npm install
git add package-lock.json
Copy link
Contributor

humphd left a comment

This is looking good, thanks for adding tests! I left some notes. I'd like others to read this as well.

@@ -0,0 +1,7 @@
const utils = require('../src/utils');

test('test for URL validation', () => {

This comment has been minimized.

Copy link
@humphd

humphd Nov 12, 2019

Contributor

Let's break these up into separate tests with a description for each of what is tested.

@manekenpix manekenpix self-requested a review Nov 14, 2019
Copy link
Collaborator

manekenpix left a comment

Looking good, address what @humphd mentioned and fixi the conflict you have with package.json and then it's good to go.

unknown and others added 2 commits Nov 9, 2019
unknown Andre Luiz Valle Rosa
@Cadesh Cadesh requested review from manekenpix and humphd Nov 14, 2019
Copy link
Collaborator

manekenpix left a comment

Nice!

@humphd

This comment has been minimized.

Copy link
Contributor

humphd commented Nov 14, 2019

Looks like package.json is out of date again, needs another rebase.

@Cadesh Cadesh dismissed stale reviews from cindyledev, humphd, and manekenpix via 6a3b728 Nov 14, 2019
@manekenpix manekenpix self-requested a review Nov 14, 2019
Copy link
Collaborator

manekenpix left a comment

It seems you merged master into your branch. Can you try to revert that?

Screenshot from 2019-11-14 15-07-51

@Cadesh

This comment has been minimized.

Copy link
Collaborator Author

Cadesh commented Nov 14, 2019

Actually the only thing I did was to manually fix the package.json conflict on GitHub that, by the way, was the same one I fixed and pushed earlier on class. I do not know why the same conflict showed up again.

@humphd
humphd approved these changes Nov 14, 2019
@humphd

This comment has been minimized.

Copy link
Contributor

humphd commented Nov 14, 2019

@Cadesh that will do the merge (fixing on GitHub). You want to do it locally.

I'm going to merge this, since it's not a big deal, and I want this landed.

@humphd humphd merged commit 099e761 into Seneca-CDOT:master Nov 14, 2019
2 checks passed
2 checks passed
ci/circleci: build Your tests passed on CircleCI!
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
Main automation moved this from In progress/Review to Done Nov 14, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Main
Done
6 participants
You can’t perform that action at this time.