[RED-1966] Adds support to an iterator and more CBP endpoints

[fbvilela]

Description

Adds support to the iterator pattern

There are now a few ways to loop through pages using this SDK

var services = new ServiceCollection();
services.AddZendeskClientWithHttpClientFactory("https://yoursubomain.zendesk.com", "[email protected]", "your_token_");
var serviceProvider = services.BuildServiceProvider();
var client = serviceProvider.GetRequiredService<IZendeskClient>();

var ticketCursorResponse = await client.Tickets.GetAllAsync(new CursorPager { Size = 5 }); // low page number to force pagination

var iteratorFactory = serviceProvider.GetRequiredService<ICursorPaginatedIteratorFactory>();
// creates the iterator with the response object of the first  request
var iterator = iteratorFactory.Create<Ticket>(ticketCursorResponse);

foreach (var ticket in iterator)
{
    Console.WriteLine("the id of this ticket is:" + ticket.Id);
} // this loop will stop at the first page

while (iterator.HasMore()) // to loop through all pages
{
    await iterator.NextPage();
    foreach (var ticket in iterator)
    {
        Console.WriteLine("the id of this ticket is:" + ticket.Id);
    }
}

// alternatively you can use .All() from the iterator
await foreach (var ticket in iterator.All())
{
    Console.WriteLine("the id of this ticket is:" + ticket.Id);
}

Adds support to more CBP endpoints

A few Resources already have CBP capabilities but were still missing the implementation on this SDK.
Could be individually reviewed by the commits that mention CBP

Updates the README/Documentation where the changes were made

[fbvilela]

@mikerogers123 I would appreciate some love on this one, too 🙏

[mikerogers123]

any chance can we remove this Console.WriteLine? Or at least use the dotnet ILogger

[fbvilela]

thanks, removed. this was not supposed to be there :)

[mikerogers123]

Just trying to wrap my head around this resource. I can see that this is only used in one place: in the private async Task ExecuteRequest(string requestUrl) method of the iterator class.

To my mind, these resource classes (and by convention anything implementing AbstractBaseResource) represent resources in Zendesk. This does not represent a particular resource and appears to be a helper method of sorts. I can see why you have gone this route - I assume to keep all HTTP requests performed by resources. This does carry weight, but s jeopardises Liskov Substitution Principle. What makes this most prevalent is the fact that the base class enforces the docsResource parameter in the base to be present, but this breaks that contract.

In terms of an alternatives I can think off off the bat...

• I think you just want access to that ApiClient object... and might be achieved via Dependency Injection rather than inheritance. I think you can just inject IZendeskApiClient into a class which creates the iterator.

• One option is to use a factory to create CursorPaginatorIterator. In this factory you could inject the ApiClient dependency needed to make the request.

• At the very least we could make the PaginatedResource internally visible because it is only intended for internal use. If we go down that route then a better option might be to have a internal class (not a resource) within the project which exposes the Client object you need. That way it can be injected internally within the package to be used for adhoc HTTP requests like this.

• Not sure what it might look like but a bigger change would be to make the response type from our CBP endpoints to be CursorPaginatedIterator<T> instead of ICursorPagination<T>. That way we remove the need for consumer to translate from one to the other and just give them the useful thing back

[fbvilela]

thanks @mikerogers123 🙏

what do you think of my last commit just pushed? I completely removed the the PaginatedResource . I agree it didn't make much sense. I switched dependancies on the CursorPaginationIterator class to now receive an ApiClient rather than a ZendeskClient. This way I can directly call the method I want. It slightly changes how we do the setup now

var client = serviceProvider.GetRequiredService<IZendeskClient>();
var apiClient = serviceProvider.GetRequiredService<IZendeskApiClient>();

var ticketCursorResponse = await client.Tickets.GetAllAsync(new CursorPager { Size = 2 });
var iterator = new CursorPaginatedIterator<Ticket>(ticketCursorResponse, apiClient);

Hence the changes in the Factory to accommodate the tests. What do you think?

[mikerogers123]

@fbvilela This is definitely an improvement. However, I think there is still one issue. The way a consumer typically would use this client is by injecting an instance of IZendeskClient. But with this suggestion/change they would also need IZendeskApiClient in order to use CursorPaginatedIterator. I can only speak from the context of JET's usage but I don't think this is ideal. Ideally, all paginated responses would "out-of-the-box" support this iteration. I'm wondering if one of the following may be better:

1. Make all cursor endpoints return a CursorPaginatedIterator instance. This conversion from the existing CursorPaginationResponse<T> to CursorPaginatedIterator<T> could perhaps be done in the AbstractBaseResource to minimise the blast radius
2. if this change was too large, then we could have a ICursorPaginatedIteratorFactory interface which consumers could use to to the conversion. That way the container can control the instantiation of IZendeskApiClient

