Add `pullrequests/:id` route to fetch PRs of a given `id` of user using GitHub API #90

swarajpure · 2020-11-26T16:23:41Z

Added pullrequests/:id API route
Fixes #89

~~To do~~

~~[ JSDoc ]~~
~~[ Swagger Annotations ]~~

Please suggest changes, if any.

Thank you!

Subsequent PRs:

Take username instead of id in pullrequests and other APIs Changed API routes and functions to take username of user along with id #95
Refactoring functions to avoid repetition of code refactored functions for pullrequests routes #115
Tests for pullrequests routes: Refactor /pullrequests API and write integration tests for them #326

whyDontI

Nice work @swarajpure ,
Please make changes as requested.

whyDontI · 2020-11-26T17:51:30Z

utils/getGithubId.js

+const getGithubId = async (id) => {
+  try {
+    const details = await userQuery.fetchUser(id)
+    return (details.user.github_id)
+  } catch (err) {
+    logger.error(`Error while fetching user: ${err}`)
+    throw err
+  }
+}
+


No need to write this function.
You can directly use already existing getUser function from userController

@swarajpure @whyDontI Maybe that function should be moved into utils for better visibility for reusability

@ankushdharkar there are more functions related to users in userController, do we need to move all of them in that case?

This is not required. The user data is already fetched and can be accessed with req.userdata.
Refer: https://github.com/Real-Dev-Squad/website-backend/blob/develop/middlewares/authenticate.js#L38

UserController is separate because all the user related functions are there. We should import functions from there to where ever we need to use it.
I don't think we should to create a separate function just to access one key (github_id).

@ankurnarkhede That is only true for logged in user, here we need user details with ID

whyDontI · 2020-11-26T17:56:23Z

controllers/pullRequestsController.js

