Small Improvements on README

[fbvilela]

Description

Just adding a few things to the README about running integration tests on a MAC OS. I'm not a C# developer so the error I was getting was not so obvious, hence the change in test/ZendeskApi.Client.IntegrationTests/Settings/ZendeskSettings.cs

I will be making changes in the future to add more support to Cursor Based Pagination to the resources where it's supported.

[fbvilela]

cc @mikerogers123 . are you still the main dev of this library? I would appreciate your help :)

[mikerogers123]

Hi @fbvilela, hope you're good. I am part of the team who aims to maintain this. Will take a look today when I get in 😀

[mikerogers123]

@fbvilela we use cake to execute tasks like running integration tests - see here. Are you suggesting this change in case some folks do not want to run the integration tests via command line?

Also (and this appears to not be well documented!) but I believe the target branch should be develop and not master. The extra "develop -> master" step is there for us maintainers to control releases at the moment

[fbvilela]

yes @mikerogers123, I saw the command to run integration tests but after inspecting the code I noticed its heavy dependency on Windows. I assume you folks work on Windows? on macOS this was an easy and quick way for me to get things up and running. I guess we could spend some time trying to make the script run in macOS if that's important but the main reason why I'm playing with this library is to add more support to Cursor Based Pagination and make the switch on the resources that do support it.

[fbvilela]

Hi Mike, I'm trying to dig into the cursor based pagination and I have a few questions. firstly, I'm getting a warning message that ZendeskApiClient is obsolete and we should use ZendeskApiClientFactory. I see the README still has the old way of creating the client, how are we supposed to create the client now?

Secondly, I'm trying to get the next page of tickets using the following code.

I got the first page of tickets, but how do I get the next page?

var loggerFactory = new LoggerFactory();
var zendeskOptionsWrapper = new OptionsWrapper<ZendeskOptions>(zendeskOptions);
var client = new ZendeskClient(new ZendeskApiClient(zendeskOptionsWrapper), loggerFactory.CreateLogger<ZendeskClient>());

var pager = new CursorPager { Size = 2 };
var ticketResponseCursor = (TicketsListCursorResponse) await client.Tickets.GetAllAsync(pager);

while (ticketResponseCursor.Meta.HasMore)
{
    // at this point, we have ticketResponseCursor.Tickets with a count of 2 (page size)
    var nextPageUrl = ticketResponseCursor.Links.Next;
    // the lines below doesn't work... but it's somewhat what I would expect
    // ticketResponseCursor = (TicketsListCursorResponse) client.fetch/get(nextPageUrl);
    // ticketReponseCursor.getNext() // would execute the request for the next page
    // ticketResponseCursor.Tickets // expected to be List<Ticket> count 2 - from the second page
}

[mikerogers123]

I assume you folks work on Windows?

Within the organisation there is a mixture of Windows and Mac, but on our team we do use Windows predominantly. My initial thoughts would be that getting the cake script running on Mac OS would be preferable - I should make an issue for that.

I'm getting a warning message that ZendeskApiClient is obsolete and we should use ZendeskApiClientFactory

This change precedes me but if you need to inline the client instantiation, then using ZendeskApiClientFactory instead of ZendeskApiClient directly isn't the cleanest since you will need an instance of IHttpClientFactory to pass it, and that factory needs to have a client registered called "zendeskApiClient".

However, using an IoC container is the easiest way to manage this as the AddZendeskClientWithHttpClientFactory abstracts the creation of the IHttpClientFactory away. Example:

var services = new ServiceCollection();

services.AddZendeskClientWithHttpClientFactory("<url>", "<username>", "<password>"); // replaces services.AddZendeskClient("<url>", "<username>", "<password>");

var serviceProvider = services.BuildServiceProvider();
var client = serviceProvider.GetRequiredService<IZendeskClient>();

If you cannot use an IoC container and want to inline the client instantiation then the best option is what you have written probably. Not too sure why it got deprecated, perhaps to encourage people to use IoC but I will check internally and get back to you.

I got the first page of tickets, but how do I get the next page?

Just seen this issue: https://github.com/justeat/ZendeskApiClient/issues/266. This is not supported based on what I can see. Looks like we are just returning the response as it presents from Zendesk, which isn't helpful. We should look at abstracting the "Next" operation away into the TicketsListCursorResponse. We just need to get around to fixing that issue. PRs welcome of course. I will add it to our backlog for prioritisation but that this point cannot make any promises on when it will be addressed

[fbvilela]

@mikerogers123 @slang25 , I have made a small change in regards to cursor base pagination. We can now set a before/after cursor to the CursorPager which allows us to loop through pages. I have also added a simple Repl Program.cs to illustrate how this can be done. what do you think? if we agree on this initial approach, my next step will be to go through all the other Resources in this SDK and make sure the appropriate endpoints that support CBP have all the necessary classes in place to do so. I would also like to put a user-agent header value, to identify requests coming from this SDK. Any suggestions?

thanks again for your feedback.

[mikerogers123]

@fbvilela

I have made a small change in regards to cursor base pagination. We can now set a before/after cursor to the CursorPager which allows us to loop through pages.

great idea and would love to see this rolled out, but can we split this out into another Pull Request? Otherwise the original intent is lost a bit. This is definitely separately deliverable to the original change of updating the README to help Mac users run integration tests

I have also added a simple Repl Program.cs to illustrate how this can be done. what do you think?

Whilst I like the idea here, I think I'd like to remain consistent with how we demo/document the other endpoints. At the moment the approach seems to be writing integration tests and add updating README with usage guidance. I personally like the idea of integration tests because they are run as part of the CI process each push so this won't get out of date. This would be part of a separate PR to this one

I would also like to put a user-agent header value

Again, this is a separate issue that would require separate discussion outside of this PR. My initial thoughts are that we already provide means for consumers to configure the HttpClient, and can therefore supply a User-Agent header. This means that they could override our default value set by the client. I wonder how much Zendesk really cares that a request is being made from this library. I imagine they care more about the client using our library but not sure.

And if they do care about a request coming from this library, it would surely only be useful to know all the instances of this client being used, which would mean enforcing it being present. Let's raise a separate issue for this, perhaps?

[fbvilela]

ok @mikerogers123 , thanks for that. I have pulled back this PR to its initial intent. should be good to merge?

[mikerogers123]

@fbvilela merged just now, thanks! There were however some integration test failures when merging to master: https://github.com/justeat/ZendeskApiClient/actions/runs/6534788092/job/17742786526?pr=313

I will try and get round to looking at it

[mikerogers123]

Okay the issue appears to be that Zendesk have removed some endpoints since we last ran these tests. https://support.zendesk.com/hc/en-us/articles/5414949730842

I am looking into this now