justeat / ZendeskApiClient

C#Β Client for working with the Zendesk API
Other
65 stars 86 forks source link

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

Closed fbvilela closed 9 months ago

fbvilela commented 12 months ago

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", "your@email.com", "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 commented 11 months ago

@mikerogers123 I would appreciate some love on this one, too πŸ™

mikerogers123 commented 11 months ago

@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 commented 11 months ago

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?

fbvilela commented 11 months ago

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 commented 10 months ago

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 commented 10 months ago

Hi Team, I have bumped the version. This is good to be merged πŸ™

fbvilela commented 9 months ago

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 commented 9 months ago

cc @justeat-iie @harryh-justeat

fbvilela commented 9 months ago

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

albertodebortoli commented 9 months ago

@brainmurphy maybe something you can help with?

fbvilela commented 9 months ago

πŸ™

cryptomail commented 9 months ago

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

cryptomail commented 9 months ago

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 commented 9 months ago

@Ud0o @UlianaShym @corneliuskopp fyi ❀️

Ud0o commented 9 months ago

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 commented 9 months ago

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

fbvilela commented 9 months ago

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 commented 9 months ago

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 commented 9 months ago

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