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

adding feature of user search #35

Merged
merged 33 commits into from
Mar 18, 2022
Merged

adding feature of user search #35

merged 33 commits into from
Mar 18, 2022

Conversation

AlexMiao7
Copy link
Contributor

@AlexMiao7 AlexMiao7 commented Mar 15, 2022 •
edited
Loading

Description

Adding three functions for user search by id/name/username using mongoose.

Related Issue

Solves #20

Type of change

  • New feature (enhancement)

How Has This Been Tested?

This schema has not been tested.

Checklist:

  • Does a similar (open or closed) pull request not already exist?
  • Is the pull request head repository a fork repository?
  • Is the pull request compare branch a development branch?
  • Is the code documented, particularly in hard-to-understand areas?
  • Does the code build without new warnings?
  • Has testing been performed that proves changes are effective and work?
  • Has a self- and/or peer-review of the code been performed?
  • Have dependent changes been merged and published in downstream modules?
  • Does all new and existing automated testing pass?
  • Is the person responsible for the repository assigned to the pull request?
  • Is the pull request linked to a project?
  • Is the pull request linked to a milestone?

For more information, refer to the Contributing Guidelines document.

@AlexMiao7 AlexMiao7 requested a review from a team as a code owner March 15, 2022 10:06
@AlexMiao7 AlexMiao7 self-assigned this Mar 15, 2022
@AlexMiao7 AlexMiao7 added the enhancement New feature or request label Mar 15, 2022
@AlexMiao7 AlexMiao7 added this to In progress in Team 5 - Assignment 1 - Backend via automation Mar 15, 2022
@AlexMiao7 AlexMiao7 added this to the Assignment 1 milestone Mar 15, 2022
@AlexMiao7 AlexMiao7 linked an issue Mar 15, 2022 that may be closed by this pull request
Implement request for user search #20
Closed
@R055A R055A assigned R055A and unassigned AlexMiao7 Mar 15, 2022
@R055A R055A moved this from In progress to Review in progress in Team 5 - Assignment 1 - Backend Mar 15, 2022
@R055A R055A moved this from Review in progress to In progress in Team 5 - Assignment 1 - Backend Mar 15, 2022
kimslor
kimslor reviewed Mar 15, 2022
View reviewed changes
Copy link
Contributor

@kimslor kimslor left a comment •
edited
Loading

Choose a reason for hiding this comment

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

This is looking pretty good. Maybe add in some unit test, or you can just manually test it somehow yourself.

@kimslor kimslor assigned kimslor and unassigned R055A Mar 15, 2022
@R055A R055A self-requested a review March 16, 2022 02:01
@R055A R055A assigned R055A and unassigned kimslor Mar 16, 2022
@R055A R055A moved this from In progress to Review in progress in Team 5 - Assignment 1 - Backend Mar 16, 2022
@R055A R055A moved this from Review in progress to In progress in Team 5 - Assignment 1 - Backend Mar 16, 2022
R055A
R055A suggested changes Mar 16, 2022
View reviewed changes
Copy link
Contributor

@R055A R055A left a comment •
edited
Loading

Choose a reason for hiding this comment

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

Please read the comments.

The model is not being used, which I think it should be.

The model is for managing the data and logic.

The controller is for managing the communication between the model and the view, so it formats input data from each into commands that can be accepted by the other. It is where we would ensure the application is not vulnerable to an SQL-i attack, for example.

controllers/user.server.controller.js Outdated Show resolved Hide resolved
controllers/user.server.controller.js Outdated Show resolved Hide resolved
@R055A R055A moved this from In progress to To do in Team 5 - Assignment 1 - Backend Mar 16, 2022
@R055A R055A moved this from To do to In progress in Team 5 - Assignment 1 - Backend Mar 16, 2022
@R055A
Copy link
Contributor

R055A commented Mar 17, 2022

I will help fix this soon

@R055A R055A self-requested a review March 17, 2022 04:50
R055A
R055A suggested changes Mar 17, 2022
View reviewed changes
Copy link
Contributor

@R055A R055A left a comment

Choose a reason for hiding this comment

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

I have left some suggestions.

It is not a good idea to rely on data being manually entered in the database for the tests to pass.

routes/user.server.routes.js Outdated Show resolved Hide resolved
models/user.server.model.js Outdated Show resolved Hide resolved
controllers/user.server.controller.js Show resolved Hide resolved
controllers/user.server.controller.js Outdated Show resolved Hide resolved
@AlexMiao7
Copy link
Contributor Author

I have left some suggestions.

It is not a good idea to rely on data being manually entered in the database for the tests to pass.

what i was thinking is that I manually inserted some data into the databbase and try if i can search those data by id, and I also try to search some data which does not exist. So what else could I do in order to test the function?

@R055A R055A self-requested a review March 17, 2022 07:25
@R055A
Copy link
Contributor

R055A commented Mar 17, 2022

I have left some suggestions.
It is not a good idea to rely on data being manually entered in the database for the tests to pass.

what i was thinking is that I manually inserted some data into the databbase and try if i can search those data by id, and I also try to search some data which does not exist. So what else could I do in order to test the function?

The problem with that is new changes have all collection data in the test database wiped before each test and only the test database is used for tests.

R055A
R055A suggested changes Mar 17, 2022
View reviewed changes
test/user.test.js Outdated Show resolved Hide resolved
test/user.test.js Outdated Show resolved Hide resolved
models/user.server.model.js Outdated Show resolved Hide resolved
controllers/user.server.controller.js Outdated Show resolved Hide resolved
models/user.server.model.js Outdated Show resolved Hide resolved
R055A
R055A suggested changes Mar 17, 2022
View reviewed changes
test/user.test.js Outdated Show resolved Hide resolved
@R055A R055A self-requested a review March 18, 2022 02:05
R055A
R055A approved these changes Mar 18, 2022
View reviewed changes
Copy link
Contributor

@R055A R055A left a comment

Choose a reason for hiding this comment

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

Sorry for the delay. LGTM!

Team 5 - Assignment 1 - Backend automation moved this from In progress to Reviewer approved Mar 18, 2022
@R055A R055A merged commit 4635365 into SE701-T5:main Mar 18, 2022
Team 5 - Assignment 1 - Backend automation moved this from Reviewer approved to Done Mar 18, 2022
@AlexMiao7 AlexMiao7 deleted the user_search branch March 18, 2022 02:16
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@kimslor kimslor kimslor left review comments

@R055A R055A R055A approved these changes

Assignees

@R055A R055A

Labels
enhancement New feature or request
Projects
No open projects
Team 5 - Assignment 1 - Backend
Done
Milestone
Assignment 1
Development

Successfully merging this pull request may close these issues.

Implement request for user search
3 participants
@AlexMiao7 @R055A @kimslor