Skip to content

[OGUI-1920] Encode filter query in route updates - #3602

Merged
isaachilly merged 9 commits into
devfrom
bug/ILG/OGUI-1920/Fix-encoding-order-of-filter-parameter-q
Sep 14, 2026
Merged

isaachilly merged 9 commits into
devfrom
bug/ILG/OGUI-1920/Fix-encoding-order-of-filter-parameter-q

Conversation

@isaachilly

@isaachilly isaachilly commented Sep 1, 2026

Copy link
Copy Markdown
Collaborator

I have JIRA issue created

  • branch and/or PR name(s) includes JIRA ID
  • issue has "Fix version" assigned
  • issue "Status" is set to "In review"
  • PR labels are selected
  • FLP integration tests were ran successful

Ticket:
The q URL parameter in ILG, containing the JSON object of active filters, is not being encoded in the correct order creating opportunity for corruption when sharing or decoding.

The changes to the code include:

  • Reversing the order of encoding to be correct and encoding the whole object not just its values.
  • Removing any special handling of new lines or quotations as encoding handles this safely now.
  • Updating tests that assert against URL strings to use their fully-encoded versions.
  • Adding round-trip URL tests so encoding/decoding changes will be held up against best practice, fully-encoded URLs.

Reviewer can test with a filter value of C:\temp on a dev instance before and after the branch's changes and note the difference:

  • Before the URL contains raw { and }and the filter value is C:<tab>emp as the \t was incorrectly consumed as a JSON escape.
  • After the URL contains only valid, percent-encoded characters and the filter value is C:\temp.

Reviewer should note the filter JSON object is now wholly-encoded as oppose to before when it was just its values.

Reviewer should note that changes to an ILG integration test will be required as it asserts against not a fully-encoded query string.

Reviewer should note the address bar decodes double quotation marks for readability but will copy the encoded version.

@isaachilly isaachilly self-assigned this Sep 1, 2026
@isaachilly
isaachilly force-pushed the bug/ILG/OGUI-1920/Fix-encoding-order-of-filter-parameter-q branch 2 times, most recently from b6e3ad3 to 59052c7 Compare September 9, 2026 06:59
@isaachilly
isaachilly marked this pull request as ready for review September 9, 2026 16:08
@isaachilly
isaachilly requested a review from graduta as a code owner September 9, 2026 16:08
Centralised URL encoding for filters.

Removed encoding in `LogFilter` so filter values are no longer pre-encoded before serialisation; keeping query generation in one place.
Update InfoLogger filter action tests to match fully encoded `q` parameter.
Adds round-trip tests in a separate mocha file.

Removes old encoding tests.
Adds a `queryString` getter on `LogFilter` that uses shared `buildUrl` utilities, and updates `Model` to rely on that property when syncing route state.
This no longer has its corresponding encoder so it's pointless as the URL at this point will contain no `'\n` strings, they will all be percent-encoded.
@isaachilly
isaachilly force-pushed the bug/ILG/OGUI-1920/Fix-encoding-order-of-filter-parameter-q branch from 59052c7 to 5e08efd Compare September 11, 2026 13:31
graduta
graduta previously approved these changes Sep 14, 2026

@graduta graduta left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I approved as my comment is not a blocker or requested change but feel free to take it into consideration

Comment thread InfoLogger/test/public/log-filter-url-mocha.js Outdated
Consolidate the URL test cases into an array and generate the tests from it.

This removes repetitive test blocks, keeps test cases in one place so it's easier to see what is being covered whilst still treating each case its own test so it's easier to debug when they fail.
@isaachilly
isaachilly merged commit df877c4 into dev Sep 14, 2026
9 checks passed
@isaachilly
isaachilly deleted the bug/ILG/OGUI-1920/Fix-encoding-order-of-filter-parameter-q branch September 14, 2026 12:48
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Development

Successfully merging this pull request may close these issues.

2 participants