Enable Retrieval of Existing Users based on userid #44

aakano684 · 2020-11-26T05:00:45Z

⚠️⚠️ ⚠️ All PRs must have a version of the format pr${number} and an override target url set up on the user-accout-test api ⚠️ ⚠️ ⚠️

Purpose

Enable Retrieval of Existing Users based on userid

Does this introduce a breaking change?

[ ] Yes
[x] No

Pull Request Type

What kind of change does this Pull Request introduce?

[ ] Bugfix
[x] Feature
[ ] Code style update (formatting, local variables)
[ ] Refactoring (no functional changes, no api changes)
[ ] Documentation content changes
[ ] Other... Please describe:

How to Test

Get the code

git clone [repo-address]
cd [repo-name]
git checkout [branch-name]
npm install

Test the code

What to Check

Verify that the following are valid

...

Other Information

noce2

Left some comments about removing preconditioned known userIds and the ways in which our intgeration tests will have to change as a result. Additionally, where we have integration tests that require a user to exist prior to their execution, it may make sense to put them in a separate .feature file and have them all utilise a common Background step that creates a new user before all scenarios are executed. See here: https://github.com/cucumber/cucumber-js/blob/master/features/background.feature#L12

Given our user ids need to be unique (the way we've setup our mongo repository object should enforce this? Please correct me if I'm wrong), we might have to think abit more about how we set that up and how we share that uniqueId (we have a world object setup for exactly this purpose) with subsequent tests that need them.

noce2 · 2020-11-29T01:02:42Z

src/main/java/io/mathdojo/useraccountservice/services/IdentityService.java

