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

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

Merged
merged 14 commits into from
Jan 22, 2024
Merged
Show file tree
Hide file tree
Changes from 6 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
13 changes: 11 additions & 2 deletions README.md
Original file line number Diff line number Diff line change
Expand Up @@ -11,7 +11,7 @@ A .netstandard NuGet package for use with the Zendesk v2 API.

#### The deprecation and replacement of Status API endpoints

More detailed information on the exact changes and motivation can be found [here](https://support.zendesk.com/hc/en-us/articles/5414949730842).
More detailed information on the exact changes and motivation can be found [here](https://support.zendesk.com/hc/en-us/articles/5414949730842).

For the sake of this library, it means the following methods have been removed:

Expand Down Expand Up @@ -50,6 +50,7 @@ Groups:
Help Center:
- GET api/v2/help_center/articles
- GET api/v2/help_center/categories
- GET api/v2/help_center/sections

Organization:
- GET /api/v2/organization_fields
Expand All @@ -63,11 +64,19 @@ Tickets:
- GET /api/v2/tickets
- GET /api/v2/tickets/{ticketId}/comments
- GET /api/v2/ticket_fields
- GET /api/v2/ticket_audits - [Cursor Variant](https://developer.zendesk.com/api-reference/ticketing/tickets/ticket_audits/#pagination)
- GET /api/v2/ticket_audits
- GET

Satisfaction ratings:
- GET /api/v2/satisfaction_ratings

Requests
- GET /api/v2/requests
- GET /api/v2/requests/{requestId}/comments

Job Statuses
- GET /api/v2/job_statuses

[Further reading on Zendesk Pagination changes](https://support.zendesk.com/hc/en-us/articles/4402610093338-Introducing-Pagination-Changes-Zendesk-API)

## 3.x.x
Expand Down
6 changes: 6 additions & 0 deletions src/ZendeskApi.Client/Extensions/HttpClientExtensions.cs
Original file line number Diff line number Diff line change
Expand Up @@ -73,5 +73,11 @@ public static Task<HttpResponseMessage> GetAsync(this HttpClient client, string

return client.GetAsync(requestUri, cancellationToken);
}

// to be used within the PaginatedResource, where the next page is a complete URL with cursors already predefined in the query string.
public static Task<HttpResponseMessage> GetPageAsync(this HttpClient client, string requestUri, CancellationToken cancellationToken = default)
{
return client.GetAsync(requestUri, cancellationToken);
}
}
}
3 changes: 2 additions & 1 deletion src/ZendeskApi.Client/IZendeskClient.cs
Original file line number Diff line number Diff line change
Expand Up @@ -28,5 +28,6 @@ public interface IZendeskClient
IHelpCenterResource HelpCenter { get; }
ILocaleResource Locales { get; }
ITagsResource Tags { get; }
IPaginatedResource PaginatedResource { get; }
}
}
}
75 changes: 75 additions & 0 deletions src/ZendeskApi.Client/Models/CursorPaginatedIterator.cs
Original file line number Diff line number Diff line change
@@ -0,0 +1,75 @@
using System;
using System.Collections;
using System.Collections.Generic;
using System.Linq;
using System.Threading.Tasks;
using Newtonsoft.Json;
using ZendeskApi.Client;
using ZendeskApi.Client.Responses;

public class CursorPaginatedIterator<T> : IEnumerable<T>
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

{

public ICursorPagination<T> Response { get; set; }

private IZendeskClient client;


private string ResponseType { get; }

public CursorPaginatedIterator(ICursorPagination<T> response, IZendeskClient client)
{
Response = response;
this.client = client;
ResponseType = response.GetType().FullName;
}

public bool HasMore() => Response.Meta.HasMore;

public IEnumerator<T> GetEnumerator()
{
return Response.GetEnumerator();
}

IEnumerator IEnumerable.GetEnumerator()
{
return Response.GetEnumerator();
}


public async Task NextPage()
{
Console.WriteLine("fetching the next page... ");
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

await ExecuteRequest(Response.Links.Next);
}

public async Task PrevPage()
{
await ExecuteRequest(Response.Links.Prev);
}

public async IAsyncEnumerable<T> All()
{
foreach (var item in Response)
{
yield return item;
}
while (HasMore())
{
await NextPage();
foreach (var item in Response)
{
yield return item;
}
}
yield break;
}

private async Task ExecuteRequest(string requestUrl)
{
var httpResponseMessage = await client.PaginatedResource.GetPage(requestUrl);
var responseBody = await httpResponseMessage.Content.ReadAsStringAsync();
Response = (ICursorPagination<T>)JsonConvert.DeserializeObject(responseBody, Type.GetType(ResponseType));
}

}
Original file line number Diff line number Diff line change
Expand Up @@ -13,18 +13,22 @@ public interface IJobStatusResource
/// <summary>
/// Shows the current statuses for background jobs running.
/// </summary>
[Obsolete("Use `GetAllAsync` instead.")]
[Obsolete("Use `GetAllAsync` with CursorPager instead.")]
Task<IPagination<JobStatusResponse>> ListAsync(
PagerParameters pagerParameters = null,
CancellationToken cancellationToken = default(CancellationToken));

/// <summary>
/// Shows the current statuses for background jobs running.
/// </summary>
[Obsolete("Use `GetAllAsync` with CursorPager instead.")]
Task<IPagination<JobStatusResponse>> GetAllAsync(
PagerParameters pagerParameters = null,
CancellationToken cancellationToken = default(CancellationToken));

Task<ICursorPagination<JobStatusResponse>> GetAllAsync(
CursorPager pagerParameters = null,
CancellationToken cancellationToken = default(CancellationToken));
#endregion

#region Show
Expand Down
14 changes: 14 additions & 0 deletions src/ZendeskApi.Client/Resources/Interfaces/IPaginatedResource.cs
Original file line number Diff line number Diff line change
@@ -0,0 +1,14 @@
using System.Net.Http;
using System.Threading;
using System.Threading.Tasks;

namespace ZendeskApi.Client.Resources.Interfaces
{
public interface IPaginatedResource
{
Task<HttpResponseMessage> GetPage(
string url,
CancellationToken cancellationToken = default);
}
}

Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,10 @@ Task<IPagination<Request>> GetAllAsync(
PagerParameters pager = null,
CancellationToken cancellationToken = default);

Task<ICursorPagination<Request>> GetAllAsync(
CursorPager pager,
CancellationToken cancellationToken = default);

Task<Request> GetAsync(
long requestId,
CancellationToken cancellationToken = default);
Expand All @@ -29,6 +33,11 @@ Task<IPagination<TicketComment>> GetAllComments(
PagerParameters pager = null,
CancellationToken cancellationToken = default);

Task<ICursorPagination<TicketComment>> GetAllComments(
long requestId,
CursorPager pager,
CancellationToken cancellationToken = default);

Task<TicketComment> GetTicketCommentAsync(
long requestId,
long commentId,
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,7 @@ namespace ZendeskApi.Client.Resources
public interface ITicketAuditResource
{
Task<TicketAuditResponse> GetAllAsync(CursorPagerVariant pager = null, CancellationToken cancellationToken = default);
Task<TicketAuditCursorResponse> GetAllAsync(CursorPager pager, CancellationToken cancellationToken = default);
Task<TicketAuditResponse> GetAllByTicketAsync(long ticketId, CancellationToken cancellationToken = default);
Task<SingleTicketAuditResponse> Get(int ticketId, int auditId, CancellationToken cancellationToken = default);
}
Expand Down
14 changes: 13 additions & 1 deletion src/ZendeskApi.Client/Resources/JobStatusResource.cs
Original file line number Diff line number Diff line change
Expand Up @@ -19,7 +19,7 @@ public JobStatusResource(
: base(apiClient, logger, "job_statuses")
{ }

[Obsolete("Use `GetAllAsync` instead.")]
[Obsolete("Use `GetAllAsync` with CursorPager instead.")]
public async Task<IPagination<JobStatusResponse>> ListAsync(
PagerParameters pagerParameters = null,
CancellationToken cancellationToken = default(CancellationToken))
Expand All @@ -29,6 +29,18 @@ public async Task<IPagination<JobStatusResponse>> ListAsync(
cancellationToken);
}

public async Task<ICursorPagination<JobStatusResponse>> GetAllAsync(CursorPager pagerParameters, CancellationToken cancellationToken = default)
{
return await GetAsync<JobStatusListCursorResponse>(
ResourceUri,
"list-job-statuses",
"ListAsync",
pagerParameters,
null,
cancellationToken: cancellationToken);
}

[Obsolete("Use `GetAllAsync` with CursorPager instead.")]
public async Task<IPagination<JobStatusResponse>> GetAllAsync(
PagerParameters pagerParameters = null,
CancellationToken cancellationToken = default(CancellationToken))
Expand Down
24 changes: 24 additions & 0 deletions src/ZendeskApi.Client/Resources/PaginatedResource.cs
Original file line number Diff line number Diff line change
@@ -0,0 +1,24 @@
using System.Net.Http;
using System.Threading;
using System.Threading.Tasks;
using Microsoft.Extensions.Logging;
using ZendeskApi.Client.Resources.Interfaces;

namespace ZendeskApi.Client.Resources
{
public class PaginatedResource : AbstractBaseResource<PaginatedResource>, IPaginatedResource
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Contributor

@mikerogers123 mikerogers123 Nov 16, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🙏

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

{
public PaginatedResource(
IZendeskApiClient apiClient,
ILogger logger)
: base(apiClient, logger, "")
{ }

public async Task<HttpResponseMessage> GetPage(string url, CancellationToken cancellationToken = default)
{
using var client = ApiClient.CreateClient();
return await client.GetAsync(url);
}
}
}

24 changes: 24 additions & 0 deletions src/ZendeskApi.Client/Resources/RequestsResource.cs
Original file line number Diff line number Diff line change
Expand Up @@ -34,6 +34,18 @@ public async Task<IPagination<Request>> GetAllAsync(
cancellationToken: cancellationToken);
}

