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.

Sign up for GitHub

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

Jump to bottom

Allow feed URL to be given during sign up #3768

Merged
merged 1 commit into from
Dec 9, 2022
Merged

Allow feed URL to be given during sign up #3768

merged 1 commit into from
Dec 9, 2022

Conversation

Eakam1007
Copy link
Contributor

Issue This PR Addresses

Resolves #3689

Type of Change

  • Bugfix: Change which fixes an issue
  • New Feature: Change which adds functionality
  • Documentation Update: Change which improves documentation
  • UI: Change which improves UI

Description

Currently, only a blog URL can be given during signup. Based on this URL, the feed discovery service returns a list of feed URLs.
This has been updated so a feed URL can also be given. The feed discovery service will check if the given URL is a feed URL, and simply return it back.
This URL is checked by performing a get request and comparing the content type header against a list of valid feed URL content types. The list of valid content types is similar to the one used by the getFeedURLs method.

  • Currrent:

    image

  • Updated:
    image

Steps to test the PR

  • Run telescope locally, including the login sso
  • Log in using one of the fake user accounts listed here
  • When prompted, click on sign up
  • Follow the prompts until you get asked for a blog URL
  • Enter a blog URL and click on 'Validate Blog'. This should list feed URLs
  • Enter a blog feed URL and click on 'Validate Blog'. This should list the same feed URL
  • Enter an invalid URL. This should show an error

Checklist

  • Quality: This PR builds and passes our npm test and works locally
  • Tests: This PR includes thorough tests or an explanation of why it does not
  • Screenshots: This PR includes screenshots or GIFs of the changes made or an explanation of why it does not (if applicable)
  • Documentation: This PR includes updated/added documentation to user exposed functionality or configuration variables are added/changed or an explanation of why it does not(if applicable)

@gitpod-io
Copy link

gitpod-io bot commented Nov 17, 2022

humphd
humphd requested changes Nov 17, 2022
View reviewed changes
Copy link
Contributor

@humphd humphd left a comment

Choose a reason for hiding this comment

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

Let's add a test to https://github.com/Seneca-CDOT/telescope/blob/master/src/api/feed-discovery/test/router.test.js as well.

Question: if we allow using a feed URL, does that mean we'll break the back-end, since it thinks the feed URL is the blog URL (HTML) when we add the user?

src/api/feed-discovery/src/util.js
'application/atom+xml',
'application/x.atom+xml',
'application/x-atom+xml',
'application/json',
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm not sure that the rss-parser module we use (see https://github.com/rbren/rss-parser) is capable of parsing all of these types. We might want to limit it to valid Atom + RSS only.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

If we limit these here, should we also limit the link types that the getFeedUrls function further down this file checks to get feed urls from blog url?

Copy link
Contributor

Choose a reason for hiding this comment

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

Let's leave it for now and circle back if/when it becomes a problem.

@Eakam1007
Copy link
Contributor Author

Eakam1007 commented Nov 18, 2022 •
edited
Loading

Question: if we allow using a feed URL, does that mean we'll break the back-end, since it thinks the feed URL is the blog URL (HTML) when we add the user?

Oh, I did not think to look into that. I will check what effect it will have.
I suspect it will only affect the places where the blog URL is linked (such as when clicking on profiles).

@alexsam29
Copy link
Contributor

Question: if we allow using a feed URL, does that mean we'll break the back-end, since it thinks the feed URL is the blog URL (HTML) when we add the user?

I'm reviewing the code right now and I tried to look at all the places in the backend that uses blogUrl to find any places where the backend might break. There were only two files that I could find: util.js and middleware.js.

In util.js the only function that uses blogUrl is getBlogBody. But that should not be affected by the use of a feed URL since the only other place that calls getBlogBody is discoverFeedUrls in middleware.js. However, if a feed URL is used that function should not be called because the changes on line 46 of this PR should prevent that.

In middleware.js, blogUrl is used to create a new profile in the function createNewProfile. But I don't believe using a feed URL for blogUrl will break anything since this is just used to store user information in the database.

@Eakam1007 Eakam1007 marked this pull request as ready for review November 19, 2022 19:43
@Eakam1007
Copy link
Contributor Author

Eakam1007 commented Nov 19, 2022 •
edited
Loading

Thanks for the info @alexsam29!
Following up on it, I checked the registration process and found that the blog URl is saved as the html_url field in the feeds model. However, I could not find anywhere where that field is being used.
I signed up locally with a feed URL and I did not notice any difference. Since the blog feed contains a link to the (html) blog url, clicking on author profiles redirects them correctly.
So, I don't think allowing a feed URl should break the back-end.

@Eakam1007
Add function to check if given url is a feed url
fd980fa 
Return url if a feed url is passed

Add tests for isFeedUrl function

Fix failing tests

Add test for sending feed URL in router.test

Fix test expectations running after test environment ends

Uncomment afterEach in router.test
@manekenpix manekenpix force-pushed the master branch 10 times, most recently from 09f143b to b6e64bd Compare December 3, 2022 19:59
humphd
humphd approved these changes Dec 9, 2022
View reviewed changes
Copy link
Contributor

@humphd humphd left a comment

Choose a reason for hiding this comment

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

Great job on this.

src/api/feed-discovery/src/util.js
'application/atom+xml',
'application/x.atom+xml',
'application/x-atom+xml',
'application/json',
Copy link
Contributor

Choose a reason for hiding this comment

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

Let's leave it for now and circle back if/when it becomes a problem.

@humphd humphd merged commit abd3ca4 into Seneca-CDOT:master Dec 9, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@humphd humphd humphd approved these changes

Assignees

@Eakam1007 Eakam1007

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Allow a blog OR feed URL to be given during sign-up
3 participants
@Eakam1007 @alexsam29 @humphd