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

IBX-6540: Added Depth sort to breadcrumbs #2115

Merged
merged 2 commits into from
Mar 12, 2024
Merged

Conversation

ViniTou
Copy link
Contributor

@ViniTou ViniTou commented Mar 1, 2024

Question Answer
Tickets https://issues.ibexa.co/browse/IBX-6450
Bug fix? yes
New feature? no
BC breaks? no
Tests pass? yes/no
Doc needed? no
License GPL-2.0

UDW columns are currently ordered based on the sequence of breadcrumbs in AccordionData. Typically, everything ran smoothly when utilizing IDs associated with deeper levels in the tree structure, where nested elements consistently had higher IDs than their parent counterparts. However, inconsistencies emerged in some cases, particularly when IDs did not follow this pattern. This issue is likely to recur in UDW if locations are moved.

The problem surfaced on Postgres, where the default sort order does not rely on IDs.

Proposing a solution by sorting columns based on depth, ensuring consistent breadcrumb order. There may be debates about whether JS/UDW should handle this, but I believe it's acceptable.

Checklist:

  • Coding standards ($ composer fix-cs)
  • Ready for Code Review

Copy link
Member

@konradoboza konradoboza left a comment

Choose a reason for hiding this comment

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

Relying by depth doesn't seem unsafe to me. Besides, adding additional SortClause wouldn't be the first time of us resolving issues on PostgreSQL. Test coverage would be nice though...

@konradoboza konradoboza requested a review from a team March 1, 2024 14:44
src/lib/QueryType/LocationPathQueryType.php Outdated Show resolved Hide resolved
Copy link

sonarcloud bot commented Mar 4, 2024

Quality Gate Passed Quality Gate passed

Issues
0 New issues
0 Accepted issues

Measures
0 Security Hotspots
No data about Coverage
0.0% Duplication on New Code

See analysis details on SonarCloud

Copy link
Contributor

@tomaszszopinski tomaszszopinski left a comment

Choose a reason for hiding this comment

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

QA approved on Ibexa DXP 3.3 commerce (mysql + postgres)

@ViniTou ViniTou merged commit 80eeb14 into 2.3 Mar 12, 2024
13 checks passed
@ViniTou ViniTou deleted the ibx-6540-breadcrumbs-depth branch March 12, 2024 12:32
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.

6 participants