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

add user profile #29

Merged
merged 30 commits into from
Apr 18, 2024
Merged

add user profile #29

merged 30 commits into from
Apr 18, 2024

Conversation

dbraquart
Copy link
Contributor

No description provided.

Signed-off-by: David BRAQUART <[email protected]>
@dbraquart dbraquart force-pushed the dbraquart/add-user-profile branch from bd5e9d4 to e0f85d9 Compare March 21, 2024 11:14
Signed-off-by: David BRAQUART <[email protected]>
Signed-off-by: David BRAQUART <[email protected]>
…ofile

# Conflicts:
#	src/main/java/org/gridsuite/useradmin/server/UserAdminController.java
#	src/main/java/org/gridsuite/useradmin/server/service/UserAdminService.java
#	src/main/resources/application-local.yml
#	src/test/java/org/gridsuite/useradmin/server/UserAdminTest.java
Signed-off-by: David BRAQUART <[email protected]>
Signed-off-by: David BRAQUART <[email protected]>
@dbraquart dbraquart force-pushed the dbraquart/add-user-profile branch from c636359 to a3ad1ec Compare March 28, 2024 07:59
Signed-off-by: David BRAQUART <[email protected]>
@dbraquart dbraquart force-pushed the dbraquart/add-user-profile branch from a3ad1ec to 87fbf39 Compare March 28, 2024 08:09
@dbraquart dbraquart changed the title WIP add user profile add user profile Mar 28, 2024
Signed-off-by: David BRAQUART <[email protected]>
@dbraquart dbraquart force-pushed the dbraquart/add-user-profile branch from 62e2037 to 154339c Compare March 28, 2024 13:51
@dbraquart dbraquart requested a review from etiennehomer March 28, 2024 14:05
Signed-off-by: David BRAQUART <[email protected]>
@dbraquart dbraquart force-pushed the dbraquart/add-user-profile branch from ca38798 to cee73e0 Compare March 28, 2024 19:16
return new UserInfos(entity.getSub(), isAdminFn.test(entity.getSub()), profileName);
}

public void update(UserInfos userInfos, Optional<UserProfileEntity> userProfileEntity) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

It's weird to give a dto and an entity as arguments to this method.
Maybe do the setSub() and setProfile directly in the service

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yes done

if (entity == null) {
return null;
}
Boolean globalValidity = null;
Copy link
Collaborator

Choose a reason for hiding this comment

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

this part should be out of the toDto() method. Just pass a boolean

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yes done

assertIsAdmin(userId);
return userAdminRepository.deleteAllBySubIn(subs);
}

@Transactional()
public void updateUser(String sub, String userId, UserInfos userInfos) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

You already have the "sub" in the userInfos

Copy link
Contributor Author

@dbraquart dbraquart Apr 2, 2024

Choose a reason for hiding this comment

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

Actually they are different. "sub" is the resource I want to access/change, while "userInfos.sub" is the new name I may want to set.

Copy link
Contributor

Choose a reason for hiding this comment

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

Should really a user sub be updatable ?

}

@Transactional()
public void updateProfile(UUID profileUuid, String userId, UserProfile userProfile) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

You already have the "profileUuid" in the userProfile

Copy link
Contributor Author

Choose a reason for hiding this comment

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

If I remove it, I will have a warning in Controller (unused param). what to do ?

Copy link
Collaborator

Choose a reason for hiding this comment

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

It's fine like this as we keep the profileUuid in the endpoint

public ResponseEntity<UserProfile> updateUser(@PathVariable("sub") String sub,
@RequestHeader("userId") String userId,
@RequestBody UserInfos userInfos) {
service.updateUser(sub, userId, userInfos);
Copy link
Collaborator

Choose a reason for hiding this comment

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

You can remove "sub" and work with userInfos nope ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

no I need both

Copy link
Collaborator

Choose a reason for hiding this comment

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

Yes as discussed, it's the classic way to update a resource with rest api

UUID id,
String name,
UUID loadFlowParameterId,
Boolean validity
Copy link
Collaborator

@etiennehomer etiennehomer Apr 2, 2024

Choose a reason for hiding this comment

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

isValid ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

renamed as allParametersLinksValid

@dbraquart dbraquart requested a review from etiennehomer April 3, 2024 13:48
assertIsAdmin(userId);
return userAdminRepository.deleteAllBySubIn(subs);
}

@Transactional()
public void updateUser(String sub, String userId, UserInfos userInfos) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Should really a user sub be updatable ?

Signed-off-by: David BRAQUART <[email protected]>
Signed-off-by: David BRAQUART <[email protected]>
@dbraquart dbraquart force-pushed the dbraquart/add-user-profile branch from 50f6375 to 472d2bd Compare April 15, 2024 07:22
Signed-off-by: David BRAQUART <[email protected]>
UserInfosEntity user = userInfosRepository.findBySub(sub).orElseThrow(() -> new UserAdminException(NOT_FOUND));
Optional<UserProfileEntity> profile = userProfileRepository.findByName(userInfos.profileName());
user.setSub(userInfos.sub());
user.setProfile(profile.orElse(null));
Copy link
Contributor

Choose a reason for hiding this comment

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

Shouldn't we return a bad_input if the profile is not define ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

we have a common habit to return 404 if we try to update a missing resource.

Copy link
Contributor

Choose a reason for hiding this comment

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

No I agree if the user is missing, but if the profile is missing ? Here if the profile does not exist we just set it to null but we don't throw.

Copy link

@dbraquart dbraquart merged commit d58a3fa into main Apr 18, 2024
3 checks passed
@dbraquart dbraquart deleted the dbraquart/add-user-profile branch April 18, 2024 09:17
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.

3 participants