Skip to content

Conversation

@rcprcp
Copy link

@rcprcp rcprcp commented Jun 22, 2022

Provide basic functionality for Views. Support getting a list of the
Views. Then, using the View id, we can get a list of the Tickets
in the View.

Copy link
Contributor

@aheritier aheritier left a comment

Choose a reason for hiding this comment

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

Hi @rcprcp

Thanks a lot for your contribution.
I think it's a good addition and I do not see any issue with the proposed code (just few minor comments).
The only missing part is to add some integration tests to avoid regressions but I know it's hard to write them without accessing to our sandbox (no thank you to Zendesk for not providing a shared environment for our community )
I you can review the 2 comments I pushed, I will try to write some tests on top of your PR ASAP

Cheers

pom.xml Outdated
<groupId>com.cloudbees.thirdparty</groupId>
<artifactId>zendesk-java-client</artifactId>
<version>0.17.1-SNAPSHOT</version>
<version>0.17.2-SNAPSHOT</version>
Copy link
Contributor

Choose a reason for hiding this comment

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

Not required in the PR

Copy link
Author

Choose a reason for hiding this comment

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

replaced old value.

@JsonIgnoreProperties(ignoreUnknown = true)
public class View implements Serializable {

private static final long serialVersionUID = 1L;
Copy link
Contributor

Choose a reason for hiding this comment

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

It's probably better to generate a random number. (IDEs are generally proposing it)

Copy link
Author

Choose a reason for hiding this comment

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

fixed.

bob added 2 commits June 23, 2022 18:16
Provide basic functionality for views. Support getting a list of the
Views. Then, using the view id, we can get a list of the Tickets
in the View.
Provide basic functionality for views. Support getting a list of the
Views. Then, using the view id, we can get a list of the Tickets
in the View.
@rcprcp
Copy link
Author

rcprcp commented Jun 23, 2022

Hello @aheritier - I have had a number of "git mishaps" with this change, but the most recent force-push should provide the changes you requested. Additionally, I removed the "rawTitle" field from the View class. In my testing, the field was always empty.

Also, I have a simple test program for this PR - please review:
https://github.com/rcprcp/ZendeskViewTest

Thanks!

@aheritier
Copy link
Contributor

Perfect @rcprcp
I will try to integrate it next week

@rcprcp
Copy link
Author

rcprcp commented Jun 30, 2022

Hi @aheritier - wondering if you need anything from me to help move this along?
Thank you.

@aheritier
Copy link
Contributor

just lack of time @rcprcp
@PierreBtz could you help to write some ITs. They should be very simple to do as far as I can see

@PierreBtz
Copy link
Collaborator

@rcprcp I took the liberty of pushing integration tests on your branch.

@PierreBtz PierreBtz merged commit 90875d4 into cloudbees-oss:master Jul 1, 2022
@rcprcp
Copy link
Author

rcprcp commented Jul 1, 2022

@aheritier , @PierreBtz - zendesk-java-client is an awesome library. I appreciate all the time and effort you've invested in creating and maintaining it. And thank you very much for fixing this PR and merging it. I am glad that I could help. :)

@rcprcp rcprcp deleted the view-support branch July 1, 2022 15:34
@PierreBtz
Copy link
Collaborator

@rcprcp thanks! Release 0.18.0 should be available shortly, I had a problem of expired gpg key, now I'm waiting for the gpg key servers to be synchronized and I hope to be able to complete the release process within the next hours...

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.

3 participants