Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Add single and multiple customer lookup and support customer search API #17

Merged
merged 8 commits into from
Sep 25, 2019

Conversation

brownac
Copy link
Contributor

@brownac brownac commented Jun 21, 2019

First pass at implementing #16 .

I followed the admin API spec pretty closely. Happy to add/remove support for certain params as you all see fit. Also happy to expand the support for customerSearch to include additional params outside of query.

Let me know what you all think. Have a good weekend!

Copy link
Member

@ryankazokas ryankazokas left a comment

Choose a reason for hiding this comment

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

@brownac well done! Much appreciated PR. Let's discuss some of these changes and then we can move towards merging.

@brownac
Copy link
Contributor Author

brownac commented Jun 24, 2019

Also, making a mental note to myself here to re-add all the class imports and turn off wildcard importing in IntelliJ.

@brownac
Copy link
Contributor Author

brownac commented Jun 25, 2019

@ryankazokas , I made some changes. Let me know what you think.

Copy link
Member

@ryankazokas ryankazokas left a comment

Choose a reason for hiding this comment

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

@brownac The only thing i'd suggest before we merge is do remove the reflection implementation in favor of just adding the query parameters in the getCustomer method if the values in the request object are not blank or null. After that and removing the new bean utils dependency from the POM I will give approval to merge.

Are you manually adding the jar to your project? That might be causing downsstream dependencies not to be loaded into your project. We are using this as well and haven't seen this problem. Could it be possible that the new dependency you added is conflicting with something else? Since we will remove the reflection implementation we won't need that anymore. Try it again after that. I'll load up your branch locally and see if i have the same problem.

ryankazokas
ryankazokas previously approved these changes Jun 27, 2019
@brownac
Copy link
Contributor Author

brownac commented Jun 27, 2019

@ryankazokas seeming to have trouble getting my build to pass due to mutation coverage % being at 85 (threshold is 87). Do you mind building my branch/leaving any suggestions as to how I can raise it?

Edit: actually maybe not?

@brownac
Copy link
Contributor Author

brownac commented Jul 1, 2019

Are you manually adding the jar to your project? That might be causing downsstream dependencies not to be loaded into your project. We are using this as well and haven't seen this problem. Could it be possible that the new dependency you added is conflicting with something else? Since we will remove the reflection implementation we won't need that anymore. Try it again after that. I'll load up your branch locally and see if i have the same problem.

@ryankazokas were you ever able to replicate this?

@ryankazokas ryankazokas dismissed their stale review July 10, 2019 12:24

Change destination branch to develop

@ryankazokas
Copy link
Member

@brownac can you change the destination branch to develop?

@ryankazokas
Copy link
Member

Are you manually adding the jar to your project? That might be causing downsstream dependencies not to be loaded into your project. We are using this as well and haven't seen this problem. Could it be possible that the new dependency you added is conflicting with something else? Since we will remove the reflection implementation we won't need that anymore. Try it again after that. I'll load up your branch locally and see if i have the same problem.

@ryankazokas were you ever able to replicate this?

I was not able to replicate this @brownac .

@brownac brownac changed the base branch from master to develop July 11, 2019 17:37
@brownac
Copy link
Contributor Author

brownac commented Jul 11, 2019

@ryankazokas I changed the base branch. Also, I figured out the issue. Turns out my project was using a mismatched version of jersey-media-json-jackson.

@rjdavis3 rjdavis3 self-assigned this Jul 11, 2019
@rjdavis3 rjdavis3 added the enhancement New feature or request label Jul 11, 2019
@rjdavis3 rjdavis3 merged commit b3fee4a into ChannelApe:develop Sep 25, 2019
# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants