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

Enable Retrieval of Existing Users based on userid #44

Merged
merged 12 commits into from
Dec 22, 2020
Merged

Enable Retrieval of Existing Users based on userid #44

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
12 commits
Select commit Hold shift + click to select a range
3f146a6
Enable Retrieval of Existing Users based on userid
Nov 26, 2020
3509d3e
update schema
Nov 26, 2020
7d70834
revert schema update
Nov 26, 2020
e759aae
remove preconfigured users and update tests
Dec 13, 2020
245ad0e
test update
Dec 13, 2020
92eb73d
test update
Dec 17, 2020
5be7a92
add backcround step for pre-configured user
Dec 18, 2020
90445c8
add timeout to integration tests before http request
Dec 22, 2020
33739e7
fix timeout to integration tests
Dec 22, 2020
321ca28
Merge branch 'develop' into ft27
aakano684 Dec 22, 2020
cc8727d
fix merge conflicts with develop
Dec 22, 2020
a3fe66d
remove timeout
Dec 22, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 6 additions & 0 deletions integration-tests/features/User.feature
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,12 @@ Feature: Features related to User Management
When I make a POST to the function at '/organisations/validOrg/users'
Then I should get a status code 201
And the response should be a superset of all the keys and values set from 'newUserRequest'

@getUserFromOrg @createdUserInOrg
Scenario: GET to /users/{userId} with valid userId returns user
When I make a GET to the function at '/organisations/validOrg/users/newUser'
Then I should get a status code 200
And the response should be a superset of all the keys and values set from 'newUserRequest'
Copy link
Contributor

Choose a reason for hiding this comment

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

The problem we have here now is that we have integration tests that now depend on one another running in a certain order. As scenarios will get more complex we generally want to avoid this. One way of resolving this is to add the steps for creating the above user within the feature itself so:

Suggested change
When I make a POST to the function at '/organisations/validOrg/users'
Then I should get a status code 201
And the response should be a superset of all the keys and values set from 'newUserRequest'
@getUserFromOrg @createdUserInOrg
Scenario: GET to /users/{userId} with valid userId returns user
When I make a GET to the function at '/organisations/validOrg/users/newUser'
Then I should get a status code 200
And the response should be a superset of all the keys and values set from 'newUserRequest'
When I make a POST to the function at '/organisations/validOrg/users'
Then I should get a status code 201
And the response should be a superset of all the keys and values set from 'newUserRequest'
@getUserFromOrg @createdUserInOrg
Scenario: GET to /users/{userId} with valid userId returns user
Given I generate a json payload called 'newUserRequestWithSpecificId'
And I make a POST to the function at '/organisations/validOrg/users'
And the response should be a superset of all the keys and values set from 'newUserRequestWithSpecificId'
When I make a GET to the function at '/organisations/validOrg/users/newUser'
Then I should get a status code 200
And the response should be a superset of all the keys and values set from 'newUserRequestWithSpecificId'