+        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())) {


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.

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

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.

noce2 · 2020-11-29T01:21:26Z

integration-tests/features/User.feature

    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'


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'

noce2

Some great stuff here. Left a comment about an improvement we could make to the repository returned by the test configuration.

noce2 · 2020-12-16T21:59:39Z

integration-tests/features/User.feature

+
+  @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'
+   Given I make a GET to the function at '/organisations/validOrg/users/specificId'
+   Then I should get a status code 200
+   And the response should be a superset of all the keys and values set from 'newUserRequestWithSpecificId'


We spoke about refactoring this already so no additional comments.

noce2 · 2020-12-16T22:05:57Z

src/main/java/io/mathdojo/useraccountservice/services/IdentityService.java

+        User retrievedUser = getUserInOrg(orgId, userId);
+        userRepo.delete(retrievedUser);
+


Although the scope of #27 was allowing existing users to be retrieved it seems we're now doing a bigger piece of updating all user and org management functionality to integrate with the repository? Have I understood that right?

Just the user functionality. Once the preconditioned users were removed it was really just a 3 line change to add the other functionality(162, 166 and 167) and it was a convenient way to see that the test changes worked.

noce2 · 2020-12-16T22:14:40Z

src/test/java/io/mathdojo/useraccountservice/TestConfig.java

+@TestConfiguration
+public class TestConfig {
+	Map<String, User> createdUsers = new HashMap<>();
+
+	@Bean
+	MathDojoUserRepository userRepo() {
+		// configured known user
+		createdUsers.put("aKnownUser", new User("aKnownUser", false, "", "", "aKnownOrg"));
+		MathDojoUserRepository userRepo = new MathDojoUserRepository() {
+
+			@Override
+			public <S extends User> Optional<S> findOne(Example<S> example) {
+				// TODO Auto-generated method stub
+				return null;
+			}
+
+			@Override
+			public <S extends User> Page<S> findAll(Example<S> example, Pageable pageable) {


I really like this approach. I do wonder whether we could make this more flexible & easier to use. Rather than a hardened hand-written config that replicates the actual bean (we had to implement all the methods) could we return a mochito version of this? That would allow us to control behaviour in the test in which it is being used. Thoughts?

It may be possible, and I would be happy to switch to a mockito version if the need for flexibility arises but I thought that a concrete reusable mock repository fit our needs better, because the behaviour should ideally be always fixed just like the actual mongo repo object.

From my understanding, using mockito wouldn't stop it from being a "concrete" implementation per se. It's more a general "code smell" if in one of your tests you end up having to implement all the methods for something that is supposed to be a mock of an implementation. It's not a hard stop for me, it's something I learned and figured was worth sharing.

noce2 · 2020-12-16T22:17:12Z

src/test/java/io/mathdojo/useraccountservice/UserAccountServiceApplicationTest.java

+import com.microsoft.azure.functions.ExecutionContext;
+import com.microsoft.azure.functions.HttpRequestMessage;
+


Seems we don't use these?

just organising imports with an eclipse shortcut

noce2 · 2020-12-16T22:17:42Z

src/test/java/io/mathdojo/useraccountservice/UserAccountServiceApplicationTest.java

@@ -33,6 +32,8 @@
 import io.mathdojo.useraccountservice.services.SystemService;

 @RunWith(SpringRunner.class)
+@ContextConfiguration(classes = TestConfig.class)


💯 🎉🎉🎉

noce2 · 2020-12-16T22:18:43Z

src/test/java/io/mathdojo/useraccountservice/UserAccountServiceApplicationTest.java

@@ -33,6 +32,8 @@
 import io.mathdojo.useraccountservice.services.SystemService;

 @RunWith(SpringRunner.class)
+@ContextConfiguration(classes = TestConfig.class)
+@SuppressWarnings({"rawtypes","unchecked"})


What does this do?

purely cosmetic for compiler warnings. We have a good reason for not adding concrete types to some objects

noce2 · 2020-12-16T22:19:22Z

src/test/java/io/mathdojo/useraccountservice/services/IdentityServiceTest.java

        Logger testLogger = mock(Logger.class);
        Mockito.when(targetExecutionContext.getLogger()).thenReturn(testLogger);
        Mockito.when(userRepo.save(Mockito.any(User.class))).thenAnswer(new Answer<User>() {
            public User answer(InvocationOnMock invocation) {
                return (User) invocation.getArguments()[0];
            }
        });
+        Mockito.when(userRepo.findById(PRECONDITIONED_KNOWN_USER_ID)).thenReturn(Optional.of(new User(PRECONDITIONED_KNOWN_USER_ID, false, "", "", PRECONDITIONED_KNOWN_ORG_ID)));


👍 🎉💯🔥🎉

noce2 · 2020-12-20T16:05:38Z

integration-tests/features/User.feature

+  Background: Create pre-configured user
+   Given I generate a json payload called 'userWithKnownId'
+   And I make a POST to the function at '/organisations/validOrg/users'	


noce2

LGTM. There's a discussion point I've put on, for which I've raised this follow on issue: #48

noce2 · 2020-12-20T16:07:36Z

integration-tests/features/support/payloads.js

-    }, 
+    },
+    userWithKnownId: {
+    	id: "knownUserId",


I am surprised that we are not having issues here. This id parameter should be unique so it really shouldn't let us register lots of users with the same "id". Correct me if I'm wrong but I thought we specified this in the configuration with mongo somewhere?

On reflection, we couldn't have noticed this earlier because we were using pre-conditioned users for the tests. Now, we probably need to add a unit test about no duplicate users and modify the tests so that they don't keep registering the same user. Happy to do that in a separate PR.

The implication/likely scenario for this is that if a user does try to signup on Auth0 (maybe the forgot whether they've signed up with a certain account before), we'll end up creating a new profile etc. What they should get is either some message saying they've registered before or actually just get back their existing profile.

By creating our own id field we are implicitly taking on the burden of enforcing uniqueness. I assumed this was always planned. Mongo has a different id field that differentiates objects. I'm happy to do the other PR as well but it's obviously outside the scope here

noce2 · 2020-12-20T16:30:46Z

src/test/java/io/mathdojo/useraccountservice/services/IdentityServiceTest.java

@@ -444,7 +444,7 @@ public void throwsErrorIfAttemptToUpdateValidUserWithNullParams() {

    @Test
    public void throwsNoErrorIfDeletingForValidUserInValidOrgId() {
-        organisationService.deleteUserFromOrg("knownOrganisationId", "knownUserId");
+        organisationService.deleteUserFromOrg("aKnownOrg", "aKnownUser");


Probably worth using the constants you created above here.

noce2

Just one question about the timeout as I don't think we should use that at the stage you've put it.

noce2 · 2020-12-22T18:39:15Z

integration-tests/features/step_definitions/request_steps.js

-When(/I make a (\w+) to the function at \'(.*)\'/, function (httpMethod, path) {
+
+When(/I make a (\w+) to the function at \'(.*)\'/,{
+    timeout: 100


Why do we have a timeout here?

aakano684

Just one question about the timeout as I don't think we should use that at the stage you've put it.

The problem is the rate limit you set on the api gateway does not allow for the background step to work. Obviously there are many ways to solve this. It's also useful if you say why you don't think it should be there as in what harm is done by having a timeout there.

aakano684 · 2020-12-22T15:55:07Z

integration-tests/features/support/payloads.js

-    }, 
+    },
+    userWithKnownId: {
+    	id: "knownUserId",


By creating our own id field we are implicitly taking on the burden of enforcing uniqueness. I assumed this was always planned. Mongo has a different id field that differentiates objects. I'm happy to do the other PR as well but it's obviously outside the scope here

noce2 · 2020-12-22T19:53:16Z

In terms of why a timeout in the request stage isn't the right approach:

The timeout essentially says, "wait until this window has passed before failing the test if the promise hasn't resolved." Here are the docs if you're curious.

Currently in the tests we have timeouts in 2 places. The first is on the startup of the app, in hooks.js, that one has to be there because it's a long running task and we want to "bail out" if for any reason it takes too long to do start.

The second timeout is for the step where we assert the status codes of responses. That one is necessary because the Azure function when deployed has a cold start period (the container in which our code runs has been de-provisioned and so Azure needs to "wake up the app").

Given you're trying to throttle requests, the timeout won't do what you're hoping. It's easy to modify the rate limit, just log onto the API gateway and see what it is for the API in question.

aakano684 · 2020-12-22T20:13:13Z

In terms of why a timeout in the request stage isn't the right approach:

The timeout essentially says, "wait until this window has passed before failing the test if the promise hasn't resolved." Here are the docs if you're curious.

Currently in the tests we have timeouts in 2 places. The first is on the startup of the app, in hooks.js, that one has to be there because it's a long running task and we want to "bail out" if for any reason it takes too long to do start.

The second timeout is for the step where we assert the status codes of responses. That one is necessary because the Azure function when deployed has a cold start period (the container in which our code runs has been de-provisioned and so Azure needs to "wake up the app").

Given you're trying to throttle requests, the timeout won't do what you're hoping. It's easy to modify the rate limit, just log onto the API gateway and see what it is for the API in question.

Sorry I still don't see what long or short term harm it does. The linked docs say nothing about what you mentioned. And it seemed to be a convenient way to add a short wait before we send http requests. Your proposed solution of changing the rate on the gateway doesn't seem to scale

noce2

:+1

Enable Retrieval of Existing Users based on userid

3f146a6

aakano684 requested a review from noce2 November 26, 2020 05:11

Ade added 2 commits November 26, 2020 05:21

update schema

3509d3e

revert schema update

7d70834

noce2cicd linked an issue Nov 28, 2020 that may be closed by this pull request

Enable Retrieval of Existing Users based on userid #27

Open

1 task

noce2 reviewed Nov 29, 2020

View reviewed changes

remove preconfigured users and update tests

e759aae

aakano684 requested a review from noce2 December 13, 2020 19:13

test update

245ad0e

noce2 reviewed Dec 16, 2020

View reviewed changes

Ade added 2 commits December 17, 2020 23:16

test update

92eb73d

add backcround step for pre-configured user

5be7a92

noce2 reviewed Dec 20, 2020

View reviewed changes

noce2 mentioned this pull request Dec 20, 2020

Prevent Duplication Registration of Users #48

Open

2 tasks

noce2 previously approved these changes Dec 20, 2020

View reviewed changes

add timeout to integration tests before http request

90445c8

aakano684 dismissed noce2’s stale review via 90445c8 December 22, 2020 14:15

Ade and others added 3 commits December 22, 2020 14:19

fix timeout to integration tests

33739e7

Merge branch 'develop' into ft27

321ca28

fix merge conflicts with develop

cc8727d

aakano684 requested a review from noce2 December 22, 2020 18:32

noce2 reviewed Dec 22, 2020

View reviewed changes

aakano684 commented Dec 22, 2020

View reviewed changes

remove timeout

a3fe66d

noce2 approved these changes Dec 22, 2020

View reviewed changes

aakano684 merged commit c8a99bd into develop Dec 22, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Enable Retrieval of Existing Users based on userid #44

Enable Retrieval of Existing Users based on userid #44

aakano684 commented Nov 26, 2020

noce2 left a comment

noce2 Nov 29, 2020

aakano684 Nov 29, 2020

noce2 Nov 29, 2020

noce2 Nov 29, 2020

noce2 left a comment

noce2 Dec 16, 2020

noce2 Dec 16, 2020

aakano684 Dec 18, 2020

noce2 Dec 16, 2020

aakano684 Dec 18, 2020

noce2 Dec 19, 2020

noce2 Dec 16, 2020

aakano684 Dec 18, 2020

noce2 Dec 16, 2020

noce2 Dec 16, 2020

aakano684 Dec 18, 2020

noce2 Dec 16, 2020

noce2 Dec 20, 2020

noce2 left a comment •

edited

Loading

noce2 Dec 20, 2020

noce2 Dec 20, 2020

noce2 Dec 20, 2020

aakano684 Dec 22, 2020

noce2 Dec 20, 2020

noce2 left a comment

noce2 Dec 22, 2020

aakano684 left a comment •

edited

Loading

aakano684 Dec 22, 2020

noce2 commented Dec 22, 2020

aakano684 commented Dec 22, 2020

noce2 left a comment

		User retrievedUser = getUserInOrg(orgId, userId);
		userRepo.delete(retrievedUser);

		import com.microsoft.azure.functions.ExecutionContext;
		import com.microsoft.azure.functions.HttpRequestMessage;

Enable Retrieval of Existing Users based on userid #44

Enable Retrieval of Existing Users based on userid #44

Conversation

aakano684 commented Nov 26, 2020

Purpose

Does this introduce a breaking change?

Pull Request Type

How to Test

What to Check

Other Information

noce2 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

noce2 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

noce2 left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

noce2 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

aakano684 left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

noce2 commented Dec 22, 2020

aakano684 commented Dec 22, 2020

noce2 left a comment

Choose a reason for hiding this comment

noce2 left a comment •

edited

Loading

aakano684 left a comment •

edited

Loading