1064 get user by login by cipous · Pull Request #1065 · gooddata/gooddata-java

cipous · 2021-08-26T14:25:16Z

fixes 1064
Its work in progress, please comment what tests are missing, not sure how to handle response in case of array of items.

ghost · 2021-08-26T14:25:21Z

Build succeeded (check pipeline).

cipous · 2021-08-26T14:27:33Z

fixes #1065

codecov-commenter · 2021-08-26T14:30:20Z

Codecov Report

Merging #1065 (9026b52) into master (2aeea4a) will decrease coverage by 0.00%.
The diff coverage is 82.35%.

❗ Current head 9026b52 differs from pull request most recent head 6a55e1c. Consider uploading reports for the commit 6a55e1c to get more accurate results

@@             Coverage Diff              @@
##             master    #1065      +/-   ##
============================================
- Coverage     83.12%   83.11%   -0.01%     
- Complexity     2597     2603       +6     
============================================
  Files           343      345       +2     
  Lines          6641     6658      +17     
  Branches        477      478       +1     
============================================
+ Hits           5520     5534      +14     
- Misses          925      927       +2     
- Partials        196      197       +1

Impacted Files	Coverage Δ
...n/java/com/gooddata/sdk/model/account/Account.java	`78.84% <ø> (ø)`
...m/gooddata/sdk/service/account/AccountService.java	`77.61% <66.66%> (-1.70%)`	⬇️
.../java/com/gooddata/sdk/model/account/Accounts.java	`100.00% <100.00%> (ø)`
...oddata/sdk/model/account/AccountsDeserializer.java	`100.00% <100.00%> (ø)`
.../sdk/service/account/AccountNotFoundException.java	`85.71% <100.00%> (+10.71%)`	⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 2aeea4a...6a55e1c. Read the comment docs.

ghost · 2021-08-27T06:41:41Z

Build succeeded (check pipeline).

ghost · 2021-08-27T07:09:33Z

Build succeeded (check pipeline).

ghost · 2021-08-27T07:12:37Z

Build succeeded (check pipeline).

cipous · 2021-08-27T07:13:32Z

@liry not sure about AT, should I write something similar what is in IT?

ghost · 2021-08-27T07:15:32Z

Build succeeded (check pipeline).

ghost · 2021-08-27T07:16:21Z

Build succeeded (check pipeline).

ghost · 2021-08-27T07:36:28Z

Build succeeded (check pipeline).

cipous · 2021-08-27T12:01:54Z

@liry please any feedback? Thanks.

liry

@liry not sure about AT, should I write something similar what is in IT?

@cipous I'm sorry for the delay, I somehow missed this comment.

In ATs, we usually verify the happy path of the service against real backend - i.e. I would add there similar test case to AccountServiceAT.getAccount. (We want to verify we haven't done a mistake in API mocks in IT's.)

We will also try to explain various test types better in the contributing guide: #1068 ;)

liry · 2021-08-27T07:43:02Z

+        notNull(email, "email");
+        notNull(organizationName, "organizationName");
+        try {
+            return restTemplate.getForObject(Account.ACCOUNT_BY_EMAIL_URI, Accounts.class, organizationName, email);


Here, we're trying to get exactly one account by specific email, right? So I would add check that exactly one account is in the response and then return only that one Account. (Throw if no user is returned.)

BTW, this resource is also able to list all domain accounts (w/o login filter), but such a service method can be added later.

Ohh, I was thinking why it returns collection. ok will address your comments soon.

Great, thank you

I did not throw any error on more than one, as login should be unique.

Also could I use this PR to add constructor to Account with all values - like login?

public Account(final String login, final String email, final String password, final String firstName, final String lastName, final List<String> ipWhitelist, final List<String> authenticationModes) { this(login, email, password, password, firstName, lastName, ipWhitelist, authenticationModes, null); }

or should I create separate issue for this.

ALso please when do you expect release of both changes?

@liry please ^ I would like to merge asap so hopefully there is release soon, thanks

And what's the use case of such a constructor? If you're talking about some new service method, I would create separate issuer for that.

We can release new version for you right after this PR is merged ;)

I have created separate PR https://github.com/gooddata/gooddata-java/pull/1071/files and issue.

It would be great, if you could release it together with these two changes. What we are aiming is to create user(get by login if does not exist) for given project, assign him some roles. We call it provisioning, with those 2 PR this should be possible with SDK only...

ghost · 2021-08-31T09:25:44Z

Build succeeded (check pipeline).

liry · 2021-08-31T21:17:44Z

And please squash your commits into one in the end because they are all about implementation of one service method. We will then merge it and release new version.

ghost · 2021-09-01T06:49:54Z

Build succeeded (check pipeline).

Co-authored-by: Libor Ryšavý <liry@users.noreply.github.com>

ghost · 2021-09-01T06:52:35Z

Build succeeded (check pipeline).

cipous · 2021-09-01T07:05:30Z

Thanks for fixing javadoc, I accepted your suggestions. Is there something else? (please with release wait for https://github.com/gooddata/gooddata-java/pull/1071/files). Thank you!

ghost · 2021-09-01T08:24:26Z

Build succeeded (gate pipeline).

https://checklist.intgdc.com/job/compliance/job/gooddata-java-merge-compliance-check/253/ : SUCCESS in 17s
https://checklist.intgdc.com/job/compliance/job/gooddata-java-fossa-scanning-zuul-el7/92/ : SUCCESS in 40s (non-voting)

cipous commented Aug 26, 2021

View reviewed changes

liry linked an issue Aug 27, 2021 that may be closed by this pull request

Support get user by login email #1064

Closed

liry reviewed Aug 27, 2021

View reviewed changes

Comment thread gooddata-java-model/src/main/java/com/gooddata/sdk/model/account/Accounts.java Outdated

liry reviewed Aug 27, 2021

View reviewed changes

Comment thread gooddata-java-model/src/main/java/com/gooddata/sdk/model/account/Accounts.java Outdated

liry reviewed Aug 27, 2021

View reviewed changes

Comment thread gooddata-java-model/src/main/java/com/gooddata/sdk/model/account/Accounts.java Outdated

liry reviewed Aug 27, 2021

View reviewed changes

Comment thread gooddata-java-model/src/main/java/com/gooddata/sdk/model/account/Accounts.java Outdated

liry reviewed Aug 27, 2021

View reviewed changes

Comment thread gooddata-java-model/src/main/java/com/gooddata/sdk/model/account/AccountsSerializer.java Outdated

liry reviewed Aug 27, 2021

View reviewed changes

Comment thread gooddata-java-model/src/main/java/com/gooddata/sdk/model/account/AccountsDeserializer.java Outdated

liry reviewed Aug 27, 2021

View reviewed changes

Comment thread gooddata-java-model/src/main/java/com/gooddata/sdk/model/account/AccountsDeserializer.java

liry reviewed Aug 27, 2021

View reviewed changes

Comment thread gooddata-java-model/src/test/java/com/gooddata/sdk/model/account/AccountsTest.java Outdated

cipous force-pushed the 1064_get_user_by_login branch from 60b54b0 to 1edd31f Compare August 27, 2021 07:15

cipous force-pushed the 1064_get_user_by_login branch from 1edd31f to b987726 Compare August 27, 2021 07:16

cipous force-pushed the 1064_get_user_by_login branch from b987726 to 0bb0bea Compare August 27, 2021 07:36

liry added the enhancement label Aug 31, 2021

liry reviewed Aug 31, 2021

View reviewed changes

Comment thread gooddata-java/src/main/java/com/gooddata/sdk/service/account/AccountService.java Outdated

liry reviewed Aug 31, 2021

View reviewed changes

Comment thread gooddata-java/src/main/java/com/gooddata/sdk/service/account/AccountService.java Outdated

liry reviewed Aug 31, 2021

View reviewed changes

Comment thread gooddata-java/src/main/java/com/gooddata/sdk/service/account/AccountService.java Outdated

liry reviewed Aug 31, 2021

View reviewed changes

Comment thread gooddata-java/src/main/java/com/gooddata/sdk/service/account/AccountService.java

1064 get user by login

6a55e1c

Co-authored-by: Libor Ryšavý <liry@users.noreply.github.com>

cipous force-pushed the 1064_get_user_by_login branch from 9026b52 to 6a55e1c Compare September 1, 2021 06:52

liry added this to the next milestone Sep 1, 2021

liry added the merge label Sep 1, 2021

ghost removed the merge label Sep 1, 2021

ghost merged commit ce310d6 into gooddata:master Sep 1, 2021

This pull request was closed.

Conversation

cipous commented Aug 26, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ghost commented Aug 26, 2021

Uh oh!

cipous commented Aug 26, 2021

Uh oh!

codecov-commenter commented Aug 26, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

ghost commented Aug 27, 2021

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

ghost commented Aug 27, 2021

Uh oh!

ghost commented Aug 27, 2021

Uh oh!

Uh oh!

cipous commented Aug 27, 2021

Uh oh!

Uh oh!

ghost commented Aug 27, 2021

Uh oh!

ghost commented Aug 27, 2021

Uh oh!

ghost commented Aug 27, 2021

Uh oh!

cipous commented Aug 27, 2021

Uh oh!

liry left a comment • 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!

Uh oh!

liry Aug 27, 2021

Choose a reason for hiding this comment

Uh oh!

cipous Aug 31, 2021

Choose a reason for hiding this comment

Uh oh!

liry Aug 31, 2021

Choose a reason for hiding this comment

Uh oh!

cipous Aug 31, 2021

Choose a reason for hiding this comment

Uh oh!

cipous Aug 31, 2021

Choose a reason for hiding this comment

Uh oh!

liry Aug 31, 2021

Choose a reason for hiding this comment

Uh oh!

cipous Sep 1, 2021

Choose a reason for hiding this comment

Uh oh!

ghost commented Aug 31, 2021

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

liry commented Aug 31, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ghost commented Sep 1, 2021

Uh oh!

ghost commented Sep 1, 2021

Uh oh!

cipous commented Sep 1, 2021

cipous commented Aug 26, 2021 •

edited

Loading

codecov-commenter commented Aug 26, 2021 •

edited

Loading

liry left a comment •

edited

Loading

liry commented Aug 31, 2021 •

edited

Loading