@createUserInOrg @errorHandling
Scenario: POST to /users with invalid body returns bad request
Expand Down
2 changes: 1 addition & 1 deletion integration-tests/features/support/payloads.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,7 +17,7 @@ module.exports.payloads = {
invalidParam: "ivalidValue"
},
newUserRequest: {
id: "user-identifier-for-mongo",
id: "newUser",
accountVerified: false,
name: "some name",
profileImageLink: "https://my.image.domain.com/it.jpg"
Expand Down
12 changes: 9 additions & 3 deletions src/main/java/io/mathdojo/useraccountservice/services/IdentityService.java
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,6 @@
package io.mathdojo.useraccountservice.services;

import java.util.Optional;
import java.util.Set;
import java.util.UUID;
import java.util.logging.Level;
Expand All @@ -20,6 +21,7 @@
public class IdentityService {

private static final String PRECONDITIONED_UNKNOWN_USER_ID = "unknownUserId";
private static final String PRECONDITIONED_KNOWN_USER_ID = "knownUserId";
private static final String PRECONDITIONED_UNKNOWN_ORGANISATION_ID = "unknownOrganisationId";
public static final String NEW_ENTITY_CANNOT_BE_ALREADY_VERIFIED_ERROR_MSG = "a new organisation cannot be created with a true verification status";
public static final String ORG_LESS_NEW_USER_ERROR_MSG = "a new user cannot be made without specifying a valid parent org";
Expand Down Expand Up @@ -122,13 +124,17 @@ private boolean isValidAccountModificationRequest(AccountRequest request) {
}

public User getUserInOrg(String expectedOrganisationId, String userId) {
String returnedOrgId = (getOrganisationById(expectedOrganisationId)).getId();
if (PRECONDITIONED_UNKNOWN_USER_ID.equals(userId)) {
String returnedOrgId = (getOrganisationById(expectedOrganisationId)).getId();
Optional<User> userToReturn = userRepo.findById(userId);
if (PRECONDITIONED_KNOWN_USER_ID.equals(userId)){
return new User(userId, true, "", "", expectedOrganisationId);
}
if (PRECONDITIONED_UNKNOWN_USER_ID.equals(userId) || !userToReturn.isPresent() || !returnedOrgId.equals(userToReturn.get().getBelongsToOrgWithId())) {
Copy link
Contributor

Choose a reason for hiding this comment

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

I think now that we are integrating with mongo, we should remove all the pre-conditioned known and unknown users as it could bite us later.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

theres quite a few tests that depend on these being there. How do you suggest we handle those

Copy link
Contributor

Choose a reason for hiding this comment

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

Sure, you mean the unit tests or the integration tests? If integration test, the comment I made on the PR as a whole explains a possible approach. We could also try a similar approach with our unit tests. I'll also be looking into whether there are any "in memory" Mongo solutions we can plug in for unit testing.

targetExecutionContext.getLogger().log(Level.WARNING,
String.format("UserId %s in Org %s could not be found", userId, expectedOrganisationId));
throw new IdentityServiceException(UNKNOWN_USERID_EXCEPTION_MSG);
}
return new User(userId, false, "a name", "https://domain.com/img.png", returnedOrgId);
return userToReturn.get();
}

/**
Expand Down
4 changes: 2 additions & 2 deletions src/test/java/io/mathdojo/useraccountservice/UserAccountServiceApplicationTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -210,7 +210,7 @@ public void testGetUserInOrgFunctionReturnsKnownUser() {

when(mockExecContext.getFunctionName()).thenReturn("getUserInOrg");
String parentOrgId = "customOrgId";
String userId = "my coolName";
String userId = "knownUserId";
User result = (User) handlerSpy.handleRequest(mockMessage,
new AccountModificationRequest(userId, parentOrgId, false, null, null),
mockExecContext);
Expand Down Expand Up @@ -313,7 +313,7 @@ public void testUpdatePermissionsThrowsNoErrorIfSuccessful() {
,UserPermission.ORG_ADMIN};
AccountModificationRequest permissionsModificationRequest = AccountModificationRequest
.Builder.createBuilder()
.withAccountId("someValidUserId")
.withAccountId("knownUserId")
.withParentOrgId("validOrgId")
.withUserPermissions(permissionsToSet)
.build();
Expand Down
10 changes: 5 additions & 5 deletions src/test/java/io/mathdojo/useraccountservice/services/IdentityServiceTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,12 +1,12 @@
package io.mathdojo.useraccountservice.services;

import static org.junit.Assert.assertEquals;
import static org.junit.Assert.assertNotNull;
import static org.junit.jupiter.api.Assertions.assertThrows;
import static org.junit.jupiter.api.Assertions.assertTrue;
import static org.mockito.Mockito.mock;

import java.util.HashSet;
import java.util.Optional;
import java.util.Set;
import java.util.logging.Logger;

Expand Down Expand Up @@ -57,6 +57,7 @@ public User answer(InvocationOnMock invocation) {
}
});


}

@Test
Expand Down Expand Up @@ -345,12 +346,11 @@ public void throwsExceptionIfAttemptToRetrieveUnknownUserFromKnownOrg() {

@Test
public void updateUserWithIdReturnsResultIfOrgAndAllParamsFilledAndValid() {
String orgId = "knownOrg";
String orgId = "aKnownOrg";

AccountRequest accountCreationRequest = new AccountRequest(false, "aName iWillChange",
"https://my.custom.domain/image-i-dont-like.png");
"https://my.custom.domain/image-i-dont-like.png", "knownUserId");
User oldUser = organisationService.createUserInOrg(orgId, accountCreationRequest);

String newName = "aName iWillNotChange";
String newProfileImageLink = "https://my.custom.domain/image-i-like.png";
AccountRequest accountModificationRequest = new AccountRequest(true, newName, newProfileImageLink);
Expand Down Expand Up @@ -473,7 +473,7 @@ public void throwsNoErrorIfSettingValidPermissionsForValidUserInValidOrgId() {
permissionsToSet.add(UserPermission.CONSUMER);
permissionsToSet.add(UserPermission.CREATOR);
permissionsToSet.add(UserPermission.ORG_ADMIN);
User modifiedUser = organisationService.updateUserPermissions("knownOrganisationId", "knownUserId",
User modifiedUser = organisationService.updateUserPermissions("aKnownOrg", "knownUserId",
permissionsToSet);

Set<UserPermission> modifiedUserPermissions = modifiedUser.getPermissions();
Expand Down