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

Add type attribute to Comment model #415

Merged
merged 3 commits into from
Nov 19, 2021

Conversation

besbes
Copy link
Contributor

@besbes besbes commented Nov 8, 2021

This adds the type attribute to the Comment model.

aheritier
aheritier previously approved these changes Nov 9, 2021
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.

LGTM
I have to check if we can add an integration test

@besbes
Copy link
Contributor Author

besbes commented Nov 9, 2021

@aheritier Thanks for the quick feedback. Let me know if I can contribute to the testing part — I didn't find an existing match so I wasn't sure how much coverage you want to have here.

@aheritier
Copy link
Contributor

when possible @besbes I try to verify that we can serialize/deserialize the objects but yes I am not sure we have some tests in ITs for Comments
Our ITs are failing, I also have to fix them. For some reason we now get a too many requests error

@besbes
Copy link
Contributor Author

besbes commented Nov 10, 2021

@aheritier Let me know how I can support here. I couldn't find instructions how to run integration tests, what's the best way to check that?

@aheritier
Copy link
Contributor

Hi @besbes

Did you test the code with a real zendesk instance?

I get an error

Caused by: java.lang.IllegalArgumentException: Cannot deserialize value of type `org.zendesk.client.v2.model.CommentType` from String "Comment": not one of the values accepted for Enum class: [VOICE_COMMENT, COMMENT]
 at [Source: UNKNOWN; byte offset: #UNKNOWN] (through reference chain: org.zendesk.client.v2.model.Comment["type"])

I updated your code and modified some tests to validate it.

could you have a look at it please ?

@aheritier aheritier self-assigned this Nov 12, 2021
@besbes
Copy link
Contributor Author

besbes commented Nov 13, 2021

@aheritier Yes I did (although not within this project because I didn't run integration tests). According to the JsonProperty javadoc this should work (and I'm using it in other projects too), so I'm not sure why it didn't for you? Happy to look into it — please just let me know how you run integration tests so I can try to reproduce.

@aheritier
Copy link
Contributor

@besbes I just added more details in #419
Does it help ?

@besbes
Copy link
Contributor Author

besbes commented Nov 13, 2021

@aheritier Yes, thank you. I found the issue — it looks like @JsonProperty on enum instances is not compatible with DeserializationFeature.READ_ENUMS_USING_TO_STRING (which we are using here). Therefore your change (using toString() to return the value for serialization makes sense. Thanks for implementing the integration test! Nothing to add from my side.

@aheritier
Copy link
Contributor

Hi @besbes

Ok it makes sense. Not sure why it was done like this (this project is old).

I'll wait for a review from @PierreBtz , @Dohbedoh or @duemir before merging it and publishing a release

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.

Something looks odd, the Comment object is only describing the COMMENT type, the VOICE_COMMENT type does not use the same model as per https://developer.zendesk.com/documentation/ticketing/managing-tickets/adding-voice-comments-to-tickets/.

As far as I understand the documentation, a VOICE_COMMENT would not properly deserialize with this code.

@aheritier
Copy link
Contributor

@PierreBtz The specific JSON is only to create a comment of type Voice AFAIU. To read it it is the same, even if I am not sure how you get the URL and specific data for a voice comment

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.

Ok, if we agree that this code can only work in "read only" mode then lgtm.

Copy link
Collaborator

@Dohbedoh Dohbedoh left a comment

Choose a reason for hiding this comment

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

Right. Though there is a value for VOICE_COMMENT, the current model does not allow to use it yet. But seems fine by me for now.

@PierreBtz PierreBtz merged commit ae0d968 into cloudbees-oss:master Nov 19, 2021
@besbes besbes deleted the feature/add-comment-type branch November 20, 2021 21:07
@besbes besbes mentioned this pull request Jun 3, 2024
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.

4 participants