added create update many users method #325
Closed
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
ajarnold87
requested review from
aheritier,
duemir,
johnou and
stephenc
as code owners
stephenc
approved these changes
Jan 24, 2020
Contributor
stephenc
left a comment
stephenc
left a comment
There was a problem hiding this comment.
@aheritier can you remove me from the code owners as I’m not actively using the Zendesk client
aheritier
suggested changes
Jan 24, 2020
Contributor
aheritier
left a comment
aheritier
left a comment
There was a problem hiding this comment.
Very good work @ajarnold87
You have one bug in the test and you might add a vararg method to ease the usage but otherwise it's good
| @@ -1,5 +1,31 @@ | |||
| package org.zendesk.client.v2; | |||
|
|
|||
| import static org.hamcrest.CoreMatchers.anyOf; | |||
Contributor
There was a problem hiding this comment.
@ajarnold87 it might be better to not reorganise the imports. You can see if your IDE is correctly configured here:
| } | ||
| } | ||
|
|
||
| @Test(timeout = 10000) |
Contributor
There was a problem hiding this comment.
Thanks a lot to add some test, it's helping a lot @ajarnold87
| user2.setEmail("example+user2$example.com"); | ||
| user2.setName("Norris Chuck"); | ||
|
|
||
| JobStatus<User> result = instance.createUsers(user1, user2); |
Contributor
There was a problem hiding this comment.
you are testing the createUsers method but you added createOrUpdateUsers
Contributor
There was a problem hiding this comment.
not your fault but there is no test for createUsers 😢
| return complete(createUsersAsync(users)); | ||
| } | ||
|
|
||
| public JobStatus<User> createOrUpdateUsers(List<User> users) { |
Contributor
There was a problem hiding this comment.
It might be useful to add varargs version like for createUsers:
public JobStatus<User> createOrUpdateUsers(User... users) {
return createOrUpdateUsers(Arrays.asList(users));
}
| user1.setName("Chuck Norris"); | ||
|
|
||
| User user2 = new User(); | ||
| user2.setEmail("example+user2$example.com"); |
Contributor
There was a problem hiding this comment.
The email is not valid and thus the test is failing. My understanding is that the email isn't valid thus the account is not created.
But we still receive 2 users in the result (well done zendesk !). The user we failed to create is returned as an empty user with id = 1
Then when we cleanup/delete the user we created in the test we are failing with
createOrUpdateUsers(org.zendesk.client.v2.RealSmokeTest) Time elapsed: 1.836 sec <<< ERROR!
org.zendesk.client.v2.ZendeskResponseException: HTTP/404: Not Found - {"error":"RecordNotFound","description":"Not found"}
at org.zendesk.client.v2.Zendesk.complete(Zendesk.java:2567)
at org.zendesk.client.v2.Zendesk.deleteUser(Zendesk.java:835)
at org.zendesk.client.v2.Zendesk.deleteUser(Zendesk.java:831)
at org.zendesk.client.v2.RealSmokeTest.createOrUpdateUsers(RealSmokeTest.java:808)
Caused by: org.zendesk.client.v2.ZendeskResponseException: HTTP/404: Not Found - {"error":"RecordNotFound","description":"Not found"}
at org.zendesk.client.v2.Zendesk$1.onCompleted(Zendesk.java:2262)
at org.zendesk.client.v2.Zendesk$1.onCompleted(Zendesk.java:2253)
at org.asynchttpclient.AsyncCompletionHandler.onCompleted(AsyncCompletionHandler.java:66)
at org.asynchttpclient.netty.NettyResponseFuture.loadContent(NettyResponseFuture.java:222)
at org.asynchttpclient.netty.NettyResponseFuture.done(NettyResponseFuture.java:257)
at org.asynchttpclient.netty.handler.AsyncHttpClientHandler.finishUpdate(AsyncHttpClientHandler.java:241)
at org.asynchttpclient.netty.handler.HttpHandler.handleChunk(HttpHandler.java:114)
at org.asynchttpclient.netty.handler.HttpHandler.handleRead(HttpHandler.java:143)
at org.asynchttpclient.netty.handler.AsyncHttpClientHandler.channelRead(AsyncHttpClientHandler.java:78)
at io.netty.channel.AbstractChannelHandlerContext.invokeChannelRead(AbstractChannelHandlerContext.java:374)
at io.netty.channel.AbstractChannelHandlerContext.invokeChannelRead(AbstractChannelHandlerContext.java:360)
at io.netty.channel.AbstractChannelHandlerContext.fireChannelRead(AbstractChannelHandlerContext.java:352)
at io.netty.handler.codec.MessageToMessageDecoder.channelRead(MessageToMessageDecoder.java:102)
at io.netty.channel.AbstractChannelHandlerContext.invokeChannelRead(AbstractChannelHandlerContext.java:374)
at io.netty.channel.AbstractChannelHandlerContext.invokeChannelRead(AbstractChannelHandlerContext.java:360)
at io.netty.channel.AbstractChannelHandlerContext.fireChannelRead(AbstractChannelHandlerContext.java:352)
at io.netty.channel.CombinedChannelDuplexHandler$DelegatingChannelHandlerContext.fireChannelRead(CombinedChannelDuplexHandler.java:438)
at io.netty.handler.codec.ByteToMessageDecoder.fireChannelRead(ByteToMessageDecoder.java:328)
at io.netty.handler.codec.ByteToMessageDecoder.channelRead(ByteToMessageDecoder.java:302)
at io.netty.channel.CombinedChannelDuplexHandler.channelRead(CombinedChannelDuplexHandler.java:253)
at io.netty.channel.AbstractChannelHandlerContext.invokeChannelRead(AbstractChannelHandlerContext.java:374)
at io.netty.channel.AbstractChannelHandlerContext.invokeChannelRead(AbstractChannelHandlerContext.java:360)
at io.netty.channel.AbstractChannelHandlerContext.fireChannelRead(AbstractChannelHandlerContext.java:352)
at io.netty.handler.ssl.SslHandler.unwrap(SslHandler.java:1475)
at io.netty.handler.ssl.SslHandler.decodeJdkCompatible(SslHandler.java:1224)
at io.netty.handler.ssl.SslHandler.decode(SslHandler.java:1271)
at io.netty.handler.codec.ByteToMessageDecoder.decodeRemovalReentryProtection(ByteToMessageDecoder.java:505)
at io.netty.handler.codec.ByteToMessageDecoder.callDecode(ByteToMessageDecoder.java:444)
at io.netty.handler.codec.ByteToMessageDecoder.channelRead(ByteToMessageDecoder.java:283)
at io.netty.channel.AbstractChannelHandlerContext.invokeChannelRead(AbstractChannelHandlerContext.java:374)
at io.netty.channel.AbstractChannelHandlerContext.invokeChannelRead(AbstractChannelHandlerContext.java:360)
at io.netty.channel.AbstractChannelHandlerContext.fireChannelRead(AbstractChannelHandlerContext.java:352)
at io.netty.channel.DefaultChannelPipeline$HeadContext.channelRead(DefaultChannelPipeline.java:1422)
at io.netty.channel.AbstractChannelHandlerContext.invokeChannelRead(AbstractChannelHandlerContext.java:374)
at io.netty.channel.AbstractChannelHandlerContext.invokeChannelRead(AbstractChannelHandlerContext.java:360)
at io.netty.channel.DefaultChannelPipeline.fireChannelRead(DefaultChannelPipeline.java:931)
at io.netty.channel.nio.AbstractNioByteChannel$NioByteUnsafe.read(AbstractNioByteChannel.java:163)
at io.netty.channel.nio.NioEventLoop.processSelectedKey(NioEventLoop.java:700)
at io.netty.channel.nio.NioEventLoop.processSelectedKeysOptimized(NioEventLoop.java:635)
at io.netty.channel.nio.NioEventLoop.processSelectedKeys(NioEventLoop.java:552)
at io.netty.channel.nio.NioEventLoop.run(NioEventLoop.java:514)
at io.netty.util.concurrent.SingleThreadEventExecutor$6.run(SingleThreadEventExecutor.java:1044)
at io.netty.util.internal.ThreadExecutorMap$2.run(ThreadExecutorMap.java:74)
at io.netty.util.concurrent.FastThreadLocalRunnable.run(FastThreadLocalRunnable.java:30)
at java.lang.Thread.run(Thread.java:748)
RecordNotFound because there is no user with ID= 1 to delete
Contributor
|
Hi @ajarnold87 Do my comments are helping you ? |
Contributor
|
Should be fixed with #345 |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Implemented #324 Please let me know your thoughts or if any recommended changes need to be made.