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.

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

Support List Locales operation #619

Conversation

topero
Copy link
Contributor

@topero topero commented Sep 12, 2023

Hello,

We needed to support the List Locales operation and this is the change that we implemented.

We tried to follow the contributing guidelines, but feel free to comment if we missed something, please.

Copy link
Collaborator

@PierreBtz PierreBtz left a comment

Choose a reason for hiding this comment

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

@topero thanks for the addition. Change lgtm, I'd like to have a test to cover any change that might happen in the API in the future.

I know it's a pain to write a test for this library since we cannot grant access to our internal ZD testing instance to external contributors, so I can take care of the test if needed.

@topero
Copy link
Contributor Author

topero commented Sep 22, 2023

Sorry for the late reply, @PierreBtz.
A couple of unit tests have been added to the pull request.

In addition, a few more attributes have been added to the "Locale" class because they were returned by our Zendesk instance although I haven't found them in Zendesk documentation.

@PierreBtz
Copy link
Collaborator

@topero sorry I had a rather busy week, I'll look at the PR beginning of next week in more details but at the first glance it looks good.
My only reservation is that you are adding undocumented fields, I usually tend to open a ticket with Zendesk support to have them confirm they really exist and update their documentation before accepting such change.
If you need the fields, it's allright, we can do it but it will delay the merge of the PR. If you don't need them, I'd suggest to not put them, this will fast-track this.

Copy link
Collaborator

@PierreBtz PierreBtz left a comment

Choose a reason for hiding this comment

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

lgtm once we sort out the fields that are not documented by Zendesk.

src/main/java/org/zendesk/client/v2/model/Locale.java Outdated Show resolved Hide resolved
src/main/java/org/zendesk/client/v2/model/Locale.java Outdated Show resolved Hide resolved
src/main/java/org/zendesk/client/v2/model/Locale.java Outdated Show resolved Hide resolved
src/main/java/org/zendesk/client/v2/model/Locale.java Outdated Show resolved Hide resolved
@topero
Copy link
Contributor Author

topero commented Oct 26, 2023

I'm sorry for the very very long delay to answer you, @PierreBtz .

On 2 October, a Zendesk Technical Account Manager was reached out with regard to this topic. In my unicorn-and-rainbows world, the documentation is updated and this PR might be kept as it was. But the reality is that I've seen no answer so far. So the undocumented fields have been removed and the tests, adjusted.

Let me know if something else needs to be modified, please.

Copy link
Collaborator

@PierreBtz PierreBtz left a comment

Choose a reason for hiding this comment

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

Great! Thanks for the contribution!

@PierreBtz PierreBtz merged commit 5466a40 into cloudbees-oss:master Nov 6, 2023
3 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants