Ojn03/feat update user #12

ojn03 · 2023-10-03T15:41:31Z

ℹ️

Closes #5

📝 Description

Added an endpoint that allows users to update information in the database so that:

Only admins can update another person's info
Members and Alumni cannot update Applicant info
Applicants can only update their own info

Briefly list the changes made to the code:

Added a PUT route in the user controller to accept queries for updateUser and direct logic accordingly. The query accepts userId param as a string and a body as a UpdateUserDTO (defined below)
defined a data transfer object class (UpdateUserDTO), to define the expected structure of the body in the put request.
updated user service to handle the request. Essentially, this first checks the database for the userid, and if it exists, updates the corresponding user with the given DTO. This then returns the updated user on success.
Added input validation to the UpdateUserDTO

✔️ Verification

Created an exampleUser (lets call him Bob) to simulate endpoint callers. Created a local MongoDB database "c4cOpsTest" with multiple users and updated Bob to check for expected behavior in the following scenarios:

change Bob's status to Admin. Given an existing userID and valid shema body-> 200 OK + update any userID's associated User and return the updated user
change Bob's status to Applicant. Given a userID that is not Bob's -> 400 Bad Request
change Bob's status to Applicant/ALUMNI. Given a UserId with Applicant Status -> 400 BAD REQUEST

🏕️ (Optional) Future Work / Notes

maybe add an error for when no parameters given
Update UpdateUserDTO input validation with Regex to check whether links are in valid format

chromium-52

💯 great job on this! Just a couple notes here and there

chromium-52 · 2023-10-04T05:14:21Z

apps/backend/src/users/types.ts

@@ -0,0 +1,31 @@
+// TODO: Probably want these types to be available to both the frontend and backend in a "common" folder


chromium-52 · 2023-10-04T05:16:52Z

apps/backend/src/users/types.ts

+  APPLICANT = 'Applicant',
+}
+
+export enum Team {


💭 should we add EBOARD as one of the possible values for this enum since the roles also contain all of the eboard positions? I think this will depend on what the purpose of the team property is (which we should follow up with product about)

Im not too familiar with how the org's structure should be represented here but this does sound sensible. I think once we get clearer specifications, we can make a quick change here

I added EBOARD to the types. We can see what the purpose of the team property is and remove it if necessary

chromium-52 · 2023-10-04T05:21:41Z

apps/backend/src/users/types.ts

+  DIRECTOR_OF_FINANCE = 'Director of Finance',
+  DIRECTOR_OF_MARKETING = 'Director of Marketing',
+  DIRECTOR_OF_RECRUITMENT = 'Director of Recruitment',
+  CO_DIRECTOR_OF_OPERATIONS = 'Co-Director of Operations',


🔧 can we take out the CO_ prefix from ops and events? It's only fairly recent that we started having co-directors (so that we'll probably have just one co-director of ops in a year or two again) and we could theoretically have multiple people with the same position for most of the other director-level positions as well. And some teams have co-tech leads too so I think it'll be better to just ignore the "co-" part and we'll know if there are co-directors if there are multiple people with the same role

chromium-52 · 2023-10-04T05:22:47Z

apps/backend/src/users/user.entity.ts

 export class User {
-  @PrimaryGeneratedColumn()
-  id: number;
+  @ObjectIdColumn()


🔧 just to make our lives easier, I think we should have this column be a number column instead so we're not dealing with too many ObjectIds (which can get a little tedious):

@Column({ primary: true }) userId: number;

chromium-52 · 2023-10-04T05:26:37Z

apps/backend/src/users/users.controller.ts

    return this.usersService.findAll();
  }
+
+  @Put(':userId')


⛏️ should this be a patch route?

chromium-52 · 2023-10-04T05:33:11Z

apps/backend/src/users/users.dto.ts

⛏️ could we be able to update the name of this file to update-user.dto.ts? Generally, for a dto, we want the name to be name-of-the-dto.dto.ts so that it's easier to tell which file contains which dto

chromium-52 · 2023-10-04T05:36:20Z

apps/backend/src/users/users.dto.ts

+  profilePicture?: string;
+
+  @IsOptional()
+  linkedIn?: string;


⛏️ for linkedIn and github properties, can we also make sure that they're links (i.e. @IsUrl())?

chromium-52 · 2023-10-04T05:40:01Z

apps/backend/src/users/users.service.ts

  }
