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

SAC-5 Update User PATCH #28

Merged
merged 35 commits into from
Jan 22, 2024
Merged

SAC-5 Update User PATCH #28

merged 35 commits into from
Jan 22, 2024

Conversation

michael-brennan2005
Copy link
Contributor

@michael-brennan2005 michael-brennan2005 commented Jan 19, 2024

Description

Link to Ticket

This PR:

  • Implements the PATCH update user endpoint, specified in the JIRA ticket
  • Adds the UserParams struct for validation and creating a models.User from, and which can hopefully be reused for POST create user requests.

How Has This Been Tested?

3 Tests can be found in tests/user_test.go:

  • Successful update (returns 200)
  • Invalid update due to invalid request parameters (returns 400)
  • Invalid update due to invalid id (returns 404)

Checklist

  • [X ] I have performed a self-review of my code
  • I have reached out to another developer to review my code
  • I have commented my code, particularly in hard-to-understand areas
  • New and existing unit tests pass locally with my changes

@garrettladley garrettladley requested review from a team, edwinliiiii, DOOduneye and garrettladley and removed request for a team January 19, 2024 21:42
@garrettladley garrettladley changed the title Sac 5 update user patch SAC-5 Update User PATCH Jan 19, 2024
Copy link
Member

@garrettladley garrettladley left a comment

Choose a reason for hiding this comment

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

Great job @michael-brennan2005 & @melodyyu754 👏 ❗Some minor feedback and some things @DOOduneye and I have to figure out. Super solid PR and attention to detail. So proud 😁

Copy link
Member

Choose a reason for hiding this comment

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

can we untrack this?

Copy link
Contributor

Choose a reason for hiding this comment

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

ignore this, we need to track it

Copy link
Contributor

Choose a reason for hiding this comment

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

Copy link
Member

Choose a reason for hiding this comment

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

can we untrack this?

backend/src/types/user_params.go Outdated Show resolved Hide resolved
backend/src/transactions/user.go Outdated Show resolved Hide resolved
backend/src/transactions/user.go Outdated Show resolved Hide resolved
backend/src/services/user.go Outdated Show resolved Hide resolved
backend/src/controllers/user.go Outdated Show resolved Hide resolved
Copy link
Member

Choose a reason for hiding this comment

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

good stuff! conceptual question for you guys, should adding an approved email domain require a code change 🤔 ? see the config directory and config.go for how we could handle this 🦀

Copy link
Member

Choose a reason for hiding this comment

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

could we test this validation here

@@ -39,3 +40,121 @@ func TestGetAllUsersWorks(t *testing.T) {
// assert that the user returned from the database is the same as the user returned from the API
assert.Equal(dbUser, respUser)
}

func TestUpdateUserWorks(t *testing.T) {
Copy link
Member

@garrettladley garrettladley Jan 19, 2024

Choose a reason for hiding this comment

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

see this new PR for an improvement to our testing helpers!

backend/src/controllers/user.go Outdated Show resolved Hide resolved
backend/src/services/user.go Outdated Show resolved Hide resolved
backend/src/types/user_params.go Outdated Show resolved Hide resolved
)

func ValidateEmail(fl validator.FieldLevel) bool {
email, err := emailaddress.Parse(fl.Field().String())
Copy link
Contributor

Choose a reason for hiding this comment

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

might be worth it to take advantage of regex over importing a utility if you have the time

backend/src/utilities/validator.go Outdated Show resolved Hide resolved
backend/src/types/user_params.go Outdated Show resolved Hide resolved
backend/src/services/user.go Outdated Show resolved Hide resolved
backend/src/services/user.go Outdated Show resolved Hide resolved
backend/src/services/user.go Outdated Show resolved Hide resolved
backend/src/services/user.go Outdated Show resolved Hide resolved
zacklassetter and others added 2 commits January 21, 2024 15:04
Co-authored-by: edwinliiiii <[email protected]>
Co-authored-by: edwinliiiii <[email protected]>
Co-authored-by: Garrett Ladley <[email protected]>
Co-authored-by: garrettladley <[email protected]>
@@ -16,3 +16,22 @@ func GetAllUsers(db *gorm.DB) ([]models.User, error) {

return users, nil
}

func UpdateUser(db *gorm.DB, id string, user models.User) (*models.User, error) {
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
func UpdateUser(db *gorm.DB, id string, user models.User) (*models.User, error) {
func UpdateUser(db *gorm.DB, id uint, user models.User) (*models.User, error) {

@@ -19,3 +23,30 @@ type UserService struct {
func (u *UserService) GetAllUsers() ([]models.User, error) {
return transactions.GetAllUsers(u.DB)
}

// Updates a user
func (u *UserService) UpdateUser(id string, params models.UpdateUserRequestBody) (*models.User, error) {
Copy link
Member

Choose a reason for hiding this comment

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

for standardization, use utilities.ValidateID to validate and convert id into a uint at some point before passing id to transaction. see UpdateTag for inspo

Copy link
Member

@garrettladley garrettladley left a comment

Choose a reason for hiding this comment

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

Also see my reply to your message on Slack

@DOOduneye DOOduneye merged commit de0127b into main Jan 22, 2024
2 checks passed
@DOOduneye DOOduneye deleted the SAC-5-Update-User-PATCH branch January 22, 2024 20:02
DOOduneye added a commit that referenced this pull request Jan 22, 2024
Co-authored-by: Melody Yu <[email protected]>
Co-authored-by: Garrett Ladley <[email protected]>
Co-authored-by: Zackary Lassetter <[email protected]>
Co-authored-by: edwinliiiii <[email protected]>
Co-authored-by: edwinliiiii <[email protected]>
Co-authored-by: garrettladley <[email protected]>
Co-authored-by: David Oduneye <[email protected]>
Co-authored-by: David Oduneye <[email protected]>
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.

5 participants