-
Notifications
You must be signed in to change notification settings - Fork 252
Add basic View support - List the Views; List the tickets in a View #491
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
Conversation
There was a problem hiding this 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
<version>0.17.1-SNAPSHOT</version> | ||
<version>0.17.2-SNAPSHOT</version> |
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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; |
There was a problem hiding this comment.
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)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
fixed.
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.
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: Thanks! |
Perfect @rcprcp |
Hi @aheritier - wondering if you need anything from me to help move this along? |
just lack of time @rcprcp |
@rcprcp I took the liberty of pushing integration tests on your branch. |
@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 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... |
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.