+
+  async updateUser(
+    UpdateUserDTO: UpdateUserDTO,


⛏️ can we make UpdateUserDTO camelCase?

Suggested change

UpdateUserDTO: UpdateUserDTO,

updateUserDTO: UpdateUserDTO,

chromium-52 · 2023-10-04T05:45:35Z

apps/backend/src/users/users.service.ts

+    await this.usersRepository.update(id, UpdateUserDTO);
+    return await this.usersRepository.findOne({
+      where: {
+        _id: { $eq: id },
+      },
+    });


⛏️ does something like this also work?

Suggested change

await this.usersRepository.update(id, UpdateUserDTO);

return await this.usersRepository.findOne({

where: {

_id: { $eq: id },

},

});

return await this.usersRepository.save(id, UpdateUserDTO);

chromium-52 · 2023-10-04T05:54:37Z

apps/backend/src/users/users.service.ts

+      );
+    }
+
+    if (


❓ does this mean that only admins can update other people's info? If so, do the two checks above (on lines 63 and 72) check anything that this check misses (in other words, is it possible for someone to change someone's info such that it triggers the checks on either 63 or 72 but not the check on line 82)?

…ggestions

chromium-52

👍 Just two small nitpicks but looks great overall!

chromium-52 · 2023-10-15T01:43:02Z

apps/backend/src/users/users.service.ts

-    if (exampleUser.status == Status.APPLICANT) {
+    const currentUser = getCurrentUser();
+
+    if (currentUser.status == Status.APPLICANT) {


⛏️

Suggested change

if (currentUser.status == Status.APPLICANT) {

if (currentUser.status === Status.APPLICANT) {

chromium-52 · 2023-10-15T01:44:09Z

apps/backend/src/users/update-user.dto.ts

+
+  @IsOptional()
+  @IsUrl({
+    protocols: ['http', 'https'],


⛏️ Very cool! If we do this, I think we should restrict the protocol to just https. Linkedin and github both support https and there's no reason to prefer http over https

ananyar807

Great Job!

ananyar807 · 2023-10-19T03:17:12Z

apps/backend/src/users/user.entity.ts


  @Column()
-  profilePicture: string;
+  profilePicture: string | null;


💭 maybe instead of having a null profilePicture, we could have a placeholder picture? Just an idea, maybe for a future enhancement

* feat: completed updateUser endpoint resolves #5 * style: used strict comparison operators and modified error messages * added error handling for objectID * feat: added input validation to UpdateUserDTO * fix: applied suggestions from PR review * fix: added domain specific URL validators for DTO * fix: converted ObjectId primary column to numbers and finalized PR suggestions * fix: strict equality and https restricted protocol --------- Co-authored-by: Harrison Kim <kim.harr@northeastern.edu>

ojn03 and others added 4 commits September 30, 2023 13:49

feat:completed updateUser endpoint resolves #5

429c9bb

style: used strict comparison operators and modified error messages

e2dc648

added error handling for objectID

ebd101b

feat: added input validation to UpdateUserDTO

f46f10c

kimharr24 requested review from ananyar807 and chromium-52 October 4, 2023 04:46

chromium-52 reviewed Oct 4, 2023

View reviewed changes

ojn03 and others added 4 commits October 7, 2023 00:38

fix: applied suggestions from PR review

4dbc239

fix: added domain specific URL validators for DTO

58b60da

fix: resolved merge conflicts with main

a046a91

fix: converted ObjectId primary column to numbers and finalized PR su…

1571920

…ggestions

kimharr24 requested a review from chromium-52 October 14, 2023 23:14

chromium-52 approved these changes Oct 15, 2023

View reviewed changes

fix: strict equality and https restricted protocol

d40835b

ananyar807 approved these changes Oct 19, 2023

View reviewed changes

kimharr24 merged commit a72c127 into main Oct 19, 2023

chromium-52 deleted the ojn03/feat-updateUser branch February 22, 2024 22:24

		@@ -0,0 +1,31 @@
		// TODO: Probably want these types to be available to both the frontend and backend in a "common" folder

+                    );
+                  }
+                  if (

	if (currentUser.status == Status.APPLICANT) {
	if (currentUser.status === Status.APPLICANT) {

Ojn03/feat update user #12

Ojn03/feat update user #12

Uh oh!

Conversation

ojn03 commented Oct 3, 2023 • edited by kimharr24 Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

ℹ️

📝 Description

✔️ Verification

🏕️ (Optional) Future Work / Notes

Uh oh!

chromium-52 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

chromium-52 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

chromium-52 Oct 15, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ananyar807 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

ojn03 commented Oct 3, 2023 •

edited by kimharr24

Loading

chromium-52 Oct 15, 2023 •

edited

Loading