+      data.items.forEach((res) => {
+        allPRs.push({
+          title: res.title,
+          url: res.url,
+          state: res.state,
+          created_at: res.created_at,
+          updated_at: res.updated_at,
+          ready_for_review: res.state === 'closed' ? false : !res.draft,
+          labels: res.labels,
+          assignees: res.assignees
+        })


You can destructure the res object.
Use shorthand syntax.

whyDontI · 2020-11-26T17:58:54Z

controllers/pullRequestsController.js

+const pullRequests = async (req, res) => {
+  try {
+    const githubId = await getGithubId(req.params.id)
+    const url = `https://api.github.com/search/issues?q=org:Real-Dev-Squad+author:${githubId}+type:pr`


Set BASE_URL= https://api.github.com at the starting of the code and reuse the BASE_URL variable whenever you please to generate the full path.

whyDontI · 2020-11-26T17:59:51Z

controllers/pullRequestsController.js

+
+const pullRequests = async (req, res) => {
+  try {
+    const githubId = await getGithubId(req.params.id)


Directly use already existing getUser function from userController

ankurnarkhede · 2020-11-27T09:12:28Z

utils/getGithubId.js

+const getGithubId = async (id) => {
+  try {
+    const details = await userQuery.fetchUser(id)
+    return (details.user.github_id)
+  } catch (err) {
+    logger.error(`Error while fetching user: ${err}`)
+    throw err
+  }
+}
+


This is not required. The user data is already fetched and can be accessed with req.userdata.
Refer: https://github.com/Real-Dev-Squad/website-backend/blob/develop/middlewares/authenticate.js#L38

ankurnarkhede · 2020-11-27T09:12:54Z

controllers/pullRequestsController.js

+      })
+      return res.send(allPRs)
+    }
+    return res.send('No pull requests found!')


Need a JSON response.

ankurnarkhede · 2020-11-27T09:16:17Z

controllers/pullRequestsController.js

+const pullRequests = async (req, res) => {
+  try {
+    const githubId = await getGithubId(req.params.id)
+    const url = `https://api.github.com/search/issues?q=org:Real-Dev-Squad+author:${githubId}+type:pr`


Add the github API URL in the config: https://github.com/Real-Dev-Squad/website-backend/blob/develop/config/default.js#L18

The org name also should be added in the config.

ankurnarkhede · 2020-11-27T09:17:40Z

controllers/pullRequestsController.js

+    return res.send('No pull requests found!')
+  } catch (err) {
+    logger.error(`Error while fetching pull requests: ${err}`)
+    return res.boom.serverUnavailable('Something went wrong please contact admin')


Return 500.

ankurnarkhede · 2020-11-28T06:47:26Z

config/development.js

@@ -10,6 +10,11 @@ module.exports = {
  enableFileLogs: false,
  enableConsoleLogs: true,

+  githubApi: {


We dont need to add this to development and test config unless the values are changing.

ankurnarkhede · 2020-11-28T06:51:50Z

controllers/pullRequestsController.js

+  try {
+    const BASE_URL = 'https://api.github.com'
+    const { user } = await fetchUser(req.params.id)
+    const url = `${BASE_URL}/search/issues?q=org:Real-Dev-Squad+author:${user.github_id}+type:pr`


We will be using the github API functionality at multiple places in the code.
We better move the PRs fetch logic to a file, say services/githubService.js where we can add all other logic around fetching data from github.

Also the getNames function at https://github.com/Real-Dev-Squad/website-backend/pull/90/files#diff-3c24610be60a29530abbeabee1a488714820262747d9542ecd1f39e29d89139aR8 does not necessarily be defined in a separate file unless that's going to be used multiple times in the repo. It can be added in the service function.

ankurnarkhede · 2020-11-28T06:55:33Z

controllers/pullRequestsController.js

@@ -0,0 +1,47 @@
+const logger = require('../utils/logger')
+const fetch = require('../lib/fetch')
+const config = require('../config')


Use the library 'config', do not read from the file. Refer in other files.

This should solve the failing CI problem

ankurnarkhede · 2020-11-28T06:56:52Z

controllers/pullRequestsController.js

+const pullRequests = async (req, res) => {
+  try {
+    const { user } = await fetchUser(req.params.id)
+    const url = `${config.baseUrl}/search/issues?q=org:${config.org}+author:${user.github_id}+type:pr`


We should use check the rate limit rules and also pass the client secret and client Id.
https://developer.github.com/v3/search/#rate-limit

Added authorization via personal access token
https://docs.github.com/en/free-pro-team@latest/rest/guides/getting-started-with-the-rest-api#authentication

They have not mentioned how to authorize using client id and client secret

ankurnarkhede · 2020-11-29T06:37:00Z

controllers/pullRequestsController.js

+const getPullRequests = async (req, res) => {
+  try {
+    const { user } = await fetchUser(req.params.id)
+    const url = `${config.githubApi.baseUrl}/search/issues?q=org:${config.githubApi.org}+author:${user.github_id}+type:pr`


As used elsewhere, use config.get('')

ankurnarkhede · 2020-11-29T06:41:16Z

controllers/pullRequestsController.js

+  try {
+    const { user } = await fetchUser(req.params.id)
+    const url = `${config.githubApi.baseUrl}/search/issues?q=org:${config.githubApi.org}+author:${user.github_id}+type:pr`
+    const { data } = await githubService.fetch(url)


Why is the fetch function moved to githubService?
As per we discussed, we were to move it to /utils directory.

Also, the API call should be made from githubService file as per mentioned in the comment: #90 (comment)

The controller should handle any response modifications and error handling.

ankurnarkhede · 2020-11-29T06:42:20Z

controllers/pullRequestsController.js

+      })
+      return res.json(allPRs)
+    }
+    return res.json('No pull requests found!')


We should send an empty array in this case.

Done with requested changes

whyDontI

Small changes requested, Rest all looks great 👍🏽

whyDontI · 2020-11-29T14:00:40Z

services/githubService.js

+    const { data } = await fetch(url)
+    return data


NIT: What if we directly return fetch call, and destructure the data at call site?
@ankurnarkhede @swarajpure

Anyway TailCallOptimization will be there, but at call site we are using await, which is not required because we are returning data directly, not a promise.
What's ideal in this case ?

I have returned fetch call and destructured the data at function call

And about your second question, we need to use await at function call, otherwise data.total_count is undefined because data is not returned yet.

whyDontI · 2020-11-29T14:03:25Z

controllers/pullRequestsController.js

+      data.items.forEach(({ title, html_url: htmlUrl, state, created_at: createdAt, updated_at: updatedAt, draft, labels, assignees }) => {
+        const allAssignees = getNames(assignees, 'login')
+        const allLabels = getNames(labels, 'name')
+        allPRs.push({
+          title: title,
+          url: htmlUrl,
+          state: state,
+          created_at: createdAt,
+          updated_at: updatedAt,
+          ready_for_review: state === 'closed' ? false : !draft,


snake_case -> camelCase -> snake_case conversion detected.
Any particular reason?

No. Fixed it.

…alues

…some changes in response

whyDontI

Great work Swaraj. 🥳
Once you are done with the GitHub Auth changes, we can merge this PR. 👍🏽

ankurnarkhede · 2020-12-02T13:17:55Z

config/custom-environment-variables.js

  githubOauth: {
    clientId: 'GITHUB_CLIENT_ID',
-    clientSecret: 'GITHUB_CLIENT_SECRET'
+    clientSecret: 'GITHUB_CLIENT_SECRET',
+    accessToken: '<accessToken>'


WHat is the access token? And how do we get it? Aren't we using client Id and client secret for making requests?

Please refer my reply to your earlier review
Posted on discord as well

Done. Authorizing now using clientId and clientSecret. Please check

ankurnarkhede

Good work @swarajpure !

added getGithubId function and pullRequests controller

0595fe0

swarajpure added the feature task Feature that has to be built label Nov 26, 2020

swarajpure requested review from ankurnarkhede, ankushdharkar, Rucha1499 and whyDontI November 26, 2020 16:24

swarajpure self-assigned this Nov 26, 2020

whyDontI suggested changes Nov 26, 2020

View reviewed changes

ankurnarkhede suggested changes Nov 27, 2020

View reviewed changes

swarajpure added 3 commits November 27, 2020 21:52

removed getGithubId function and tweaks in response object

960f9d6

added JSDoc and Swagger Docs and moved getNames to lib

2a1d53e

getNames function

5d616fd

ankurnarkhede suggested changes Nov 28, 2020

View reviewed changes

swarajpure added 2 commits November 29, 2020 11:40

moved functions to services and removed config from test and development

cddeed0

file linking fixed

4152827

ankurnarkhede suggested changes Nov 29, 2020

View reviewed changes

swarajpure changed the title ~~[WIP] Added pullrequests/:id API route~~ Added pullrequests/:id API route Nov 29, 2020

refactored code as per requested changes

f9ea2a0

whyDontI suggested changes Nov 29, 2020

View reviewed changes

swarajpure added 3 commits November 30, 2020 16:41

returning fetch call and destructing object as function call

c5f36de

returning keys as camelCase and using shorthand syntax for keys and v…

cae714a

…alues

added authorization via access token

7f0714b

swarajpure requested review from ankurnarkhede and whyDontI November 30, 2020 13:42

swarajpure mentioned this pull request Dec 1, 2020

[WIP]Create a /member/:id page that shows more details of every member Real-Dev-Squad/website-members#49

Closed

swarajpure added the enhancement New feature or request label Dec 2, 2020

using config variables instead of env, removed getNames function and …

157ba58

…some changes in response

whyDontI previously approved these changes Dec 2, 2020

View reviewed changes

ankurnarkhede suggested changes Dec 2, 2020

View reviewed changes

authorization using clientId and clientSecret

5c8a473

swarajpure dismissed whyDontI’s stale review via 5c8a473 December 2, 2020 18:10

ankurnarkhede approved these changes Dec 3, 2020

View reviewed changes

ankurnarkhede merged commit 68fe6e6 into Real-Dev-Squad:develop Dec 3, 2020

swarajpure deleted the pullrequests branch March 16, 2021 18:22

swarajpure changed the title ~~Added pullrequests/:id API route~~ Add pullrequests/:id route Apr 24, 2021

swarajpure changed the title ~~Add pullrequests/:id route~~ Add pullrequests/:id route to fetch PRs of a given id of user using GitHub API Apr 24, 2021

Add pullrequests/:id route to fetch PRs of a given id of user using GitHub API #90

Add pullrequests/:id route to fetch PRs of a given id of user using GitHub API #90

Conversation

swarajpure commented Nov 26, 2020 • edited Loading

whyDontI left a 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

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

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

whyDontI left a 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

whyDontI left a comment • edited Loading

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

ankurnarkhede left a comment

Choose a reason for hiding this comment

Add `pullrequests/:id` route to fetch PRs of a given `id` of user using GitHub API #90

Add `pullrequests/:id` route to fetch PRs of a given `id` of user using GitHub API #90

swarajpure commented Nov 26, 2020 •

edited

Loading

whyDontI left a comment •

edited

Loading