Speedygeek / ZendeskApi_v2

C# wrapper for the Zendesk API
Other
170 stars 200 forks source link

Macro.Action.Value can also be an array #123

Closed skreisoft closed 9 years ago

skreisoft commented 9 years ago

Whenever I try and 'Get' a Macro that has an 'action' of type 'comment_value' its sending back an array with first item 'channel:all' and second item in the array is the text content, but the Macro.Action.Value property is of type string and the json deserialization throws an exception when encountering a type of array.

https://developer.zendesk.com/rest_api/docs/core/macros

mozts2005 commented 9 years ago

This looks like a bug to me. If you can send a unit test that will help us get to this faster.

abhiranjankumar00 commented 9 years ago

Hi, is there any progress on this?

mozts2005 commented 9 years ago

looks like we are waiting on a test case / sample code to repro the issue with.

abhiranjankumar00 commented 9 years ago

Get all macros lead to this error for me.

ZendeskApi api = new ZendeskApi_v2.ZendeskApi(zendeskUrl, username, password);
var macros = api.Macros.GetAllMacros();

call leads to the above error.

Error message is

An unhandled exception of type 'Newtonsoft.Json.JsonReaderException' occurred in Newtonsoft.Json.dll

Additional information: Error reading string. Unexpected token: StartArray. Path 'macros[2].actions[0].value', line 1, position 2587.


This can be the possible bug.

skreisoft commented 9 years ago

Steps to reproduce:

1) Create Macro via Zendesk Web UI and add action of type "Ticket: Comment/Description". 2) Retrieve macro by using c# client method "GetMacroById"

Workaround: Thanks for contacting Zendesk support. I'm Chris from our Tier 2 support team and I'll be assisting you further.

You may update the action value with a string via API. This will replace the existing array with the string value specified in your PUT request.

However if the macro is updated from the agent interface, the action value will be updated to an array containing the channel attribute and the comment text.

mozts2005 commented 9 years ago

I think we need to change the Action to have a

 IList<string> Value { get; set; }

Then use a custom converter for JSON.net to handle the case where the value is a single item.

I have run some test and found that I am able to make this work.

My only issue with adding it at this time is that this would be a breaking change.

skreisoft commented 9 years ago

Totally agree, but would be rather simple to fix by adding .First() or [0] to existing code.

abhiranjankumar00 commented 9 years ago

My only issue with adding it at this time is that this would be a breaking change.

Yes, it will. If there's a way to know how many projects are using this call, we would be able to act on it.

It appears to me (might be wrong) that a good percentage of macros are inaccessible with the current code (will lead to runtime error on existing projects).

Introducing the above change will help people to fix the issue at compile time, which is better than fixing it with try-catch after getting their hands burnt.

@skreisoft can be able provide correct numbers.

abhiranjankumar00 commented 9 years ago

Hi, is there any progress/plan of action here?

mozts2005 commented 9 years ago

I guess the only thing left is to decide when to accept pull request #159 and what version number to release as? What is your opinion?

abhiranjankumar00 commented 9 years ago

Great!

Sorry, I haven't followed this project (just started on .net) closely until recently, so I can't comment on the release :(