Skip to content

Api all users#65

Merged
Caleb-Cohen merged 7 commits into
mainfrom
apiAllUsers
Mar 30, 2023
Merged

Api all users#65
Caleb-Cohen merged 7 commits into
mainfrom
apiAllUsers

Conversation

@Caleb-Cohen

Copy link
Copy Markdown
Collaborator

Do not merge until #33 is completed

Closes #57 (replace with issue number)

Description

Created endpoint that returns all users.

Abstracted user test

Testing

execute npm run test

Type of change

Please remove all except for everything applicable, and then remove this line.

  • New feature (non-breaking change which adds functionality)
  • Change to documentation/GitHub flows/CICD

Screenshots

Please include any screenshots if there are visual changes.

Checklist:

  • Changes have new/updated automated tests, if applicable
  • I have performed a self-review of my own code
  • I have added comments on any new, hard-to-understand code
  • Changes have been manually tested
  • Changes generate no new errors or warnings

@Caleb-Cohen Caleb-Cohen left a comment

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

added some comments

Comment thread server/api/users/users.test.ts Outdated
Comment thread server/test/utils.ts
}

export default createUser;
function getExpectedUserObject(user: User) {

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

abstracted user compares for testing

Base automatically changed from #33-remove-get-user-profile-endpoint to main March 26, 2023 08:13

@timmyichen timmyichen left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Mostly looks good, just left some comments about changes I think would be good

Comment thread server/api/users/users.controller.ts Outdated
Comment thread server/api/users/users.controller.ts Outdated
Comment thread server/api/users/users.test.ts Outdated
Comment thread server/api/users/users.test.ts Outdated
Comment thread server/api/users/users.test.ts Outdated
Comment thread package.json Outdated
"docker:deps": "docker-compose up -d",
"heroku-postbuild": "npm run build && npx sequelize-cli db:migrate",
"jest": "cross-env NODE_ENV=test npx jest --runInBand",
"jest": "cross-env NODE_ENV=test npx jest --runInBand --forceExit",

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

I could not get this to pass github actions testing without it. It would hang otherwise.

@devonzara devonzara left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Looks good for the most part, few minor changes I'd suggest...

Comment thread server/api/users/users.controller.ts Outdated
Comment thread server/api/users/users.controller.ts Outdated
Comment thread server/api/users/users.controller.ts Outdated
Comment thread server/api/users/users.test.ts Outdated
Comment thread server/api/users/users.test.ts Outdated

@MatthewBozin MatthewBozin left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

LGTM

@Caleb-Cohen Caleb-Cohen merged commit ed5bb9a into main Mar 30, 2023
@Caleb-Cohen Caleb-Cohen deleted the apiAllUsers branch March 30, 2023 23:40
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Create new API route that returns all users

4 participants