public async Task<ICursorPagination<Request>> GetAllAsync(
CursorPager pager,
CancellationToken cancellationToken = default)
{
return await GetAsync<RequestsCursorResponse>(
ResourceUri,
"list-requests",
"GetAllAsync",
pager,
cancellationToken: cancellationToken);
}


public async Task<Request> GetAsync(
long requestId,
Expand Down Expand Up @@ -79,6 +91,17 @@ public async Task<IPagination<TicketComment>> GetAllComments(
cancellationToken);
}

public async Task<ICursorPagination<TicketComment>> GetAllComments(long requestId, CursorPager pager, CancellationToken cancellationToken = default)
{
return await GetWithNotFoundCheckAsync<TicketCommentsListCursorResponse>(
string.Format(CommentsResourceUri, requestId),
"getting-comments",
$"GetAllComments({requestId})",
$"Could not find any comments for request {requestId} as request was not found",
pager,
cancellationToken);
}

public async Task<TicketComment> GetTicketCommentAsync(
long requestId,
long commentId,
Expand Down Expand Up @@ -121,5 +144,6 @@ public async Task<Request> UpdateAsync(
return response?
.Request;
}

}
}
13 changes: 13 additions & 0 deletions src/ZendeskApi.Client/Resources/Ticket/TicketAuditResource.cs
Original file line number Diff line number Diff line change
@@ -1,3 +1,4 @@
using System;
using System.Threading;
using System.Threading.Tasks;
using Microsoft.Extensions.Logging;
Expand All @@ -17,6 +18,7 @@ public TicketAuditResource(IZendeskApiClient apiClient, ILogger logger) : base(a
{
}

[Obsolete("Use `GetAllAsync` with CursorPager parameter instead.")]
public async Task<TicketAuditResponse> GetAllAsync(CursorPagerVariant pager = null, CancellationToken cancellationToken = default)
{
return await GetAsync<TicketAuditResponse>(
Expand All @@ -27,6 +29,17 @@ public async Task<TicketAuditResponse> GetAllAsync(CursorPagerVariant pager = nu
cancellationToken);
}

public async Task<TicketAuditCursorResponse> GetAllAsync(CursorPager pager, CancellationToken cancellationToken = default)
{
return await GetAsync<TicketAuditCursorResponse>(
ResourceUri,
"list-all-ticket-audits",
"GetAllAsync",
pager,
null,
cancellationToken);
}

public async Task<TicketAuditResponse> GetAllByTicketAsync(long ticketId, CancellationToken cancellationToken = default)
{
return await GetAsync<TicketAuditResponse>(
Expand Down
Original file line number Diff line number Diff line change
@@ -1,3 +1,4 @@
using System;
using System.Collections;
using System.Collections.Generic;
using Newtonsoft.Json;
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,15 @@
using System.Collections.Generic;
using Newtonsoft.Json;
using ZendeskApi.Client.Models;

namespace ZendeskApi.Client.Responses
{
[JsonObject]
public class JobStatusListCursorResponse : CursorPaginationResponse<JobStatusResponse>
{
[JsonProperty("job_statuses")]
public IEnumerable<JobStatusResponse> JobStatuses { get; set; }

protected override IEnumerable<JobStatusResponse> Enumerable => JobStatuses;
}
}
15 changes: 15 additions & 0 deletions src/ZendeskApi.Client/Responses/Request/RequestsCursorResponse.cs
Original file line number Diff line number Diff line change
@@ -0,0 +1,15 @@
using System.Collections.Generic;
using Newtonsoft.Json;
using ZendeskApi.Client.Models;

namespace ZendeskApi.Client.Responses
{
[JsonObject]
public class RequestsCursorResponse : CursorPaginationResponse<Request>
{
[JsonProperty("requests")]
public IEnumerable<Request> Requests { get; set; }

protected override IEnumerable<Request> Enumerable => Requests;
}
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,15 @@
using System.Collections.Generic;
using Newtonsoft.Json;
using ZendeskApi.Client.Models;

namespace ZendeskApi.Client.Responses
{
[JsonObject]
public class TicketAuditCursorResponse : CursorPaginationResponse<TicketAudit>
{
[JsonProperty("audits")]
public IEnumerable<TicketAudit> Audits { get; set; }

protected override IEnumerable<TicketAudit> Enumerable => Audits;
}
}
3 changes: 3 additions & 0 deletions src/ZendeskApi.Client/ZendeskClient.cs
Original file line number Diff line number Diff line change
Expand Up @@ -89,5 +89,8 @@ public ZendeskClient(IZendeskApiClient apiClient, ILogger logger = null)

private Lazy<ITagsResource> TagsLazy => new Lazy<ITagsResource>(() => new TagsResource(_apiClient, _logger));
public ITagsResource Tags => TagsLazy.Value;

private Lazy<IPaginatedResource> PaginatedResourceLazy => new Lazy<IPaginatedResource>(() => new PaginatedResource(_apiClient, _logger));
public IPaginatedResource PaginatedResource => PaginatedResourceLazy.Value;
}
}
Loading