Thoughts?

[fbvilela]

thanks @mikerogers123,
I think your first suggestion would be cool but also a big and breaking change. Since we don't know exactly how many customers are using this SDK, we would much prefer to not change existing behaviour.

about your second idea, I might be missing something here... would this factory be responsible for creating a IZendeskApiClient to pass to the CursorPaginatedIterator ?

another idea, could we perhaps make a new method in the ZendeskClient.cs in order to execute the request?
This class only has Resources so I'm not sure we would be breaking any sort of design pattern here.

[fbvilela]

🙏

[fbvilela]

Let me know if there's anything else we could work on. it would be great to finish off this work :)

[mikerogers123]

Hi @fbvilela apologies for the delay here.

would this factory be responsible for creating a IZendeskApiClient to pass to the CursorPaginatedIterator ?

Yes! So the issue I have with the current approach is that consumers now need to know about IZendeskApiClient, purely for the sake if instantiating this CursorPaginatedIterator. This exposes details/inner workings of the package they dont really care about. They just want to create an instance of CursorPaginatedIterator, which is where Dependency Injection and Factory Pattern would come into it

[mikerogers123]

@fbvilela overall I'm a big fan of using the iterator suggested here. I think we just need to iron out the precise implementation as I have outlined in the above comments 👍

[fbvilela]

Hi @mikerogers123 :)

I think I finally got it... or at least I'm close. thanks for your patience here as I learn my way through C#.
I have pushed a commit that has changes to the README which are also in the description of this PR now.
See how the code reads...

is this in line with what you had in mind?

[mikerogers123]

I'm not sure having a static member of this factory is a good idea. Being static suggests that this IZendeskClient field member can be adopt a value at compile-time. When in fact, this value gets mutated by calling GetApiClient. If possible I think this class should remain as-is, and just expose a non-static factory method, inline with the Factory pattern. I think this is possible, since we are discussing having the IZendeskApiClient abstracted inside the iterator/factory in another convo, which negates the need for this new method to be exposed on this class

[mikerogers123]

@fbvilela is it possible to revert the change to this file? I don't think we use theis GetApiClient method anywhere other than tests

[mikerogers123]

I think we can probably move the iterator/factory to a more suitable folder/namespace. Perhaps something like ZendeskApi.Client.Pagination or something?

[mikerogers123]

I think we are getting closer to the end result here. There are still a few points I would like to make...

This serviceProvider field member should not be static, it should be readonly. We should also not be injecting the IServiceProvder into a class. This endangers running into the Service Locator anti-pattern. I won't go into too much detail here but in short the IoC container is the thing responsible for choosing the correct dependency - in general you don't want a consuming class acting as a locator. The usual pattern for injecting dependencies in dotnet is closer to the following

namespace ZendeskApi.Client.Pagination
{
    public interface ICursorPaginatedIteratorFactory
    {
        CursorPaginatedIterator<T> Create<T>(ICursorPagination<T> response);
    }

    public class CursorPaginatedIteratorFactory : ICursorPaginatedIteratorFactory
    {
        // readonly field as only set in constructor
        private readonly IZendeskApiClient zendeskApiClient;

        public CursorPaginatedIteratorFactory(IZendeskApiClient _zendeskApiClient)
        {
            // inject IZendeskApiClient after registering in the container
            zendeskApiClient = _zendeskApiClient;
        }

        public CursorPaginatedIterator<T> Create<T>(ICursorPagination<T> response)
        {
            return new CursorPaginatedIterator<T>(response, zendeskApiClient);
        }
    }
}

This means a consumer looks like the following:

public class YourConsumerClass
{
    private readonly CursorPaginatedIteratorFactory iteratorFactory;

    public YourConsumerClass(ICursorPaginatedIteratorFactory _iteratorFactory)
    {
        iteratorFactory = _iteratorFactory;
    }

    public void YourMethod(ICursorPagination<YourType> response)
    {
        // Use the factory to get the iterator
        CursorPaginatedIterator<YourType> iterator = iteratorFactory.Create(response);

        // Rest of your logic using the iterator...
    }
}

To summarise, from where you are we may consider

• alter ICursorPaginatedIteratorFactory:
  • inject IZendeskApiClient directly, instead of the IServiceProvider
  • make the IZendeskApiClient field member private, readonly - not static
• Register your new interface/implementation in the IoC container. We have helper methods which we recommend using here.

Aside

I want to reiterate that, in an ideal world, consumers would be able to iterate over the response from paginated endpoints without injecting another dependency on top of IZendeskClient. One approach we already discussed was to change the return type of all CBP endpoints to be the iterator itself. We discounted this because of the blast-radius of the change. It's worth noting that this is probably not that dangerous. As you have explored recently these existing CBP methods don't work properly so people are probably not using it meaningfully. So now is probably the best time to make that change. I do understand your hesitancy though and it may be one for us owners to pick up if you wanted to.

Another approach that works around this is to use the adapter pattern. At the moment the CBP endpoints return ICursorPagination<T>. An adapter class would adapt the adapt the method signature of the CBP methods to return CursorPaginatedIterator<T>. It would work in a similar way to the IteratorFactory you have but the usage is different. You would expose the adapted resources on the IZendeskClient interface so users would consume like so:

client.AdaptedTicketAudit.GetAllAsync()

where the AdaptedTicketAudit is named for clarity but essentially the underlying resource would implement a different interface to ITicketAuditResource

Not suggesting this adapter as an approach because the amount of code changes required here is massive and I prefer the formerly suggested. But it just came to my mind so wanted to share in case it sparked anything.

[fbvilela]

thanks @mikerogers123, I believe I made the (small) changes you requested 😅
to be very clear, I'm a Ruby developer who picked up this work of updating the C# SDK in order to unblock Zendesk from rolling out the OBP deprecation. How is it looking after this last commit? The usage I wrote in the PR description remains unchanged and I'm registering the Factory here

[mikerogers123]

Thanks for that @fbvilela the changes look good overall. Have added a few more minor comments to sort, then we should be close to closing this one

[mikerogers123]

@fbvilela is it possible to revert the change to this file? I don't think we use theis GetApiClient method anywhere other than tests

[mikerogers123]

I don't think we need this?

[mikerogers123]

can we add a namespace to this class? And then move it into the Pagination folder?

[fbvilela]

thanks @mikerogers123 🙏 I made the changes you requested apart from removing the GetApiClient method used in the tests... I actually needed it so I could create the CursorPaginatedIteratorFactory in the test environment. see my last commit. I'm not 100% sure this is how things should be done in the test env but it's all passing ✅ after this change

[mikerogers123]

thanks @mikerogers123 🙏 I made the changes you requested apart from removing the GetApiClient method used in the tests... I actually needed it so I could create the CursorPaginatedIteratorFactory in the test environment. see my last commit. I'm not 100% sure this is how things should be done in the test env but it's all passing ✅ after this change

@fbvilela awesome stuff. I am happy to see this merged! Only last thing is to bump the version number 😆 after that we can approve/merge.

Annoyingly though, I am leaving Just Eat Takeaway in the next day or so. This means that I cannot merge / release the changes. You will have to get one of the other admins in on this just to nurse it over the line.

If I don't hear again then it has been a pleasure working with you on the recent changes. Great to see this library get some TLC ❤️

[fbvilela]

Hi Team, I have bumped the version. This is good to be merged 🙏

[fbvilela]

If I could have contributor's permissions I would bother you way less :)
🙏

Let me know if there's anything else we can do to get this merged. Thanks

[fbvilela]

cc @justeat-iie @harryh-justeat

[fbvilela]

@albertodebortoli 👋 seems like your main dev for this repo has left the company. Can anyone else help push this trough?

[albertodebortoli]

@brainmurphy maybe something you can help with?

[fbvilela]

🙏

[cryptomail]

Thank you so much team....Looking forward to moving forward on this.

[cryptomail]

BTW, we do have an impending deadline on our switchover to CBP from OBP. This PR facilitates that switchover for our mutual customers and provides a seamless transition. We are targeting end of January for this to begin, so any help in making this transition easier, and more expedient for our mutual customers is appreciated. Please feel free to reach out. cc @brainmurphy @albertodebortoli

[mikerogers123]

@Ud0o @UlianaShym @corneliuskopp fyi ❤️

[Ud0o]

Hi @fbvilela, apologies for the radio silence since Mikes departure.

I'll look at getting this merged in and a new package published once the build is passing, if you can update the code accordingly.

Thanks

[cryptomail]

@Ud0o Thanks so much, we'll look into it ASAP, we'll probably have it sorted very early next week (Monday :) ❣️

[fbvilela]

thanks @Ud0o , Can you approve the CI build so we can see if we get a green build? 🙏

I'm thinking the failure could have been because methods were made obsolete in TicketAuditResource (which I added tests for within the integration tests ) but we still had unit tests calling the methods without the obsolete flag

[Ud0o]

Hi @fbvilela @cryptomail,

Thats been merged and released now, small transient error with one of the new integration tests, but a rerun seemed to have solved it.

You can find version 7.0.7 here

Thanks for the changes, and if we are slow to reply again please tag anyone from Mikes comment here. Our team created and maintains this repository, but since its inception our goals have moved into a different problem space not involving Zendesk, hence the long reply.

I'll kick off a conversation internally about getting either or both of you setup with contributor/write roles.

Until next time! 👍

[cryptomail]

@Ud0o thanks so much, we really appreciate it! We look forward to helping this project forward in the future.