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

Closes #552: Remove GraphQL implementation #1295

Merged
merged 4 commits into from
Nov 10, 2020

Conversation

manekenpix
Copy link
Member

@manekenpix manekenpix commented Nov 6, 2020

Issue This PR Addresses

Closes #552
Progress towards #1224
Progress towards #1225

Type of Change

Description

This removes the graphql implementation and documentation. Since removing graphql breaks the search functionality for author, I made some changes so the front-end uses the same endpoint to search both authors and posts, taking advantage of what was added in #1280.

Other changes:

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)

@c3ho
Copy link
Contributor

c3ho commented Nov 6, 2020

I'll review this tomorrow. I saved a branch of our current Telescope in case I need to remember some GraphQL stuff!

cindyorangis
cindyorangis previously approved these changes Nov 7, 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.

I tested this locally using Docker on Windows.
I was able to search for posts using the search button. As mentioned, searching using the Author filter doesn't work.

This PR doesn't fully remove all GraphQL from Telescope. There are still bits of it in our
Banner, my poorly implemented About page (it has graphql in multiple places unfortunately)
https://github.com/Seneca-CDOT/telescope/blob/master/src/frontend/src/templates/template.js
https://github.com/Seneca-CDOT/telescope/blob/master/src/frontend/gatsby-node.js.

I was surprised that none of the other components broke in this PR

@manekenpix
Copy link
Member Author

manekenpix commented Nov 8, 2020

@cindyledev

I was able to search for posts using the search button. As mentioned, searching using the Author filter doesn't work

I just tested it searching by Author and it works. Can you tell me what to do to reproduce your results?

This PR doesn't fully remove all GraphQL from Telescope

This is intentional. The graphql pieces left in the front-end are necessary to build pages (I think gatsby needs it) or to fetch metadata. In both cases, they don't talk to the graphql server in the back-end, which is what was removed in this PR.

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.

This is awesome, thanks for doing it, and sorry to throw away all the good work you, @c3ho and others did. It was great to experiment with this.

Do we still need to do scriptSrc: ["'self'", 'cdn.jsdelivr.net', "'unsafe-inline'"], in the express app if we rip out the graphql playground (the cdn I mean)?

src/frontend/src/components/SearchPage/SearchPage.jsx Outdated Show resolved Hide resolved
throw new Error(res.statusText);
}
const searchResults = await res.json();
// ES values property contains an array of objects with a (feed) id property
Copy link
Contributor

Choose a reason for hiding this comment

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

This comment seems wrong, we pass post ids, no?

Also, I think we should do this transform from {id} to {id, url} on the server, either in this PR or another. Let's prepare the results to be useful right away from the REST api. I actually thought we were doing this in another PR, which might still be in review? cc @c3ho.

Copy link
Contributor

Choose a reason for hiding this comment

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

I think we merged all new ES related PRs last week. I checked the code and we don't store feedID at least from what I see

Copy link
Member Author

Choose a reason for hiding this comment

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

I'll change this. I mainly deleted what's not needed anymore but I didn't really pay much attention to the code that's going to stay.
Also, I think the backend already sends {id, url} (I think it's like this since the very beginning of telescope) it does it here, so I'll remove the transform (sorry about that, again I just deleted stuff without putting too much attention on what's going to stay).

Copy link
Contributor

Choose a reason for hiding this comment

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

Awesome, thank you both for looking into this. I'm reviewing this code again since we're touching it; sorry to make work for you @manekenpix. We can file follow-ups on anything we find vs. delaying this surgery from being completed.

Copy link
Member Author

@manekenpix manekenpix Nov 9, 2020

Choose a reason for hiding this comment

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

My bad, the endpoint returning {id, url} is GET /posts, not the one used for searching. I just changed the indexer to return the same thing, and removed the transformation from the front-end.

Copy link
Contributor

@c3ho c3ho left a comment

Choose a reason for hiding this comment

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

Tested:
Confirmed search was not firing after onChange
Confirmed default search is Post
Confirmed search results by searching Telescope for Post and James for Author only authors with James in their name were displayed

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.

Awesome.

@manekenpix manekenpix merged commit 87075df into Seneca-CDOT:master Nov 10, 2020
@manekenpix manekenpix deleted the issues/552-remove-graphql branch November 10, 2020 00:40
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area: back-end area: elasticsearch Elasticsearch Related issues and pull requests area: front-end
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Remove GraphQL implementation
4 participants