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

Add Feeds needs to be now to be connected front end to back end #748

Merged
merged 6 commits into from
Mar 2, 2020

Conversation

c3ho
Copy link
Contributor

@c3ho c3ho commented Feb 27, 2020

Issue This PR Addresses

Fixes #745

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

Connected this to the back end, I had to repurpose the + button to add feeds for now! Sorry @Silvyre.

Easiest way to test this is to just change the index.js under pages to display myfeeds component. To add a feed use the + button after inputting name and feed info, make sure devTools is opened prior to clicking the button so you can evaluate the response from the server. A 409 error means your feed already exists, to get around it just modify some of the characters for the blog url.

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)

Copy link
Contributor

@Silvyre Silvyre left a comment

Choose a reason for hiding this comment

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

Thank you so much for your work on this! A few comments:

src/frontend/src/pages/myfeeds.js Show resolved Hide resolved
src/frontend/src/pages/myfeeds.js Show resolved Hide resolved
src/frontend/src/pages/myfeeds.js Outdated Show resolved Hide resolved
src/frontend/src/pages/myfeeds.js Outdated Show resolved Hide resolved
@Silvyre
Copy link
Contributor

Silvyre commented Feb 27, 2020

I'm having some trouble testing the POSTing functionality to due technical shortcomings (VM struggles to run a Gatsby development build plus Docker, etc).

While I struggle with getting Docker Toolbox running, hopefully some other reviewers can cover for me, here.

@Silvyre
Copy link
Contributor

Silvyre commented Feb 27, 2020

Alright, managed to configure my new environment. I finally was able to use this component to successfully POST a feed (RSS feed for Mozilla's blog), but attempting to handle that feed ended up causing my backend to crash:

image

Any ideas, @c3ho?

@c3ho
Copy link
Contributor Author

c3ho commented Feb 27, 2020

@Silvyre I just checked the dev tools thing, apparently using process.env.telescopeURL appends a weird /undefined in between the telescopeURL and /feeds route. So on my machine it became http://localhost:3000/undefined/feeds. Can you try hardcoding the url for the fetch and let me know if it still crashes?

I hard coded the url and re-tested, it was displaying the proper output without crashing whatsoever.

Also I don't know why but the front-end portion is unable to find process.env.API_URL


async function addFeed() {
try {
const response = await fetch(`${process.env.telescopeUrl}/feeds/`, {
Copy link
Contributor

Choose a reason for hiding this comment

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

Here, telescopeUrl should be replaced with API_URL and provided with a default value.

For example, borrowing code from gatsby-config.js:

try {
  const telescopeUrl = process.env.API_URL || `http://localhost:${process.env.PORT || 3000}`;
  const response = await fetch(`${telescopeUrl}/feeds/`, {

Copy link
Contributor

Choose a reason for hiding this comment

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

Agreed ^

Copy link
Contributor

@Silvyre Silvyre left a comment

Choose a reason for hiding this comment

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

Now that I have a working environment, I can confirm that this feature does indeed work as intended!

I was able to add the aforementioned Mozilla feed and verify its addition by checking the /opml route:

<outline text="60407f78e2" title="Mozilla&apos;s blog" type="rss" xmlUrl="https://blog.mozilla.org/rss" htmlUrl="https://blog.mozilla.org"/>

Great work!

Copy link
Contributor

@Silvyre Silvyre left a comment

Choose a reason for hiding this comment

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

Just make that change, and it's an approve from me: #748 (comment)

Copy link
Contributor

@Grommers00 Grommers00 left a comment

Choose a reason for hiding this comment

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

I got it working - but agreed with @Silvyre changes.

Silvyre
Silvyre previously approved these changes Mar 1, 2020
Copy link
Contributor

@Silvyre Silvyre left a comment

Choose a reason for hiding this comment

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

Fantastic work! 👍 👍

@cindyorangis
Copy link
Contributor

How do I go about testing this?

@Silvyre
Copy link
Contributor

Silvyre commented Mar 1, 2020

How do I go about testing this?

Here's how I tested:

  1. Run the backend and frontend locally
  2. Navigate to the /myfeeds page, i.e. localhost:8000/myfeeds
  3. Enter a blog feed author and URL into the supplied text fields (e.g. Mozilla, https://blog.mozilla.org/rss)
  4. Press the '+' button to add the feed to Telescope
  5. After waiting a minute or so, observe the feed's presence within a response served by any of our backend /feed/* routes designed for Telescope feed sharing, e.g. localhost:3000/feed/opml

cindyorangis
cindyorangis previously approved these changes Mar 1, 2020
Copy link
Contributor

@cindyorangis cindyorangis left a comment

Choose a reason for hiding this comment

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

Wow this is neat! I added a feed for Cindy Cash for testing, how do I remove it? XD

@c3ho
Copy link
Contributor Author

c3ho commented Mar 1, 2020

We don't have a way to do that yet haha

@Silvyre
Copy link
Contributor

Silvyre commented Mar 1, 2020

Wow this is neat! I added a feed for Cindy Cash for testing, how do I remove it? XD

Provided you're running your own instance of Redis, your feed will disappear when you kill that instance. :)

As @c3ho mentioned, we'll want to add functionality to the My Feeds page to remove added feeds in the future.

@cindyorangis
Copy link
Contributor

Not sure if this is a bug but I have two different localhost:8000/myfeeds and localhost:3000/myfeeds looks different.
Annotation 2020-03-01 171326

Annotation 2020-03-01 171326_

I ran docker-compose up --build
then in another terminal npm run develop and went to both URLs

@Silvyre
Copy link
Contributor

Silvyre commented Mar 1, 2020

Not sure if this is a bug but I have two different localhost:8000/myfeeds and localhost:3000/myfeeds looks different.

Yeah, this is already causing problems.

@c3ho
Copy link
Contributor Author

c3ho commented Mar 2, 2020

Merging as this was a rebase

@c3ho c3ho merged commit 1f69edd into Seneca-CDOT:master Mar 2, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add Feeds needs to be now to be connected front end to back end
4 participants