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

Bug api entities order #5402

Merged
merged 2 commits into from
Dec 22, 2017
Merged

Conversation

escopecz
Copy link
Sponsor Member

Q A
Bug fix? Y
New feature? N
Automated tests included? Y
Related user documentation PR URL N
Related developer documentation PR URL N
Issues addressed (#s or URLs) N
BC breaks? N
Deprecations? N

Description:

Mautic API Library has a QueryBuilder which generates order queries as array like this:

[
    [
        'col' => 'someCol',
        'dir' => 'DESC',
    ]
]

This array will be URL encoded and sen as a GET query param. The problem is that Mautic didn't have handling of such ordering definition implemented for getEntities methods.

This PR also fix a small issue where the validateOrderByClause removed dots from the column names. So e.dateModified would be changed to edateModified and then the buildOrderByClauseFromArray would add it again because it's missing the dot and so the column name would become e.edateModified instead of e.dateModified. I don't know if this caused some real world bug, but it was causing new unit tests to fail.

Steps to reproduce the bug:

  1. Try to get a list of entities via API. For example segments.
  2. Order them with GET query params order[0][col]=dateModified&order[0][dir]=DESC

You should get a list of segments with random order.

Steps to test this PR:

  1. Checkout this PR.
  2. Test again. You should get list of your segments ordered by dateModified in descendent direction.

@escopecz escopecz added bug Issues or PR's relating to bugs ready-to-test PR's that are ready to test labels Nov 30, 2017
@alanhartless alanhartless added this to the 2.12.1 milestone Dec 13, 2017
@alanhartless alanhartless added the pending-test-confirmation PR's that require one test before they can be merged label Dec 19, 2017
@javjim javjim self-assigned this Dec 19, 2017
@Hadamcik Hadamcik self-assigned this Dec 21, 2017
@Hadamcik
Copy link
Contributor

SQL validated for adding ORDER but API returns entities not ordered by specified key. Not sure why

@Hadamcik Hadamcik removed their assignment Dec 21, 2017
@javjim
Copy link

javjim commented Dec 21, 2017

Having same issue as @dreiser the order is not being applied

@escopecz
Copy link
Sponsor Member Author

I don't know guys, it works for me whatever column or direction I use. @dreiser can you help me debug this since I cannot replicate the problem?

We tried a little debug in a private chat. So you tried to get the SQL query with echo $q->getQuery()->getSql(); in CommonRepository.php:1338 and it generated query with ORDER BY l0_.date_modified DESC. Can you try to find out what's going on since that point?

@alanhartless
Copy link
Contributor

Works for me as well. @dreiser @javjim-mautic Are you guys able to get the CURL request out of postman or whatever app you're using to test so we can see what the request looks like?

@javjim
Copy link

javjim commented Dec 21, 2017

It was a really lame postman error on my side. viewing on Pretty option it ordered by the ID, after some help from @alanhartless we figured it out when looking it as raw data....

Working properly @escopecz thanks!!!

@javjim javjim added ready-to-commit PR's with 2 successful tests, 1 approval, automated tests and docs and is ready to be merged and removed pending-test-confirmation PR's that require one test before they can be merged ready-to-test PR's that are ready to test labels Dec 21, 2017
@javjim javjim removed their assignment Dec 21, 2017
@alanhartless alanhartless merged commit 1820a9c into mautic:staging Dec 22, 2017
@dbhurley dbhurley removed the ready-to-commit PR's with 2 successful tests, 1 approval, automated tests and docs and is ready to be merged label Dec 22, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Issues or PR's relating to bugs
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants