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 5 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
28 changes: 24 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
@@ -1,7 +1,9 @@
using Asp.Versioning;
using Microsoft.AspNetCore.Http;
using Microsoft.AspNetCore.Mvc;
using Microsoft.AspNetCore.WebUtilities;
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 +23,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 +48,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();
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 queryParamsDictionary is declared but never populated or used. The query parameters are being added directly to the URI using QueryHelpers.AddQueryString. This unused variable should be removed along with the unnecessary conditional logic at lines 67-69 that references it.

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

if (!string.IsNullOrWhiteSpace(searchQuery))
{
uri = QueryHelpers.AddQueryString(uri, "search", searchQuery);
}

return queryParamsDictionary.Count == 0
? uri
: $"{uri}&{string.Join("&", queryParamsDictionary.Select(kvp => $"{kvp.Key}={kvp.Value}"))}";
}
}
}
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
30 changes: 22 additions & 8 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,11 @@ export default class ActiveCampaignFormsModalElement
@state()
_totalPages = 1;

@state()
_searchQuery = "";

#filterTimeout?: NodeJS.Timeout;

constructor() {
super();

Expand All @@ -47,6 +52,10 @@ export default class ActiveCampaignFormsModalElement
this.#checkApiAccess();
}

disconnectedCallback() {
clearTimeout(this.#filterTimeout);
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 clearTimeout call may fail if #filterTimeout is undefined. While clearTimeout handles undefined gracefully in browsers, the explicit check on line 93-95 suggests this code expects defined values. Consider adding a conditional check or calling super.disconnectedCallback() to follow the component lifecycle pattern.

Suggested change
clearTimeout(this.#filterTimeout);
super.disconnectedCallback();
if (this.#filterTimeout !== undefined) {
clearTimeout(this.#filterTimeout);
}

Copilot uses AI. Check for mistakes.
}

async #checkApiAccess() {
if (!this.#activecampaignFormsContext || !this.#configurationModel) return;

Expand All @@ -58,10 +67,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 +84,26 @@ 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));
// Clear existing timeout
if (this.#filterTimeout) {
clearTimeout(this.#filterTimeout);
}

this._filteredForms = result;
this.#filterTimeout = setTimeout(async () => {
this._currentPageNumber = 1;
await this.#loadForms(this._currentPageNumber, this._searchQuery);
}, 500);
Comment on lines +95 to +103
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 timeout is not cleared when the component is destroyed, which could lead to attempted state updates after the element is removed from the DOM. Add cleanup in a disconnectedCallback or similar lifecycle method to clear this.#filterTimeout.

Copilot uses AI. Check for mistakes.
Comment on lines +100 to +103
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] The 500ms debounce timeout is hardcoded. Consider extracting this to a constant (e.g., SEARCH_DEBOUNCE_MS) to improve maintainability and make it easier to adjust the debounce delay if needed.

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