Skip to content

Bugfix/v16/activecampaign/pagination filtering #284

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.

Sign up for GitHub

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

Merged
merged 8 commits into from
Oct 23, 2025
Merged

Bugfix/v16/activecampaign/pagination filtering #284

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
a1a28d3
Update pagination and filtering actions
acoumb Oct 22, 2025
b8f07f0
Update ActiveCampaign backoffice version
acoumb Oct 22, 2025
3cb8c08
Add debouncing feature
acoumb Oct 23, 2025
1798828
Modify timeout delay
acoumb Oct 23, 2025
584d3c1
Use QueryHelpers and clear timeout on disconnected callback.
acoumb Oct 23, 2025
1b730ce
Remove unused declarations
acoumb Oct 23, 2025
6187c87
Add timeout check on disconnected
acoumb Oct 23, 2025
710ea33
PR updates
acoumb Oct 23, 2025
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
27 changes: 23 additions & 4 deletions ...Cms.Integrations.Crm.ActiveCampaign/Api/Management/Controllers/GetFormsPagedController.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@
using Microsoft.AspNetCore.Http;
using Microsoft.AspNetCore.Mvc;
using Microsoft.Extensions.Options;
using System.Web;
Copy link

Copilot AI Oct 23, 2025

Choose a reason for hiding this comment

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

The System.Web import at line 6 is unnecessary since QueryHelpers.AddQueryString from Microsoft.AspNetCore.WebUtilities is being used for URL encoding. Remove the unused System.Web import.

Suggested change
using System.Web;

Copilot uses AI. Check for mistakes.
using Umbraco.Cms.Api.Common.Builders;
using Umbraco.Cms.Integrations.Crm.ActiveCampaign.Configuration;
using Umbraco.Cms.Integrations.Crm.ActiveCampaign.Models.Dtos;
Expand All @@ -21,15 +22,13 @@ public GetFormsByPageController(IOptions<ActiveCampaignSettings> options, IHttpC
[ProducesResponseType(StatusCodes.Status404NotFound)]
[ProducesResponseType(StatusCodes.Status403Forbidden)]
[ProducesResponseType(StatusCodes.Status500InternalServerError)]
public async Task<IActionResult> GetForms([FromQuery] int? page = 1)
public async Task<IActionResult> GetForms([FromQuery] int? page = 1, string? searchQuery = "")
Copy link

Copilot AI Oct 23, 2025

Choose a reason for hiding this comment

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

The searchQuery parameter should be marked with [FromQuery] attribute for consistency with the page parameter and to make the API contract explicit. Without it, model binding may behave unexpectedly in certain scenarios.

Suggested change
public async Task<IActionResult> GetForms([FromQuery] int? page = 1, string? searchQuery = "")
public async Task<IActionResult> GetForms([FromQuery] int? page = 1, [FromQuery] string? searchQuery = "")

Copilot uses AI. Check for mistakes.
{
try
{
var client = HttpClientFactory.CreateClient(Constants.FormsHttpClient);

var requestUriString = page == 1
? $"{client.BaseAddress}{ApiPath}&limit={Constants.DefaultPageSize}"
: $"{client.BaseAddress}{ApiPath}&limit={Constants.DefaultPageSize}&offset={(page - 1) * Constants.DefaultPageSize}";
var requestUriString = BuildRequestUri(client.BaseAddress.ToString(), page ?? 1, searchQuery);

var requestMessage = new HttpRequestMessage
{
Expand All @@ -48,5 +47,25 @@ public async Task<IActionResult> GetForms([FromQuery] int? page = 1)
.Build());
}
}

private string BuildRequestUri(string baseAddress, int page, string searchQuery)
{
var uri = $"{baseAddress}{ApiPath}?limit={Constants.DefaultPageSize}";
Copy link

Copilot AI Oct 23, 2025

Choose a reason for hiding this comment

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

The URI construction manually appends '?limit=' without checking if ApiPath already contains query parameters. If ApiPath includes a query string, this will produce malformed URIs. Use QueryHelpers.AddQueryString for the limit parameter as well to handle this correctly.

Suggested change
var uri = $"{baseAddress}{ApiPath}?limit={Constants.DefaultPageSize}";
var uri = QueryHelpers.AddQueryString($"{baseAddress}{ApiPath}", "limit", Constants.DefaultPageSize.ToString());

Copilot uses AI. Check for mistakes.

Dictionary<string, string> queryParamsDictionary = new Dictionary<string, string>();
Copy link

Copilot AI Oct 23, 2025

Choose a reason for hiding this comment

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

[nitpick] Use object initializer syntax for better readability: var queryParamsDictionary = new Dictionary<string, string>(); or Dictionary<string, string> queryParamsDictionary = new(); (if using C# 9+).

Suggested change
Dictionary<string, string> queryParamsDictionary = new Dictionary<string, string>();
var queryParamsDictionary = new Dictionary<string, string>();

Copilot uses AI. Check for mistakes.
if (page > 1)
{
queryParamsDictionary.Add("offset", ((page - 1) * Constants.DefaultPageSize).ToString());
}

if (!string.IsNullOrWhiteSpace(searchQuery))
{
queryParamsDictionary.Add("search", HttpUtility.UrlEncode(searchQuery));
}

return queryParamsDictionary.Count == 0
? uri
: string.Format("{0}&{1}", uri, string.Join("&", queryParamsDictionary.Select(kvp => $"{kvp.Key}={kvp.Value}")));
Copy link

Copilot AI Oct 22, 2025

Choose a reason for hiding this comment

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

The URI building logic concatenates query parameters manually. Consider using UriBuilder or QueryHelpers.AddQueryString for more maintainable and robust query string construction.

Copilot uses AI. Check for mistakes.
Copy link

Copilot AI Oct 23, 2025

Choose a reason for hiding this comment

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

[nitpick] Use string interpolation instead of string.Format for better readability: $\"{uri}&{string.Join(\"&\", queryParamsDictionary.Select(kvp => $\"{kvp.Key}={kvp.Value}\"))}\"

Suggested change
: string.Format("{0}&{1}", uri, string.Join("&", queryParamsDictionary.Select(kvp => $"{kvp.Key}={kvp.Value}")));
: $"{uri}&{string.Join("&", queryParamsDictionary.Select(kvp => $"{kvp.Key}={kvp.Value}"))}";

Copilot uses AI. Check for mistakes.
}
}
}
5 changes: 1 addition & 4 deletions src/Umbraco.Cms.Integrations.Crm.ActiveCampaign/Client/generated/types.gen.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -64,6 +64,7 @@ export type GetFormsData = {
path?: never;
query?: {
page?: number;
searchQuery?: string;
};
url: '/umbraco/activecampaign-forms/management/api/v1/forms';
};
Expand All @@ -73,10 +74,6 @@ export type GetFormsErrors = {
* The resource is protected and requires an authentication token
*/
401: unknown;
/**
* Payment Required
*/
402: unknown;
/**
* Forbidden
*/
Expand Down
2 changes: 1 addition & 1 deletion src/Umbraco.Cms.Integrations.Crm.ActiveCampaign/Client/public/umbraco-package.json
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,7 +1,7 @@
{
"id": "Umbraco.Cms.Integrations.Crm.ActiveCampaign",
"name": "Umbraco CMS Integrations: CRM - ActiveCampaign",
"version": "5.0.0",
"version": "5.1.0",
"extensions": [
{
"name": "Umbraco EntryPoint",
Expand Down
4 changes: 2 additions & 2 deletions ...co.Cms.Integrations.Crm.ActiveCampaign/Client/src/context/activecampaign-forms.context.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -28,8 +28,8 @@ export class ActiveCampaignFormsContext extends UmbControllerBase {
this.#configurationModel.setValue(data);
}

async getForms(page?: number) {
return await this.#repository.getForms(page);
async getForms(page?: number, searchQuery?: string) {
return await this.#repository.getForms(page, searchQuery);
}

async getForm(id: string) {
Expand Down
18 changes: 9 additions & 9 deletions ...ms.Integrations.Crm.ActiveCampaign/Client/src/modal/activecampaign-forms-modal.element.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -29,6 +29,9 @@ export default class ActiveCampaignFormsModalElement
@state()
_totalPages = 1;

@state()
_searchQuery = "";

constructor() {
super();

Expand Down Expand Up @@ -58,10 +61,10 @@ export default class ActiveCampaignFormsModalElement
await this.#loadForms();
}

async #loadForms(page?: number) {
async #loadForms(page?: number, searchQuery?: string) {
this._loading = true;

const { data } = await this.#activecampaignFormsContext.getForms(page);
const { data } = await this.#activecampaignFormsContext.getForms(page, searchQuery);
if (!data) {
this._loading = false;
return;
Expand All @@ -75,21 +78,18 @@ export default class ActiveCampaignFormsModalElement
this._loading = false;
}

#handleFilterInput(event: UUIInputEvent) {
async #handleFilterInput(event: UUIInputEvent) {
Copy link

Copilot AI Oct 23, 2025

Choose a reason for hiding this comment

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

The method is marked as async but doesn't await the setTimeout callback. This can lead to unhandled promise rejections if the loadForms call fails. Consider removing async from the method signature since the actual async work happens in the timeout callback.

Suggested change
async #handleFilterInput(event: UUIInputEvent) {
#handleFilterInput(event: UUIInputEvent) {

Copilot uses AI. Check for mistakes.
let query = (event.target.value as string) || '';
query = query.toLowerCase();
this._searchQuery = query;

Copy link

Copilot AI Oct 22, 2025

Choose a reason for hiding this comment

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

When filtering is applied, the page number should be reset to 1 to avoid displaying empty results if the current page exceeds the filtered result set. Consider resetting this._currentPageNumber to 1 before calling #loadForms.

Suggested change
this._currentPageNumber = 1;

Copilot uses AI. Check for mistakes.
const result = !query
? this._forms
: this._forms.filter((form) => form.name.toLowerCase().includes(query));

this._filteredForms = result;
await this.#loadForms(this._currentPageNumber, this._searchQuery);
}
Comment on lines 90 to 104
Copy link

Copilot AI Oct 22, 2025

Choose a reason for hiding this comment

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

The filter input handler triggers a server call on every keystroke without debouncing, which could result in excessive API requests. Consider implementing debouncing to reduce server load and improve performance.

Copilot uses AI. Check for mistakes.

async #onPageChange(event: UUIPaginationEvent) {
this._currentPageNumber = event.target?.current;

await this.#loadForms(this._currentPageNumber);
await this.#loadForms(this._currentPageNumber, this._searchQuery);
}

#renderPagination() {
Expand Down
1 change: 0 additions & 1 deletion ...ions.Crm.ActiveCampaign/Client/src/property-editor/form-picker-property-editor.element.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,7 +15,6 @@ export default class ActiveCampaignFormPickerElement extends UmbElementMixin(Lit

@property({ type: String })
public value = "";


@state()
private _form: FormDtoModel = {
Expand Down
4 changes: 2 additions & 2 deletions ....Integrations.Crm.ActiveCampaign/Client/src/repository/activecampaign-forms.repository.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -28,8 +28,8 @@ export class ActiveCampaignFormsRepository extends UmbControllerBase {
return { data };
}

async getForms(page?: number) {
const { data, error } = await tryExecute(this, ActiveCampaignFormsService.getForms({ query: { page } }));
async getForms(page?: number, searchQuery?: string) {
const { data, error } = await tryExecute(this, ActiveCampaignFormsService.getForms({ query: { page, searchQuery } }));

if (error || !data) {
return { error };
Expand Down
2 changes: 1 addition & 1 deletion ...co.Cms.Integrations.Crm.ActiveCampaign/Umbraco.Cms.Integrations.Crm.ActiveCampaign.csproj
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,7 +16,7 @@
<PackageIconUrl></PackageIconUrl>
<PackageProjectUrl>https://github.com/umbraco/Umbraco.Cms.Integrations/tree/main-v16/src/Umbraco.Cms.Integrations.Crm.ActiveCampaign</PackageProjectUrl>
<RepositoryUrl>https://github.com/umbraco/Umbraco.Cms.Integrations</RepositoryUrl>
<Version>5.0.0</Version>
<Version>5.1.0</Version>
<Authors>Umbraco HQ</Authors>
<Company>Umbraco</Company>
<PackageTags>Umbraco;Umbraco-Marketplace</PackageTags>
Expand Down