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 support for Article.content_tag_ids field - resolves #557 #558

Conversation

andy-may-at
Copy link
Contributor

N.B. a potential risk of adding a new field to the model seems to be that any existing code using the client that was doing a overwrite of an existing article (by calling Zendesk.updateArticle(article) (without having recently read the article in question from the API, or where the Article instance has been created by copying a known set of fields from another Article instance) would have been preserving any pre-existing content_tags_id value on the article.
But if we add the field to the model, then any code like this will now be updating the content_tags_id field to be empty
This edge-case change in behaviour seems worthwhile, but may be worth documenting in release notes.

@andy-may-at
Copy link
Contributor Author

I would have been tempted to add a test for this in the RealSmokeTest, but I don't know what data's in that instance, so can't do so

@andy-may-at
Copy link
Contributor Author

D'Oh!
I've only just realised that the client doesn't support fetching of the Content Tag API resource
https://developer.zendesk.com/api-reference/help_center/help-center-api/content_tags/

I'd be happy to implement that in a PR as well.
Would you prefer me to roll the two together into a single issue/PR or to keep them separate?

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.

Would you prefer me to roll the two together into a single issue/PR or to keep them separate?

On one hand both changes are orthogonal so two PRs make sense, but on the other hand since your PR is the first using the content tag API, it also make sense grouping them. So I guess up to you, use the solution that requires the less work for you :)

This edge-case change in behaviour seems worthwhile, but may be worth documenting in release notes.

Agreed, let's not over-engineer this, I'll mark this PR as breaking to remember about it in the release notes.

@PierreBtz PierreBtz added enhancement breaking Breaking change labels Mar 9, 2023
@andy-may-at
Copy link
Contributor Author

On one hand both changes are orthogonal so two PRs make sense, but on the other hand since your PR is the first using the content tag API, it also make sense grouping them. So I guess up to you, use the solution that requires the less work for you

Theres some value in this change on it's own, and it's marginally simpler for me if we just merge this & I just rebase my draft PR for the rest of the content tag stuff, so I vote for that :)

@PierreBtz PierreBtz merged commit 366da49 into cloudbees-oss:master Mar 13, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants